1623413 - Selection.collapse fails with NS_ERROR_FAILURE if called for two different editable blocks

Status: RESOLVED FIXED

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

Description

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

Attachment: selection-failure.html

Loading the attached file throws with NS_ERROR_FAILURE.

Comment 1

[Mirko Brodesser (:mbrodesser-Igalia)]

Interesting, this indeed only happens when there are two divs. For the error to occur, it suffices if the first div is contenteditable. Works on Chrome.

Comment 2

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

!frameSelection->IsValidSelectionPoint(aPoint.Container()) check fails inside Selection::Collapse().

Internally, the first collapse() call gets null from GetAncestorLimit() while the second call gets the parent div (the grandparent of the text node bar) and then fails aNode->IsInclusiveDescendantOf(limiter) check.

Not sure what an ancestor limit is here, could you give me a thought?

Comment 3

[Mirko Brodesser (:mbrodesser-Igalia)]

(In reply to Kagami :saschanaz from comment #2)

!frameSelection->IsValidSelectionPoint(aPoint.Container()) check fails inside Selection::Collapse().

Internally, the first collapse() call gets null from GetAncestorLimit() while the second call gets the parent div (the grandparent of the text node bar) and then fails aNode->IsInclusiveDescendantOf(limiter) check.

Not sure what an ancestor limit is here, could you give me a thought?

The idea seems to be, that the ancestor-limiter limits a Selection to the ancestor-limiter's descendants (https://searchfox.org/mozilla-central/rev/6cd54550a27e2f6ca0755a25328f769e41e524f4/layout/generic/nsFrameSelection.h#892-893). However, the comment in the code has to be taken with a grain of salt, because it's potentially outdated.

Comment 4

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

Weird that this bug doesn't happen when refreshing the page with the focus on the browser chrome. GetAncestorLimit() returns null for both <div> and passes IsValidSelectionPoint().

The comment of IsValidSelectionPoint says:

In the case of NO limiter all points are valid since you are in a topmost iframe. (browser or composer)

But in this case a <div> gets no limiter. Is this expected?

Comment 5

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

Well, navigation (e.g., ArrowDown key press) should be limited in the ancestor limit, but it might be better Selection API calls to ignore it especially when the caller is content script. If Selection allows any callers to ignore it, I guess that editor would be broken. For example, editor would collapse selection to outside of active editing host and that causes focus move.

(In reply to Kagami :saschanaz from comment #4)

Weird that this bug doesn't happen when refreshing the page with the focus on the browser chrome. GetAncestorLimit() returns null for both <div> and passes IsValidSelectionPoint().

The comment of IsValidSelectionPoint says:

In the case of NO limiter all points are valid since you are in a topmost iframe. (browser or composer)

But in this case a <div> gets no limiter. Is this expected?

Well, not expected, but I guess that the reason is focus event is not fired on non-focused DOM window:
https://searchfox.org/mozilla-central/rev/202a285024f174c2d2bf2152d9cba90a03723eab/dom/base/nsFocusManager.cpp#1454-1455,1459-1460,1464-1470,1517-1519

Comment 6

[Firefox Bug Husbandry Bot]

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Comment 8

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

This keeps happening whenever I write test cases...

Comment 9

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

Attachment: Bug 1623413 - Part 1: Skip selection point check when called from JS r=masayuki

Comment 10

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

Attachment: Bug 1623413 - Part 2: Rename Collapse() to CollapseInLimiter() r=masayuki

Depends on D88702

Comment 11

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

Attachment: Bug 1623413 - Part 3: Skip limiter check in CollapseToStart/End r=masayuki

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0eef32d6a454
Part 1: Skip selection point check when called from JS r=masayuki
https://hg.mozilla.org/integration/autoland/rev/75ed1b8a5c67
Part 2: Rename Collapse() to CollapseInLimiter() r=masayuki
https://hg.mozilla.org/integration/autoland/rev/d3d67293f115
Part 3: Skip limiter check in CollapseToStart/End r=masayuki

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/25370 for changes under testing/web-platform/tests

Comment 14

[Alexandru Michis [:malexandru]]

Backed out 3 changesets (Bug 1623413) for causing failures in Selection_collapse.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/271f918d32bf8e4fb80065b37eeeb647e97e527b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cweb%2Cplatform%2Ctests%2Ctest-linux1804-64%2Fdebug-web-platform-tests-e10s%2Cwpt2&tochange=ae2a56ca44dc538b7d3a3ade4722d00174b23a2f&fromchange=234caec324644beec0bb6fbc8fa1b1467807b4bf&selectedTaskRun=YcDJxzaSRbWD4uDL5qX7Ng.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=314761630&repo=autoland&lineNumber=2168

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f419d6b635a
Part 1: Skip selection point check when called from JS r=masayuki
https://hg.mozilla.org/integration/autoland/rev/5d57d36010c1
Part 2: Rename Collapse() to CollapseInLimiter() r=masayuki
https://hg.mozilla.org/integration/autoland/rev/aa032cbc9455
Part 3: Skip limiter check in CollapseToStart/End r=masayuki

Comment 16

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/6f419d6b635a
https://hg.mozilla.org/mozilla-central/rev/5d57d36010c1
https://hg.mozilla.org/mozilla-central/rev/aa032cbc9455

Comment 17

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

Upstream PR merged by moz-wptsync-bot