1993351 - Caret position gets lost (moved to end) when textarea frame is reconstructed, after deleting the first character

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P2)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:

1. Load attached testcase.
2. Click the very start of the textarea, and press "Delete" on your keyboard to delete that first character.
   (Alternately, you can click just after that first character and press Backspace; that has the same outcome for me.)
3. Watch the caret (vertical bar) very carefully, when the textarea changes to cyan.

ACTUAL RESULTS:
Caret jumps to the end of the textarea.

EXPECTED RESULTS:
Caret should not jump.

If I repeat the STR but I delete a character in the middle of the text, then the caret does not jump. So there's something special about the situation when the caret is at the very start here.

(I'm not sure offhand if this belongs in layout or DOM:Editor; I don't know offhand where we save the caret/selection state to restore after frame reconstruction. Starting in DOM:Editor but feel free to reclassify as-appropriate.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screencast of bug reproducing when I delete first character, vs. not-reproducing when I delete a character in the middle

Comment 2

[Emilio Cobos Álvarez [:emilio]]

DOM: Selection?

But also we should try to do better than reframing for white-space changes...

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)

But also we should try to do better than reframing for white-space changes...

Fair point. That's somewhat orthogonal to the caret-position-getting-lost-when-we-do-have-to-reframe [for whatever reason], though, so I spun off bug 1993374 on that.

Comment 4

[Daniel Holbert [:dholbert]]

Regression range:
INFO: Last good revision: 7a6d6b986a1ed263db6d19ecbd56e2f4210870cf (2020-12-10)
INFO: First bad revision: 1130661c79c222fb1acd29b7ec5dc5202cdd0d2d (2020-12-11)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7a6d6b986a1ed263db6d19ecbd56e2f4210870cf&tochange=1130661c79c222fb1acd29b7ec5dc5202cdd0d2d

Suspected regressor (only one to mention selection):
https://hg-edge.mozilla.org/mozilla-central/rev/2a7e04756c232eb775140500ef2d370fa608eb50
Kagami Sascha Rosylight — Bug 1681615 - Detect selection changes through the setters r=masayuki

saschanaz, could you take a look here? This is causing a papercut on google.com (see bug 1985376).

Comment 5

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

Set release status flags based on info from the regressing bug 1681615

Comment 6

[Kagami Rosylight [:saschanaz] (they/them) (inactive until 2025-11-14)]

xul.dll!mozilla::TextControlState::SelectionProperties::SetIsDirty() Line 382 (d:\firefox\obj-aarch64-pc-windows-msvc\dist\include\mozilla\TextControlState.h:382)
xul.dll!mozilla::TextControlState::SetValueWithoutTextEditor(mozilla::AutoTextControlHandlingState & aHandlingSetValue) Line 2975 (d:\firefox\dom\html\TextControlState.cpp:2975)
xul.dll!mozilla::TextControlState::SetValue(const nsTSubstring<char16_t> & aValue, const nsTSubstring<char16_t> * aOldValue, const mozilla::EnumSet<mozilla::TextControlState::ValueSetterOption,unsigned int> & aOptions) Line 2658 (d:\firefox\dom\html\TextControlState.cpp:2658)
xul.dll!mozilla::TextControlState::SetValue(const nsTSubstring<char16_t> & aValue, const mozilla::EnumSet<mozilla::TextControlState::ValueSetterOption,unsigned int> & aOptions) Line 288 (d:\firefox\obj-aarch64-pc-windows-msvc\dist\include\mozilla\TextControlState.h:288)
xul.dll!mozilla::TextControlState::UnbindFromFrame(nsTextControlFrame * aFrame) Line 2441 (d:\firefox\dom\html\TextControlState.cpp:2441)
xul.dll!mozilla::dom::HTMLTextAreaElement::UnbindFromFrame(nsTextControlFrame * aFrame) Line 218 (d:\firefox\dom\html\HTMLTextAreaElement.cpp:218)
xul.dll!nsTextControlFrame::Destroy(mozilla::FrameDestroyContext & aContext) Line 130 (d:\firefox\layout\forms\nsTextControlFrame.cpp:130)
xul.dll!nsBlockFrame::DoRemoveFrame(mozilla::FrameDestroyContext & aContext, nsIFrame * aDeletedFrame, unsigned int aFlags) Line 7285 (d:\firefox\layout\generic\nsBlockFrame.cpp:7285)
xul.dll!nsBlockFrame::RemoveFrame(mozilla::FrameDestroyContext & aContext, mozilla::FrameChildListID aListID, nsIFrame * aOldFrame) Line 6438 (d:\firefox\layout\generic\nsBlockFrame.cpp:6438)
xul.dll!nsFrameManager::RemoveFrame(mozilla::FrameDestroyContext & aContext, mozilla::FrameChildListID aListID, nsIFrame * aOldFrame) Line 123 (d:\firefox\layout\base\nsFrameManager.cpp:123)
xul.dll!nsCSSFrameConstructor::ContentWillBeRemoved(nsIContent * aChild, nsCSSFrameConstructor::RemoveFlags aFlags) Line 7560 (d:\firefox\layout\base\nsCSSFrameConstructor.cpp:7560)
xul.dll!nsCSSFrameConstructor::RecreateFramesForContent(nsIContent * aContent, nsCSSFrameConstructor::InsertionKind aInsertionKind) Line 8476 (d:\firefox\layout\base\nsCSSFrameConstructor.cpp:8476)
xul.dll!mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList & aChangeList) Line 1687 (d:\firefox\layout\style\RestyleManager.cpp:1687)
xul.dll!mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags aFlags) Line 3301 (d:\firefox\layout\style\RestyleManager.cpp:3301)
xul.dll!mozilla::RestyleManager::ProcessPendingRestyles() Line 3390 (d:\firefox\layout\style\RestyleManager.cpp:3390)
xul.dll!mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush aFlush) Line 4622 (d:\firefox\layout\base\PresShell.cpp:4622)
xul.dll!mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush aType) Line 1512 (d:\firefox\obj-aarch64-pc-windows-msvc\dist\include\mozilla\PresShell.h:1512)
xul.dll!mozilla::dom::Document::DetermineProximityToViewportAndNotifyResizeObservers() Line 18576 (d:\firefox\dom\base\Document.cpp:18576)
xul.dll!nsRefreshDriver::Tick::<lambda>(mozilla::dom::Document & aDoc) Line 2515 (d:\firefox\layout\base\nsRefreshDriver.cpp:2515)
xul.dll!nsRefreshDriver::RunRenderingPhase<`lambda at D:\firefox\layout\base\nsRefreshDriver.cpp:2512:7'>::<lambda>() Line 1316 (d:\firefox\layout\base\nsRefreshDriver.cpp:1316)

The layout change calls TextControlState::UnbindFromFrame and then ultimately TextControlState::SetValueWithoutTextEditor, which internally branches based on if (!mValue.Equals(aHandlingSetValue.GetSettingValue())).

If I tap arrow-up key instead of delete in the repro, mValue doesn't change, which makes it enter the no-change branch, where it calls SetIsDirty. And later RestoreSelectionState::Run() checks the dirty state and calls mFrame->SetSelectionRange.

What's different with delete is that mValue does change, which makes in enter another branch, which does not set the mIsDirty bit because literally no selection change happened. That makes mFrame->SetSelectionRange not being called, which surprisingly... puts the cursor at the end, instead of the start? Why?

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Setting the cached selection as dirty when reframing (from UnbindFrame or so) makes sense to me fwiw. The DOM that's selected is going away after all. Does that work?

As for the start vs. end not sure, but we're recreating the anonymous dom so I wouldn't be surprised the handling gets a bit funky.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #6)

That makes mFrame->SetSelectionRange not being called, which surprisingly... puts the cursor at the end, instead of the start? Why?

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)

As for the start vs. end not sure, but we're recreating the anonymous dom so I wouldn't be surprised the handling gets a bit funky.

As a literal/mechanical answer for "why does the cursor get placed at the end": I observed in a pernosco trace for the Google webcompat bug that it's because we reach this line:
https://searchfox.org/firefox-main/rev/ab5bf2401717970560597083d6ac72915bde2d89/editor/libeditor/EditorBase.cpp#3704

CollapseSelectionTo(EditorRawDOMPoint(&aTextNode, aString.Length()), error);

(That's creating a point at position aString.Length(), i.e. at the end of the string.)

That might not be precisely what you were asking, but I'm just noting it as a fact/breadcrumb in case it's useful.

Comment 9

[Daniel Holbert [:dholbert]]

(It looks like we reach that line in a "good" pernosco run, too, though, so maybe it's just a question of how we use the resulting selection... not sure.)

In any case, if it's useful, here's a pernosco trace with this bug's reduced testcase, showing calls to nsCaret::CaretPositionFor, and focused on the first such call where we're requesting the position at the end rather than the start (aSelection->mOffset is 4 rather than 0):
https://pernos.co/debug/NNSdaVADx6L57dQ7Xt-GqQ/index.html#f{m[BTWK,CwmS_,t[2Q,ReY_,f{e[BTWK,CwmK_,s{ac7hLQAAA,bDfg,uGvWnIw,oGwZmew___/

Comment 10

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

Yep, EditorBase (TextEditor instance) collapse Selection to the end when a value is set. Then, TextControlState will adjust Selection if it's required. To make it run, TextControlState::UnbindFromFrame needs to mark the cached selection dirty because it needs to set new independent Selection to the same as cached last selection before the previous frame is destroyed.

I wonder, we need to manage whether we need to restore Selection and whether the selection has been changed with different flags?

Comment 11

[Kagami Rosylight [:saschanaz] (they/them) (inactive until 2025-11-14)]

Thanks! For now let's see whether that works.

Comment 12

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

Set release status flags based on info from the regressing bug 1681615

Comment 13

[Kagami Rosylight [:saschanaz] (they/them) (inactive until 2025-11-14)]

Attachment: Bug 1993351 - Set selection as dirty when unbinding from frame r=masayuki,emilio

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/11fd6a9e9ea7
https://hg.mozilla.org/integration/autoland/rev/cd8010c180f8
Set selection as dirty when unbinding from frame r=masayuki

Comment 15

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

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/9c5a10468e47
https://hg.mozilla.org/integration/autoland/rev/f21ba25a8f44
Revert "Bug 1993351 - Set selection as dirty when unbinding from frame r=masayuki" for causing wpt failures @selection-after-whitespace-change.html.

Comment 17

[agoloman]

Backed out for causing wpt failures @selection-after-whitespace-change.html.

• Backout link
• Push with failures
• Failure Log

Comment 18

[Kagami Rosylight [:saschanaz] (they/them) (inactive until 2025-11-14)]

We ignore Home in macOS, even for webdriver? Huh.

Comment 19

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

Upstream PR merged by moz-wptsync-bot

Comment 20

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

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #18)

We ignore Home in macOS, even for webdriver? Huh.

Ah, IIRC, Home, End keys on macOS are initialized by NativeKeyBindings and we don't have shortcut key definition. Let me check it...

Comment 21

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

Shortcut keys are not defined for Home and End on macOS and Linux.
https://searchfox.org/firefox-main/rev/04f343bf04ce512276a27788ef21f06e995b9496/dom/events/ShortcutKeyDefinitions.cpp#110-131

Hmm, but NativeKeyBindings initialize that for tests...
https://searchfox.org/firefox-main/rev/04f343bf04ce512276a27788ef21f06e995b9496/widget/cocoa/NativeKeyBindings.mm#528-549

Comment 22

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

Ah, they are just scroll to start/end instead of moving caret.
https://searchfox.org/firefox-main/rev/04f343bf04ce512276a27788ef21f06e995b9496/widget/cocoa/NativeKeyBindings.mm#181-182

Comment 23

[Kagami Rosylight [:saschanaz] (they/them) (inactive until 2025-11-14)]

On Chrome it works though, is it a potential webcompat issue by itself? 🤔

Comment 24

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

Yeah, could be, but our strategy is that we should prefer the same behavior as the native apps on the platform. So, we should change when the users complain about the same behavior as the native apps.

Comment 25

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

https://searchfox.org/firefox-main/rev/04f343bf04ce512276a27788ef21f06e995b9496/testing/web-platform/tests/editing/other/typing-around-link-element-at-collapsed-selection.tentative.html#159-163

(I wonder, how Safari behaves?)

Comment 26

[Kagami Rosylight [:saschanaz] (they/them) (inactive until 2025-11-14)]

Safari only scrolls it. Chrome does the same, but if it's single-line text control then it falls back to the line start/end navigation.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/dfa52bf95cd9
https://hg.mozilla.org/integration/autoland/rev/a0e6fc954079
Set selection as dirty when unbinding from frame r=masayuki

Comment 28

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

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

Comment 29

[Narcis Beleuzu [:NarcisB]]

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

Comment 30

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

Upstream PR merged by moz-wptsync-bot

Comment 31

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

The patch landed in nightly and beta is affected.
:saschanaz, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox145 to wontfix.

For more information, please visit BugBot documentation.

Comment 32

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: Users of google.com get unexpected text cursor behavior
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: See comment #0
• Risk associated with taking this patch: low
• Explanation of risk level: Limited behavior change in unbound frame case
• String changes made/needed: No
• Is Android affected?: yes

Comment 33

[Kagami Rosylight [:saschanaz] (they/them) (inactive until 2025-11-14)]

Attachment: Bug 1993351 - Set selection as dirty when unbinding from frame r=masayuki,emilio

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

Comment 34

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Unexpected text cursor behavior in google.com
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: See comment #0
• Risk associated with taking this patch: low
• Explanation of risk level: Limited behavior change in unbound frame case
• String changes made/needed: No
• Is Android affected?: yes

Comment 35

[Kagami Rosylight [:saschanaz] (they/them) (inactive until 2025-11-14)]

Attachment: Bug 1993351 - Set selection as dirty when unbinding from frame r=masayuki,emilio

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

Comment 36

[Peter Magyari (Desktop QA)]

Verified fixed using Nightly 146.0a1 (20251019211057) on Windows 10, MacOS 15 and Ubuntu 24.04.

Comment 37

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/06faecc45d20
https://hg.mozilla.org/releases/mozilla-beta/rev/c35c80ee91cd

Comment 38

[Peter Magyari (Desktop QA)]

Verified fixed using Beta 145.0b5 (20251022093404)

Comment 39

[Pascal Chevrel:pascalc]

Kagami, your patch does not apply cleanly to the ESR140 branch, could you update it please? Thanks