Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Match querySelector for pseudo elements (::slotted()) #463

Closed
TakayoshiKochi opened this issue May 31, 2017 · 27 comments
Closed

Match querySelector for pseudo elements (::slotted()) #463

TakayoshiKochi opened this issue May 31, 2017 · 27 comments
Labels
topic: shadow Relates to shadow trees (as defined in DOM)

Comments

@TakayoshiKochi
Copy link
Member

(See crbug.com/710797 for the background)

The problem is, that shadowRoot.querySelector('::slotted(div)') (or querySelectorAll) will not return anything even when the context object (the shadowRoot) contains <slot> and a <div> is slotted to it.

Demo:
http://jsbin.com/jafetekipi/edit?html,js,output
(currently both Blink and WebKit fail)

MDN document of Document.querySelector() says:

CSS Pseudo-elements will never return any elements, as specified in the Selectors API

Note that this "Selectors API" links to Selectors API level 1 (an old spec).
The latest draft as of today (Selectors Level4) doesn't say about how querySelector() should work for pseudo-elements, and neither does DOM spec.

In the past, ::before or ::first-line etc. never matched any real element in DOM tree, but ::slotted(div) can match a real element in DOM tree, even though it is in a different tree.

Here I see the following problems in the current specs:

  1. DOM spec isn't clear about whether querySelector can match some pseudo elements (::slotted()).
  2. DOM spec isn't clear about whether querySelector can return an element outside the node tree of the context object.
  3. ::slotted() is a pseudo element, but can point to a real element.

For 3, this is not a matter of DOM spec. The rationale for ::slotted() being a pseudo element is that it points to an element in another tree, and we wanted to restrict the selector in parentheses to a simple/compound selector (not allowing complex selector). If it were a combinator, any selector would be allowed to the right side.

Note that issue 2 is also relevant to
WICG/webcomponents#78 (Support >>> combinator in static profile).

@TakayoshiKochi
Copy link
Member Author

Let me get back to this.
As 3 is a matter of CSS selectors, let's decide on question 1 and 2.

For 1, I'd vote for NO for consistency with other pseudo elements.
For 2, I'd vote for YES, as it is clear if the query's intention is to pierce through tree boundary, if the selector contains such combinator (assuming >>> is for real). I cannot think of any accidental leak of shadow tree internal from query outside the tree.

So if developers want shadowRoot.querySelector('::slotted(div)') to match, we need to define another combinator (e.g. /slotted/ ?) that works almost like ::slotted().

Any opinions? @rniwa @annevk

@annevk
Copy link
Member

annevk commented Jul 13, 2017

I agree with NO on 1. I don't see what kind of object we could even return. I don't agree with 2 given the discussion in WICG/webcomponents#78 hasn't concluded favorably to such an addition.

And 1 is tracked by #185 by the way, which is awaiting input from @tabatkins.

@TakayoshiKochi
Copy link
Member Author

@tabatkins ping?

1 similar comment
@TakayoshiKochi
Copy link
Member Author

@tabatkins ping?

@TakayoshiKochi
Copy link
Member Author

Until WICG/webcomponents#78 i s resolved, I'm okay with NO for 2 as a conservative option.

If no one objects, I'd like to make necessary edits to the DOM spec, but before then, @niwa and @tabatkins could you sanity check? Also @tabatkins, any suggestion about the necessity for changing any of CSS selectors spec is welcome.

@rniwa
Copy link
Collaborator

rniwa commented Aug 9, 2017

What's the case where no.2 becomes relevant? Only shadow boundary piercing combinator? Then "no" since our position is to not support that.

@TakayoshiKochi
Copy link
Member Author

Yes, as far as I know only /deep/, ::shadow (v0-only) and possibly ::slotted() (if 1 is yes).

@tabatkins
Copy link
Contributor

Ugh, sorry for not responding.

So, there's definitely nothing capable of representing pseudo-elements in JS, so nothing to return if the selector is ::before or something. But ::slotted(), like ::part(), is a pseudo-element in syntax, but doesn't point to a pseudo-element in the DOM; there's a real element there that's being pointed to, it's just not visible using the standard tree-walking combinators. I believe these should work, and return the element they point to. (In other words, I think YES is the correct answer for 1.)

On the spec-language front, dbaron recently rewrote the "match a selector against a tree" algorithm, and I need to do a more detailed review of it now. I can make sure it handles this case.

@TakayoshiKochi
Copy link
Member Author

(For reference, ::part() is talking about https://tabatkins.github.io/specs/css-shadow-parts/#part-attr , or WICG/webcomponents#300, and "match a selector against a tree" is https://drafts.csswg.org/selectors-4/#match-against-element)

Thanks Tab for the comment, so you think YES for 1, then it implies YES for 2 as well?
If that's the case, shadowRoot.querySelector(':host') would return the real element as well?
Also, what if ::part() points to an element that is in a closed shadow tree - shall it return null?

If we allow not-in-the-same-tree element to be returned, implementation has to take care of each exceptional pseudo elements (::slotted(), ::part(), and so on) to be matched against nodes in different node trees, so it definitely looks revising "match a selector against a tree" algorithm is necessary.

@rniwa
Copy link
Collaborator

rniwa commented Aug 10, 2017

I don't think we should do that. It would mean it's a lot easier to accidentally access nodes outside your own shadow tree in the library code, etc... that takes an arbitrary selector and runs it against a DOM tree.

@tabatkins
Copy link
Contributor

You only "accidentally" access nodes that you explicitly ask for. It doesn't give you access to anything you don't already have access to; qS(":host") is the same as just following the .host pointer on your shadow tree; qs("::slotted(...)") is the same as doing some tree-walking on your own. Closed shadow trees don't expose themselves via selectors, so won't expose themselves this way either.

I don't know of any other way to "accidentally" access anything outside of your tree; any selectors other than those explicitly mentioned are always limited to just your current tree.

Care would have to be taken with ::part() so that closed trees don't return anything, but that's it, I think.

@rniwa
Copy link
Collaborator

rniwa commented Aug 17, 2017

There is a lot of library/framework code that takes in an arbitrary selector as an input. Those code can then accidentally start traversing / mutating shadow tree if querySelector started returning nodes across shadow boundaries.

@tabatkins
Copy link
Contributor

Yeah, but what part of that is "accidental"? Again, the only way to traverse the boundaries is using a selector that traverses the boundaries. There's only a handful of specific pseudos that do this; nothing else will. What's wrong with library/framework code being able to cross shadows when the user of the library explicitly asks them to?

@rniwa
Copy link
Collaborator

rniwa commented Aug 18, 2017

Yeah, but what part of that is "accidental"? Again, the only way to traverse the boundaries is using a selector that traverses the boundaries. There's only a handful of specific pseudos that do this; nothing else will. What's wrong with library/framework code being able to cross shadows when the user of the library explicitly asks them to?

The problem is that accidentally passing a selector which crosses a shadow boundary isn't really explicit. Whether something is concerned as explicit is quite subjective so I'm not certain if we can come to an agreement on that.

What I can say that is that we'd object to making querySelector returning a node outside of its own tree, and would not implement it in WebKit.

@TakayoshiKochi
Copy link
Member Author

I see the gap that @tabatkins asking any concrete example that can accidentally get the node outside the tree, while @rniwa saying it's technically possible that happens, without concrete example.
Possibly you can make an example that you take user input as a selector string and just run querySelector() with the input literally without any sanitization. But I don't think this a good argument.

My take on the gap is that where you are coming from, i.e. the world that "open" shadow root is the default or the world that "closed" shadow root is the default. But there is no such default for the current Element.attachShadow and you have to specify either mode. The world needs both.

In general, we allowed access from inner to outer, such as ShadowRoot.host or HTMLSlotElement.assignedNodes(), whereas we are restrictive from outer to inner, especially for closed shadow trees, such as Element.shadowRoot.

So what is wrong with this?: allow cross-boundary element to be returned for querySelector() if the selector explicitly contains cross-boundary pseudos such as :host, ::slotted(), ::part() if the matching element is visible from the context object? I.e. you can get querySelector(":host") from either in open or closed shadow tree, while querySelector("::part()") or querySelector("::-webkit-progress-bar") would never match any element under closed or user-agent shadow tree.

I think nothing is wrong, the only justification for not returning those that I can agree is at the last line in @rniwa's response, "querySelector returning a node outside of its own tree" (its = context object's).

I haven't heard anyone requesting querySelector(":host") to return shadow host (Blink currently returns null). Probably this is because we already have ShadowRoot.host. querySelector("::slotted(...)") is also possible with existing APIs (though it needs extra steps - so developers may ask in the future). So unless we have compelling use cases that

  1. using querySelector() is far easier than mix of existing APIs (polyfilling)
  2. querySelector() can perform much better than using polyfilled one

I think it's fine to restrict

  1. no matched pseudo elements returned for querySelector
  2. querySelector doesn't return any element outside its context object's node tree

@rniwa
Copy link
Collaborator

rniwa commented Aug 18, 2017

In general, we allowed access from inner to outer, such as ShadowRoot.host or HTMLSlotElement.assignedNodes(), whereas we are restrictive from outer to inner, especially for closed shadow trees, such as Element.shadowRoot.

