1785801 - Selections across blocks in contentEditable content can insert text in the wrong position

Status: RESOLVED FIXED

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

Description

[Marijn Haverbeke]

Attachment: test.html

When the text selection, in an editable element, starts at the end of a non-empty block, and continues into another block, text typed by the user will be inserted before the content of the first block. This only occurs if the start point of the selection points into the block itself, not when it points at the end of a text node inside of the block.

See attached page for an example. On load, the page will create an editable element with two blocks in it, and set the selection from the end of the first to the start of the second. Typing a 'b' will result in 'bac' instead of the expected 'abc'.

Comment 1

[Sean Feng [:sefeng]]

I think Masayuki knows how it works?

Comment 2

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

After joining the <div> elements, we get selection collapsed at start of the merged <div>. I guess that this is a new regression.

Comment 3

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

The regression range is: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cd1ca5184c73edfc4af351ad4c89ea994311625b&tochange=1eb0b24a4248fcec28864419fb71f1988748025d

Must be a regression of bug 1756933.

Comment 4

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

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

Comment 5

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

Ah, https://searchfox.org/mozilla-central/diff/b9c8d7886430adbb05b3cdd58153dfc8fec86d6c/editor/libeditor/HTMLEditor.cpp#4771

-   DebugOnly<nsresult> rvIgnored = RangeUpdaterRef().SelAdjJoinNodes(
-       EditorRawDOMPoint(&aRightContent, oldLeftNodeLen), aLeftContent,
-       atRightContent.Offset(), JoinNodesDirection::LeftNodeIntoRightNode);

but, now:

    DebugOnly<nsresult> rvIgnored = RangeUpdaterRef().SelAdjJoinNodes(
        EditorRawDOMPoint(&aContentToKeep, std::min(removingContentLength,
                                                    aContentToKeep.Length())),
        aContentToRemove, *removingContentIndex, GetJoinNodesDirection());

atRightContent was EditorDOMPoint atRightContent(&aRightContent), however, we use const Maybe<uint32_t> removingContentIndex = aContentToRemove.ComputeIndexInParentNode() now. Typically, the value becomes -1 than old code.

On the other hand, I named the argument as aOffsetOfRemovedContent. Perhaps, we should fix this in RangeUpdater::SelAdjJoinNodes.

Comment 6

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

I named the params with this patch. However, I didn't change the offset computation nor the caller side.

Comment 7

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

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

Comment 8

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

Attachment: Bug 1785801 - Make `RangeUpdater::SelAdjJoinNodes` take the ex-offset of right node r=m_kato!

In bug 1739524, I misunderstood the meaning of aOffset of SelAdjJoinNodes.

After joining 2 nodes, and a point points right node which will have ex-left
node content, the point needs to point ex-start of the right node to keep
next insertion point as-is. Therefore, it's not useful with new join nodes
direction, it needs to know the ex-offset of the right node.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/feedf26a9193
Make `RangeUpdater::SelAdjJoinNodes` take the ex-offset of right node r=m_kato

Comment 10

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

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

Comment 11

[Cristina Cozmuta (:CrissCozmuta)]

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

Comment 12

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

Upstream PR merged by moz-wptsync-bot

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting?

Comment 14

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

Thanks, I'll request to uplift the patch (but it could require new patch for ESR because I touched around there a lot).

Comment 15

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

Attachment: Bug 1785801 - Make `RangeUpdater::SelAdjJoinNodes` take the ex-offset of right node r=m_kato

In bug 1739524, I misunderstood the meaning of aOffset of SelAdjJoinNodes.

After joining 2 nodes, and a point points right node which will have ex-left
node content, the point needs to point ex-start of the right node to keep
next insertion point as-is. Therefore, it's not useful with new join nodes
direction, it needs to know the ex-offset of the right node.

This is a backport patch of https://phabricator.services.mozilla.com/D155438 for
ESR 102.

Comment 16

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

Comment on attachment 9291265 [details]
Bug 1785801 - Make RangeUpdater::SelAdjJoinNodes take the ex-offset of right node r=m_kato!

Beta/Release Uplift Approval Request

• User impact if declined: Editable web apps may be broken if web apps set selection by themselves. This is potentially risky if web apps want to put caret at start or end of an element.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Fixing a simple mistake and it's tested by the automated test.
• String changes made/needed:
• Is Android affected?: Yes

Comment 17

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

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9291265 [details]
Bug 1785801 - Make RangeUpdater::SelAdjJoinNodes take the ex-offset of right node r=m_kato!

Approved for 105.0b7.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e9dc9ef42fd3

Comment 20

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

Comment on attachment 9292784 [details]
Bug 1785801 - Make RangeUpdater::SelAdjJoinNodes take the ex-offset of right node r=m_kato

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This bug potentially causes breaking web apps which has contenteditable, a new regression from 99 and the regression reason is an easy mistake.
• User impact if declined: User may see odd insert text (etc) result if web apps manage Selection by themselves.
• Fix Landed on Version: 105, 106
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This fixes a simple mistake of the last patch of 1756933, that caused by my misunderstanding.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9292784 [details]
Bug 1785801 - Make RangeUpdater::SelAdjJoinNodes take the ex-offset of right node r=m_kato

Approved for 102.3esr.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/484dbd3856dc