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

Support `>>>` combinator in static profile #78

Closed
hayatoito opened this Issue May 25, 2015 · 67 comments

Comments

Projects
None yet
@hayatoito
Copy link
Member

hayatoito commented May 25, 2015

Title: [Shadow]: Figure out a good replacement for /deep/ in testing scenarios (bugzilla: 28591)

Migrated from: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28591


comment: 0
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28591#c0
Dimitri Glazkov wrote on 2015-05-01 17:22:53 +0000.

One thing that immediately popped up once we started talking about removing shadow-piercing combinators is that WebDriver-based tests are frequently interested in reaching into shadow trees to grab a specific element to test:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/chromedriver/test/run_py_tests.py&sq=package:chromium&q=testShadowDom*&l=877

Web Driver actually has something currently that attempts to solve the problem: http://www.w3.org/TR/webdriver/#switching-to-hosted-shadow-doms

However, the feedback I got from ChromeDriver folks is that it's way too verbose and awkward to use for the most common case (see above).

Maybe the solution is just specifying deepQuerySelector for WebDriver spec. However, I want to make sure this is not just a one-off -- seems like this could be needed in other contexts.


comment: 1
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28591#c1
Anne wrote on 2015-05-02 07:34:03 +0000.

  1. We shouldn't do features for testing. That's bad.

  2. I remain convinced that in the open case we should provide a myriad of features that cross the "deep" to aid with selection, event delegation, composition, etc.


comment: 2
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28591#c2
Elliott Sprehn wrote on 2015-05-03 00:41:03 +0000.

(In reply to Anne from comment #1)

  1. We shouldn't do features for testing. That's bad.

  2. I remain convinced that in the open case we should provide a myriad of
    features that cross the "deep" to aid with selection, event delegation,
    composition, etc.

+1, we should keep /deep/ in the static profile for querySelector. Before we had it authors kept rolling their own (we saw this on multiple occasions).


comment: 3
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28591#c3
Anne wrote on 2015-05-04 06:12:37 +0000.

Note that an alternative is that we introduce .deepQuery() or some such.


comment: 4
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28591#c4
Elliott Sprehn wrote on 2015-05-04 06:21:02 +0000.

(In reply to Anne from comment #3)

Note that an alternative is that we introduce .deepQuery() or some such.

deepQuery is not enough, you don't want to match a descendant selector across a ShadowRoot boundary since ".a .b" means something really different. You'd still need a special combinator to signal where the scope crossing should be in the selector expression.

ex.
.panel .image

All images inside panels contained in a single scope.

.panel /deep/ .image

All images anywhere below a panel, even if they're inside a nested widget.

This is important because it maintains the "don't accidentally cross a boundary" principle.

We need something like ::shadow as well.


comment: 5
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28591#c5
Tab Atkins Jr. wrote on 2015-05-05 00:12:16 +0000.

(In reply to Elliott Sprehn from comment #4)

(In reply to Anne from comment #3)

Note that an alternative is that we introduce .deepQuery() or some such.

deepQuery is not enough, you don't want to match a descendant selector
across a ShadowRoot boundary since ".a .b" means something really different.
You'd still need a special combinator to signal where the scope crossing
should be in the selector expression.

ex.
.panel .image

All images inside panels contained in a single scope.

.panel /deep/ .image

All images anywhere below a panel, even if they're inside a nested widget.

This is important because it maintains the "don't accidentally cross a
boundary" principle.

Yeah, trying to move the shadow-crossing quality to the core of the method doesn't work. It's much less flexible, as you note, and doesn't compose with anything else similar. The correct approach is to just embrace the "static profile" of selectors http://dev.w3.org/csswg/selectors/#static-profile and leave /deep/ there. (Or >>>, as it's now called.)


comment: 6
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28591#c6
Hayato Ito wrote on 2015-05-07 08:43:56 +0000.

(In reply to Tab Atkins Jr. from comment #5)

(In reply to Elliott Sprehn from comment #4)

(In reply to Anne from comment #3)

Note that an alternative is that we introduce .deepQuery() or some such.

deepQuery is not enough, you don't want to match a descendant selector
across a ShadowRoot boundary since ".a .b" means something really different.
You'd still need a special combinator to signal where the scope crossing
should be in the selector expression.

ex.
.panel .image

All images inside panels contained in a single scope.

.panel /deep/ .image

All images anywhere below a panel, even if they're inside a nested widget.

This is important because it maintains the "don't accidentally cross a
boundary" principle.

Yeah, trying to move the shadow-crossing quality to the core of the method
doesn't work. It's much less flexible, as you note, and doesn't compose
with anything else similar. The correct approach is to just embrace the
"static profile" of selectors
http://dev.w3.org/csswg/selectors/#static-profile and leave /deep/ there.
(Or >>>, as it's now called.)

Is there any existing clients who use static-profile?
Does it mean '/deep/' can be used in particular APIs?


comment: 7
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28591#c7
Tab Atkins Jr. wrote on 2015-05-07 15:55:04 +0000.

(In reply to Hayato Ito from comment #6)

Is there any existing clients who use static-profile?
Does it mean '/deep/' can be used in particular APIs?

It's for querySelector()/etc.


comment: 8
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28591#c8
Hayato Ito wrote on 2015-05-08 02:25:11 +0000.

(In reply to Tab Atkins Jr. from comment #7)

(In reply to Hayato Ito from comment #6)

Is there any existing clients who use static-profile?
Does it mean '/deep/' can be used in particular APIs?

It's for querySelector()/etc.

Thanks.

Can everyone agree that '/deep/' is okay to be used in querySelector()?

I think we are assuming that adding something to static profile is zero-overhead to the performance of dynamic profile.


comment: 9
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28591#c9
Tab Atkins Jr. wrote on 2015-05-08 17:29:16 +0000.

(In reply to Hayato Ito from comment #8)

(In reply to Tab Atkins Jr. from comment #7)

(In reply to Hayato Ito from comment #6)

Is there any existing clients who use static-profile?
Does it mean '/deep/' can be used in particular APIs?

It's for querySelector()/etc.

Thanks.

Can everyone agree that '/deep/' is okay to be used in querySelector()?

I think we are assuming that adding something to static profile is
zero-overhead to the performance of dynamic profile.

Correct. At worst, it's a check during grammar verification, to note that this isn't valid in the current context and so the selector should be considered grammar-violating.

@pemrouz

This comment has been minimized.

Copy link

pemrouz commented Sep 21, 2015

This seems like quite a basic affordance, without which there is an unbridgeable gap between the Chrome and Firefox implementations (utilise/all#1 | doc). We rely quite heavily on this ability. It looks like everyone is on the same page too (i.e. good in JS, bad in CSS). Is there any chance /deep/ or >>> in querySelector/All could be included in v1?

@hayatoito hayatoito changed the title [Shadow]: Figure out a good replacement for /deep/ in testing scenarios (bugzilla: 28591) Support /deep/ in static profile (bugzilla: 28591) Dec 3, 2015

@hayatoito

This comment has been minimized.

Copy link
Member Author

hayatoito commented Dec 3, 2015

I renamed the subject of this issue to more appropriate one.

@hayatoito

This comment has been minimized.

Copy link
Member Author

hayatoito commented Dec 3, 2015

AFAIK,

  • Blink folks are okay to support /deep/ in static profile.
  • Apple objected to /deep/ in any case.
  • I do not know other browser vendor's opinion on this.

Please feel free to correct me if I am wrong.

@pemrouz

This comment has been minimized.

Copy link

pemrouz commented May 28, 2016

Thanks, @hayatoito.

Are there any more updates on this issue?

For anyone opposing this: is there a sensible alternative solution/proposal (perhaps something like :has-shadow-root?) so we can recursively crawl (at least open) shadow roots, rather than naively checking every element?

/cc @annevk @rniwa @travisleithead

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented May 31, 2016

is there a sensible alternative solution/proposal (perhaps something like :has-shadow-root?) so we can recursively crawl (at least open) shadow roots, rather than naively checking every element?

What are use cases that require walking across all those shadow trees?

@pemrouz

This comment has been minimized.

Copy link

pemrouz commented May 31, 2016

Essentially, we have a small core that stores (browser-like) resources and emits an event when they change.

ripple(name)       // getter
ripple(name, body) // setter
ripple.on('change', (name[, change]) => {})

Other modules like the view layer simply listens for changes and updates anything on the page that is affected. For example, components are simply transformation functions of data, so registering a new version of a component would redraw all instances of that custom element. This amounts to the following:

ripple.on('change', name => document.querySelectorAll(name).map(d => d.draw())) 

Other types of resources may also change, such as stylesheets or data, and the view layer would use different selectors ([css~="${name}"] and [data~="${name}"]) to find all the instances that need updating.

Now, since there may be relevant custom elements to update in the shadows of other custom elements (majority case), instead of document.querySelectorAll(name) a small utility is used to search across all shadow roots if the browser supports it.

Being able to use querySelectorAll to identify matching elements to update across the application results in a super-eloquent solution that lets the browser handle this robustly. Without this basic affordance, you would need to spawn your own DOM-like structure to keep track of things which would be very painful and kill interoperability.

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented May 31, 2016

Now, since there may be relevant custom elements to update in the shadows of other custom elements (majority case), instead of document.querySelectorAll(name) a small utility is used to search across all shadow roots if the browser supports it.

But anyone could add a new instance of a custom element dynamically to any shadow tree so you'd end up continuously monitoring mutations to every shadow tree. This is extremely inefficient. Just walking the entire composed tree to find all instances of a given custom element alone is too expensive.

If you're concerned about tracking all instances of custom elements, wrapping custom elements' lifecycle callbacks might be a better approach (i.e. track lifecycle callbacks in your framework, and then delegate back to each custom element later).

@pemrouz

This comment has been minimized.

Copy link

pemrouz commented Jun 1, 2016

There is no need to watch for mutations. You just call .draw on the newly added element. For browsers that support the Custom Element lifecycle functions, the attachedCallback is pointed to the same function so it wouldn't be necessary (it's idempotent, so there's no harm if it's called multiple times anyway though).

The lifecycle callbacks are wrapped, which is what allows for the dynamic registry of components in the first place. The problem is not with the lifecycle callbacks however. It's when something else changes, like receiving new data/stylesheets/components from the server, and then you need to find the elements to rerender. If you are suggesting to use the {attached,detached}Callbacks to build some sort of internal map and keep track of every instance, then that is exactly what would be nice to avoid and rely on querySelectorAll for.

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Jun 1, 2016

If you are suggesting to use the {attached,detached}Callbacks to build some sort of internal map and keep track of every instance, then that is exactly what would be nice to avoid and rely on querySelectorAll for.

I'm saying that providing such an API is undesirable because looking for all elements in the composed tree with querySelectorAll would be extremely inefficient and expensive.

A better approach would be for each component to keep track of their subcomponents within its shadow tree (e.g. at construction time), and recursively call draw on them. Alternatively, adding some sort of a option to MutationObserver so that it can track new elements of a given type being added to a shadow tree might work.

@pemrouz

This comment has been minimized.

Copy link

pemrouz commented Jun 1, 2016

I understand, but either way we are searching for instances of elements across the composed tree. The suggestion to implement this in JS would be (a) orders of magnitude more expensive/inefficient than if done natively (b) a lot of unnecessary management overhead in all hot code paths (c) hard to do reliably (d) more assumptions, worse for interoperability and the different contexts components can work in (e) requires redrawing everything from the top-level component down for every change.

One of the reasons it's currently so fast (2x optimised React on DBMonster) is because (a) there are no intermediate data structures to manage and (b) we can jump immediately to a lower part of the tree and then recursively draw from there, rather than always starting right at the top.

This solution is also more in the spirit of progressive enhancement (in contrast to e.g. extending MutationObservers) and even if every framework built their own shadow-dom-walker-tracker (itself undesirable), being able to easily walk shadows is a more generally common use case, like the testing issue in the original thread, hence why it makes sense to favour the more generic solution.

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Jun 1, 2016

[B]eing able to easily walk shadows is a more generally common use case, like the testing issue in the original thread, hence why it makes sense to favor the more generic solution.

The whole point of shadow DOM API is to provide encapsulation. If your app needs to constantly break that encapsulation, then it's probably better not to use shadow DOM API in the first place.

In terms of testing, I don't think reaching into shadow trees of a component while testing a web page that uses a given component is a common scenario. You'd typically unit-test each component separately, and the integration test would be testing the page using public API of each component.

In fact, I'd argue it's an anti-pattern. For example, if a library author published a component and various websites that use that component started poking into its shadow tree in websites' tests then the library author may no longer be able to change its shadow tree structure at his/her will because those changes may break those websites.

@pemrouz

This comment has been minimized.

Copy link

pemrouz commented Jun 1, 2016

I expected this more ideological argument and hence tried to sidestep (with "at least open"). Let's clarify two general use cases for Web Components:

(1) Componentisation of applications: For example, using a fractal architecture where each component composes other subcomponents structured in the same way

(2) Embedding third-party leaf components (e.g. google-maps, facebook-like, twitter-counts, etc)

Open shadows are suitable for the former and closed shadows for the latter (completely bullet-proof encapsulation). You still want to be able to benefit from things like style encapsulation, but there is more involved with application-level components, like state management.

It appears you are being dismissive of the former use case, in which case Web Components becomes relegated to just being a poor man's iframe (iframe is arguably better for the latter use case, since they are far more "strongly encapsulated": e.g. separate component registries, @font-face, etc).

For testing, you are again extrapolating your conclusion from one narrow case: I agree relying on the implementation of a third-party component would be brittle. But this isn't the only use case. It's reasonable to dispatch an event in one part of the application and make assertions about what changed on the other side. The view that everything should be controllable and observable from the top-level component's public API of the component is obviously impractical and brittle (every subcomponent has to proxy everything).

Hence I think it is worth first explicitly clarifying your view on open shadows and the former use case. Is the design goal of shadows to simply maximise encapsulation so that they are not useful throughout an application and are only useful as third-party leaf nodes? If yes, then open shadows should be dropped and this purpose made more clear. If not, then the need for some sensible way to recursively walk shadows (until hitting a closed one) becomes evident and we can go back to discussing whether APIs like this are more suitable in JS or native.

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Jun 2, 2016

You still want to be able to benefit from things like style encapsulation, but there is more involved with application-level components, like state management.

I've been using shadow DOM to write our performance tracking tool, and many of "components" have to track dozens of states. Yet, I've never found it useful having to reach into some other component's shadow tree in order to get access to those states. In practice, all those states are stored in some sort of JavaScript objects and properties, and components should be exposing some public API to access those states.

It's reasonable to dispatch an event in one part of the application and make assertions about what changed on the other side.

In such tests, there should be a test that verifies a given component dispatches an event, and a separate test which verifies that the given component responds to the event. You could even manually inspect those components' shadow trees during testing but that doesn't require anything akin to querySelectorAll since you're unlikely to be testing every single component simultaneously.

The view that everything should be controllable and observable from the top-level component's public API of the component is obviously impractical and brittle (every subcomponent has to proxy everything).

There is composed option in EventInit that makes the event propagate across shadow boundaries. That should be sufficient for that kind of inter-component communications. Using /deep/ to do that kind of inter-component communication is precisely the anti-pattern we want to discourage.

Is the design goal of shadows to simply maximize encapsulation so that they are not useful throughout an application and are only useful as third-party leaf nodes?

I disagree with your premise that maximizing encapsulation is not useful for components within an application given my experience of having successfully writing a web app that's made up of dozens of closed shadow tree components.

Having said that, Apple's position has always been that open mode of shadow DOM is a mistake. The only reason we have "open mode" shadow tree is because we made a compromise to keep both modes without a default during April 2015 F2F meeting for the sake of moving shadow DOM API as a whole forward.

@pemrouz

This comment has been minimized.

Copy link

pemrouz commented Jun 3, 2016

I'm not sure what exactly your perf tools do: I agree that explicitly proxying and transforming state is a good thing. However, it's not the only use case. Having built multiple apps in this manner, the issue is that this approach does not scale. When you have a component 10 layers deep and you need to change it's requirement, every component in the path in between now has to explicitly proxy the new data down, which makes this approach very brittle.

This is not about "inter-component communication" (?). Only events are used for that in a "data down, actions up" paradigm. This is about either a deep or recursive API being necessary for the framework layer to manage updates. It might be beneficial to read more around things like Redux + React (but as aforementioned, it's not strictly just data, see for example Om Next (30:36 onwards) where they maintain a live index of all instances on the page).

The composed event is very relevant here: it's the symmetrical counterpart to this feature. Specifically, it enables dispatching an event which may pass through many layers all of whom do not need to explicitly re-emit the event. It's "upward piercing".

In such tests, there should be a test that verifies a given component dispatches an event, and a separate test which verifies that the given component responds to the event. You could even manually inspect those components' shadow trees during testing but that doesn't require anything akin to querySelectorAll since you're unlikely to be testing every single component simultaneously.

It appears you are erasing the problem here by suggesting to abandon integration tests for unit tests (two separate unit tests)? Testing end-to-end flows in an application happens across components. The introspection required by the testing layer, similar to the framework layer, transcends the component tree and it can't be expected to use the public API of components for this purpose.

It's good to know Apple is opposed to open shadows. However, I think going forward, unless your goal is to sabotage the usability of open shadows, it's better to not mix the two problems. There's very little reason to oppose this hook for open shadows (especially given existence of upward piercing), and once you all work out how to make closed shadows more usable, then you can discuss deprecating open shadows altogether.

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Jun 3, 2016

This is not about "inter-component communication" (?). Only events are used for that in a "data down, actions up" paradigm. This is about either a deep or recursive API being necessary for the framework layer to manage updates. It might be beneficial to read more around things like Redux + React (but as aforementioned, it's not strictly just data, see for example Om Next (30:36 onwards) where they maintain a live index of all instances on the page).

The composed event is very relevant here: it's the symmetrical counterpart to this feature. Specifically, it enables dispatching an event which may pass through many layers all of whom do not need to explicitly re-emit the event. It's "upward piercing".

Oh I see. This is exactly one weakness I see with the current web components as well. But I don't think exposing shadow roots is the right solution for this. What we need is an event-like system that propagates things downwards instead of upwards. e.g. dispatchEventInSubtree which would fire an event on all elements under a given element. But I don't think using an event is the right construct here. I think a better approach would be adding a new custom element callback which gets some sort of a message, and that can then pass it down to other custom elements within its shadow tree with some sort of default delegation mechanism.

@pemrouz

This comment has been minimized.

Copy link

pemrouz commented Jun 3, 2016

^ Both are great alternatives I haven't thought about before 👍. If you have a small example of how their API's might look, I can try and give some feedback from the perspective of my use cases.

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Jun 3, 2016

Sorry, I don't have a time to design new API like that at the moment but I'm more than happy to revisit this in a couple of months.

@TakayoshiKochi

This comment has been minimized.

Copy link
Member

TakayoshiKochi commented Aug 9, 2016

Let me continue discussion on this thread, here's the summary so far.

The original proposal was, to allow '>>>' (shadow-piercing combinator) in the static profile only.

Chrome had /deep/ combinator for Shadow DOM v0, which could be used for both static/dynamic
profiles but found that using it in CSS stylesheets (i.e. dynamic profile) caused significant performance issues, as it is often used for theme-like rules (e.g. body >>> .fancycheckbox { ... }), at every style recalculation the engine had to match the selector against all the nodes.

The idea was to limit this combinator for querySelector/querySelectorAll, which runs one-off. The answer to theme-like styling as of now is to use CSS custom properties, but no good solutions for JS other than recursively applying querySelector on each shadow root (example).

The concerns so far are

  • use cases are primarily for testing (WebDriver etc.)
  • breaks encapsulation of Shadow DOM
  • operation is ineffective (this is for cases when it is used for observer-like pattern?)

If Shadow DOM is closed mode-only, this combinator of course doesn't make sense, but for open shadow trees, providing a way to select node in shadow tree should make sense.

One alternative idea from @annevk was to have .deepQuery() instead of the combinator,
but what it matches against e.g. .deepQuery('.a .b') and .querySelector('.a >>> .b') is different.
In .deepQuery() all normal descendant combinator mean deep descendant, or .a and .b
have to be in the same tree? It could be either way, but I guess explicit distinction is better
(normal descendant vs. deep desendant).

@TakayoshiKochi

This comment has been minimized.

Copy link
Member

TakayoshiKochi commented Aug 9, 2016

I think the current situation is like when we had to include jQuery as platform didn't provide querySelector, so wouldn't it be nice if the platform provides the API to select nodes deep into open Shadow DOM, without having the recursive polyfill.

Any comments and opinions are welcome!

@hayatoito

This comment has been minimized.

Copy link
Member Author

hayatoito commented Aug 9, 2016

Please remove "breaks encapsulation of Shadow DOM" from the concerns because >>> must not cross closed shadow roots in any sense.

An open shadow root already provides Element.shadowRoot. So the use case of >>> should be the same for Element.shadowRoot, basically.

The point is that >>> in the static profile is just a utility feature which can be polyfill-able by JavaScript.
However, having >>> in the Web Platform makes the Web faster, and reduces the burdens of web developers.

>>> does not provide any value for closed shadow roots.

@TakayoshiKochi

This comment has been minimized.

Copy link
Member

TakayoshiKochi commented Aug 9, 2016

"The concerns so far" is the summary in this thread, and IIUC the main point that Apple opposes having this is breaking the encapsulation, as in #78 (comment).

Thanks for the clarification.

@hayatoito

This comment has been minimized.

Copy link
Member Author

hayatoito commented Aug 9, 2016

Yeah, I would like to clarify that >>> in the static profile is just a syntax sugar for Element.shadowRoot and querySelector APIs.

The point is that having this syntax sugar in the Web Platform is worth while or not.

@tabatkins

This comment has been minimized.

Copy link
Member

tabatkins commented Aug 9, 2016

Yes, @hayatoito is exactly right. >>> is nothing more nor less than sugar for the .shadowRoot. Anne's older .deepQuery() idea is strictly weaker, since you can't control when you pierce through roots, plus

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Aug 9, 2016

Given we have a fundamental disagreement with the usefulness of shadowRoot in the first place, I don't find that argument convincing at all. What we need is a concrete list of use cases (there were a couple of good ones earlier in the thread), and we need to see if /deep/ is the right API to provide for those use cases or not.

For example, propagating Rect-render-like states to subcomponents is NOT an appropriate use case for /deep/ since it won't be one off case to bypass encapsulation. This use case is better met by a new custom element callback designed to propagate states top-down.

Writing integration tests for a Web app consisting of hundreds of components is another use case that came up. I'd argue that such an integration test should not be relying on internal states of each component. Instead, they should be testing based on each component's public interface. However, if authors really wanted to do white-box testing across components, they could simply write an equivalent of /deep/ in JavaScript. And this would be fine for testing scenarios because an extra runtime during testing wouldn't hurt end users.

@annevk annevk changed the title Support `>>>` combinator in static profile (bugzilla: 28591) Support `>>>` combinator in static profile Sep 6, 2017

@equinusocio

This comment has been minimized.

Copy link

equinusocio commented Oct 2, 2017

\deep\ is deprecated in shadow dom v1 (that ship the standard ::slotted() and css style-hooks). This issue can be closed.

@TakayoshiKochi

This comment has been minimized.

Copy link
Member

TakayoshiKochi commented Oct 3, 2017

@equinusocio no, /deep/ is deprecated for "dynamic profile" (i.e. use in CSS styles), while here we are discussing any way for "static profile" (use from querySelector() API).

So far some options:

  1. Keep /deep/ and >>> for static profile
  2. Introduce an alternative API (e.g. collectMatchingElementsInFlatTree)
  3. Other?

The use case is to find some element in the entire flat tree, but the controversial point is the nature of /deep/ is too powerful against Shadow DOM's encapsulation.

The problem of not having this feature is that finding such an element in flat tree is inefficient in the current available APIs.

@pemrouz

This comment has been minimized.

Copy link

pemrouz commented Oct 3, 2017

Well, there is a subtle semantics difference. We’re essentially checking whether :matches(selector) is true or not for each element, and collecting elements that do match so we’re not scope-matching a selectors string but instead we’re matching a selector against an element which uses a different scoping object. This is because using a node from different trees as the scope doesn’t make sense in evaluating a selector as well as for the implementation simplicity.

We’re adopting this API in some Safari code for now, and if this turns out to be useful, we’d make a more refined proposal.

@rniwa do you have any updates of how this is going in Safari?

@phistuck

This comment has been minimized.

Copy link

phistuck commented Oct 3, 2017

The use case is to find some element in the entire flat tree, but the controversial point is the nature of /deep/ is too powerful against Shadow DOM's encapsulation.

That sounds less like a use case to me, but more like a summary of how you would find an element... It would be better to provide a concrete example for a non-testing scenario where this would be needed, I think.

@tabatkins

This comment has been minimized.

Copy link
Member

tabatkins commented Oct 7, 2017

Look at any non-WC page that searches for elements in the page with querySelector/All(), for any reason.

Now, imagine they switched to make heavy use of WC, such that there was a nice hierarchy of shadow trees.

Boom, that's a use-case. (Unless you can reasonably argue that, in the new version of the page, they'll have no need to search for elements from the top level of the page anymore, and can rely on specific elements to do all the searching locally within their own tree. That would be a difficult argument to make, I think.)

And as a reminder, not having >>> in qSA() has nothing to do with safety or encapsulation. You can emulate its effects perfectly, if with some effort and cost, by manually parsing the selector, then tree-walking the open shadow trees and running selectors yourself. It only exposes information that is already freely exposed via DOM APIs.

And the alternative API proposal (querySelectorAgainstFlatTree) is weaker than >>>, because you can't control when you walk down a tree, and it can't reproduce the power of ::slotted().

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Oct 7, 2017

A use case is a concrete scenario / story. Imagining any page that doesn't use web components that use querySelectorAll to start using web components is too abstract and generic to be called as a use case.

As a counter example, I have a website (perf.webkit.org) with ~20k lines of JS/HTML/CSS code which didn't used to use web components and used querySelector and querySelectorAll. I've successfully switched to start using shadow DOM & custom elements without ever needing to find elements across shadow boundaries with querySelector or querySelectorAll.

As far as I can tell, using >>> to find elements across component/shadow boundaries is an anti-pattern because each component tends to have a separate internal state that needs stay in sync with its DOM tree. Mutating them directly would tend to result in each component's internal states getting out of sync with DOM nodes. For example, in apps that use React-like top-down reconstruction of DOM nodes & diff'ing, mutated DOM nodes may get thrown away and re-constructed next time the component renders itself. So for any DOM mutations made across component/shadow boundaries to make sense, the mutator must observe all DOM mutations across shadow boundaries, and re-apply the changes as needed. This leads to very inefficient, intrusive, and fragile code. That's exactly the kind of issues encapsulation should prevent.

Even for testing, the need for querySelectorAll never came up because we could just use JS functions to find the right element to do the necessary white box testing.

And the alternative API proposal (querySelectorAgainstFlatTree) is weaker than >>>, because you can't control when you walk down a tree, and it can't reproduce the power of ::slotted().

This is precisely why we like that approach better. The biggest problem we have with the proposed API is the we'd have to introduce a very complicated shadow boundary transitioning code into our selector matching engine.

@robdodson

This comment has been minimized.

Copy link

robdodson commented Oct 8, 2017

Even for testing, the need for querySelectorAll never came up because we could just use JS functions to find the right element to do the necessary white box testing.

Can you give me an example? The most compelling use case I've heard so far for needing a shadow piercing querySelector is for testing. Did you instead implement your own tree walking function?

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Oct 8, 2017

Even for testing, the need for querySelectorAll never came up because we could just use JS functions to find the right element to do the necessary white box testing.

Can you give me an example? The most compelling use case I've heard so far for needing a shadow piercing querySelector is for testing. Did you instead implement your own tree walking function?

Basically, we decided not to write tests that pierce across shadow boundaries. We wrote unit tests for each component where we reach out into each shadow tree and check its state, and once we've done that, we never reached into shadow trees in our integration tests. This is how builtin HTML elements work by the way. As a web developer, you never test internal implementation details of builtin HTML elements. The best you can do is to check its publicly exposed states, and that's exactly what we do with our components, and I'd say it's working fairly well for us.

@ChadKillingsworth

This comment has been minimized.

Copy link

ChadKillingsworth commented Oct 8, 2017

Basically, we decided not to write tests that pierce across shadow boundaries.

That falls apart when you have interactive elements in shadow roots (like <button>). You end up writing public accessors just to test the component.

Having worked with this a lot (and being the author of a very popular gist on this subject), I don't want the shadow piercing combinator. What I do want is a selector that only works for testing that selects from the current element's shadow root:

document.querySelector('custom-element => button');
// Must be currently written as:
document.querySelector('custom-element').shadowRoot.querySelector('button');

However most of this could be easily overcome by syntactic sugar or special methods in testing frameworks.

@equinusocio

This comment has been minimized.

Copy link

equinusocio commented Oct 8, 2017

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Oct 8, 2017

Basically, we decided not to write tests that pierce across shadow boundaries.

That falls apart when you have interactive elements in shadow roots (like ). You end up writing public accessors just to test the component.

I don't follow. For testing interactive elements, we do white box testing by exposing its shadow tree. There's no need for a component that uses another interactive components to reach into its shadow tree.

Having worked with this a lot (and being the author of a very popular gist on this subject), I don't want the shadow piercing combinator. What I do want is a selector that only works for testing that selects from the current element's shadow root:

document.querySelector('custom-element => button');
// Must be currently written as:
document.querySelector('custom-element').shadowRoot.querySelector('button');

That should already work. ShadowRoot.prototype.querySelector and ShadowRoot.prototype.querySelectorAll already exist.

@ChadKillingsworth

This comment has been minimized.

Copy link

ChadKillingsworth commented Oct 8, 2017

There's no need for a component that uses another interactive components to reach into its shadow tree.

I disagree. For full integration tests, I need to click and interact with elements just like the user would. I frequently have form elements, anchors and other interactive elements inside a shadow tree. The alternative is using selenium to click on specific offsets in the viewport - and that is extremely fragile.

That should already work. ShadowRoot.prototype.querySelector and ShadowRoot.prototype.querySelectorAll already exist.

It does - but it's very verbose and difficult to work with. Authors want the convenience that the shadow piercing combinator provided. This problem really only exists because selenium locates elements based on query selectors. It's cumbersome to use shadow dom in that environment.

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Oct 8, 2017

There's no need for a component that uses another interactive components to reach into its shadow tree.

I disagree. For full integration tests, I need to click and interact with elements just like the user would. I frequently have form elements, anchors and other interactive elements inside a shadow tree. The alternative is using selenium to click on specific offsets in the viewport - and that is extremely fragile.

But you wouldn't do this with builtin elements. You wouldn't reach into video element's controls and start messing with their buttons because you trust that video element is implemented & tested correctly on its isolation.

That should already work. ShadowRoot.prototype.querySelector and ShadowRoot.prototype.querySelectorAll already exist.

It does - but it's very verbose and difficult to work with. Authors want the convenience that the shadow piercing combinator provided. This problem really only exists because selenium locates elements based on query selectors. It's cumbersome to use shadow dom in that environment.

Since what you want in your use case can be easily built as a shim on top of the existing capability, I still don't see why this needs to be implemented in the browser.

@ChadKillingsworth

This comment has been minimized.

Copy link

ChadKillingsworth commented Oct 9, 2017

But you wouldn't do this with builtin elements. You wouldn't reach into video element's controls and start messing with their buttons because you trust that video element is implemented & tested correctly on its isolation.

True - but again that doesn't scale up to the way web components are used today. This is due to two base reasons:

  1. Custom elements cannot properly participate in a form. Form elements inside the component have to be tested with elements outside of the component.
  2. As you combine base elements into distinct sets, the parent element becomes an app and testing requires interacting with the children in integration tests.
<my-app>
  #shadowRoot
    <form>
      <name-fields>
        #shadowRoot
          <input type="text" name="fullname">
      </name-fields>
      <form-submission></form-submission>
    <form>
</my-app>

Take the above example. How do you test the form submission works? This is of course a contrived example, but the answer shouldn't be "don't do that". Having distinct grouping of input fields is handy to share between several forms.

As you continue to compose custom elements together, how else do your properly test the combined total? How do you test <my-app> in a final integration test in a browser?

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Oct 9, 2017

The form submission issue is tracked by #187, which we're intending to discuss at W3C TPAC this year.

As you continue to compose custom elements together, how else do your properly test the combined total? How do you test <my-app> in a final integration test in a browser?

Even in that scenario, piercing across shadow boundaries isn't really a good way to write tests. When the implementation details of each component changes, you'd be forced to update all those integration tests.

A better way to accomplish this would be relying on public APIs of each component to test, and make sure each component has a separate unit tests for public API updating its internal states correctly. If there isn't adequate API support for writing whatever tests, then you can add test/debug only API which exposes necessary states/information. We do this in WebKit's C++ code by exposing extra JS objects which lets tests expose & manipulate internal states of WebKit even though doing so in production/user environment won't be okay due to security concerns.

@ChadKillingsworth

This comment has been minimized.

Copy link

ChadKillingsworth commented Oct 9, 2017

We do this in WebKit's C++ code by exposing extra JS objects which lets tests expose & manipulate internal states of WebKit even though doing so in production/user environment won't be okay due to security concerns.

This is exactly why a special selector would be nice for selenium only. I don't think adding it in general to the static profile is warranted at all and would just encourage bad things.

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Oct 9, 2017

We do this in WebKit's C++ code by exposing extra JS objects which lets tests expose & manipulate internal states of WebKit even though doing so in production/user environment won't be okay due to security concerns.

This is exactly why a special selector would be nice for selenium only. I don't think adding it in general to the static profile is warranted at all and would just encourage bad things.

Okay. That more or less matches our expectation. Does something like the API I proposed in #78 (comment) work for you? Instead of having an explicit >>>, this API just finds any element which matches a given selector in any shadow tree.

@zhaoz

This comment has been minimized.

Copy link

zhaoz commented Oct 10, 2017

The whole point of shadow DOM API is to provide encapsulation. If your app needs to constantly break that encapsulation, then it's probably better not to use shadow DOM API in the first place.

For YouTube, the benefit of shadow DOM API is encapsulation of styling as a default. There are still benefits for global styling and the ability to select elements through shadow boundaries.
That said, focusing on the dynamic profile, I do agree that this is mostly useful for testing and debugging. This is the majority (and possibly only) use case that YouTube has.

We’re experimenting with collectMatchingElementsInFlatTree (implemented in https://trac.webkit.org/changeset/208878) which finds all elements that match a given selector in each tree instead of an explicit >>> to cross a shadow boundary

Is collectMatchingElementsInFlatTree on the element prototype? E.g. allowing for this to work on a subtree? I want to be able to do something like someDiv.collectMatchingElementsInFlatTree().

Also, does this API select for trees within a shadow root? or does it only match an element in isolation? Like, are queries like #main x-foo valid if both x-foo and #main were in the same shadowRoot?

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Oct 18, 2017

Is collectMatchingElementsInFlatTree on the element prototype? E.g. allowing for this to work on a subtree? I want to be able to do something like someDiv.collectMatchingElementsInFlatTree().

Our API currently hangs off of the global object and you have to pass in the someDiv but if we're standardizing it, then we'd most certainly add it on an element just like what you wrote.

Also, does this API select for trees within a shadow root? or does it only match an element in isolation? Like, are queries like #main x-foo valid if both x-foo and #main were in the same shadowRoot?

#main x-foo to apply an element only if both x-foo and #main are in the same tree.

@tabatkins

This comment has been minimized.

Copy link
Member

tabatkins commented Oct 18, 2017

Oh, wow, that's a much more significant restriction in power than I thought you were talking about.

@WebAppsWG

This comment has been minimized.

Copy link

WebAppsWG commented Oct 18, 2017

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Feb 20, 2018

Again, we're not interested in implementing >>> or /shadow/ in the static profile as currently proposed.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Feb 21, 2018

I agree and have said as much upstream in w3c/csswg-drafts#640 (comment). I suggest that we continue the discussion there as this affects a document maintained by the CSS WG. I'll add that document to https://github.com/w3c/webcomponents/blob/gh-pages/README.md so it's easier to find from this repository going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.