With an explicit API. The problem here is that querySelector and querySelectorAll are existing API that used to never return a node outside its own tree regardless of what input was given to the functions. For example, you could have iterated over every CSS rule in the stylesheet and executed querySelectorAll on every selector, and you wouldn't have gotten any node that doesn't belong to its own node tree. If we made the proposed change to querySelector and querySelectorAll, however, they would start returning nodes outside its own node tree. I would consider that as a show stopper.

In general, we shouldn't be modifying the behavior of an existing DOM API to start returning a node outside its own node tree without an explicit opt-in that doesn't conceivably come up as a part of existing use of the API.

While the author must supply a selector string that contains ::slotted() and ::part() to querySelector and querySelectorAll to return a node outside its own node tree, passing an arbitrary string to those two functions is a common scenario a lot of libraries and frameworks do implement. Now, such library/framework code can easily rely on the fact that the node returned by querySelector and querySelectorAll belong to the same node tree as the context object; e.g. library/framework might cache the results of querySelectorAll at the root node. The library could be adding properties returned by querySelectorAll for some processing. All these things are exactly the kind of things that ought to be avoided by the encapsulation shadow tree provides.

@rniwa
Copy link
Collaborator

rniwa commented Aug 18, 2017

I'd add one more reason that we'd object to this feature: implementation complexity.

Making querySelector and querySelectorAll work across shadow boundaries would require a substantial reworking of our CSS JIT engine, and we're quite frankly uninterested in implementing that.

@tabatkins
Copy link
Contributor

The problem is that accidentally passing a selector which crosses a shadow boundary isn't really explicit. Whether something is concerned as explicit is quite subjective so I'm not certain if we can come to an agreement on that.

Again, how is that accidental? You don't slip on the keyboard and, whoops, a ::slotted() shows up in your selector. You write a selector like that on purpose. You are trying to target something outside your shadow. Why is it useful to prevent libraries from using this, when they can just walk outside the tree on their own? In the cases where a library explicitly wants to allow targeting inside/outside a shadow tree, without this it would have to provide a side-channel for communicating that you want to hop into/out of a shadow before applying a selector, or do selector parsing on its own.

Making querySelector and querySelectorAll work across shadow boundaries would require a substantial reworking of our CSS JIT engine, and we're quite frankly uninterested in implementing that.

This is substantially different from supporting the selectors in stylesheets?

Now, such library/framework code can easily rely on the fact that the node returned by querySelector and querySelectorAll belong to the same node tree as the context object; e.g. library/framework might cache the results of querySelectorAll at the root node. The library could be adding properties returned by querySelectorAll for some processing. All these things are exactly the kind of things that ought to be avoided by the encapsulation shadow tree provides.

I still don't understand the failure mode being alluded to here. Can you give a concrete example?

@TakayoshiKochi
Copy link
Member Author

This is substantially different from supporting the selectors in stylesheets?

In the case of Blink, usually querySelector/querySelectorAll first collects a set of elements that will be matched against the given selector, but for a selector which contains /deep/, Blink has a special path to match that selector against nodes in all descendant tree scopes.

If we'd support :host, ::slotted(), ::part, we'd add some more special paths to collect more elements that have to be matched against.

@rniwa
Copy link
Collaborator

rniwa commented Aug 20, 2017

The problem is that accidentally passing a selector which crosses a shadow boundary isn't really explicit. Whether something is concerned as explicit is quite subjective so I'm not certain if we can come to an agreement on that.

Again, how is that accidental? You don't slip on the keyboard and, whoops, a ::slotted() shows up in your selector. You write a selector like that on purpose. You are trying to target something outside your shadow. Why is it useful to prevent libraries from using this, when they can just walk outside the tree on their own? In the cases where a library explicitly wants to allow targeting inside/outside a shadow tree, without this it would have to provide a side-channel for communicating that you want to hop into/out of a shadow before applying a selector, or do selector parsing on its own.

Well, the person who wrote ::slotted(~) was almost certainly not doing so accidentally. The problem here is that it's very easy for some code to accidentally pass such a selector to an existing library that's not designed to work across shadow boundaries.

The scenario works like this. 1. there was a generic code written with shadow tree in mind which takes an arbitrary CSS selector and access & mutates the node returned by the selector. 2. an author uses that generic code in a shadow tree, or more likely some code which the author deployed ends up depending on such generic code. 3. The selector given to the generic code in (1) exposes a node outside its node tree, and breaks the code & causes unintended code effects.

Note that step (3) doesn't necessarily invoke the author explicitly giving a selector that crosses shadow boundaries to the generic code (1). The generic code (1) itself could be traversing through selectors in every stylesheet in the node tree and automatically processing them to polyfill missing CSS features, etc...

This is substantially different from supporting the selectors in stylesheets?

