-
Notifications
You must be signed in to change notification settings - Fork 14
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
How should Element Timing interact with Shadow DOM? #3
Comments
It should be the same for open/closed and encapsulation should be preserved. So I think you should not expose anything from shadow trees, basically. |
What is the benefit of 'encapsulation' when we are trying to measure the performance of a website? I think it would be very unfortunate if shadow trees cannot be measured and act like black boxes. |
It's not about benefits, it's a design invariant. What you probably need to do is define some ElementInternals API to let a custom element decide what kind of timing it wants to expose to the outside. |
Ok, I think this is fine as long as the performance of slots can be monitored in practice. In other words, following the example on MDN, if I have this slot in a shadow tree of a
And I instantiate the element like this:
then In terms of how to spec it, the check would basically be to not observe |
Correct / you might wanna check that the element's root is a document associated with a browsing context or some such instead. Depends a bit on your other preconditions. |
Ok thanks @annevk! I'm not super familiar with Shadow DOM, could you elaborate on what's the design invariant for open shadow DOM here? Hayato suggested we could have an opt-in for open shadow, something like:
Is that something you'd be strongly opposed to? |
Yeah, we don't have that kind of thing for other APIs, such as mutation observers. If we did things like that it should be done consistently and also agreed upon by the wider Web Components community. Thus far we've been very conservative, including for open shadow roots. |
I am afraid that that's not what we want here, in practice. For example, given that:
Then, if a document tree uses
Then, we could get almost nothing useful information here; We only get info about "bar". We couldn't get any info about foo1, foo2, nor foo3. I'm afraid that this is not what we want. |
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: w3c/element-timing#3 WICG/webcomponents#816 Bug: 879270 Change-Id: Id4c0d65e0f0e086c6c6dfa29cc019a4c1ef6ab1f
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: w3c/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}
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: w3c/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}
…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: w3c/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
…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: w3c/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
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: w3c/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}
@npm1 can you describe what is going to ship in Chromium, and can the spec be updated to reflect that if it's a conservative approach? I spotted a link to this issue in https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/image_element_timing.cc?l=113&rcl=85892919ac9a65a0a99924a43f7256c1f6bd36eb, and there is an actual check there that could be put in the spec somewhere to match. |
We're ignoring shadow DOM elements even if they have their elementtiming attribute set, for now. I attempted to reflect this in the spec as follows: In https://wicg.github.io/element-timing/#sec-element-processing we iterate over doc's descendants (for shadow DOM I think it needs to be shadow-including descendant). |
Although the current shape of the specification makes sense, I think there is a need to break down the use case for how a web site wants to measure elements created using shadow DOM and to ensure the patterns are well understood and can be used effectively. @npm1 Has this work been done? I recently spoke with the msn.com team and they stated they have been unable to use ElementTiming for some of their components due to this limitation are currently reliant on JavaScript performance.mark. Would be happy to put them in touch with folks to talk this through. Should it be a new issue given this issue seems clearly about ensuring the shadow DOM does not bleed over its encapsulation? |
@toddreifsteck - Yeah, it'd be great if you/they can open a new issue and outline the reasons as to why they can't use ElementTiming for their use case. |
Thinking about this problem (after discussion at TPAC), I see a few possible solutions: Option 1: top-down As an analogue, consider element.getInnerHTML({
includeShadowRoots: true,
closedRoots: [shadowRoot1, shadowRoot2, ...]
}); Patching this onto const observer = new PerformanceObserver(/* ... */);
observer.observe({
entryTypes: ["element"],
includeShadowRoots: true,
closedRoots: [shadowRoot1, shadowRoot2, ...]
}); This would recursively pierce into open shadow roots, but closed shadow roots would need to be explicitly enumerated. Pros: Simplest to implement from the Option 2: bottom-up An alternative solution would be something like cross-root aria delegation, but allowing a component to opt-in to exposing its customElements.define('x-foo', class extends HTMLElement {
constructor() {
super();
this.attachShadow({ mode: "open", delegatesElementTiming: true })
.innerHTML = '<div elementtiming="foo"></div>'
}
}); Pros: Could piggy-back on existing cross-root aria delegation API (assuming it expands to allow non-aria attributes and to delegate multiple elements+attributes from one shadow root). Component authors opt-in. Option 3: "it just works" A third option is to make Pros: Aligns with existing implementation of LCP (in Chromium at least, AIUI). No extra work for web authors. In a sense, the Based on this, my suggestion would be either option 3 or option 1. |
We definitely shouldn't do option (3). It fundamentally goes against the encapsulation principle of shadow DOM. |
I'm also not a big fan of option 1 as written. We shouldn't have a separate API for open and closed. The design for declarative shadow trees doesn't have consensus. cc @smaug---- |
How about another option: Option 4: global opt-in Similar to option 2, but doesn't require hoisting customElements.define('x-foo', class extends HTMLElement {
constructor() {
super();
this.attachShadow({
mode: "open",
expose: ['elementtiming']
})
.innerHTML = '<div elementtiming="foo"></div>'
}
}); Or declaratively (rough sketch based on DSD): <x-foo>
<template shadowroot="open" expose="elementtiming">
<div elementtiming="foo"></div>
</template>
</x-foo> Pros: no distinction between open/closed, future-proof to allow exposing more attributes (the |
I think of these I prefer option 4, especially if we can figure out other use cases for the expose attribute. It is very explicit. |
@smaug---- In our research for cross-root ARIA delegation/reflection, we identified several attributes that would be useful to expose outside of shadow roots ( Although maybe there are some I missed? Or that may be defined in the future? /cc @Westbrook |
We discussed this at TPAC, and plan to continue the discussion in a near-future WG call. I think that the option that @dominiccooney outlined seems reasonable:
|
What's a shadow element? A shadow host? (A custom element taking care of exposing the encapsulated information from its shadow tree to its tree makes the most sense to me, which sounds like option 2 above.) |
Indeed, apologies. |
This would expose multiple elements from the shadow root, if there are multiple customElements.define('x-foo', class extends HTMLElement {
constructor() {
super();
this.attachShadow({ mode: "open", delegatesElementTiming: true }).innerHTML = `
<!-- foo and bar are both exposed -->
<div elementtiming="foo"></div>
<div elementtiming="bar"></div>
`;
}
}); (I'm assuming option 2 above.) |
Yup, it would expose those timings to the global timeline. We'd need to think about "namespacing" them so it's clear they are coming from the custom element. |
Ah, I misunderstood. I thought that (like cross-root ARIA), the timings would need to be re-exported up the tree (option 2). If they are exported globally (with namespacing), then that sounds more like option 4. So could we expand option 4 with something like this? customElements.define('x-foo', class extends HTMLElement {
constructor() {
super();
this.attachShadow({
mode: "open",
+ expose: {
+ attributes: ['elementtiming'],
+ namespace: 'someGloballyUniqueString'
+ }
})
.innerHTML = '<div elementtiming="foo"></div>'
}
}); Would the Context on Salesforce's use of shadow DOMFor context, the Salesforce CRM app is not a light DOM with a sprinkling of shadow roots; it's basically nothing but shadow roots (deeply nested). So for us to take advantage of element timing with option 2, we would need to re-export possibly dozens of times up the tree, which is not ideal. So my preference is probably option 4. |
The option you outlined above works for me. Are you interested in pushing its spec and/or implementation? I'm happy to help on either front! :) |
I wonder what @rniwa thinks of option 4. Why is it better than 2? And how exactly would exposure happen? Would the data end up being copied somewhere or would there be some pointer into the shadow root available globally? |
Option 2 seems most straight forward & matches the other existing APIs. Fundamentally, it's problematic when either users or components can't decide whether to export element timing or not. It seems particularly problematic for each component author to make this decision for all uses of that component regardless of where they appear. |
Thinking about this some more, I don't know that we even need an explicit opt-in, as the So, option 5:
That way we don't expose any details but the timing to the non-shadow DOM, unless the element explicitly opts-in for that, This would match what's happening in practice for LCP, where the element's timing is reported, but without a pointer to the actual element. I guess we could use the same opt-in to enable exposing the actual element to LCP. Edit: I guess this is an expansion of option 3 above.. |
The cons of option 2 seems to outweigh their benefits, IMO.
There is no point in adding |
A component could want to collect timing data locally and only give a more abstract view to the outside world. The whole point is that these are building blocks for elements, treating them as if they are normal elements just hidden from view defeats the point. And that's also why making |
What do you mean by "locally"? Components don't have their own execution contexts, right? |
Within the logic created for that component. And no, they don't. |
I don't believe it will be neither ergonomic nor desired for each component to start collecting its own metrics. Under that assumption, we need to figure out if the |
FYI - we're planning to discuss this on the WG call today at 10am PT/7pm CET |
I assume this means the value of the
Is there value in exposing the ID if the element itself is exposed? Overall Option 5 works from my POV. It is a bit un-ergonomic that one would have to deeply traverse all shadow roots and run |
Indeed.
That's what we do for element timing in general, so doing the same seemed consistent. The value is in case the element itself was removed (and hence is null). |
Element Timing exposes rendering timing information about images (and we want to add text soon). It has some attributes which contain useful information for attribution such as
element
. However, right now we are not specifying anything special about elements that are contained in shadow trees (not even if those are closed). What should we do in these cases? I guess we can split this question into two:Possible alternatives I can think of:
element
set to null.element
to the shadow host.I'd like some feedback here. Any thoughts? @annevk @rniwa
Also tagging @hayatoito who was the one who suggested the opt-in option for open shadow trees and said we should not expose the elements of closed shadow trees. I understand this constraint (and I will need to explicitly add checks to the spec) but we also want to enable developers to measure the performance of their website, and shadow trees can affect it, so ideally we can still expose valuable attribution.
The text was updated successfully, but these errors were encountered: