2007618 - Caret position confused after backspace delete, leading to erroneous further deletions (part 2)

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

Oddly, we still reproduce the bug in attachment 9532874 [details] even though the similar tests in bug 2005895 pass.

Original report:

This happening in Firefox 140 ESR, but also current Nightly.

STR:
Open the attached HTML document.
Place the cursor into the third empty line.
Press backspace, observe the cursor position, press backspace again.

Actual:
After the first backspace the caret is at the beginning of the second line. (When run in Thunderbird, the cursor disappears completely.)
After the second backspace, the full stop (.) at the end of the second line is deleted.

Variation:
Open the attached HTML document.
Place the cursor into the third empty line.
Press backspace, observe the cursor position, type a letter.
Again, after the first backspace, the cursor location is confused, typing a letter goes back to the third line.

Expected:
After the first backspace, the caret should be at the end of the second line.

Alice, is this a regression? This erroneous behaviour seems new.

Comment 1

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

Okay, the remaining issue is the case that a white-space only Text is there between the paragraphs like:

<p>abc<br> </p> <p>{}<br></p>

Comment 2

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

Attachment: Bug 2007618 - Make `AutoEmptyBlockAncestorDeleter::GetNewCaretPosition` ignore invisible `Text` nodes r=m_kato!

The remaining case of bug 2005895 is, there is a invisible Text
between paragraphs like:

<p>abc</p> <p>{}<br></p>

The reason is, HTMLEditUtils::GetPreviousContent is not called with
WalkTreeOption::IgnoreWhiteSpaceOnlyText. Therefore, I tried to
add the option in AutoEmptyBlockAncestorDeleter::GetNewCaretPosition.
However, the handling is odd [1]:

  static bool IsContentIgnored(const nsIContent& aContent,
                               const WalkTreeOptions& aOptions) {

<snip>

    if (aOptions.contains(WalkTreeOption::IgnoreWhiteSpaceOnlyText) &&
        aContent.IsText() &&
        const_cast<Text*>(aContent.AsText())->TextIsOnlyWhitespace()) {
      return true;
    }

So, whether the Text is actually visible or not is not tested.
Therefore, this does not work with preformatted content. However, I'd
like to uplift this to ESR140 so that it's too risky to change the
extant HTMLEditUtils implementation. Therefore, this patch makes
GetNewCaretPosition ignore invisible Text nodes directly.

1. https://searchfox.org/firefox-main/rev/20a1fb35a4d5c2f2ea6c865ecebc8e4bee6f86c9/editor/libeditor/HTMLEditUtils.h#3268-3269,3279-3282

Comment 3

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

FYI: HTMLEditUtils::IsContentIgnored will be removed in bug 1996500.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/05c50f4921e3
https://hg.mozilla.org/integration/autoland/rev/e2aa416202e6
Make `AutoEmptyBlockAncestorDeleter::GetNewCaretPosition` ignore invisible `Text` nodes r=m_kato

Comment 5

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

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

Comment 6

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

FYI: I'll request uplifts tomorrow.

Comment 7

[Cosmin Sabou [:CosminS]]

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

Comment 8

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

Tesing:

• beta: https://treeherder.mozilla.org/jobs?repo=try&revision=46b08f21dfe618cc7ca970a8238f3ae95da52891
• esr140: https://treeherder.mozilla.org/jobs?repo=try&revision=c847aab5a895b76ed22f3741edd0fdea0ab4348b

Comment 9

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

Attachment: Bug 2007618 - Make `AutoEmptyBlockAncestorDeleter::GetNewCaretPosition` ignore invisible `Text` nodes

The remaining case of bug 2005895 is, there is a invisible Text
between paragraphs like:

<p>abc</p> <p>{}<br></p>

The reason is, HTMLEditUtils::GetPreviousContent is not called with
WalkTreeOption::IgnoreWhiteSpaceOnlyText. Therefore, I tried to
add the option in AutoEmptyBlockAncestorDeleter::GetNewCaretPosition.
However, the handling is odd [1]:

  static bool IsContentIgnored(const nsIContent& aContent,
                               const WalkTreeOptions& aOptions) {

<snip>

    if (aOptions.contains(WalkTreeOption::IgnoreWhiteSpaceOnlyText) &&
        aContent.IsText() &&
        const_cast<Text*>(aContent.AsText())->TextIsOnlyWhitespace()) {
      return true;
    }

So, whether the Text is actually visible or not is not tested.
Therefore, this does not work with preformatted content. However, I'd
like to uplift this to ESR140 so that it's too risky to change the
extant HTMLEditUtils implementation. Therefore, this patch makes
GetNewCaretPosition ignore invisible Text nodes directly.

1. https://searchfox.org/firefox-main/rev/20a1fb35a4d5c2f2ea6c865ecebc8e4bee6f86c9/editor/libeditor/HTMLEditUtils.h#3268-3269,3279-3282

Original Revision: https://phabricator.services.mozilla.com/D277498

Comment 10

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: If the initial content of contenteditable or designMode is created with source file, paragraphs may be separated with whitespaces like:

<p>
foo
</p>
<p>
<br>
</p>

(the line break after the first </p>). In this case, Backspace from the second paragraph does not work. This may block developers to write tests, etc.

• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: This patch just does: when considering the new caret position after deleting empty paragraph, this patch makes the code ignore the invisible Text node and look for another leaf node again.
• String changes made/needed: no
• Is Android affected?: yes

Comment 11

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

Attachment: Bug 2007618 - Make `AutoEmptyBlockAncestorDeleter::GetNewCaretPosition` ignore invisible `Text` nodes

The remaining case of bug 2005895 is, there is a invisible Text
between paragraphs like:

<p>abc</p> <p>{}<br></p>

The reason is, HTMLEditUtils::GetPreviousContent is not called with
WalkTreeOption::IgnoreWhiteSpaceOnlyText. Therefore, I tried to
add the option in AutoEmptyBlockAncestorDeleter::GetNewCaretPosition.
However, the handling is odd [1]:

  static bool IsContentIgnored(const nsIContent& aContent,
                               const WalkTreeOptions& aOptions) {

<snip>

    if (aOptions.contains(WalkTreeOption::IgnoreWhiteSpaceOnlyText) &&
        aContent.IsText() &&
        const_cast<Text*>(aContent.AsText())->TextIsOnlyWhitespace()) {
      return true;
    }

So, whether the Text is actually visible or not is not tested.
Therefore, this does not work with preformatted content. However, I'd
like to uplift this to ESR140 so that it's too risky to change the
extant HTMLEditUtils implementation. Therefore, this patch makes
GetNewCaretPosition ignore invisible Text nodes directly.

1. https://searchfox.org/firefox-main/rev/20a1fb35a4d5c2f2ea6c865ecebc8e4bee6f86c9/editor/libeditor/HTMLEditUtils.h#3268-3269,3279-3282

Original Revision: https://phabricator.services.mozilla.com/D277498

Comment 12

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: If the initial content of contenteditable or designMode is created with source file, paragraphs may be separated with whitespaces like:

<p>
foo
</p>
<p>
<br>
</p>

(the line break after the first </p>). In this case, Backspace from the second paragraph does not work. This may block developers to write tests, etc.

NOTE: Unfortunately, the white-space:pre cases do not pass in ESR140. It must have been fixed in 141 or later, but not uplifted. Anyway, the case is not important because of non-realistic case.

• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: This patch just does: when considering the new caret position after deleting empty paragraph, this patch makes the code ignore the invisible Text node and look for another leaf node again.
• String changes made/needed: no
• Is Android affected?: yes

Comment 13

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

Upstream PR merged by moz-wptsync-bot

Comment 14

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/bd7102ab9db3
https://hg.mozilla.org/releases/mozilla-beta/rev/62e9d36cb0e8

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/83c5f148c99a

Comment 16

[Lorand Janos]

Attachment: Nightly148.mp4

I've verified on the latest Nightly build (148.0a1 from 2025-12-29) using a Samsung Galaxy S25 Ultra with the following result:

• On both versions of verification, the caret remains on the same third line after the first backspace, but unlike before when on the second backspace it jumped to a line between the third and the second, now after the second backspace, the caret is on the end of the second line.
• In case of the second version, after deleting the letter with the backspace, the next backspace gets the caret to the end of the second line.

Marking the ticket as verified on 148.

Comment 17

[Lorand Janos]

Attachment: 147Beta8.mp4

As this fix appeared among the uplifted bugs for Beta 8, I did check the behavior on the latest Beta as well, giving that the uplift appeared 2 days ago, and Beta 8 appeared 10 hours ago. But it seems like the behavior is still the same as before. Should this be checked on Beta 9? Thank you!

Comment 18

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

That's wired. I cannot reproduce this bug anymore even on Android within Nightly (with ATOK Pro nor Gboard). If you don't use wrong build to check, I think you find another bug.

Comment 19

[Lorand Janos]

Attachment: Edited_20251230_102822.mp4

Re-verified on the latest Nightly build (148.0a1 from 2025-12-29) and on the latest Beta (147.0b8) build.

Devices used:

• Samsung Galaxy Note 10 (Android 12);
• OnePlus 13R (Android 15);
• Google Pixel 7 (Android 16).

Marking the ticket as verified on 147 and 148.