Open Bug 1455894 Opened 2 years ago Updated 3 months ago

Need to teach selection about the flattened tree / frame tree instead of the light tree

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

Webcompat Priority P2

People

(Reporter: emilio, Unassigned, NeedInfo)

References

(Blocks 4 open bugs)

Details

(Whiteboard: [webcompat:p2][layout:backlog:2019q3][needs-wpt-?])

nsRange keeps all these offsets around in terms of the light tree.

This means that it's fundamentally flawed for Shadow DOM, and all the selection APIs, find, and such are going to be really broken when people use Shadow DOM.

We probably can afford not to fix the selection APIs to ship the first version of Shadow DOM, since they're probably not relied upon, but we should probably support basic functionality like selecting and searching in Shadow DOM.
My intuition from my not-in-depth understanding of how these APIs are consumed is that they mostly care about the flattened tree / frame tree + placeholders, and that you already can't rely on keeping ranges around before and after a flush (since script may have ran and may have fiddled with the DOM).

Is that right, Mats? If so, what do you (and the others?) think about keeping ranges in terms of the frame tree?

Seems easier than doing it in terms of the flattened tree, since a linked structure is already setup by the frame constructor, instead of having to manually compute the right links / insertion points / etc every time.

Is anyone aware of any API that would require to use ranges for undisplayed stuff?
Flags: needinfo?(mats)
Summary: Need to teach nsRange about the flattened tree. → Need to teach nsRange about the flattened tree / frame tree instead of the light tree
Range itself isn't supposed to cross shadow boundaries. Selection may.
And Range deals with DOM (sub)trees, whether or not the tree is in shadow.

Sounds like this bug is about selection, not about range.
Also, remember that currently selection handling crossing shadow DOM is unspecified.

There are some proposals, at least https://github.com/yoichio/public-documents/blob/master/selectionapiforshadow.md
(In reply to Olli Pettay [:smaug] from comment #2)
> Range itself isn't supposed to cross shadow boundaries. Selection may.
> And Range deals with DOM (sub)trees, whether or not the tree is in shadow.
> 
> Sounds like this bug is about selection, not about range.

Hmm, I guess so, yeah. I was assuming that it would've been easier to modify nsRange than all its consumers, but that may not work for everything I guess.

Bug 1455891 is kind of its own beast from what I've seen (nsFind uses two iterators to deal with NAC in <textarea>, we'd need to change to a stack, but even that wouldn't preserve the composed tree order, which I suspect is what we want...).

Anyway, I guess we can discuss that there.
No longer blocks: 1455891
Summary: Need to teach nsRange about the flattened tree / frame tree instead of the light tree → Need to teach selection about the flattened tree / frame tree instead of the light tree
(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> My intuition from my not-in-depth understanding of how these APIs are
> consumed is that they mostly care about the flattened tree / frame tree +
> placeholders, and that you already can't rely on keeping ranges around
> before and after a flush (since script may have ran and may have fiddled
> with the DOM).
> 
> Is that right, Mats? 

I'm not sure what the question is exactly, but nsRanges observes
DOM mutations and update themselves by "gravitating" towards
the nearest node, to (mostly) keep the selection intact.
See the methods after this line:
https://searchfox.org/mozilla-central/rev/f8e72641772293702e76e75620b777b730b71b72/dom/base/nsRange.cpp#516

> If so, what do you think about keeping ranges in terms of the frame tree?

We used to keep selection state in the frame tree and it was very buggy,
which is why we switched to keep the selection state in the DOM instead:
see bug 679183 and bug 619273.
I think it would be a mistake to go back to a frame solution.

> Is anyone aware of any API that would require to use ranges for undisplayed
> stuff?

I'm probably misunderstanding your question, but the DOM Range API
works perfectly fine on display:none/contents nodes...
Flags: needinfo?(mats)
(In reply to Olli Pettay [:smaug] from comment #2)
> Range itself isn't supposed to cross shadow boundaries. Selection may.
> And Range deals with DOM (sub)trees, whether or not the tree is in shadow.

Right, that's how we could solve this actually -- but splitting up
the Selection into one Range that's in the light DOM and another
Range that's in the shadow DOM.  E.g. if you select over a shadow
boundary like so: "out[erinn]er" you'd end up with:
Selection:
  Range[0]: "outer" 3, "outer" 5
  Range[1]: "inner" 0, "inner" 3

That way we shouldn't violate any assumptions (or if we do, that's
just existing bugs that we should fix).  The code to support that
already exists for the most part since that's what we do when
the user selects over 'user-select:none' islands. See
nsRange::ExcludeNonSelectableNodes. I think it would be fairly
easy to amend that to split ranges at shadow DOM boundaries.
Note that this is a longstanding problem: selection can create pretty bogus ranges (or could at one point) that span across shadow boundaries.  We had this problem with XBL too.

The real answer is that you can't use a single range to represent selection in a shadow DOM world.  Now we just have to convince other UAs to allow non-single-range selections at all.  :(
(In reply to Mats Palmgren (:mats) from comment #5)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> > My intuition from my not-in-depth understanding of how these APIs are
> > consumed is that they mostly care about the flattened tree / frame tree +
> > placeholders, and that you already can't rely on keeping ranges around
> > before and after a flush (since script may have ran and may have fiddled
> > with the DOM).
> > 
> > Is that right, Mats? 
> 
> I'm not sure what the question is exactly, but nsRanges observes
> DOM mutations and update themselves by "gravitating" towards
> the nearest node, to (mostly) keep the selection intact.
> See the methods after this line:
> https://searchfox.org/mozilla-central/rev/
> f8e72641772293702e76e75620b777b730b71b72/dom/base/nsRange.cpp#516
> 
> > If so, what do you think about keeping ranges in terms of the frame tree?
> 
> We used to keep selection state in the frame tree and it was very buggy,
> which is why we switched to keep the selection state in the DOM instead:
> see bug 679183 and bug 619273.
> I think it would be a mistake to go back to a frame solution.

Ah, makes sense, thanks!

> > Is anyone aware of any API that would require to use ranges for undisplayed
> > stuff?
> 
> I'm probably misunderstanding your question, but the DOM Range API
> works perfectly fine on display:none/contents nodes...

I was meaning that if we did it using the frame tree it wouldn't work for undisplayed stuff at all, but given the above that's not a solution.

(In reply to Mats Palmgren (:mats) from comment #6)
> (In reply to Olli Pettay [:smaug] from comment #2)
> > Range itself isn't supposed to cross shadow boundaries. Selection may.
> > And Range deals with DOM (sub)trees, whether or not the tree is in shadow.
> 
> Right, that's how we could solve this actually -- but splitting up
> the Selection into one Range that's in the light DOM and another
> Range that's in the shadow DOM.  E.g. if you select over a shadow
> boundary like so: "out[erinn]er" you'd end up with:
> Selection:
>   Range[0]: "outer" 3, "outer" 5
>   Range[1]: "inner" 0, "inner" 3

I guess we need arbitrary selections, not just two, since you can have arbitrarily nested shadow trees... But yeah, that sounds fine I think.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> The real answer is that you can't use a single range to represent selection
> in a shadow DOM world.  Now we just have to convince other UAs to allow
> non-single-range selections at all.  :(

Perhaps the best way forward is to implement a prototype using multi-range
Selection?  To demonstrate that it's a viable, perhaps better, alternative.

Fwiw, TabAtkins was positive to multi-range Selection last this came up on
www-style (in the context of selection in Grid layout etc, which doesn't
really behave sanely without them).  Now, with their LayoutNG effort,
might be a good time to win them over to this model...
Priority: -- → P3
Blocks: shadowdom
No longer blocks: shadowdom-initial-release
Duplicate of this bug: 1409959
Component: DOM → DOM: Core & HTML

Looks like WebKit fixed this (https://bugs.webkit.org/show_bug.cgi?id=151380), so we're the only engine that supports Shadow DOM and doesn't support this properly...

Flags: needinfo?(emilio)

Hmm, in which way did they fix it. I guess not implementing the proposed selection handling yet, but something random.

Flags: needinfo?(svoisen)
Blocks: 1545367
Flags: webcompat? → webcompat+
Whiteboard: [webcompat:p2]
Flags: needinfo?(svoisen)
Whiteboard: [webcompat:p2] → [webcompat:p2][layout:backlog:2019q3]

See bug 1547409. Migrating webcompat priority whiteboard tags to project flags.

Webcompat Priority: --- → P2

For testing reference, the following web platform tests exist which seem to test selection behavior in shadow roots:

  • html/editing/editing-0/contenteditable/contentEditable-slotted-inherit.html
  • shadow-dom/untriaged/shadow-trees/upper-boundary-encapsulation/test-009.html
  • shadow-dom/untriaged/user-interaction/ranges-and-selections/test-001.html
  • shadow-dom/untriaged/user-interaction/ranges-and-selections/test-002.html
  • shadow-dom/untriaged/elements-and-dom-objects/shadowroot-object/shadowroot-methods/test-004.html

However, since Firefox nightly and Safari 87 preview don't differ in whether they pass or fail those tests, I'm guessing none of them cover this bug. But if WebKit did indeed fix their implementation in the referenced commit above, then we can likely convert the tests it added to their tree as the basis for some new testdriver-based web platform tests here.

Whiteboard: [webcompat:p2][layout:backlog:2019q3] → [webcompat:p2][layout:backlog:2019q3][needs-wpt-?]
Flags: webcompat+
You need to log in before you can comment on or make changes to this bug.