element-timing icon indicating copy to clipboard operation
element-timing copied to clipboard

How should Element Timing interact with Shadow DOM?

Open npm1 opened this issue 6 years ago • 46 comments

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.

npm1 avatar May 09 '19 20:05 npm1

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

hayatoito avatar May 10 '19 05:05 hayatoito

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.

annevk avatar May 10 '19 10:05 annevk

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.

npm1 avatar May 10 '19 18:05 npm1

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

annevk avatar May 11 '19 07:05 annevk

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

npm1 avatar May 16 '19 15:05 npm1

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.

annevk avatar May 16 '19 15:05 annevk

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?

npm1 avatar May 16 '19 15:05 npm1

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.

annevk avatar May 16 '19 18:05 annevk

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

hayatoito avatar May 17 '19 03:05 hayatoito

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

foolip avatar Jul 24 '19 10:07 foolip

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

npm1 avatar Jul 24 '19 15:07 npm1

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 avatar Jun 30 '22 17:06 toddreifsteck

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

yoavweiss avatar Jul 01 '22 08:07 yoavweiss

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.

nolanlawson avatar Sep 13 '22 18:09 nolanlawson

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

rniwa avatar Sep 15 '22 05:09 rniwa

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

annevk avatar Sep 15 '22 14:09 annevk

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.

nolanlawson avatar Sep 16 '22 17:09 nolanlawson

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.

smaug---- avatar Sep 16 '22 17:09 smaug----

@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

nolanlawson avatar Sep 16 '22 21:09 nolanlawson

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.

yoavweiss avatar Sep 20 '22 14:09 yoavweiss

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

annevk avatar Sep 21 '22 08:09 annevk

What's a shadow element? A shadow host?

Indeed, apologies.

yoavweiss avatar Sep 21 '22 08:09 yoavweiss

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

nolanlawson avatar Sep 21 '22 14:09 nolanlawson

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.

yoavweiss avatar Sep 21 '22 14:09 yoavweiss

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.

nolanlawson avatar Sep 21 '22 18:09 nolanlawson

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

yoavweiss avatar Sep 26 '22 08:09 yoavweiss

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?

annevk avatar Sep 27 '22 16:09 annevk

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.

rniwa avatar Sep 27 '22 16:09 rniwa

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 avatar Sep 29 '22 06:09 yoavweiss

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?

yoavweiss avatar Sep 29 '22 06:09 yoavweiss