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

How should Element Timing interact with Shadow DOM? #3

Open
npm1 opened this issue May 9, 2019 · 37 comments
Open

How should Element Timing interact with Shadow DOM? #3

npm1 opened this issue May 9, 2019 · 37 comments

Comments

@npm1
Copy link
Collaborator

npm1 commented May 9, 2019

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:

  • What should be done for open trees?
  • What should be done for closed trees?

Possible alternatives I can think of:

  • 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 to the shadow host.
  • 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.

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.

@hayatoito
Copy link

hayatoito commented May 10, 2019

@npm1 Thank you for filing an issue!

@annevk @rniwa, could you help @npm1? We'd appreciate your thoughts to move Element Timing API forward.

cc: @chrishtr just in case.

@annevk
Copy link
Contributor

annevk commented May 10, 2019

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.

@npm1
Copy link
Collaborator Author

npm1 commented May 10, 2019

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.

@annevk
Copy link
Contributor

annevk commented May 11, 2019

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.

cc @domenic @tkent-google

@npm1
Copy link
Collaborator Author

npm1 commented May 16, 2019

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 my-paragraph custom element:

<p><slot name="my-text">My default text</slot></p>

And I instantiate the element like this:

<my-paragraph>
	<span slot="my-text" id='theid'>Let's have some different text!</span>
</my-paragraph>

then document.getElementById('theid') works (i.e. returns the element), which means the element used to replace the slot is not part of the shadow DOM, correct?

In terms of how to spec it, the check would basically be to not observe element if "element's` root is a shadow root"?

@annevk
Copy link
Contributor

annevk commented May 16, 2019

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.

@npm1
Copy link
Collaborator Author

npm1 commented May 16, 2019

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:

observer.observe({entryTypes: ['element'], includesOpenShadow: true});

Is that something you'd be strongly opposed to?

@annevk
Copy link
Contributor

annevk commented May 16, 2019

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.

@hayatoito
Copy link

hayatoito commented May 17, 2019

@npm1

Ok, I think this is fine as long as the performance of slots can be monitored in practice.

I am afraid that that's not what we want here, in practice.

For example, given that:

<my-paragraph>'s shadow tree is:

  <p><slot name="my-text">My default text</slot></p>

<my-app>'s shadow tree is:

  <h1><slot name="my-title">My default Title</slot></h1>
  <my-paragraph><span slot="my-text" id='foo1'>foo1</span></my-paragraph>
  <my-paragraph><span slot="my-text" id='foo2'>foo2</span></my-paragraph>
  <my-paragraph><span slot="my-text" id='foo3'>foo3</span></my-paragraph>

Then, if a document tree uses <my-app>, as such:

  <my-app><p><slot name="my-title" id='bar'>bar</slot></p></my-app>

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.

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
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 <chrishtr@chromium.org>
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
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 <chrishtr@chromium.org>
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662391}
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 <chrishtr@chromium.org>
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
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 <chrishtr@chromium.org>
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
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 <chrishtr@chromium.org>
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662391}
@foolip
Copy link
Member

foolip commented Jul 24, 2019

@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.

@npm1
Copy link
Collaborator Author

npm1 commented Jul 24, 2019

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-process-loaded-image, step 4 checks whether the root is a document (for shadow DOM it would be a shadowRoot).

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).

@toddreifsteck
Copy link

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?

@yoavweiss
Copy link
Collaborator

@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.

@nolanlawson
Copy link

nolanlawson commented Sep 13, 2022

Thinking about this problem (after discussion at TPAC), I see a few possible solutions:


Option 1: top-down

As an analogue, consider getInnerHTML from Declarative Shadow DOM. This allows for:

element.getInnerHTML({
  includeShadowRoots: true,
  closedRoots: [shadowRoot1, shadowRoot2, ...]
});

Patching this onto PerformanceObserver:

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 PerformanceObserver user's perspective, no need for component opt-in. Has precedent in getInnerHTML.
Cons: Open-shadow-root components cannot "opt out" – they are opted-in by default.


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 elementtimings up the tree:

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.
Cons: Multiple nested shadow roots would need to delegate all the way up the tree, which is awkward to implement for both component authors and page authors.


Option 3: "it just works"

A third option is to make elementtiming "just work," even within closed/open shadow roots.

Pros: Aligns with existing implementation of LCP (in Chromium at least, AIUI). No extra work for web authors. In a sense, the elementtiming attribute is already an "opt-in," since there's no other reason for an element to use it except to get Element Timings.
Cons: No way for component authors to opt-out if (for whatever reason) they use the elementtiming attribute but don't actually want to get Element Timings. No other precedent (I can think of) for where an attribute on an element in a shadow root is "observable" by script with only access to the global scope.


Based on this, my suggestion would be either option 3 or option 1.

@rniwa
Copy link

rniwa commented Sep 15, 2022

We definitely shouldn't do option (3). It fundamentally goes against the encapsulation principle of shadow DOM.

@annevk
Copy link
Contributor

annevk commented Sep 15, 2022

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----

@nolanlawson
Copy link

nolanlawson commented Sep 16, 2022

How about another option:

Option 4: global opt-in

Similar to option 2, but doesn't require hoisting elementtimings all the way up the tree:

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 expose attribute could accept a space-separated list), no need to hoist multiple times – it's a global opt-in. Works declaratively.
Cons: no way to expose elementtiming for one element but not another. (Not sure this is a common use case.) Breaks encapsulation from the POV of shadow roots enclosing the shadow root doing the exposeing.

@smaug----
Copy link

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.
The problem it does have is that if one uses expose with a closed shadow root, it does break encapsulation.
But, in some sense expose is like a C++ friend class, you let some specific other API to poke into the internals.

@nolanlawson
Copy link

@smaug---- In our research for cross-root ARIA delegation/reflection, we identified several attributes that would be useful to expose outside of shadow roots (aria-*, role, for, list, popup, etc.), but in each of those cases I think it would be best to only expose the attribute "one level up" from the shadow root. I admit I can't think of any attributes other than elementtiming where we really just want to expose it directly at the global level.

Although maybe there are some I missed? Or that may be defined in the future?

/cc @Westbrook

@yoavweiss
Copy link
Collaborator

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:

  • By default open/closed shadow roots are seen as a single element from Element Timing's perspective, and the first paint inside the element counts as its render time.
  • Option 2 would override the custom element's element timing with some internal element's timing
  • Using the element-timing attribute directly on a shadow element would expose that internal element's timing to the global timeline.

@annevk
Copy link
Contributor

annevk commented Sep 21, 2022

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.)

@yoavweiss
Copy link
Collaborator

What's a shadow element? A shadow host?

Indeed, apologies.

@nolanlawson
Copy link

Using the elementtiming attribute directly on a shadow element would expose that internal element's timing to the global timeline.

This would expose multiple elements from the shadow root, if there are multiple elementtiming attributes inside of the shadow root, correct?

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.)

@yoavweiss
Copy link
Collaborator

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.

@nolanlawson
Copy link

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 PerformanceElementTiming have a namespace property then?

Context on Salesforce's use of shadow DOM

For 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.

@yoavweiss
Copy link
Collaborator

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! :)

@annevk
Copy link
Contributor

annevk commented Sep 27, 2022

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?

@rniwa
Copy link

rniwa commented Sep 27, 2022

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.

@yoavweiss
Copy link
Collaborator

yoavweiss commented Sep 29, 2022

Thinking about this some more, I don't know that we even need an explicit opt-in, as the elementtiming attribute is already an opt-in in and of itself.

So, option 5:

  • Any elementtiming attribute inside of a shadow tree would queue up an ElementTiming entry for the global observers. By default such entries would have an element attribute which points to the top-most shadow host*, and an id attribute which contains the shadow host's ID if any. The element timing's identifier would be properly reflected, and would provide developers with a clue as to which actual element queued up the entry.
    • "top most shadow host" - the ancestor shadow host that's part of the non-shadow DOM
  • If the shadow DOM provides an opt-in (e.g. when attached), the internal element and its id would be exposed in the corresponding ElementTiming attributes.

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..

@yoavweiss
Copy link
Collaborator

Option 2 seems most straight forward & matches the other existing APIs.

The cons of option 2 seems to outweigh their benefits, IMO.

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.

There is no point in adding elementtiming attributes if one does not want to expose those timings. What's the deveoper benefit of providing multiple, nested opt-ins?

@annevk
Copy link
Contributor

annevk commented Sep 29, 2022

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 elementtiming be some kind of magical feature is not a great idea. We want exposure to be a very conscious decision and we don't want to end up with a handful of differently named attributes that end up doing that I think.

@yoavweiss
Copy link
Collaborator

What do you mean by "locally"? Components don't have their own execution contexts, right?

@annevk
Copy link
Contributor

annevk commented Sep 29, 2022

Within the logic created for that component. And no, they don't.

@yoavweiss
Copy link
Collaborator

I don't believe it will be neither ergonomic nor desired for each component to start collecting its own metrics.
So it makes sense that if the component emits metrics, they'd be emitted to the global performance observers.

Under that assumption, we need to figure out if the elementtiming attribute is in itself a good enough opt-in, or we need developers to jump through more hoops (and if so, to what purpose).

@yoavweiss
Copy link
Collaborator

FYI - we're planning to discuss this on the WG call today at 10am PT/7pm CET

@nolanlawson
Copy link

The element timing's identifier would be properly reflected, and would provide developers with a clue as to which actual element queued up the entry.

I assume this means the value of the elementtiming attribute, not the ID of the element itself?

If the shadow DOM provides an opt-in (e.g. when attached), the internal element and its id would be exposed

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 shadowRoot.querySelector("[elementtiming=foo]") to find the elementtiming=foo element, but that does give an incentive for the opt-in. (Assuming open shadow – with closed shadow, the opt-in would be mandatory.)

@yoavweiss
Copy link
Collaborator

I assume this means the value of the elementtiming attribute, not the ID of the element itself?

Indeed.

Is there value in exposing the ID if the element itself is exposed?

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).

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

No branches or pull requests

9 participants