Bug 218007 - REGRESSION(r266295): DOMSelection's addRange and containsNode behave incorrectly when selection crosses shadow boundaries
Summary: REGRESSION(r266295): DOMSelection's addRange and containsNode behave incorrec...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on: 215755
Blocks:
  Show dependency treegraph
 
Reported: 2020-10-20 21:37 PDT by Ryosuke Niwa
Modified: 2020-10-23 18:16 PDT (History)
16 users (show)

See Also:


Attachments
test (822 bytes, text/html)
2020-10-20 21:37 PDT, Ryosuke Niwa
no flags Details
Patch (30.06 KB, patch)
2020-10-22 16:39 PDT, Darin Adler
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (33.20 KB, patch)
2020-10-22 16:56 PDT, Darin Adler
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (33.37 KB, patch)
2020-10-23 09:08 PDT, Darin Adler
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2020-10-20 21:37:20 PDT
Created attachment 411957 [details]
test

After https://trac.webkit.org/changeset/266295, addRange and containsNode of DOMSelection
behaves incorrectly when the selection crosses shadow boundaries.
Comment 1 Darin Adler 2020-10-21 08:52:02 PDT
Oh, maybe I have this correct in the "live range" mode but incorrect in the old mode.
Comment 2 Darin Adler 2020-10-21 09:46:08 PDT
Half of this is a bug in the super-crazy addRange implementation we have currently, not in the standards-compliant one that is behind the "live range selection enabled" flag.

The other half is a bug in containsNode that is unconditionally wrong.
Comment 3 Darin Adler 2020-10-21 10:04:51 PDT
I’m confused about the addRange half of this. It doesn’t seem like you can use the Selection API to create a selection that crosses shadow boundaries, unless there is a bug. For example, setBaseAndExtent is not supposed to do anything if either node has a root that is not the document.

I get lost in the attached test case. Doesn’t the specification say that calling this function should do nothing?

    getSelection().setBaseAndExtent(document.body, 0, document.querySelector('b[slot=second]').firstChild, 5);
Comment 4 Ryosuke Niwa 2020-10-21 16:36:07 PDT
(In reply to Darin Adler from comment #3)
> I’m confused about the addRange half of this. It doesn’t seem like you can
> use the Selection API to create a selection that crosses shadow boundaries,
> unless there is a bug. For example, setBaseAndExtent is not supposed to do
> anything if either node has a root that is not the document.
> 
> I get lost in the attached test case. Doesn’t the specification say that
> calling this function should do nothing?
> 
>     getSelection().setBaseAndExtent(document.body, 0,
> document.querySelector('b[slot=second]').firstChild, 5);

No, this is saying that we're setting selection from the (body,0) to (#text, 5) where #text is the first child of b element that gets assigned to a slot. Because b is in the same tree as body, this is totally valid. In the flat tree, it gets moved into inside the slot inside the shadow tree but that's irrelevant for the purpose of determining what's allowed / disallowed in DOM API.
Comment 5 Darin Adler 2020-10-22 16:30:37 PDT
(In reply to Ryosuke Niwa from comment #4)
> No, this is saying that we're setting selection from the (body,0) to (#text,
> 5) where #text is the first child of b element that gets assigned to a slot.
> Because b is in the same tree as body, this is totally valid. In the flat
> tree, it gets moved into inside the slot inside the shadow tree but that's
> irrelevant for the purpose of determining what's allowed / disallowed in DOM
> API.

Why do we have to use such a complicated example? What makes this critical to reproducing the bug?
Comment 6 Darin Adler 2020-10-22 16:39:20 PDT
Created attachment 412143 [details]
Patch
Comment 7 Darin Adler 2020-10-22 16:56:32 PDT
Created attachment 412145 [details]
Patch
Comment 8 Darin Adler 2020-10-23 09:03:56 PDT
Patch is finished and 100% ready to review. Failure is just because I didn’t regenerate the expected.txt file and it’s missing a PASS line.
Comment 9 Darin Adler 2020-10-23 09:08:33 PDT
Created attachment 412185 [details]
Patch
Comment 10 Ryosuke Niwa 2020-10-23 10:35:57 PDT
Comment on attachment 412185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412185&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:965
> -        if (isPointInRange(*range, makeBoundaryPoint(end)))
> +        if (contains<ComposedTree>(*range, makeBoundaryPoint(end)))

I think this code will look better if it were range->contains<ComposedTree>(makeBoundaryPoint(end)) instead.

> Source/WebCore/dom/SimpleRange.h:72
> +template<TreeType> bool contains(const SimpleRange&, const Node&);
> +
> +template<TreeType> bool intersects(const SimpleRange&, const SimpleRange&);
> +template<TreeType> bool intersects(const SimpleRange&, const Node&);

I think these two functions are probably better off being member functions of SimpleRange.

> LayoutTests/editing/selection/selections-across-trees.html:27
> +

Can we also check that focusNode is still nodeInSlot?
Comment 11 Darin Adler 2020-10-23 12:32:47 PDT
Comment on attachment 412185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412185&action=review

>> LayoutTests/editing/selection/selections-across-trees.html:27
>> +
> 
> Can we also check that focusNode is still nodeInSlot?

I do check that above.

But here, the test selects all the children of the body, so focusNode will *not* be nodeInSlot.

What are you asking for?
Comment 12 Darin Adler 2020-10-23 12:34:53 PDT
Comment on attachment 412185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412185&action=review

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:965
>> +        if (contains<ComposedTree>(*range, makeBoundaryPoint(end)))
> 
> I think this code will look better if it were range->contains<ComposedTree>(makeBoundaryPoint(end)) instead.

I guess it’s a matter of taste. I don’t think we should add member functions to SimpleRange, even though it would make the syntax nicer here. There are advantages to free functions vs. member functions in C++, like being able to convert the first argument’s type.

>> Source/WebCore/dom/SimpleRange.h:72
>> +template<TreeType> bool intersects(const SimpleRange&, const Node&);
> 
> I think these two functions are probably better off being member functions of SimpleRange.

OK, same debate.

I’m open to eventually changing this, but please consider that we can also have intersects functions overloaded for all sorts of different combinations.
Comment 13 Darin Adler 2020-10-23 13:23:35 PDT
I’m going to land, and address Ryosuke’s comments as follow ups once I understand better what he’s looking for.
Comment 14 Darin Adler 2020-10-23 14:04:46 PDT
Committed r268940: <https://trac.webkit.org/changeset/268940>
Comment 15 Radar WebKit Bug Importer 2020-10-23 14:05:20 PDT
<rdar://problem/70633710>
Comment 16 Ryosuke Niwa 2020-10-23 16:55:32 PDT
(In reply to Darin Adler from comment #12)
> Comment on attachment 412185 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412185&action=review
> 
> >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:965
> >> +        if (contains<ComposedTree>(*range, makeBoundaryPoint(end)))
> > 
> > I think this code will look better if it were range->contains<ComposedTree>(makeBoundaryPoint(end)) instead.
> 
> I guess it’s a matter of taste. I don’t think we should add member functions
> to SimpleRange, even though it would make the syntax nicer here. There are
> advantages to free functions vs. member functions in C++, like being able to
> convert the first argument’s type.

Sure, one thing to note is though that this will make it harder for people to find where these functions are declared / defined. Maybe we can add Source/WebCore/dom/Contains.h?
Comment 17 Ryosuke Niwa 2020-10-23 16:57:49 PDT
(In reply to Darin Adler from comment #11)
> Comment on attachment 412185 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412185&action=review
> 
> >> LayoutTests/editing/selection/selections-across-trees.html:27
> >> +
> > 
> > Can we also check that focusNode is still nodeInSlot?
> 
> I do check that above.
> 
> But here, the test selects all the children of the body, so focusNode will
> *not* be nodeInSlot.
> 
> What are you asking for?

Right, this is why in the original test I posted, I've added the following range instead:
const range = document.createRange();
range.setStart(shadowRoot.firstChild, 0);
range.setEnd(shadowRoot, 1);
getSelection().addRange(range);

What I'm saying that is that we should check that the selection doesn't get changed when we try to add a range within a shadow root.
Comment 18 Darin Adler 2020-10-23 17:00:30 PDT
Comment on attachment 412185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412185&action=review

>>>> LayoutTests/editing/selection/selections-across-trees.html:27
>>>> +
>>> 
>>> Can we also check that focusNode is still nodeInSlot?
>> 
>> I do check that above.
>> 
>> But here, the test selects all the children of the body, so focusNode will *not* be nodeInSlot.
>> 
>> What are you asking for?
> 
> Right, this is why in the original test I posted, I've added the following range instead:
> const range = document.createRange();
> range.setStart(shadowRoot.firstChild, 0);
> range.setEnd(shadowRoot, 1);
> getSelection().addRange(range);
> 
> What I'm saying that is that we should check that the selection doesn't get changed when we try to add a range within a shadow root.

That’s what this does:

    shouldBe("getSelection().setBaseAndExtent(document.body, 0, nodeInSlot, 5); selectionAddRange(shadowNode, 0, shadowRoot, 1); getSelection().focusNode", "nodeInSlot");

Are you asking me to add it? It’s already there!
Comment 19 Darin Adler 2020-10-23 17:02:03 PDT
Because:

    selectionAddRange(shadowNode, 0, shadowRoot, 1)

*is*

    const range = document.createRange();
    range.setStart(shadowRoot.firstChild, 0);
    range.setEnd(shadowRoot, 1);
    getSelection().addRange(range);

Because shadowNode is shadowRoot.firstChild, and selectionAddRange does what you said above.

And then it does:

    shouldBe(focusNode, nodeInSlot)

I used your test! Is there something more you want me to add?
Comment 20 Ryosuke Niwa 2020-10-23 17:05:43 PDT
(In reply to Darin Adler from comment #18)
> Comment on attachment 412185 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412185&action=review
> 
> >>>> LayoutTests/editing/selection/selections-across-trees.html:27
> >>>> +
> >>> 
> >>> Can we also check that focusNode is still nodeInSlot?
> >> 
> >> I do check that above.
> >> 
> >> But here, the test selects all the children of the body, so focusNode will *not* be nodeInSlot.
> >> 
> >> What are you asking for?
> > 
> > Right, this is why in the original test I posted, I've added the following range instead:
> > const range = document.createRange();
> > range.setStart(shadowRoot.firstChild, 0);
> > range.setEnd(shadowRoot, 1);
> > getSelection().addRange(range);
> > 
> > What I'm saying that is that we should check that the selection doesn't get changed when we try to add a range within a shadow root.
> 
> That’s what this does:
> 
>     shouldBe("getSelection().setBaseAndExtent(document.body, 0, nodeInSlot,
> 5); selectionAddRange(shadowNode, 0, shadowRoot, 1);
> getSelection().focusNode", "nodeInSlot");
> 
> Are you asking me to add it? It’s already there!

Ah, ok. Sorry, I missed selectionAddRange part.
Comment 21 Darin Adler 2020-10-23 17:50:14 PDT
(In reply to Ryosuke Niwa from comment #16)
> Sure, one thing to note is though that this will make it harder for people
> to find where these functions are declared / defined. Maybe we can add
> Source/WebCore/dom/Contains.h?

That’s an interesting point.

There is really a family of tree ordering/structure functions:

treeOrder
contains
intersects
intersection
unionRange (annoying name)
intersectingNodes
characterDataOffsetRange
containedNodes (to be added, maybe)
(a few more to come, likely matching concepts in DOM specifications)

I think that all of these kind of "go together". There are versions that work on nodes, boundary points, and ranges. They are sort of cousins of these:

parent
first/lastChild
next/previousSibling
contains
isDescendantOf
rootNode
document

I could lean more toward making everything member functions. Or maybe put many of these into a TreeStructure.h header?
Comment 22 Ryosuke Niwa 2020-10-23 18:16:03 PDT
(In reply to Darin Adler from comment #21)
> (In reply to Ryosuke Niwa from comment #16)
> > Sure, one thing to note is though that this will make it harder for people
> > to find where these functions are declared / defined. Maybe we can add
> > Source/WebCore/dom/Contains.h?
> 
> That’s an interesting point.
> 
> There is really a family of tree ordering/structure functions:
> 
> treeOrder
> contains
> intersects
> intersection
> unionRange (annoying name)
> intersectingNodes
> characterDataOffsetRange
> containedNodes (to be added, maybe)
> (a few more to come, likely matching concepts in DOM specifications)
> 
> I think that all of these kind of "go together". There are versions that
> work on nodes, boundary points, and ranges. They are sort of cousins of
> these:
> 
> parent
> first/lastChild
> next/previousSibling
> contains
> isDescendantOf
> rootNode
> document
> 
> I could lean more toward making everything member functions. Or maybe put
> many of these into a TreeStructure.h header?

We do have NodeTraversal.h so we could put them there. Or maybe something like NodeAlgorithm.h? If these functions are also supposed to work with RenderObject's then maybe TreeAlgorithm.h is more appropriate but I'm not sure that's really the way to go. It's probably better to have separate versions for Node vs. RenderObject and other kinds of trees.