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

Shadow tree encapsulation and Element Timing #816

Closed
npm1 opened this issue May 17, 2019 · 33 comments
Closed

Shadow tree encapsulation and Element Timing #816

npm1 opened this issue May 17, 2019 · 33 comments

Comments

@npm1
Copy link

npm1 commented May 17, 2019

We're incubating the ElementTiming API which exposes rendering timings of elements of a website (namely, image and text content). I filed internally WICG/element-timing#3 to attempt to clarify what the interaction with shadow elements should be (in particular regarding what we can expose from the shadow tree). I realize that an issue there will not be noticed by relevant folks so I figure I can get more attention by filing an issue here.

I understand that there is a desire to have some encapsulation principles. Where are those principles stated? I'd be interested to understand the motivations of them. In this particular case, I'm interested in understanding if/why it would be desirable to hide important performance information from developers when they use shadow trees. I stated some options on how to handle in the other issue, restating here:

  • Do no special handling: expose entries, with all the attributes set.
  • Expose PerformanceElementTiming entries but with some attributes such as element set to null.
  • Expose PerformanceElementTiming entries, but setting the element attribute value to the shadow host / shadow root.
  • Do not expose entries at all.
  • Make it opt-in so that a developer must set a bit in order to get entries from shadow DOM.

CC people on the other issue @hayatoito @annevk

@domenic
Copy link
Collaborator

domenic commented May 17, 2019

Using the shadow host makes the most sense to me. The principles at https://glazkov.com/2011/01/14/what-the-heck-is-shadow-dom/ still make sense to this day; things inside the shadow tree, like a slider track or thumb, are implementation details of the actually-interesting elements, i.e. the shadow hosts.

@annevk
Copy link
Collaborator

annevk commented May 19, 2019

@domenic, what do you mean by using the shadow host?

I think what makes the most sense, given that we now have ElementInternals, is to define a way for a custom element to participate in exposing its timing information. It can then forward any relevant details from its shadow tree that it might have access to.

@domenic
Copy link
Collaborator

domenic commented May 20, 2019

I meant

Expose PerformanceElementTiming entries, but setting the element attribute value to the shadow host / shadow root.

I agree we could have something that allows more complicated delegation, but for the cases where shadow DOM is being used to encapsulate internal implementation details (like the range input in Dimitri's blog post), the timing of the element as a whole (as represented by the shadow host) is more interesting than the timing of any individual piece.

@annevk
Copy link
Collaborator

annevk commented May 20, 2019

So where are the entries exposed? I thought it was a global method/getter?

@rniwa
Copy link
Collaborator

rniwa commented May 20, 2019

For the purpose of element timing, elements with shadow trees should be treated like any builtin element with shadow trees. Given element timing currently only supports measuring img and SVG image elements and elements with background images for now, the only shadow host with a background image is measurable as is.

Since elements inside shadow trees are considered as implementation details, no element marked with elementtiming inside shadow trees should be reported to the performance timeline.

@npm1
Copy link
Author

npm1 commented May 21, 2019

So where are the entries exposed? I thought it was a global method/getter?

The entries are exposed to PerformanceObserver. An observer can receive entries from elements corresponding to its Window.

Maybe it's worth clarifying how entries are created right now. I recently updated the explainer: there's an entry per image, per background-image (per affected element), and per group of text nodes (for example, to try to group text nodes belonging to a single paragraph under the same p element).

Exposing a single entry for each shadow tree could be an option, though it seems incomparable with these other entries. I'm also unsure how we'd define the time the shadow tree is 'ready': when all of the images and text nodes associated with it are ready?

Given element timing currently only supports measuring img and SVG image elements and elements with background images for now, the only shadow host with a background image is measurable as is.

We're also adding support for groups of text nodes (grouped under a single element).

Since elements inside shadow trees are considered as implementation details, no element marked with elementtiming inside shadow trees should be reported to the performance timeline.

So the only options are aggregating entries into an entry per shadow tree (I guess not including nested shadow trees, I'm not sure if there's a name for this), or omitting any entries of shadow trees altogether?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 21, 2019
We initially exposed these elements but this seems to break design
invariants. It is still unclear what we can do with elements in shadow
trees, but for now it is safest to not expose them at all.

Relevant threads:
WICG/element-timing#3
WICG/webcomponents#816

Bug: 879270
Change-Id: Id4c0d65e0f0e086c6c6dfa29cc019a4c1ef6ab1f
@rniwa
Copy link
Collaborator

rniwa commented May 21, 2019

We need to omit any entry originating within a shadow tree. We consider being able to detect that there is a shadow tree as a bug / undesirable as well.

@npm1
Copy link
Author

npm1 commented May 21, 2019

I don't understand what you mean by that. If I attached a shadow root to one of my elements, how would I not know that there is a shadow tree?

@rniwa
Copy link
Collaborator

rniwa commented May 21, 2019

Things inside a shadow tree shouldn't leak outside the shadow tree is the general principle. For that reason, if element timing was done inside a shadow tree, then those entries shouldn't be globally visible.

@LarsDenBakker
Copy link

As an author of applications built with web compoments and shadow dom, I would hate to lose out on being able to use the element timing API at all.

@annevk
Copy link
Collaborator

annevk commented May 22, 2019

You would, unless there's a way for a custom element to expose element timing somehow. (Similar to how your custom element can become form-associated.)

@npm1
Copy link
Author

npm1 commented May 22, 2019

So why is having elementtiming attribute set not explicit enough then?

@annevk
Copy link
Collaborator

annevk commented May 22, 2019

Because we don't want to leak the details of the shadow tree as that might lead to the light tree taking dependencies on details that ought to be changeable. That's what the encapsulation is for.

@npm1
Copy link
Author

npm1 commented May 22, 2019

Ok, so what do you propose for a way for a custom element to expose element timing somehow? How would that look like?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 22, 2019
We initially exposed these elements but this seems to break design
invariants. It is still unclear what we can do with elements in shadow
trees, but for now it is safest to not expose them at all.

Relevant threads:
WICG/element-timing#3
WICG/webcomponents#816

Bug: 879270
Change-Id: Id4c0d65e0f0e086c6c6dfa29cc019a4c1ef6ab1f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1622470
Reviewed-by: Chris Harrelson <[email protected]>
Commit-Queue: Nicolás Peña Moreno <[email protected]>
Cr-Commit-Position: refs/heads/master@{#662391}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 22, 2019
We initially exposed these elements but this seems to break design
invariants. It is still unclear what we can do with elements in shadow
trees, but for now it is safest to not expose them at all.

Relevant threads:
WICG/element-timing#3
WICG/webcomponents#816

Bug: 879270
Change-Id: Id4c0d65e0f0e086c6c6dfa29cc019a4c1ef6ab1f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1622470
Reviewed-by: Chris Harrelson <[email protected]>
Commit-Queue: Nicolás Peña Moreno <[email protected]>
Cr-Commit-Position: refs/heads/master@{#662391}
@annevk
Copy link
Collaborator

annevk commented May 23, 2019

I would expect it to be something similar to form-associated custom elements, which recently landed in the HTML Standard. You might wanna reach out to @tkent-google.

@hawkinsw
Copy link

hawkinsw commented May 30, 2019

FYI: I have contacted the relevant parties here at Mozilla about getting their feedback on this issue. The parties most interested in this issue are out on PTO this week. I hope that delay is okay? @npm1 @bgrins

@rniwa
Copy link
Collaborator

rniwa commented May 30, 2019

We discussed this briefly during the conference call this week. Apparently @tkent-google has suggested two alternatives:

  1. Exposing Performance on ShadowRoot
  2. Add a boolean argument to attachShadow's options to specify whether element timing would be exposed or not

I objected to (2) since it breaks the encapsulation of shadow roots.

Another alternative I suggested would be making PerformanceObserver and other functions on Performance take a list of shadow roots as an argument as we're planning to do for selection (see issue #79).

@smaug----
Copy link

If not obvious, I agree with @annevk and @rniwa here. By default nothing should be leaked from shadow DOM. PerformanceObserver taking list of shadow roots sounds like a possible solution to this, at least worth to investigate some more.

@diervo
Copy link

diervo commented Jun 5, 2019

I think for complex use cases like ours, where we have components from multiple authors running in the same page and the elements to be loaded are "somewhat unknown" until runtime, passing a fixed set to the Observer API will be tricky.

So having a flag either on attachShadow on in ElementInternals would be preferable since it will give us holistic control (assuming shadow creation has an abstraction, which most frameworks do).

I objected to (2) since it breaks the encapsulation of shadow roots.

@rniwa would you object even if the flag will make the observability opaque?

Basically something similar to what @domenic suggested: if I flag a shadow to be "perf-observable", whatever is reported is tied to the custom-element and none of the internals,

So any marked image or svg inside (not sure about light-shadow), will be aggregated an reported as it was in the host custom element.
Ex:

  <my-button>
    #shadow
      <img src="/icon/foo.png">
      <img src="/icon/bar.png">
  </my-button>

/* 
Observer entry will be: {
  element: my-button,
  identifier: // we probably want to not leak this neither?
  url: // probably null or an array?
  startTime: the first url
  duration: the last url - startTime (aggregation)
}
*/

As a final though, we would love to see this implemented whatever way makes encapsulation and implementors happy :)

@annevk
Copy link
Collaborator

annevk commented Jun 6, 2019

Could someone explain what the problems are with taking the same approach as with form-associated custom elements? I suggested that and since then folks have mentioned alternatives, but not provided rationale for why those alternatives are better.

@domenic
Copy link
Collaborator

domenic commented Jun 6, 2019

The main reason is that this is about shadow DOM, not custom elements. The problem can occur with no custom elements in sight, so a custom elements specific solution would not be helpful in those cases.

@annevk
Copy link
Collaborator

annevk commented Jun 6, 2019

I guess I'll reiterate that I think allowing shadow roots and non-custom elements was a mistake. We'll keep running into this if we decide those are as important as custom elements.

(That is, attachShadow() should be on ElementInternals.)

@npm1
Copy link
Author

npm1 commented Jun 10, 2019

  1. Add a boolean argument to attachShadow's options to specify whether element timing would be exposed or not

I objected to (2) since it breaks the encapsulation of shadow roots.

Can you explain why it breaks encapsulation? If the flag is passed, the component author has agreed to expose the performance information.

Another alternative I suggested would be making PerformanceObserver and other functions on Performance take a list of shadow roots as an argument as we're planning to do for selection (see issue #79).

As @diervo mentioned this is not a scalable solution for the Element Timing case.

Could someone explain what the problems are with taking the same approach as with form-associated custom elements? I suggested that and since then folks have mentioned alternatives, but not provided rationale for why those alternatives are better.

If folks are fine declaring that shadow DOM should never be used without custom elements, then that seems fine to me. However, given opt in from the custom element, can't we expose the information to the PerformanceObservers of the window? That is, both the component author and the component user should be able to look at the information with the opt in from the component author.

@rniwa
Copy link
Collaborator

rniwa commented Jun 10, 2019

Can you explain why it breaks encapsulation? If the flag is passed, the component author has agreed to expose the performance information.

Because it leaks nodes inside a shadow tree. In general, we don't provide that kind of opt-in. Opt-in we have deciding whether shadowRoot is exposed or not, and whether Event.composedPath would contain nodes inside the shadow tree or not. Note that in both cases, the users of the API is explicitly accessing the shadow DOM related API. There is no chance of nodes inside a shadow tree suddenly appearing in an existing API such that existing code can stumble upon them. That literally is the whole point of shadow DOM API.

@npm1
Copy link
Author

npm1 commented Jun 10, 2019

Ok, maybe we should split this into two questions:

  1. How to allow shadow roots / custom elements to monitor the performance of their shadow elements. This could be solved with @annevk's idea to use ElementInternals, although it's not clear to me where these entries would be dispatched (PerformanceObserver? That would be confusing as it is currently window-scoped).

  2. How to expose 'implicit registration' (heuristic-based entries about important content, for example the largest content) to the website by default. This could be solved with @domenic's idea to use the shadow host as the culprit.

@rniwa
Copy link
Collaborator

rniwa commented Jun 10, 2019

Right, those are two distinct scenarios. Note that in the case of nested shadow roots, mechanism for (2) must be used in conjunction with (1) because a shadow host which resides inside another shadow root must not be exposed to the global object. There needs to be an explicit opt-in mechanism for the author of shadow root in such a case, or some kind of propagation from shadow tree to the shadow host as we do for focus; e.g. if an element E1 inside a shadow tree S1 inside another shadow root S2 is focused, then E1 is exposed as S1.activeElement and S1.host is exposed as S2.activeElement and finally document.activeElement would point to S2.host. A similar mechanism is needed here.

@domenic
Copy link
Collaborator

domenic commented Jun 14, 2019

I've been thinking about this a lot over the last week and arrived at a point that I'd like some feedback on. Which is, perhaps open/closed shadow DOM is the right switch for this.

First, consider that with a scoped-to-the-shadow-DOM solution plus open shadow DOM, you could always do tree traversal to access the information, it would just be more awkward. In the past, we've decided to leave things like this awkward (e.g. finding the "real" activeElement requires tree traversal), but perhaps for monitoring-type performance APIs, we should allow the information to accumulate without the extra code.

Second, consider how we're seeing shadow DOM (and perhaps web components more generally) in use today. I see two main uses:

  1. The original vision, of allowing encapsulated "leaf node" components like those already in the browser. See e.g. Dimitri's early blog post.
  2. Usage as the foundation for a framework, where everything---including the app itself---becomes a web component. This is similar to how apps are structured using React, Ember, Angular, etc., but based on web components. One example of this in action is https://shop.polymer-project.org/; if you inspect, you can see that from <shop-app> on down, it's a bunch of shadow roots.

I claim that (1) benefits from strong encapsulation, even for performance monitoring APIs. If you're using a neato date picker, and the month dropdown is slow, you (the app author) don't actually want to know about the month dropdown. You just want to know that the date picker component you're using is slow. This is similar to browser built-ins: app developers don't care what implementation detail of <textarea> or <select> or whatever is slow; they just want a pointer to the element. So (1) should be using closed shadow DOM, and performance APIs should point at the shadow hosts.

Whereas (2) is more using encapsulation as a convenience, mostly for the style encapsulation. The boundaries are looser, and open shadow DOM is usually used, permitting them to be crossed if it's expedient or necessary for the app developer. In these cases, when you're using shadow DOM as an app-level component framework, it'd be nicer if performance monitoring APIs were able to peek into your app's shadow DOM, preferably without tree traversal + aggregation code.

I'm curious if this makes any sense to others. Although I've always been interested in web components for (1), I think we have to recognize that a lot of authors like using it for (2). Saying that you can't get performance data inside a shadow DOM, even an open shadow DOM, without extensive back and forth, seems like it would drive authors away from either performance APIs or from (2).

@LarsDenBakker
Copy link

@domenic I can concur from a real usage perspective. At ING we've been using web components for the past 3 years doing both (1) and (2).

We have a web component based design system for leaf components, buttons and date pickers etc. But we also use web components all the way up the tree as a component model for higher level components. Both are built by different teams with a different focus/specialty and with different requirements. For the higher level components style encapsulation is the primary reason for using shadow dom, so that different teams can easily distribute components including styles. Losing out on things like element performance timing because of that would be a shame.

To me closed/open shadow roots always seemed like the intended way to differentiate between the two use cases.

@rniwa
Copy link
Collaborator

rniwa commented Jun 15, 2019

I don't think exposing nodes in a shadow root without users of the API explicitly requesting nodes inside the shadow root is acceptable. Consider MutationObserver for example. When the mutation observer monitors the subtree, it doesn't also monitor shadow tree.

The problem here is that the existing scripts and code that uses the performance timeline to aggregate information, for example, shouldn't be encountering a node inside a shadow tree. That goes against the goal of avoiding accidental encountering of nodes within a shadow tree. In general, nodes in an open or closed shadow tree should never be exposed to API on document or window without an explicit opt-in from both the author of the shadow tree as well as the users of the API; e.g. scripts monitoring the performance timeline.

To reiterate my point earlier, at minimum, we need to an explicit method on Performance which asks for composed entries instead of exposing nodes in shadow trees to existing clients.

@domenic
Copy link
Collaborator

domenic commented Jun 15, 2019

The existing clients concern is exactly the sort of feedback I was hoping for; thank you @rniwa. I agree that if we were to allow easy (no manual tree traversal) code for getting performance entries in open shadow trees, the page author collecting those entries should explicitly opt in to it.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 19, 2019
…n Shadow Trees, a=testonly

Automatic update from web-platform-tests
[ElementTiming] Do not expose elements in Shadow Trees

We initially exposed these elements but this seems to break design
invariants. It is still unclear what we can do with elements in shadow
trees, but for now it is safest to not expose them at all.

Relevant threads:
WICG/element-timing#3
WICG/webcomponents#816

Bug: 879270
Change-Id: Id4c0d65e0f0e086c6c6dfa29cc019a4c1ef6ab1f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1622470
Reviewed-by: Chris Harrelson <[email protected]>
Commit-Queue: Nicolás Peña Moreno <[email protected]>
Cr-Commit-Position: refs/heads/master@{#662391}

--

wp5At-commits: 6272cd07e39bf7c67f58f30db41568033355b1dd
wpt-pr: 16939
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Jun 19, 2019
…n Shadow Trees, a=testonly

Automatic update from web-platform-tests
[ElementTiming] Do not expose elements in Shadow Trees

We initially exposed these elements but this seems to break design
invariants. It is still unclear what we can do with elements in shadow
trees, but for now it is safest to not expose them at all.

Relevant threads:
WICG/element-timing#3
WICG/webcomponents#816

Bug: 879270
Change-Id: Id4c0d65e0f0e086c6c6dfa29cc019a4c1ef6ab1f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1622470
Reviewed-by: Chris Harrelson <[email protected]>
Commit-Queue: Nicolás Peña Moreno <[email protected]>
Cr-Commit-Position: refs/heads/master@{#662391}

--

wp5At-commits: 6272cd07e39bf7c67f58f30db41568033355b1dd
wpt-pr: 16939
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Jul 23, 2019
We initially exposed these elements but this seems to break design
invariants. It is still unclear what we can do with elements in shadow
trees, but for now it is safest to not expose them at all.

Relevant threads:
WICG/element-timing#3
WICG/webcomponents#816

Bug: 879270
Change-Id: Id4c0d65e0f0e086c6c6dfa29cc019a4c1ef6ab1f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1622470
Reviewed-by: Chris Harrelson <[email protected]>
Commit-Queue: Nicolás Peña Moreno <[email protected]>
Cr-Commit-Position: refs/heads/master@{#662391}
@rniwa
Copy link
Collaborator

rniwa commented Oct 12, 2019

As I understand, the element timing spec has been updated to reflect the discussion here to not expose elements within shadow trees. Closing this issue for now.

@rniwa rniwa closed this as completed Oct 12, 2019
@npm1
Copy link
Author

npm1 commented Oct 15, 2019

Did I miss a resolution to the fundamental problem here? There seems to be no way to measure performance within shadow DOM, even if the author of the shadow tree wants to opt in. There are many instances where this seems desirable, for instance when web components encapsulate a web app. I don't think this should be closed, unless that problem is tracked elsewhere.

@rniwa
Copy link
Collaborator

rniwa commented Oct 15, 2019

Did I miss a resolution to the fundamental problem here? There seems to be no way to measure performance within shadow DOM, even if the author of the shadow tree wants to opt in. There are many instances where this seems desirable, for instance when web components encapsulate a web app. I don't think this should be closed, unless that problem is tracked elsewhere.

Please file a new issue to track that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants