1475869 - Provide a shadow DOM-piercing querySelector and querySelectorAll for extensions

Status: UNCONFIRMED

(WebExtensions :: General, enhancement, P3)

Description

[mrmr1993]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180703115938

Steps to reproduce:

This is a follow-up to bug 1439153.

Currently, extensions can target an element or collection of elements with a CSS selector via querySelector/querySelectorAll. By design, these do not pierce shadow DOMs, so that page/component developers can keep their components separate.

For extensions designed to work on a variety of pages, there may be elements that would match their CSS selector in the composed tree if there were no shadow roots, but instead the match is 'blocked' by the shadow root. Perhaps worse, querySelector may match elements which are children of a shadow host but which would not match in their position in the composed tree (because of the <slot>s they end up in).

To give extensions the option of working on the composed tree, which gives them a far simpler and more representative model of the page to query, it would be useful to have a composedQuerySelector/composedQuerySelectorAll that query CSS selectors on the composed DOM tree as if it were the DOM tree. That is, these new functions should work as querySelector/querySelectorAll except that they treat the children of a shadow root as if they were exactly the children of its shadow host.

In keeping with bug 1439153, open and closed shadow roots should be treated the same, to ensure that extensions work as intended everywhere, and the APIs should probably not be available to the pages themselves.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

This is of course against how selectors work. Selectors work on trees, trees of trees, not on flattened tree. So it isn't quite clear to me how this would work.
For example, if there are unassigned elements under the host and there are also elements under ShadowRoot, which one should be selected in cases like first-child selector?

Comment 2

[mrmr1993]

> For example, if there are unassigned elements under the host and there are also elements under ShadowRoot, which one should be selected in cases like first-child selector?

