Closed Bug 2014622 Opened 6 months ago Closed 1 month ago

`nsINode::GetFlattenedTreeParentNodeForSelection()` and `nsContentUtils::ComparePoints<TreeKind::FlatForSelection>()` work with nodes node in the flattened tree

Categories

(Core :: DOM: Core & HTML, defect)

defect

Tracking

()

RESOLVED FIXED
154 Branch
Tracking Status
firefox154 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(4 files)

It returns the parent node of a child even if the child is a child of shadow host.
https://searchfox.org/firefox-main/rev/133582f487ff8291ec10bd524db52db0b8ed363e/dom/base/nsIContentInlines.h#52,57,81-83,86,94-97

I guess this solved wrong selection/range handling issue, but this should not be required after fixing bug 2012637.

The original report including the summary may be wrong.

From Selection API point of view, nodes which are not part of the flattened tree are valid range boundaries since Selection API does not aware of shadow DOM. However, currently, we set the may-cross-shadow-boundary-range if and only if the boundaries can be converted to boundaries in the flattened tree. Otherwise, we fail to set the range.

On the other hand, we convert the points into the ShadowRoot in such case. However, this may cause wrong comparing result with another range boundary.

So, I think GetFlattenedTreeParentNodeForSelection() solves this issue with returning the shadow host element in such case even though it's wrong from the flattened tree point of view, but it must be the right thing.

I think we should explain this in the comment and add some tentative tests for the cases.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Summary: `nsINode::GetFlattenedTreeParentNodeForSelection()` should never return the parent node when it's a child of a shadow host and not assigned to any `<slot>` → `nsINode::GetFlattenedTreeParentNodeForSelection()` and `nsContentUtils::ComparePoints<TreeKind::FlatForSelection>()` work with nodes node in the flattened tree

Selection API accepts any DOM node in the non-flattened tree as a
container of a range boundary. Therefore, even if a node is not
formatted in the flattened tree, e.g., inclusive descendants of
unassigned children of a shadow host element or fallback content of
<slot> which has some assigned nodes, it can be a valid container of
selection range boundaries and valid argument of Selection API and
Range API.

Therefore, when climbing up the flattened tree for selection, the
internal APIs should treat the non-formatted nodes are also connected
to the flattened tree. In this patch, the non-formatted children are
connected after the attached shadow root or assigned nodes. The former
is defined as so in the shadow-including tree order [1]. However, I've
not found the definition for the latter. Therefore, I align the behavior
to the former behavior for making the code simpler.

When comparing the range boundaries in the flattened tree for selection
with the non-formatted nodes, we may need to use the position in the end
of the formatted node. However, currently, setting it as
nsRange::mCrossShadowBoundaryRange does not work well with some tests.
Therefore, this patch does not touch around that.

On the other hand, Chrome returns false from Selection.containsNode
if its node parameter is set to a non-formatted node. The spec [2]
defines with a fuzzy word, "visually". It's possible to understand that
non-formatted nodes never visually appear. And I tried to make it
compare with treating the positions of non-formatted nodes as end of
formatted ancestor element. However, it needs complicated code and
causes regressions in some tests, and anyway it's incompatible with
Chrome. Therefore, this patch makes Selection::ContainsNode returns
false if the given node is not formatted in the flattened tree.

  1. https://dom.spec.whatwg.org/#concept-shadow-including-tree-order
  2. https://w3c.github.io/selection-api/#dom-selection-containsnode

According to testing with simple cases, the intent of the shadow DOM
boundary crossing mode of ContentSubtreeIterator is to list up only
the formatted nodes in the flattened tree. I.e., neither unassigned
children of a shadow host element nor unused fallback content of a
<slot> should be listed up. However, the boundaries of
nsRange::mCrossShadowBoundaryRange may be in unformatted node. Then,
the comparison of the start or end container with the range in the
flattened tree will fail due to treated as disconnected. Thus, various
assertions will fail.

Therefore, this patch makes ContentSubtreeIterator stores the range
which starts from and end at formatted nodes. If the original range
boundaries are not in formatted nodes, it treats the range boundary as
collapsed at end of the ShadowRoot or <slot> which causes the node
is unformatted.

Additionally, ContentSubtreeIterator uses nsINode::GetFirstChild()
etc, which are for TreeKind::DOM. So, it may dive into unformatted
nodes. This patch makes the utility methods templated with TreeKind
and use the templated APIs or ChildIteratorBase to access the DOM
with valid APIs.

Fixing ContentSubtreeIterator bugs makes stop failing in some cases.
Then, AbstractRange::CollectClientRectsAndText() is changed and
Highlight::RangesAtPoint() starts appending ranges in a shadow
which is covered by an assigned node of a <slot> in the shadow since
selecting a <slot> succeeds the hit-testing in its assigned nodes.

