Quill: Use both DOM selected API and selected attribute to avoid regression for react-quill and Primefaces since quill 1.3.0

[jxmai]

Background: Primefaces is the UI framework for Java EE (https://www.primefaces.org/)

http://www.oracle.com/technetwork/articles/java/java-primefaces-2191907.html

github repo: https://github.com/primefaces/primefaces

Primefaces texteditor is based on quill

---

Steps for Reproduction

The analysis: https://github.com/primefaces/primefaces/issues/2802

This issue is not easily reproducible using quill js only, but the main issue is option.selected is not exactly the same as option.hasAttribute('selected'). This regression was brought by https://github.com/quilljs/quill/pull/1576

---

As you can see in the watch

Expected behavior:

We don't expect to change the behavior of option.hasAttribute('selected')

Actual behavior:

option.hasAttribute('selected') is not the same as option.selected

The consequence is

texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7 Uncaught TypeError: Cannot read property 'innerHTML' of null
    at e.value (texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7)
    at new e (texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7)
    at texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7
    at Array.map (<anonymous>)
    at e.value (texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7)
    at e.value (texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7)
    at e.value (texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7)
    at texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7
    at Array.forEach (<anonymous>)
    at e.value (texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7)

Platforms:

All browsers, all OS

Version:

Since 1.3.0

[jhchen]

The difference between selected and getAttribute is precisely why it was changed -- to fix the bug that was reported. The expected and actual behavior reported here are not actually behaviors. Please provided the necessary details and we can re-evaluate.

[jxmai]

@jhchen Here is the proof

Before you merged the PR: #1576

The committer said:

Demo of the change:

https://codepen.io/alexkrolick/pen/awRxWz?editors=0010

Note that the highlighted values in the toolbar don't reset to default with the Snow theme in React-Quill, which is another bug (zenoamaro/react-quill#175).

Clearly the committer knew the commit cannot fully address their issue but would bring a regression. This is clearly unacceptable

The difference between selected and getAttribute is precisely why it was changed -- to fix the bug that was reported.

Exactly ! and that can also explain the problem. :+1: The changes can address their issue, but it could break others to let them fall into the regression. See https://github.com/quilljs/quill/issues/1762#issuecomment-343322514.

---

If you take a look at https://github.com/zenoamaro/react-quill/issues/175#issuecomment-313901868

You will see the exact problem we are facing here.

Quote:

So... it seems the main thing to do is figure out a way to 1) get React to stop swallowing the selected attribute on options in selects or 2) add an alternate identifier for Quill to use to determine the selected state, that still emits the right change events

The second point shows that the changes in #1576 need to have a follow-up patch. However, I don't see such patch anywhere

The comment said add an *Alternative option, but they ended up removing the previous option. The DOM selected attribute is not a replacement of selected attribute as one might be true while the other can be false and vise versa (https://stackoverflow.com/questions/6003819/what-is-the-difference-between-properties-and-attributes-in-html). More details in https://github.com/quilljs/quill/issues/1762#issuecomment-343322514*

I made a comment below and did not receive any response

https://github.com/zenoamaro/react-quill/issues/175#issuecomment-336217910

Here is the reproducer from the previous commiter:

No, the reproducer is valid for them, but we have the exact opposite problem. :disappointed: Please look at the analysis in https://github.com/quilljs/quill/issues/1762#issuecomment-343322514

https://codepen.io/alexkrolick/pen/qjMrEN?editors=0010

Software regression is the most unbearable issue (We should never break user space) and I hope you seriously consider fixing this regression.

Please reopen this ticket, and please either enhance https://github.com/quilljs/quill/pull/1576/commits/83bc6912304f6347e9264ae94cd1a9881a2478fb or revert it.

Side note: We are close to release another community version and we are appreciate if you can address this issue as soon as possible. Thanks a lot. :smile:

---

/cc @alexkrolick
/cc @mertsincan @tandraschko @melloware @rapster @erickdeoliveiraleal

[melloware]

Could you solve it for everyone by checking both?

if (option.hasAttribute('selected') || option.selected === true) {

[jxmai]

In Alex's sample: https://codepen.io/alexkrolick/pen/qjMrEN?editors=0010. The node created by react does not have selected attribute, then hasAttribute('selected') will always be false. As a result,

As Alex's analysis in https://github.com/zenoamaro/react-quill/issues/175#issuecomment-313901868, they will get Cannot read property 'innerHTML' of null.

---

In our case, we have the exact opposite result.

so we fell into the same bug as Alex after his change.

texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7 Uncaught TypeError: Cannot read property 'innerHTML' of null
    at e.value (texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7)
    at new e (texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7)
    at texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7
    at Array.map (<anonymous>)
    at e.value (texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7)
    at e.value (texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7)
    at e.value (texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7)
    at texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7
    at Array.forEach (<anonymous>)
    at e.value (texteditor.js.xhtml?ln=primefaces&v=6.2-SNAPSHOT:7)

---

The most peaceful solution without breaking each other is as exactly as @melloware 's proposal. :+1:

[jxmai]

@jhchen A PR was raised: https://github.com/quilljs/quill/pull/1813.

[jhchen]

@jxmai I think you are trying to be helpful, but your data dump of text and screenshots is anything but. I think the disconnect is what you think is a behavior I think is an implementation detail. In Alex's report he provided the user facing issue and connected the dots to implementation, which led to a merged PR.

What I need from you is why getAttribute('selected') is a problem for the user. I understand the difference between a property and attribute -- I need to know why it is wrongly used as is your claim.

So I will ask again for expected and actual behaviors, not implementation details. You are also missing the steps for reproduction and a coherent description of the problem (again not implementation details), which is actually 60% of what we ask for in our Contributing Guide.

[melloware]

@jhchen we were still bit by this issue again. The PR seems like a harmless fix for you to add even though it was documented above. Was also reported on Stack Overflow again today: https://stackoverflow.com/questions/63247737/problem-with-primefaces-texteditor-in-dialog-after-update-from-primefaces-7-to-8

Is there any way we can get this minor fix put into QuillJS to prevent this in the future?