1925635 - Space character disappears from contenteditable, when typing after doing Ctrl+Backspace (or option+Backspace) to delete a word

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:

1. Load attached testcase.
2. Click after def in the editable area.
3. Ctrl+Backspace to delete def.
4. Type g

EXPECTED RESULTS:
abc g

ACTUAL RESULTS:
abcg (no space)

This affects Gmail's email composer -- tracked in bug 1925606 -- and makes it pretty painful to type emails if you've got Ctrl+backspace as part of your muscle-memory for editing (as I do). Though in some cases you might manage to avoid it, possibly due to some Gmail A/B testing, per bug 1925606 comment 5.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screencast of bug

Comment 2

[Daniel Holbert [:dholbert]]

This is not a regression; I can reproduce as far back as 2009-12-31.

Chromium and WebKit seem to avoid this by replacing the trailing space with   when I do Ctrl+Backspace to delete the "def" text.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (to demonstrate   being swapped in for the space character in other browsers)

Chromium and WebKit seem to avoid this by replacing the trailing space with   when I do Ctrl+Backspace to delete the "def" text.

Here's a testcase to demonstrate this. If I do Ctrl+backspace to delete the "def" text, and then click the button: Chromium and WebKit both show <div>abc&nbsp;</div>, whereas Firefox shows <div>abc </div>

(Also: using this testcase in Firefox, if I simply press backspace 3 times instead of doing Ctrl+Backspace, then we give expected-results, and the button reveals that we've got this innerHTML with a trailing br element: <div>abc <br></div> and that trailing <br> seems to be sufficient to avoid this bug, probably because it prevents the whitespace from being at the very end of the element. Maybe we should be consistent about inserting that <br> when the user does Ctrl+backspace, too?)

Comment 4

[Daniel Holbert [:dholbert]]

ni=masayuki as the expert on editor code -- masayuki, I wonder if this is a known issue and/or if there's anything that can be done easily here, particularly given that this affects Gmail. Thanks!

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: semi-reference-case for testcase 1

Here's a sort-of-reference-case for testcase 1, where this works as-expected, one time at least.

I've just added a <br> element up-front, which is how Gmail manages to avoid tripping this bug in some of my Firefox profiles per bug 1925606 comment 5.

(However: in this reference case, we delete that <br> as soon as you start typing new characters into the textfield, so it's not a persistent fix here. Maybe gmail had a hackaround that made it persist somehow -- whether intentionally or unintentionally -- not sure.)

Comment 6

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

Oh, it looks like a bug of both layout and editor. I think that layout should not render the trailing white-space if it's collapsible even in editable text node. On the other hand, editor should insert a <br> when it becomes invisible.

(I think probably I filed the layout bug because I remember this issue.)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)

Created attachment 9432321 [details]
semi-reference-case for testcase 1
[...]
(However: in this reference case, we delete that <br> as soon as you start typing new characters into the textfield, so it's not a persistent fix here. Maybe gmail had a hackaround that made it persist somehow -- whether intentionally or unintentionally -- not sure.)

Aha! This "we delete that <br>" behavior seems to be a recent behavior-change, I think from bug 1925606 [EDIT: copypaste typo -- I meant "I think from bug 1923251".] So that's why we're starting to notice this issue a bit more on real-world web properties.

It sounds like that was an intended behavior-change, based on the part-2 commit message over there ("Make HTMLEditor clean up unnecessary padding line breaks when deleting something immediately before it"). But I suspect we didn't realize that these line breaks had a useful benefit in some cases, as they did in e.g. Gmail here.

Comment 8

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

Attachment: Bug 1925635 - part 1: Port `test_bug1094000.html` to WPT r=m_kato!

Depends on D225039

Comment 9

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

Attachment: Bug 1925635 - part 2: Port `test_bug1355792.html` to WPT r=m_kato!

Depends on D230360

Comment 10

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

Attachment: Bug 1925635 - part 3: Add some WPTs to check after deleting selected content across table cells r=m_kato!

Depends on D230361

Comment 11

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

Attachment: Bug 1925635 - part 4: Make `HTMLEditor::AutoDeleteRangesHandler::Run` and its callees handle some processes directly immediately after deleting content r=m_kato!

Currently, the post-processor,
HTMLEditor::OnEndHandlingTopLevelEditSubActionInternal, deletes unnecessary
new empty inline ancestors and inserts a padding <br> at caret point.

The cause of the reported bug, <br> is not inserted properly when deleting the
last word of a block with word deletion, is hard to understand because the
centralizing the post-processes approach makes its code complicated because
it requires to check (or guess) a lot of situations and the changes of deletion
handlers harder.

Therefore, this patch makes each delete handler handles these post-processes
immediately if and only if it's preparation of inserting content.

Especially, AutoBlockElementsJoiner::HandleDeleteNonCollapsedRange becomes
doing them in multiple places. However, it fixed a lot of existing bugs.

Depends on D230362

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3433a2d6cbcd
part 1: Port `test_bug1094000.html` to WPT r=m_kato
https://hg.mozilla.org/integration/autoland/rev/b9ee5a52e135
part 2: Port `test_bug1355792.html` to WPT r=m_kato
https://hg.mozilla.org/integration/autoland/rev/37e3129c7a6a
part 3: Add some WPTs to check after deleting selected content across table cells r=m_kato
https://hg.mozilla.org/integration/autoland/rev/9cef3db4f874
part 4: Make `HTMLEditor::AutoDeleteRangesHandler::Run` and its callees handle some processes directly immediately after deleting content r=m_kato

Comment 13

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

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

Comment 14

[Atila Butkovits]

Backed out for causing failures at test_editor.xhtml.

Backout link: https://hg.mozilla.org/integration/autoland/rev/47ba45802ce02c53a87b5df103afe118ab266c30

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=9cef3db4f874203d2a55ee8e67771b7b771c475d&selectedTaskRun=UCVWaLxmSEeQ_lUbxhZsAw.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=485998198&repo=autoland&lineNumber=5136

Comment 15

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

Upstream PR was closed without merging

Comment 16

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

Well, the test starts editing with the following state:

<body><pre>[Test text]</pre><br></body>

Then, "cut" should delete the selected text node and new empty <pre> should have <br>. However, after applying the patch, the <pre> is replaced with <br>. This is unexpected behavior change, so, it detects a bug of the last patch.

Comment 17

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

Err, no, the <pre> is removed only by in the plaintext email mode. Before applying the patch, the <pre> is removed, but after applying the patch, the <pre> is replaced with a <br>.

Comment 18

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

Okay, I got it. At here, <pre> has no child before applying the patch but there is a <br> after applying the patch. Therefore, this will insert a <br>.

Comment 19

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

Attachment: Bug 1925635 - part 5: Make delete handlers call `DeleteMostAncestorMailCiteElementIfEmpty` before calling `InsertPaddingBRElementIfNeeded` r=m_kato!

Currently, HTMLEditor::DeleteMostAncestorMailCiteElementIfEmpty is called
immediately before finishing everything of HTMLEditor::HandleDeleteSelection.

The method deletes most ancestor mailcite element if it becomes completely
empty, but if it becomes only having a <br>, it replaces the mailcite with
with a <br>. So, inserting a padding line break is now inserted before
it's called so that the result has unexpectedly been changed.

This patch makes each InsertPaddingBRElementIfNeeded caller calls
DeleteMostAncestorMailCiteElementIfEmpty to keep current behavior.
Unfortunately, there is no test about this behavior in various cases.

Depends on D230363

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11a086d0c87e
part 1: Port `test_bug1094000.html` to WPT r=m_kato
https://hg.mozilla.org/integration/autoland/rev/5686a6d37143
part 2: Port `test_bug1355792.html` to WPT r=m_kato
https://hg.mozilla.org/integration/autoland/rev/a7a52d10d111
part 3: Add some WPTs to check after deleting selected content across table cells r=m_kato
https://hg.mozilla.org/integration/autoland/rev/49011eccede4
part 4: Make `HTMLEditor::AutoDeleteRangesHandler::Run` and its callees handle some processes directly immediately after deleting content r=m_kato
https://hg.mozilla.org/integration/autoland/rev/dbf5ed920623
part 5: Make delete handlers call `DeleteMostAncestorMailCiteElementIfEmpty` before calling `InsertPaddingBRElementIfNeeded` r=m_kato

Comment 21

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

Attachment: Bug 1925635 - part 6: Follow up patch against bug 1935698

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a912704bc24
part 6: Follow up patch against bug 1935698 CLOSED TREE

Comment 23

[amarc]

https://hg.mozilla.org/mozilla-central/rev/11a086d0c87e
https://hg.mozilla.org/mozilla-central/rev/5686a6d37143
https://hg.mozilla.org/mozilla-central/rev/a7a52d10d111
https://hg.mozilla.org/mozilla-central/rev/49011eccede4
https://hg.mozilla.org/mozilla-central/rev/dbf5ed920623
https://hg.mozilla.org/mozilla-central/rev/3a912704bc24

Comment 24

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

Upstream PR merged by moz-wptsync-bot

Comment 25

[Daniel Bodea [:danibodea]]

I can reproduce this issue in Release v134.0.1 and verify the fix in Beta v135.0b4 and Nightly v136.0a1. This verification was performed on Windows 10, MacOS 11 and Ubuntu 22. Closing as varified; thank you.