quill-delta-parser icon indicating copy to clipboard operation
quill-delta-parser copied to clipboard

Behavior of inserts with unknown attributes

Open jasperwissels opened this issue 6 years ago • 5 comments

Describe the bug When attributes are unknown to the renderer it does not render the text, just a newline

The delta code which generates the Problem

{"ops":[{"insert":"1"},{"attributes":{"unknownAttribute":{"id": 123}},"insert":"2"},{"insert":"3"},{"insert":"\n"}]}
Expected :'<p>123</p>'
Actual   :'<p>1</p><p>3</p><p><br></p>'

Another example : Lexer without registering Bold listener (HeadingContentTest.php):

{"ops":[
  {"insert":"xyz","attributes":{"bold":true}},
  {"insert":"\n"},
  {"insert":"regular"},
  {"insert":"bold","attributes":{"bold":true}},
  {"insert":"italic", "attributes":{"italic":true}},
  {"insert":"\n","attributes":{"header":1}},
  {"insert":"xyz\n"}
]}

Expected :'<p>xyz</p><h1>regularbold<em>italic</em></h1><p>xyz</p>'
Actual   :'<p><br></p><h1>regularbold<em>italic</em></h1><p>xyz</p>'

Here you see that de garbage-cleaning kinda works sometimes

Expected behavior Expected behavior is to just render the text as a regular insert. This is - as far as i understand it - how quilljs behaves.

What do you think?

jasperwissels avatar Jul 11 '19 13:07 jasperwissels

Good question. In a practical use case you would never provide a "quill editor button" (like bold) without parsing its content right? This would not make sense i assume. In other words, why enable the BOLD button option but remove the bold quill delta listenere?

nadar avatar Jul 11 '19 15:07 nadar

Our use case is as follows : we have reviewThreads in our editor - where writers and copywriters can work together on text, highlight certain parts and place comments. This is a feature that is editor-side only, and should be ignored while parsing on the public article.

jasperwissels avatar Jul 12 '19 08:07 jasperwissels

I am going to make some tests, but i assume the problem might be here:

https://github.com/nadar/quill-delta-parser/blob/master/src/listener/Text.php#L48

nadar avatar Jul 14 '19 10:07 nadar

Does Quill state anything official about this? Apart from the website behaving in a certain way. I'm thinking about whether they state something about the meaning of an insert with unrecognized attributes.

I have the feeling we can't be sure how something should be rendered. E.g. what should be done with these deltas if their attributes are unrecognized?

  • {"insert":"\n","attributes":{"header":1}}
  • {"insert":{"video":"..."}}
  • {"insert":{"mention":{...}}}
  • {"insert":"<script>alert('foo')</script>","attributes":{"codeExample":true}}
  • {"insert":"<script>alert('foo')</script>","attributes":{"renderRawHtml":true}}

I think another way to deal with this would be to not render such deltas at all. And for the reviewThreads example, to make it explicit with a separate 'public' listener which renders only the insert string.

lode avatar Jul 14 '19 11:07 lode

I have some second thoughts on my previous comment. I now think we should indeed render the insert as is and ignore just the attribute.

In the example ops I had:

  • the header and codeExample just render as text, sad to loose the formatting, but okay
  • the video and mention should not render anything, we should only render the insert if it is a string
  • the renderRawHtml really looses it function and becomes ugly, but at least it won't pose a security risk since insert without attribute will be escaped

In most cases it is not what you want from the render. But if you explicitly remove listeners, that's what you get.

Btw, thinking of another use case: meta description elements for search engines. If you generate those from the quill displayed on the page, you don't want any elements like bold etc because it's not allowed in there. But you do want the plain text.

lode avatar Jul 21 '22 20:07 lode

i try to keep the issue tracker clean, please reopen if necessary.

nadar avatar Sep 14 '23 13:09 nadar