The idea is that it should work as the page actually functions: the elements under the host should be completely ignored (except where they fill slots in the shadow root's tree), and the children of the shadow root should be considered in their place.

In general, the extension has no idea about the structure of a page's components, and so knowing what the children that can be slotted into its content are is next-to-useless.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

emilio might have an opinion here.

I still don't quite understand how it would work - it would be basically a selector on the flattened tree, which is not what selectors normally do.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

I'm not sure we'd want something like this. As Olli said, selectors work on the tree of trees. Adding this special mode would mean a huge amount of code size, or a branch for every parent / previous sibling access in selector-matching, which isn't great.

Also, it's not probably quite as seamless as you'd expect, since <slot>s are on the flat tree. So a selector like div > span would not match a slotted span ever.

Comment 5

[mrmr1993]

> Adding this special mode would mean a huge amount of code size, or a branch for every parent / previous sibling access in selector-matching, which isn't great.

I'm not sure this is true: the C++ code can use a template to decide the branch at compile time, so that the branching is optimised out (at least for the existing querySelector/querySelectorAll), like is already done in nsINode::FindMatchingElements. I'm imagining wrapping a template<bool traverseShadowTrees> around nsCSSRuleProcessor::SelectorMatchesTree, ::SelectorListMatches and nsINode::FindMatchingElements. I don't know Rust well, so I'm not sure what the story is on that side..

> Also, it's not probably quite as seamless as you'd expect, since <slot>s are on the flat tree.

I see. Is there a way to access the flat tree from the selector code?

> I'm not sure we'd want something like this.

Beyond the code complexity, were there any concerns about adding this? I'm not very familiar with the codebase, and haven't written any C++ in a while, but I'm happy to give it a try if it's a viable idea.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

(In reply to mrmr1993 from comment #5)
> > Adding this special mode would mean a huge amount of code size, or a branch for every parent / previous sibling access in selector-matching, which isn't great.
> 
> I'm not sure this is true: the C++ code can use a template to decide the
> branch at compile time, so that the branching is optimised out (at least for
> the existing querySelector/querySelectorAll), like is already done in
> nsINode::FindMatchingElements.

That is exactly the 'code size' part of the comment above. You either add a bunch of branches on super-hot code, or duplicate already-bloaty code.

> I'm imagining wrapping a template<bool
> traverseShadowTrees> around nsCSSRuleProcessor::SelectorMatchesTree,
> ::SelectorListMatches and nsINode::FindMatchingElements. I don't know Rust
> well, so I'm not sure what the story is on that side..

That code is no longer in the tree, for Rust it'd be similar, though again, a whole lot of generated code.

> > Also, it's not probably quite as seamless as you'd expect, since <slot>s are on the flat tree.
> 
> I see. Is there a way to access the flat tree from the selector code?

Yes, I don't see why you wouldn't be able to access the flat tree from the selectors code. We already have a flattened_tree_parent function for inheritance, you'd need to add prev / next siblings.

Note that <slot>s are on the flat tree so not sure it's what you meant, but yeah you could conceivably skip slots. Though you can do slot { display: block } and they'd be part of the layout tree so you'd want to match them I assume.

> > I'm not sure we'd want something like this.
> 
> Beyond the code complexity, were there any concerns about adding this? I'm
> not very familiar with the codebase, and haven't written any C++ in a while,
> but I'm happy to give it a try if it's a viable idea.

Sure, it's doable, it's just complexity / maintenance / code-size / performance, plus the fact that I'm not sure it's the right model: You want to also handle general display: contents elements, and that's not really feasible with such a mechanism.

This would all need to be Rust though (servo/components/selectors, etc.).

Comment 7

[mrmr1993]

I've put together a rough prototype, not including any special slot handling or really anything very interesting. I tried to use templates, which meant touching callsites across the code; otherwise, the only changes are adding new ways to walk the DOM into shadow trees and wiring things up to the JS side.

At the moment, it's only exposed on the Element interface, so document.composedQuerySelector, shadowRoot.composedQuerySelector, etc. won't work.

> That is exactly the 'code size' part of the comment above.

Sorry, I misunderstood before. Is there a good way to measure how bad the situation is from builds of the prototype?

> Note that <slot>s are on the flat tree so not sure it's what you meant, but yeah you could conceivably skip slots. Though you can do slot { display: block } and they'd be part of the layout tree so you'd want to match them I assume.

Leaving them in should only affect the child and sibling selectors, right? I think it's probably fine to leave them in, I can't see them being that useful if the extension really doesn't know what the site will look like ahead of time.

> I'm not sure it's the right model: You want to also handle general display: contents elements, and that's not really feasible with such a mechanism.

I'm not sure I get this. The idea of this API is to get around elements segmenting the DOM and 'hiding' elements. Is that something that needs handling with display: contents elements too?

Comment 8

[mrmr1993]

Attachment: Prototype of Element.composedQuerySelector/composedQuerySelectorAll without <slot> handling

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8992681 [details] [diff] [review]
Prototype of Element.composedQuerySelector/composedQuerySelectorAll without <slot> handling

Review of attachment 8992681 [details] [diff] [review]:
-----------------------------------------------------------------

If what you want is this, an enum in MatchingContext or something like that is better, since the branches aren't hot enough to warrant the code bloat I guess, but this is pretty different from what you proposed. This doesn't use the flattened tree, just jumps over shadow roots for ancestor combinators, which is quite different.

::: servo/components/style/dom.rs
@@ +253,5 @@
> +
> +    /// Returns the next children in the flat tree in pre-order, optionally
> +    /// scoped to a subtree root.
> +    #[inline]
> +    fn next_in_deep_preorder(&self, scoped_to: Option<Self>) -> Option<Self> {

Also, this is a lot of code duplication, it'd be better to just recurse into shadow roots instead I think.

Comment 10

[mrmr1993]

Attachment: Prototype of Element.composedQuerySelector/composedQuerySelectorAll with <slot> handling

I've made your suggested changes, thanks Emilio.

This patch also includes code for walking through <slot>s to the slotted elements. The <slot>s are included, since they can be rendered by changing them from display: contents.

I tried a few ways to simplify the get_slotted_nodes/of_slotted nodes interface, but I couldn't convince the borrow checker that self.slotted_nodes() lives long enough to create the iterator in style::gecko::wrapper. Is there a better way to do this?

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8993393 [details] [diff] [review]
Prototype of Element.composedQuerySelector/composedQuerySelectorAll with <slot> handling

Review of attachment 8993393 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/selectors/matching.rs
@@ +421,5 @@
> +            if context.shadow_mode() == ShadowMode::Transparent {
> +                if let Some(s) = element.assigned_slot() {
> +                    // This element is slotted, look for the previous element in the same slot.
> +                    let mut prev = None;
> +                    for slotted in s.get_slotted_nodes().iter().filter_map(|n| s.of_slotted_node(n)) {

Yeah, this is the kind of code I don't want to add unconditionally unfortunately. I thought you were going to keep the behavior of the previous version.

This is not really acceptable I think, perf or maintainability-wise. Maybe let's keep the previous version, with ShadowMode (like this one) instead of the generics, then maybe call the API piercingQuerySelector, documenting that it only jumps over shadow roots?

Also, I'm not sure what's the process of getting a new API exposed to add-ons.

Finally, this would need tests to land.

::: servo/ports/geckolib/glue.rs
@@ +2046,5 @@
>          quirks_mode,
>      )
>  }
>  
> +unsafe fn generic_QueryFirst(

This naming doesn't follow the rust conventions. The other functions don't because they need to be called from C++.

Why not passing another boolean to QueryFirst / QueryAll instead of having two different functions?

Comment 12

[mrmr1993]

> Yeah, this is the kind of code I don't want to add unconditionally unfortunately. I thought you were going to keep the behavior of the previous version.

I'm worried that leaving it out compromises the API: there are still nodes that the extension can't query properly, it's just that we've traded shadow hosts' slotted children for it's shadow DOM descendents.

Would it be acceptable to add an mNextInSlot/mPreviousInSlot and accessors nextElementInSlot/previousElementInSlot to Element (akin to nsINode::mNextSibling/mPreviousSibling), so that this code could be a simple lookup?

> I'm not sure what's the process of getting a new API exposed to add-ons.

This should already be done via the webidl annotations for the functions.

I'll start preparing another patch with these changes, and without the slots code. Hopefully we can work something out for slots after it lands.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

(In reply to mrmr1993 from comment #12)
> > Yeah, this is the kind of code I don't want to add unconditionally unfortunately. I thought you were going to keep the behavior of the previous version.
> 
> I'm worried that leaving it out compromises the API: there are still nodes
> that the extension can't query properly, it's just that we've traded shadow
> hosts' slotted children for it's shadow DOM descendents.

How so? The slotted children need to be normal DOM children of the host. You can query them with normal querySelector already.

> Would it be acceptable to add an mNextInSlot/mPreviousInSlot and accessors
> nextElementInSlot/previousElementInSlot to Element (akin to
> nsINode::mNextSibling/mPreviousSibling), so that this code could be a simple
> lookup?

No, growing nsINode by two words is not a good trade-off for an add-on only API.

> > I'm not sure what's the process of getting a new API exposed to add-ons.
> 
> This should already be done via the webidl annotations for the functions.

Yes, I was just mentioning that I'm not sure if there's a process that we need to follow or not.

Comment 14

[David Durst [:ddurst]]

(Marking P3 to get it off triage, smaug please re-triage if you think that's incorrect.)

Comment 15

[mrmr1993]

Attachment: Element.composedQuerySelector/composedQuerySelectorAll without <slot> handling

> How so? The slotted children need to be normal DOM children of the host. You can query them with normal querySelector already.

This is true, but assumes you know where the <slot>s are. An extension running on a previously-unseen page can't know when/where it needs to look inside a <slot> without traversing the DOM itself.

> I'm not sure if there's a process that we need to follow or not.

Sorry, I misunderstood you. There didn't seem to be one over in bug 1439153, but I might have missed some out-of-channel things.

> No, growing nsINode by two words is not a good trade-off for an add-on only API.

That seems fair. I'm going to look into carrying a list of the indices into the <slot>s in the MatchingContext, which should make the slot code nearly as simple, and only effects objects used by the selectors code.

Comment 16

[mrmr1993]

Attachment: Element.composedQuerySelector/composedQuerySelectorAll with <slot> handling

This version re-introduces <slot> handling and adds tests for it.

Keeping track of the current index in each slot <slot> is now done with a vector in both the traversal (dom_apis.rs) and matching (matching.rs) code, avoiding the expensive search for the current index in the slot each time from before. The MatchingContext carries this vector of slot indices from the traversal to the matching code, and its pop_slot_index/push_slot_index functions use copy-on-write semantics to minimise the amount of vector copying between the two codepaths.