Bug 218007 – REGRESSION(r266295): DOMSelection's addRange and containsNode behave incorrectly when selection crosses shadow boundaries

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

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.

[Darin Adler]

Oh, maybe I have this correct in the "live range" mode but incorrect in the old mode.

[Darin Adler]

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.

[Darin Adler]

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);

[Ryosuke Niwa]

(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.

[Darin Adler]

(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?

[Darin Adler]

Created attachment 412143 [details]
Patch

[Darin Adler]

Created attachment 412145 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

Created attachment 412185 [details]
Patch

[Ryosuke Niwa]

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?

[Darin Adler]

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?

[Darin Adler]

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.

[Darin Adler]

I’m going to land, and address Ryosuke’s comments as follow ups once I understand better what he’s looking for.

[Darin Adler]

Committed r268940: <https://trac.webkit.org/changeset/268940>

[Radar WebKit Bug Importer]

<rdar://problem/70633710>

[Ryosuke Niwa]

(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?

[Ryosuke Niwa]

(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.

[Darin Adler]

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!

[Darin Adler]

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?

[Ryosuke Niwa]

(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.

[Darin Adler]

(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?

[Ryosuke Niwa]

(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.