1923251 - Make `HTMLEditor` clean up unnecessary padding line break which is immediately after inserting content

Status: REOPENED

(Core :: DOM: Editor, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Our editor inserts a <br> or a preformatted line break before block boundary when it's required. However, our editor won't delete it when it becomes unnecessary when the content immediately before the line break is changed. I think that we should make it clean up the unnecessary line break for preventing multiple expected results in WPT.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1923251 - part 1: Make `HTMLEditor` clean up unnecessary padding line break when inserting something immediately before it r=m_kato

Both Chrome and Safari clean up unnecessary padding line breaks when they become
unnecessary. Therefore, we should follow that.

Depends on D225037

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1923251 - part 2: Make `HTMLEditor` clean up unnecessary padding line breaks when deleting something immediately before it r=m_kato

Unfortunately, the code does not work well only in white-space: pre-line.
The rending itself is broken, so, it won't work well on Firefox anyway and
this style is not so useful with editable content. Therefore, this patch
does not fix the issue.

Depends on D225038

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b61267a75e51
part 1: Make `HTMLEditor` clean up unnecessary padding line break when inserting something immediately before it r=m_kato
https://hg.mozilla.org/integration/autoland/rev/526e6dccb137
part 2: Make `HTMLEditor` clean up unnecessary padding line breaks when deleting something immediately before it r=m_kato

Comment 4

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/48633 for changes under testing/web-platform/tests

Comment 5

[Serban Stanca [:SerbanS]]

Backed out for causing build bustages in HTMLEditor.cpp.

• Backout link
• Push with failures
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp(4389,14): error: unused variable 'isNewLinePreformatted' [-Werror,-Wunused-variable]

Comment 6

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, it's used only in MOZ_ASSERT...

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8982b3724097
part 1: Make `HTMLEditor` clean up unnecessary padding line break when inserting something immediately before it r=m_kato
https://hg.mozilla.org/integration/autoland/rev/7a9e2f6ed76e
part 2: Make `HTMLEditor` clean up unnecessary padding line breaks when deleting something immediately before it r=m_kato

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8982b3724097
https://hg.mozilla.org/mozilla-central/rev/7a9e2f6ed76e

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 12

[Daniel Holbert [:dholbert]]

I think this might have made us more likely to trigger bug 1925635 (which itself seems to be a longstanding bug, but seems to have only started causing trouble on real-world sites in Nightly in the last few days, possibly due to this bug making it easier to trigger).

Comment 13

[Daniel Holbert [:dholbert]]

In particular: the trailing <br> line-break here (described in comment 0) turned out to have actually been functional in many real-world cases, in that it served as a hackaround to prevent us from hitting bug 1925635.

Quick STR to demonstrate that:
(1) Open the "semi-reference-case" from bug 1925635: https://bugzilla.mozilla.org/attachment.cgi?id=9432321
(2) Click after "def" and press Ctrl+backspace
(3) Type "ggg" and notice that the space was preserved (hooray) -- however if you have devtools open, you might notice that the br disappeared...
(4) Ctrl+backspace to delete "ggg"
(5) Type "hhh"

ACTUAL RESULTS:
In Nightlies that include this bug's patch (after comment 9 here), you end up seeing "abchhh" with no space.

EXPECTED RESULTS:
You should see "abc hhh" at the end of step 5, just like you saw "abc ggg" at the end of step 3.

Various web properties (Gmail, Google Docs comments, maybe Fastmail) seem to have been relying on that, whether intentionally or unintentionally, and they're currently broken in Nightly as a result (per bug 1925606, bug 1926087).

Until we're confident in a fix for bug 1925635, we may want to backout the change here -- masayuki, does that sound OK to you? Note that it's RC week -- merge day is next Monday -- so we might want to err on the side of caution and opt for a backout for the short term, rather than the alternative approach of leaving this patch in & trying to quickly land a fix for bug 1925635. And if we do backout here, I'm imagining we'd re-land this bug's patches together with a fix for bug 1925635 next week [or when we're ready] as part of the 134 nightly cycle - how does that sound?

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, the patches contain WPT updates. So, I'd like to back them out after the merge. Is that fine for you?

(Although I'd like to post a patch for bug 1925635 today.)

Comment 15

[Daniel Holbert [:dholbert]]

See slack DM for some musings; but tl;dr, that seems maybe-fine, if we can get a fix for the other bug landed very soon (though I don't know how feasible that is, without knowing contenteditable code/edge-cases super-well; and I don't want you/reviewers to be under pressure to get that fix written/reviewed/landed).

Whatever route we take, the primary goals at this point are:

• to address the Nightly usability-regression ASAP, so that gmail, gdoc-comments, fastmail, etc. aren't broken (bug 1925606, bug 1926087) and Nightly users aren't inadvertently sending emails with missing spaces and/or being frustrated by mysteriously disappearing spaces while typing emails for much longer.
• ...and to avoid shipping that regression to beta (next week).

I think that means we have to do one of the following in the next day or two (the sooner the better): either back out this bug's code-changes, or land a fix for bug 1925635. (Either of those should address the regression.)

The backout option seems trivially safe; we could leave the WPT changes in-place if it's administratively annoying to back them out, and we could just annotate them as known-failures or skipped. (That's a bit of manual work but probably not too much trouble.)

Whereas I'm less sure about the timeline and risk for the "land a fix for bug 1925635" option. Hence, I'd personally lean towards the backout option, unless you've got an extremely-high-confidence patch nearly ready for bug 1925635.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, sorry, I didn't realize the message due to focus on the work.

Now, I finished writing the patch, and testing on tryserver. I'll post the patch to bug 1925635, and anyway, I'll prepare the backout patch except WPT changes for the last resort.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I added a lot of edge cases into new WPTs, but it detects a lot of traditional issues. So, I think it's reasonable to back this out.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1923251 - Backout D226092 and D225039 except WPT part

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1923251 - Backout almost all of D225038 except WPT part

This does not backout the changes in EditorDOMPoint.h, WSRunObject.h and
template method instantiation in WSRunObject.cpp to make some other my local
changes keep working. (They won't affect to any behavior.)

Depends on D226631

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

dholbert: Could you handle the backout patches (D226631 and D226632)? I need to be away due to the limitation of working time. I'll be back tomorrow morning. So, if you don't have much time, it'll handle that tomorrow.

Test result: https://treeherder.mozilla.org/jobs?repo=try&resultStatus=pending%2Crunning%2Cusercancel%2Cretry%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=2a8b5a8a642df96a6c1085279e9bd025a13b65af

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: Bug 1923251 - Annotate one more expected-failure as part of this bug's backout.

Comment 22

[Daniel Holbert [:dholbert]]

Thanks! And, sure - happy to land those backout patches for you.

It looks like the Try run in comment 20 found one missing expected-failure-annotation, so I added a patch to the stack to add that annotation.

Here are try runs to be sure there aren't other missed annotations, before landing:
(mach try auto, maybe sufficient):
https://treeherder.mozilla.org/jobs?repo=try&revision=fe638fb280234507489d3d34a802157a0c021d04
(mach try fuzzy with all WPT tasks, for good measure in case 'auto' isn't sensitive enough):
https://treeherder.mozilla.org/jobs?repo=try&revision=19a9b2226cf8f1eefb5ee3a6b6f7a1a2bb8e6d55

In an hour or two when those are done, I'll go ahead and trigger lando, if they don't find any additional annotations that we need here.

Comment 23

[Daniel Holbert [:dholbert]]

Try's looking good, so I triggered lando for the backouts.

(Before landing, I used the phab UI to add a brief mention of which revision corresponds to which bug/part, for convenience of any hg-viewers trying to tie the backouts to bugs -- particularly between this bug and bug 1925331 which we're backing out in a combined commit here).

--> Reopening.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca6f3852c6ab
Backout D226092 and D225039 except WPT part
https://hg.mozilla.org/integration/autoland/rev/c94760684a9b
Backout almost all of D225038 except WPT part
https://hg.mozilla.org/integration/autoland/rev/d64b89501be6
Annotate one more expected-failure as part of this bug's backout.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

dholbert: Thank you very much, especially for the follow up patch!

Comment 26

[Daniel Holbert [:dholbert]]

Sure! Thanks for the quick action on preparing the backout patches.

I'd suggest we re-land this (essentially backing out the backouts :) ) next week after the merge, together with the patch for bug 1925635 (once it's ready).

Comment 27

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/d64b89501be6

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ca6f3852c6ab
https://hg.mozilla.org/mozilla-central/rev/c94760684a9b