Emilio Cobos Álvarez (:emilio) Reporter
	Description • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio) Reporter
	Comment 1 • 6 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 6 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 6 years ago

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

	Emilio Cobos Álvarez (:emilio) Reporter
	Comment 4 • 6 years ago

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

	Mats Palmgren (inactive)
	Comment 5 • 6 years ago

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

	Mats Palmgren (inactive)
	Comment 6 • 6 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 6 years ago

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

	Emilio Cobos Álvarez (:emilio) Reporter
	Comment 8 • 6 years ago

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

	Mats Palmgren (inactive)
	Comment 9 • 6 years ago

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