Always HTML escape ampersand in dropdown

[mvorisek]

extracted from #3205

And fix many double encoding issues.

All escape functions must be lossless and must return the same value when called again on a reverse function.

When using preserveHTML=false and the text is read from innerHTML input (<option>xxx</option>), & must be encoded in HTML code as & to be preserved 1:1, This is due & -> & browser HTML normalization. Explained in [1], [2], [3].

[1] original issue: #2782
[2] browser behaviour: https://jsfiddle.net/wdyjfvz0/
[3] how to encode innerHTML for <option> tag: https://3v4l.org/E2BQY

[mvorisek]

@lubber-de please test and let me know if there is any usecase it breaks. This PR itself does not intend to fully fix #3199.

I am quite sure about the 2nd commit. Together with the 1st commit it fixes #3205 (review) well.

I am not fully sure about the 3rd commit, as "get values" API does encode only multiple values which is inconsistent design. But it does fix most issues of my local repros. So if such change does not break anything, I propose to land it and fix the API design later in a separate commit.

[lubber-de]

Unfortunately this still breaks in 2 situations when this PR is applied (by latest commit 9c732da)

1. Original Issue: #1373, Original PR: #1770

FUI 2.9.4

https://jsfiddle.net/lubber/23htdnw0/1/
--> works

FUI 2.10.0 beta (current develop 45ac3fe)

https://jsfiddle.net/lubber/tdjmzscr/1/
--> works

Your PR applied