The failing test is:
https://searchfox.org/firefox-main/rev/eec2f8dc61cc9f851fe5303cc05fc015d3128e6f/testing/web-platform/tests/shadow-dom/HighlightRegistry-highlightsFromPoint.html#170-174,176-181,185-187,197-201

It may converts a TreeKind::DOM point to a point whose container is
a non-flattened node which is a descendant of a shadow host child or
a <slot> in TreeKind::DOM. Therefore, the name should explain it
may return non-flattened node point.

Pushed by masayuki@d-toybox.com: https://github.com/mozilla-firefox/firefox/commit/5d0bbcfec88c https://hg.mozilla.org/integration/autoland/rev/874f68ac15dc part 1: Make `nsINode` API for `TreeKind::FlatForSelection` treat nodes not a part of flattened tree correctly for Selection API r=smaug,dom-core-reviewers https://github.com/mozilla-firefox/firefox/commit/03349e2f2e14 https://hg.mozilla.org/integration/autoland/rev/a4e05af4890e part 2: Reimplement `ContentSubtreeIterator` r=smaug,dom-core-reviewers https://github.com/mozilla-firefox/firefox/commit/3e49bd9b5f11 https://hg.mozilla.org/integration/autoland/rev/373123e888ab part 3: Fix the new WPT failure of `shadow-dom/HighlightRegistry-highlightsFromPoint.html` r=jjaschke https://github.com/mozilla-firefox/firefox/commit/176e99914642 https://hg.mozilla.org/integration/autoland/rev/cc4cdd329058 part 4: Rename `RangeBoundaryBase::AsRangeBoundaryInFlatTree()` r=smaug

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

Upstream PR merged by moz-wptsync-bot

Pushed by ctuns@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/6bd59cd533d1 https://hg.mozilla.org/mozilla-central/rev/0c06b2cb63aa Revert "Bug 2014622 - part 4: Rename `RangeBoundaryBase::AsRangeBoundaryInFlatTree()` r=smaug" https://github.com/mozilla-firefox/firefox/commit/fc96d178bed9 https://hg.mozilla.org/mozilla-central/rev/dcfaaafc17aa Revert "Bug 2014622 - part 3: Fix the new WPT failure of `shadow-dom/HighlightRegistry-highlightsFromPoint.html` r=jjaschke" https://github.com/mozilla-firefox/firefox/commit/982160f9b93d https://hg.mozilla.org/mozilla-central/rev/1698e74688c5 Revert "Bug 2014622 - part 2: Reimplement `ContentSubtreeIterator` r=smaug,dom-core-reviewers" https://github.com/mozilla-firefox/firefox/commit/d78bc8e4f908 https://hg.mozilla.org/mozilla-central/rev/3cc708f7b29b Revert "Bug 2014622 - part 1: Make `nsINode` API for `TreeKind::FlatForSelection` treat nodes not a part of flattened tree correctly for Selection API r=smaug,dom-core-reviewers"

Backed out for causing Bug 2052026

Status: RESOLVED → REOPENED
Flags: needinfo?(masayuki)
Resolution: FIXED → ---
Target Milestone: 154 Branch → ---

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

Upstream PR merged by moz-wptsync-bot

Status: REOPENED → ASSIGNED
Flags: needinfo?(masayuki)
Pushed by masayuki@d-toybox.com: https://github.com/mozilla-firefox/firefox/commit/3b33428f9286 https://hg.mozilla.org/integration/autoland/rev/1404f5a62772 part 1: Make `nsINode` API for `TreeKind::FlatForSelection` treat nodes not a part of flattened tree correctly for Selection API r=smaug,dom-core-reviewers https://github.com/mozilla-firefox/firefox/commit/88dc2ae7cf60 https://hg.mozilla.org/integration/autoland/rev/cb00e004db6f part 2: Reimplement `ContentSubtreeIterator` r=smaug,dom-core-reviewers https://github.com/mozilla-firefox/firefox/commit/d3710534b754 https://hg.mozilla.org/integration/autoland/rev/bc906b1d5fd1 part 3: Fix the new WPT failure of `shadow-dom/HighlightRegistry-highlightsFromPoint.html` r=jjaschke https://github.com/mozilla-firefox/firefox/commit/d3828ed70bfe https://hg.mozilla.org/integration/autoland/rev/9bc0b43fe364 part 4: Rename `RangeBoundaryBase::AsRangeBoundaryInFlatTree()` r=smaug

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

Upstream PR merged by moz-wptsync-bot

QA Whiteboard: [qa-triage-done-c155/b154]
Regressions: 2063757
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: