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

Element Timing API for images #326

Closed
1 task done
npm1 opened this issue Nov 16, 2018 · 21 comments
Closed
1 task done

Element Timing API for images #326

npm1 opened this issue Nov 16, 2018 · 21 comments
Assignees
Labels
Mode: breakout Work done during a time-limited breakout session Priority: urgent Progress: in progress Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review Topic: real user measurement Venue: Web Performance WG Venue: WICG

Comments

@npm1
Copy link

npm1 commented Nov 16, 2018

こんにちはTAG!

I'm requesting a TAG review of:

Further details (optional):

  • Relevant time constraints or deadlines: Chrome has implemented this feature but we haven't fleshed out the spec yet. We'd probably like to launch early 2019.
  • We have a Chrome launch bug (sorry, Googlers only), which received privacy and security approvals.

Security and Privacy Questionnaire: https://docs.google.com/document/d/15T51ya-7GCtDQGF_ukGAPXBkygI6WFZBV9e9Bc8mjic/edit

You should also know that...

The current proposal is for ElementTiming for <img> elements, but we received feedback at TPAC that including background images in our first iteration could also be very helpful. So we may add that. Later versions of this API would like to expose other elements, but for most we will need to think about text rendering timing. We are deferring that to a future version of this API.

We'd prefer the TAG provide feedback as (please select one):

  • leave review feedback as a comment in this issue and @-notify @npm1 and @tdresser
@travisleithead travisleithead self-assigned this Jan 22, 2019
@plinss plinss added this to the 2019-02-05-f2f milestone Jan 22, 2019
@dbaron dbaron self-assigned this Jan 22, 2019
@cynthia
Copy link
Member

cynthia commented Feb 6, 2019

It's a bit difficult to judge whether or not this is safe based on just the fact that it was "approved" under the privacy and security review process within Google. Could you fill out the S&P questionnaire or at least share us the review report at the very least?

@travisleithead
Copy link
Contributor

travisleithead commented Feb 6, 2019

Hey thanks for the review request! Some initial thoughts:

  • I assume only elements that belong to the top-level browsing context are considered for auto-inclusion in the timings? E.g., many ad frameworks manipulate iframes to fill considerable space in the viewport and which oclude other content. Since iframes are not being considered (at this point) in this spec (just img elements and maybe elements with background images), this is additional variability that would potentially decrease reporting value.
  • The auto-inclusion seems nice, but will add a tremendous amount of variability to the feature that seems like it will be challenging to rationalize and get interop across user agents--thus if an origin wants to compare data across user agents, the correlation may be difficult, leading to uncertainty and lack of confidence in the feature.
  • 'elementtiming' as an attribute on an element seems quite redundant. Perhaps something a bit shorter like 'timingname' or 'timingid'?
  • Would like additional clarity on the cross-origin availability of this data. E.g., is it proposed that all "hero" timings (including the auto-added items) be available in the performance entry lists for all browsing contexts or just the top browsing context?

@travisleithead
Copy link
Contributor

Some additional thoughts:

  • We suppose that the elementtiming attribute, if present in a closed ShadowRoot, would still be included in the Performance timeline? This might break an invarient of the closed ShadowDOM principles?

@cynthia
Copy link
Member

cynthia commented Feb 6, 2019