https://jsfiddle.net/lubber/gmh5reoy/8/
--> fails (I cannot remove the 3rd entry (item 3" inches) inside the 2nd dropdown (made of select) anymore once selected ... seems the internal comparison between hidden input and data attribute does not match anymore)

2. Original Issue: #2782, Original PR: #2790

Mentioned example c)

FUI 2.9.4

https://jsfiddle.net/lubber/q8hL21yg/1/
--> works

FUI 2.10.0 beta (current develop 45ac3fe)

https://jsfiddle.net/lubber/f86g9L3v/2/
--> works

Your PR applied

https://jsfiddle.net/lubber/zap03L4k/5/
--> fails (When preserveHTML is false, the ampersand is double encoded)

I havent checked the other modules changes yet (they are the same), but i feel, always encoding the ampersand, instead of checking for already encoded htmlentities (the regex you changed from ["'<>]|&(?![\d#A-Za-z]{1,12};) to ["&'<>] ) should not be changed. At least this will prevent double encoding

[mvorisek]

1. Original Issue: HTML entity encoding behavior in multiselect dropdown #1373, Original PR: [Dropdown] multiselect values encoding, label cannot be removed, quotes missing in select dropdown #1770

This is (will be) fixed in #3199 already.

2. Original Issue: [Dropdown] Multiple dropdown doesn't show selected label if the selected value has ampersand "&" #2782, Original PR: fix(dropdown): encoded select values, multiple maxselections #2790

This issue is given by browser behaviour - https://jsfiddle.net/0m21broh/. Without no code, browser parses/normalizes & as if it was &. This is legal, as both & and & render as &.

I propose to fix this issue later once this PR and #3199 is landed. The solution would be to to revert & to & where obtained as early as possible.

I havent checked the other modules changes yet (they are the same), but i feel, always encoding the ampersand, instead of checking for already encoded htmlentities (the regex you changed from ["'<>]|&(?![\d#A-Za-z]{1,12};) to ["&'<>] ) should not be changed. At least this will prevent double encoding

It must be changed, otherwise values containing & are saved as & which is a bug. Dropdown must never mutate values and all encoding functions must be strictly bijective.

With this said, I propose to merge this PR as is, then let me finish #3199 and then let's look into the & with preserveHTML: false. This is present only when the values are initialized thru interpretable HTML (<option>xxx</option>), not when using attributes or JS.

[lubber-de]

Don't get me wrong: I don't want to merge PRs which knowingly break previously fixed/working functionality (and isn't a intended breaking design change like #3200). Even if i trust you, to fix this in a later PR.

Everything that worked before should work afterwards. The develop branch is supposed to be the always working/approved one.
So, if the issues are fixed in another PR already, merge them into this one and i am fine with that 🙂 And your original #3199 would also be fixed then (?)

Without no code, browser parses/normalizes & as if it was &. This is legal, as both & and & render as &.

Well, then we could also argue why the & is encoded at all if surrounded by space when a single & is still valid. (2.9.4 and 2.10 beta dont do this, your change does). Now people still have single & in their data which would break now.

It must be changed, otherwise values containing & are saved as & which is a bug. Dropdown must never mutate values and all encoding functions must be strictly bijective.

Ok, agreed, still needs testing (espcially for the other modules and their usages)

[mvorisek]

Don't get me wrong: I don't want to merge PRs which knowingly break previously fixed/working functionality (and isn't a intended breaking design change like #3200). Even if i trust you, to fix this in a later PR.

Everything that worked before should work afterwards. The develop branch is supposed to be the always working/approved one. So, if the issues are fixed in another PR already, merge them into this one and i am fine with that 🙂 And your original #3199 would also be fixed then (?)

100% agree and yes, #3199 would be fixed with this and #3205.

Without no code, browser parses/normalizes & as if it was &. This is legal, as both & and & render as &.

It turned out the fix is easy. I checked all .html() calls (and there are not innerHtml/outerHtml).

All expect 1 are irrelevant as they are either for preserveHTML=true or https://github.com/fomantic/Fomantic-UI/blob/2.9.4/src/definitions/modules/dropdown.js#L3129.

So only one .html() result was needed to be fixed/"assume ampersand unescaped".

I also checked all other files/modules affected by 987a21b and all .html() usages in them do not have to be fixed.

Works well on all repros I tested.

To integrate this, please:

1. review this PR for correctness
2. review Fix quote and ampersand HTML escape in dropdown #3205 for correctness and test all edge usecases
3. if everything is fine, merge this PR and then immediatelly Fix quote and ampersand HTML escape in dropdown #3205 (there are no git conflicts expected)

[mvorisek]

@lubber-de might you look into this?

After this PR and #3205, the develop branch should be fixed /wo any BC break.

[lubber-de]

Unfortunately this still breaks in 1 situation when this PR is applied (by latest commit a995f2f)

1. Original Issue: #1373, Original PR: #1770

FUI 2.9.4

https://jsfiddle.net/lubber/23htdnw0/1/
--> works

FUI 2.10.0 beta (current develop 45ac3fe)

https://jsfiddle.net/lubber/tdjmzscr/1/
--> works

Your PR applied

https://jsfiddle.net/lubber/gmh5reoy/10/
--> FAIL: 1st dropdown: When (item 3" inches) is selected and removed again, it is not listed in the dropdown anymore
--> FAIL: I (still) cannot remove the entry (item 3" inches) inside the 2nd (select) dropdown anymore
--> fails, regardless, if preserveHTML is true or false

However, i tried the same using your latest commit of #3205 : And there it works(!) https://jsfiddle.net/lubber/e5ru0239/1/

So, why don't we just merge the other PR where everything seems to be fixed (i would need to check all the other use-cases there as well) instead of this "in-between" PR here? (which still breaks the above mentioned situation)

[mvorisek]

However, i tried the same using your latest commit of #3205 : And there it works(!) https://jsfiddle.net/lubber/e5ru0239/1/

So, why don't we just merge the other PR where everything seems to be fixed (i would need to check all the other use-cases there as well) instead of this "in-between" PR here? (which still breaks the above mentioned situation)

That is great and expected.

This PR is solely about "ampersand and double encoding fixing".

I prefer to merge this first and then the 2nd PR separately, as both PRs fix something different, so this way squashed history will be much more readable. Develop branch will be "stable" afterwards.

[lubber-de]

Please see my comment for the "always encode ampersand for non dropdown functions which dont expect a reverse function" at #3205 (comment) (i should have commented that in this PR as you want this one to be merged first

[lubber-de]

LGTM. Thanks for your patience!