1625970 - Underline action can't be reversed on text that contains inserted links

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Simona Badau, Desktop QA]

Attachment: 2020-03-30_17h23_07.gif

Affected versions

• Firefox 74
• Firefox 75 beta 10
• Nightly 76.0a1

Affected platforms

• Windows 10
• Mac OS X 10.15
• Ubuntu 18.04

Steps to reproduce

1. Open Firefox and log into your Gmail account.
2. Start writing a new email and insert a link
3. Select all the text and hit Ctrl+U keys to underline all the text
4. Select all the text again and press the Ctrl+U keys to cancel the underline of the text

Expected result

• Underline action can be set and unset.

Actual result

• Once I underline the entire text, I can't reverse it. I can only reverse the Underline action if I do not include the text section that has the inserted link.

Regression range

• First bad: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1367114a0633863badba4331e7f1c9ddbd84d1ee&tochange=43ce5f2968ea046eeea78089e4f80efc09c8fb8a
• Last good: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8bb7084014ff82fb8ec04332cd0eca4b69e7ec05&tochange=43ce5f2968ea046eeea78089e4f80efc09c8fb8a
• Potentially regressed by:
  Bug 1574222 - Serialize getComputedStyle on text-decoration properly. r=emilio,dholbert

Additional notes

• I reproduced this issue on Gmail and Outlook.
• This issue is not reproducible on Yahoo.
• please see the attached screenshot for more details.

Comment 1

[Boris Chiou [:boris]]

I'm trying to figure out what gmail does when hitting Ctrl+U key. It seems it just adds <u></u> tag for the selected text. In Gecko, we cannot remove <u></u> if the selected area includes a link. Not 100% sure what its JS code does for this case, but I guess it may use getCompustedStyle() to check text-decoration property value, and Gecko's serialization is a little bit different from other browsers (see wpt.fyi). Is there any way to look at the script code when hitting Ctrl+U or the underline button on the toolbox?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Could we contact the Gmail folks to see what are they relying on? That would inform whether we should change our behavior or not.

Browser behavior is all over the place but I think our serialization code is correct / the most sensible one.

Comment 3

[Mike Taylor [:miketaylr]]

This sort of reminds me of Bug 833889. Ksenia, could you help find the relevant code bits that handle the underline functionality?

(I suspect Ksenia can find it faster than the Gmail team will reply 😃)

Comment 4

[Ksenia Berezina [:ksenia]]

Attachment: 1625970.html

They are using just document.execCommand( 'underline', false, null) . I've attached a reduced test case (to reproduce highlight the text and press "U" button).

As Mike mentioned this is (sort of) similar to https://bugzilla.mozilla.org/show_bug.cgi?id=833889

Both gmail and outlook using RoosterJs editor, so it can be reproduced on their demo site as well https://microsoft.github.io/roosterjs/index.html (looks like it's part of microsoft)

Comment 5

[Mike Taylor [:miketaylr]]

Thanks Ksenia!

Boris, setting ni? to make sure you see Comment #4.

Comment 6

[Boris Chiou [:boris]]

I'm still debugging this. Looks like doTagRemoval is always false even when we set <u> already. I am trying to figure out why HTMLEditor::GetInlineProperty doesn't work well for underline case (i.e. u tagName). Thanks for providing the reduced test case. It's really helpful.

Comment 7

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #6)

I'm still debugging this. Looks like doTagRemoval is always false even when we set <u> already. I am trying to figure out why HTMLEditor::GetInlineProperty doesn't work well for underline case (i.e. u tagName). Thanks for providing the reduced test case. It's really helpful.

And perhaps we have to update the mapping from <u> to its css property.

Comment 8

[Boris Chiou [:boris]]

Ok. I got the root cause: text-decoration also serializes text color value, and we split the selected text into 2 sections: firstValue and theValue. Based on the reduced test case, the firstValue is the normal text without the link, so its computed style is underline rgb(0, 0, 0). The theValue is the text with the link, so its computed style is underline rgb(0, 0, 238). We have a special check which compares firstValue and theValue. If they are different, we make aAll to false, and if aAll is false, we set doTagRemoval to false.

The conclusion is: the color component in text-decoration makes the logical incorrect because we assume its computed value only contains text-decoration-line component. (Note: bug 1574222 changes the serialization of the color component in text-decoration, i.e. it always serializes the text color now, even if it's currentColor.) So I will update the css property we use for underline: replace text-decoration with text-decoration-line, as comment 7 mentioned.

Comment 9

[Boris Chiou [:boris]]

Attachment: Bug 1625970 - Use text-decoration-line for underline and strikeThrough command.

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1625970 - Update test cases.

We use text-decoration-line for underline and strikeThrough, so the
innerHTML would be a little bit different from others.

Spec issue: github.com/w3c/editing/issues/241.

Comment 11

[Boris Chiou [:boris]]

Attachment: Bug 1625970 - Compare isSet flags between first value and others for text-decoration related HTML properties.

underline and strike use text-decoration property, which is a
shorthand and may include other longhand property values, e.g.
text-decoration-color. In order to set aAll flag correctly, we
should not compaure the computed values between firstValue and
theValue. Using isSet flag should be more accurate.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8bbe879dca0b
Compare isSet flags between first value and others for text-decoration related HTML properties. r=masayuki

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/8bbe879dca0b

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:boris, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 15

[Simona Badau, Desktop QA]

Verified as fixed using the latest Firefox beta 77 (20200525134724) and latest Nightly 78.0a1 on Windows 10 x64, Ubuntu 18.04 x64 and Mac OS X 10.15.