This would be a interesting feature for larger scale projects (with disciplined development practices and a strict review process), but feels like (in the average case) this is a property that could easily be "leaked" out into the wild unwillingly by busy developers on a tight schedule. Given that there is potential that this could also leak out in templates/libraries/components, it feels like there should be a mechanism that acts as a global off switch (equivalent of #ifdef DEBUG 0) so that you don't have to go in and patch up a bunch of third party code.

@cynthia cynthia removed the extra time label Feb 6, 2019
@npm1
Copy link
Author

npm1 commented Feb 8, 2019

Replying to the comments:

It's a bit difficult to judge whether or not this is safe based on just the fact that it was "approved" under the privacy and security review process within Google. Could you fill out the S&P questionnaire or at least share us the review report at the very least?

Sure! Here it is https://docs.google.com/document/d/15T51ya-7GCtDQGF_ukGAPXBkygI6WFZBV9e9Bc8mjic/edit

  • I assume only elements that belong to the top-level browsing context are considered for auto-inclusion in the timings? E.g., many ad frameworks manipulate iframes to fill considerable space in the viewport and which oclude other content. Since iframes are not being considered (at this point) in this spec (just img elements and maybe elements with background images), this is additional variability that would potentially decrease reporting value.

Yes, only resources in the current browsing context are measured. An iframe can still measure its own resources though (every window has its own performance object). And the entry creation will not be based strictly on visibility, so elements occluded by transparent iframes would still be reported.

  • The auto-inclusion seems nice, but will add a tremendous amount of variability to the feature that seems like it will be challenging to rationalize and get interop across user agents--thus if an origin wants to compare data across user agents, the correlation may be difficult, leading to uncertainty and lack of confidence in the feature.

We’re thinking of adding some way to identify the element due to this problem (and to help make the API more actionable), for example the URL. Do you think that would help with this concern? Note that entries that are auto-included can be distinguished from entries that are registered via the ‘name’ attribute.

  • 'elementtiming' as an attribute on an element seems quite redundant. Perhaps something a bit shorter like 'timingname' or 'timingid'?

Yea we could go with those, but we wanted to choose something that would not collide with other usage. We’ll discuss these options with the WebPerf WG.

  • Would like additional clarity on the cross-origin availability of this data. E.g., is it proposed that all "hero" timings (including the auto-added items) be available in the performance entry lists for all browsing contexts or just the top browsing context?

Hero timings will be available in the browsing context that contains the element. If a top-level browsing context contains an , then the entry will show up there. If this context has an iframe which contains another , then the entry for this image will be available only in the iframe. This is consistent with how it is handled in ResourceTiming.

  • We suppose that the elementtiming attribute, if present in a closed ShadowRoot, would still be included in the Performance timeline? This might break an invarient of the closed ShadowDOM principles?

I agree that a closed ShadowRoot should not report any timing information to DOM trees containing it.

This would be a interesting feature for larger scale projects (with disciplined development practices and a strict review process), but feels like (in the average case) this is a property that could easily be "leaked" out into the wild unwillingly by busy developers on a tight schedule. Given that there is potential that this could also leak out in templates/libraries/components, it feels like there should be a mechanism that acts as a global off switch (equivalent of #ifdef DEBUG 0) so that you don't have to go in and patch up a bunch of third party code.

Not sure I understand what you mean by ‘leak’ here? Javascript should intentionally query for entries or use PerformanceObserver in order to obtain the entries.

@npm1
Copy link
Author

npm1 commented Feb 8, 2019

Oops forgot to mention, we recently decided a modification for reporting the timing. I modified the explainer Google Doc but here is a summary of the change:

  • For images that pass the timing allow check (same origin or with Timin-Allow-Origin headers), report the load time, display time (after image has loaded), and calculate the intersectionRect when the image is displayed.
  • For images that do not pass the timing allow check, report only the load time (not the display time). In this case, we compute the intersectionRect when image has loaded so that we can deliver the entry at this time. Otherwise we'd have to wait until display time to compute the rect and deliver the entry and we'd leak some of this display timing information unintentionally.

@cynthia
Copy link
Member

cynthia commented Feb 9, 2019

Not sure I understand what you mean by ‘leak’ here? Javascript should intentionally query for entries or use PerformanceObserver in order to obtain the entries.

Leak was a poor choice of words. The concern is the potential overhead of having this enabled on a element, and libraries/components sloppily being released in the equivalent of debug mode. It feels like the top level should be able to flip the switch off to reduce overhead.

@npm1
Copy link
Author

npm1 commented Feb 11, 2019

Leak was a poor choice of words. The concern is the potential overhead of having this enabled on a element, and libraries/components sloppily being released in the equivalent of debug mode. It feels like the top level should be able to flip the switch off to reduce overhead.

Ah, so this is a performance concern? In a scenario where a website has a lot of components which spam elements containing the elementtiming attribute, I would expect the large DOM size to by far outweigh the performance cost of elementtiming. That is, all else being equal, I suspect that having/not having the elementtiming attribute in this situation won't cause significant performance differences. I could run an experiment to see if that's the case, assuming that I understood your concern correctly?

@npm1
Copy link
Author

npm1 commented Mar 19, 2019

I'd like to have TAG review for Element Timing for text as well (see explainer). Should I file a new TAG review or can it be done in this one?

@npm1
Copy link
Author

npm1 commented Apr 26, 2019

Ping, I have not heard an update from this review in months.

Also want to update: regarding closed shadow DOM, I would say that such elements should still be exposed because they also affect the performance of a website. The need to measure performance is separate from the desire to hide tree structure information within components.

@annevk
Copy link
Member

annevk commented May 17, 2019

FWIW, as I noted elsewhere, it's not acceptable to include shadow trees (of any kind) without a much wider discussion on w3c/webcomponents. We have a couple specific holes for open shadow roots, but they don't generalize to any new feature.

@npm1
Copy link
Author

npm1 commented May 17, 2019

Yes, I submitted the issues to clarify what is acceptable. For now, for the purpose of this review, we can assume shadow elements are not exposed.
WICG/element-timing#3
WICG/webcomponents#816

@annevk
Copy link
Member

annevk commented May 20, 2019

To be clear, exposing the mere existence of a shadow tree is considered an encapsulation violation.

@kenchris kenchris assigned kenchris and unassigned travisleithead May 22, 2019
@kenchris kenchris removed this from the 2019-02-05-f2f milestone May 22, 2019
@dbaron dbaron changed the title Element Timing API for img Element Timing API (images and text) May 22, 2019
@dbaron
Copy link
Member

dbaron commented May 22, 2019

@kenchris and I are looking at this in a breakout in the TAG face-to-face in Reykjavík.

I think we're both OK with using this review to cover both images and text -- though based on a quick skim it seems like text is currently covered only in the explainer but not in the text. Is that correct? That said, it seems like it might also make sense to separate if they're going to progress at substantially different rates. It seems like the text parts probably need to be developed a good bit more before we can really review them, since it's not yet clear what they are.

A few thoughts:

  • one thing I'm a little concerned about is implicit registration. It seems like it might have substantial performance overhead since finding out whether something should be implicitly registered seems like it would happen relatively late in the loading process, which would therefore require everything to be tracked up until that point, which might have a bit of overhead. Is that the case? (I notice there's a "TODO: fix implicit registration.") in the explainer.)
  • does the API support vector images, or is it just raster images? It's a little unclear.
  • defining when images are fully loaded requires a little care for animated images. Is the concern about the first frame of an animated raster image?
  • there should probably be a formal-ish definition of the elementtiming attribute somewhere that is intended to be moved into the HTML spec eventually (and when that happens, this spec could link to it).
  • How much discussion has there been of the naming here? Is it better to call a bunch of different things "element" or to call image specific information "image", etc.?
  • This seems like it's tightly tied to the work that's done in the Web Performance Working Group -- to what extent has it been discussed with the other participants of that working group, and is there a plan to move it there? (There were some points above that you said you would discuss there.)
  • We're also curious what the level of urgency is -- are there plans to ship this soon? Are those plans separate for images versus text? (And if so, that also increases the urgency of bringing into the Web Performance WG and getting it discussed effectively there...)

@dbaron dbaron added Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review and removed Progress: Pending response labels May 22, 2019
@npm1
Copy link
Author

npm1 commented May 22, 2019

I think we're both OK with using this review to cover both images and text -- though based on a quick skim it seems like text is currently covered only in the explainer but not in the text. Is that correct?

That is true - text is currently covered in the explainer but not in the spec (I imagine that's what you meant).

That said, it seems like it might also make sense to separate if they're going to progress at substantially different rates. It seems like the text parts probably need to be developed a good bit more before we can really review them, since it's not yet clear what they are.

I'll try to speed up the process of adding text to the spec draft, I'm just a bit bogged down in work right now :)

  • one thing I'm a little concerned about is implicit registration. It seems like it might have substantial performance overhead since finding out whether something should be implicitly registered seems like it would happen relatively late in the loading process, which would therefore require everything to be tracked up until that point, which might have a bit of overhead. Is that the case? (I notice there's a "TODO: fix implicit registration.") in the explainer.)

Yep implicit registration is a bit of a problem right now, and we believe that the current approach (implicitly register elements that occupy a large portion of the viewport) is not good enough because many websites do not contain any element that occupies a significant portion of the viewport... The performance overhead concern is valid, but I do believe it would be very valuable to expose something here. The overhead would come from keeping track of all elements that have painted, and when the first paint occurs computing the intersectionRect and other attributes. I believe the overhead is acceptable, but can only know when we attempt to ship. An alternative would be to ship without any implicit registration - I'm hoping to avoid that.

  • does the API support vector images, or is it just raster images? It's a little unclear.

It supports both. As long as the it's in an <img>, <image> inside <svg>, or background-image.

  • defining when images are fully loaded requires a little care for animated images. Is the concern about the first frame of an animated raster image?

Not sure I follow this question... can you clarify?

  • there should probably be a formal-ish definition of the elementtiming attribute somewhere that is intended to be moved into the HTML spec eventually (and when that happens, this spec could link to it).

It is already defined in the DOM section

  • How much discussion has there been of the naming here? Is it better to call a bunch of different things "element" or to call image specific information "image", etc.?

Not a lot... I think it's plausible to have an entryType for image and one for text. Filed an issue

  • This seems like it's tightly tied to the work that's done in the Web Performance Working Group -- to what extent has it been discussed with the other participants of that working group, and is there a plan to move it there? (There were some points above that you said you would discuss there.)

This API has also been discussed with the group and there is consensus that it is useful. We do intend to move it from incubation into the group eventually. I'll take a note to followup on the promised discussion on the 'elementtiming' attribute name, as well as maybe the entryType issue.

  • We're also curious what the level of urgency is -- are there plans to ship this soon? Are those plans separate for images versus text? (And if so, that also increases the urgency of bringing into the Web Performance WG and getting it discussed effectively there...)

We'd like to ship in September. I'd like to point out: this has been presented to the group. I can share meeting notes with you if that would be helpful, but what other type of discussion are you envisioning? In any case, the spec is not ready, so I'll send a ping to members of the group from other browser vendors to provide feedback if they wish, once the spec is in better shape.

@kenchris
Copy link

Not sure I follow this question... can you clarify?

I believe David was referring to whether you track when the first frame loaded or when all the frames did (think .GIF file)

@ylafon
Copy link
Member

ylafon commented May 23, 2019

It can also refer to progressive PNG as you have a first blurry paint before the complete image is loaded.

@ylafon
Copy link
Member

ylafon commented May 23, 2019

Also could it be used as a way to detect resource blockers, like if the measured time is too short, it is likely that the original picture was replaced by a 1x1 transparent gif, or something like that. Is there a mitigation against this?

@kenchris
Copy link

Have you thought about rate limiting the feature? So that you cannot use it on each load, or by introducing some time before you can measure again?

@npm1
Copy link
Author

npm1 commented May 23, 2019

I believe David was referring to whether you track when the first frame loaded or when all the frames did (think .GIF file)

First paint after the image is loaded and fully decoded.

Also could it be used as a way to detect resource blockers, like if the measured time is too short, it is likely that the original picture was replaced by a 1x1 transparent gif, or something like that. Is there a mitigation against this?

A resource blocker returning a 1x1 GIF is easily detectable by looking at the intrinsic size of the image. And even if it returns the same size, the image onload can be used to time whether it is the trivial image or the expensive image. In other words, Element Timing is not needed to detect the resource blocker. The mitigation is that we return a rounded rendering timestamp (nearest 8ms).

Have you thought about rate limiting the feature? So that you cannot use it on each load, or by introducing some time before you can measure again?

No. As a developer, I would expect that an element with elementtiming attribute will be exposed always. Then there is the question of whether entries from implicit registration (right now, occupying a large portion of the viewport) are expensive to compute. For that, we need further investigation to determine whether or not this causes any performance problem. If it does, I'd prefer changing the criteria for implicit registration so that it is faster to compute, rather than rate limiting.

@torgo torgo added this to the 2019-06-19-telecon milestone Jun 18, 2019
@dbaron dbaron changed the title Element Timing API (images and text) Element Timing API for images Jul 17, 2019
@dbaron
Copy link
Member

dbaron commented Jul 17, 2019

So in hindsight, my response to #326 (comment) . At this point we're done reviewing the Element Timing API for images, but we should probably look a bit more at the text bits since we didn't really look at those much yet.

So the current state of this review is that I'm going to retitle it back to what it was originally, and say that at this point we're satisfied with the responses so far... and then I'm going to file a separate issue for text, unless you'd prefer to do that.

Thanks for requesting TAG review, and for all your responses to our concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mode: breakout Work done during a time-limited breakout session Priority: urgent Progress: in progress Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review Topic: real user measurement Venue: Web Performance WG Venue: WICG
Projects
None yet
Development

No branches or pull requests

10 participants