Yes. For selectors like ::slotted, we have a specialized code path where we collect & apply them within the node tree they appear instead of making selector code match across shadow boundaries since the latter approach is way too expensive and complicated to implement.

@TakayoshiKochi
Copy link
Member Author

@tabatkins do you have any updates on this?

Especially I'd like to hear if there is necessity or strong reason to make a selector work in a consistent way as much as possible in stylesheets and for querySelector API.
IMO, as the querySelector API is a DOM API, and we already have some discrepancies (e.g. cannot select pseudo elements by querySelector) between CSS and DOM, not returning an element outside of the context object's tree and not matching boundary-piercing pseudos are acceptable solutions here.

@tabatkins
Copy link
Contributor

There is a strong reason to make the querySelector API work the same - if the browser can run the selector in a stylesheet, there's no good reason to disallow giving authors access to the same matching capability in JS. Seemingly-arbitrary restrictions make APIs more complicated for authors, harder to learn and confusing to use. I guarantee you that if an author asks "Why can't I match an element with one of these selectors? The browser can!" there's no satisfying answer you can give them. "Rewrite your code to split the selector into two, with the tree-crossing done manually in JS between them" is a terrible answer that will just make them angry, and rightfully so.

That said, if we really do think it's a necessity to impose a "must stay in starting tree" restriction, I won't oppose it if we couple it with an issue tracking adding a flag or new method that actually evaluates selectors properly, the same as the browser does. Before we impose this restriction in the spec I'd like to figure out what the new API will look like; I dont' want a querySelector2(...) sort of thing, or some huge unwieldy name that people won't want to type.


Note that step (3) doesn't necessarily invoke the author explicitly giving a selector that crosses shadow boundaries to the generic code (1). The generic code (1) itself could be traversing through selectors in every stylesheet in the node tree and automatically processing them to polyfill missing CSS features, etc...

I read this as an argument for the exact opposite case. CSS polyfill code wants to faithfully polyfill CSS, which includes correctly evaluating selectors that people would use in CSS. I suspect this restriction would break as many cases as it fixes.

I understand that we sometimes need to impose seemingly-arbitrary restrictions for implementation reasons, but let's defend it on those grounds, not pretend we're doing authors a favor here. Carving out pieces of the platform that UAs can use but authors can't isn't helpful to them.

@rniwa
Copy link
Collaborator

rniwa commented Aug 28, 2017

Okay, I still disagree with your arguments, but I would re-iterate that we're not willing to implement a feature where querySelector would return a node outside its own tree in WebKit due to the implementation complexity it would add.

That is, whatever flag or new method you're proposing is not something we ever want to implement in WebKit due to the implementation complexity even as a separate API.

@TakayoshiKochi
Copy link
Member Author

TakayoshiKochi commented Aug 29, 2017

That said, if we really do think it's a necessity to impose a "must stay in starting tree" restriction, I won't oppose it if we couple it with an issue tracking adding a flag or new method that actually evaluates selectors properly, the same as the browser does. Before we impose this restriction in the spec I'd like to figure out what the new API will look like; I dont' want a querySelector2(...) sort of thing, or some huge unwieldy name that people won't want to type.

Let me make sure one more thing.

Putting aside pseudos, if I understand the whatwg DOM spec,
https://dom.spec.whatwg.org/#dom-parentnode-queryselector
https://dom.spec.whatwg.org/#scope-match-a-selectors-string

  1. Return the result of evaluate a selector s against node’s root using scoping root node.

not returning elements outside the tree of the context object is status quo, right?

Or, according to changes in
https://drafts.csswg.org/selectors-4/#changes
under section 18 API Hooks,

Note that earlier versions of this section defined a section on evaluating a selector, but that section is no longer present. Specifications referencing that section should instead reference the algorithm to match a selector against a tree.

Is this change not applied to the DOM spec (but how we pass one or more root elements from querySelector()?)?

@annevk annevk added the topic: shadow Relates to shadow trees (as defined in DOM) label Feb 20, 2018
@annevk
Copy link
Member

annevk commented Mar 8, 2018

Given the resolution of WICG/webcomponents#78 and likely resolution of w3c/csswg-drafts#640 I'm inclined to close this. The only thing we might need to clarify with pseudo-elements is that they're not returned.

@annevk
Copy link
Member

annevk commented Mar 8, 2018

but how we pass one or more root elements from querySelector()?

That's not a feature on offer at the moment.

@TakayoshiKochi
Copy link
Member Author

Let's close this as status quo.

My understanding is that with the current state of the relevant specs, querySelector() works as
intended.

Until we have something concrete that we can receive a pseudo element as a DOM object,
we can live with the status quo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: shadow Relates to shadow trees (as defined in DOM)
Development

No branches or pull requests

4 participants