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

surprising (if not understandable) results for first-paint when document.visibilityState === 'hidden' #40

Closed
mikesherov opened this issue Feb 8, 2019 · 24 comments

Comments

@mikesherov
Copy link

Consider the following code:

<html>
<head>
<script>
var observer = new PerformanceObserver(function(list) {
  list.getEntries().forEach(function(entry) {
    console.log(entry.startTime);
  });
});

  observer.observe({entryTypes: ['paint']});
</script>
</head>
<body>hello world</body>
</html>

If the document is hidden when this page is first loaded, e.g. it's a background tab that got loaded after restarting chrome, first-paint and first-contentful-paint will only trigger once the tab goes into the foreground. Once the tab goes into the foreground, entry.startTime will be the moment the tab is foregrounded - navigationStart.

While this makes sense because the browser is actually not painting until you foreground... this is a footgun:

  1. footgun accounted for in Perfume.js: Zizzamia/perfume.js@c07e769)
  2. footgun not mentioned in Google resources about FCP: https://developers.google.com/web/fundamentals/performance/user-centric-performance-metrics#tracking_fpfcp

I see 3 solutions:

  1. not fire paint timings if the observer was registered while the document's visibilityState is hidden.
  2. require browsers to note the time they would start rendering had the tab been foregrounded, but this seems like a hard solution to implement.
  3. leave it as is but add notes to the spec and relevant popular online resources noting this footgun.

Thoughts?

@tdresser
Copy link
Contributor

tdresser commented Feb 8, 2019

We've had some discussion about this previously, though I wasn't able to dig it up quickly.

I think #3 + an API which makes it easy to check if the page was ever backgrounded is the best solution. Relying on visibilitychanged is a bit annoying, especially if you have weird timing issues with FCP firing before your visibilitychanged listener is registered.

I agree we should add a note to the spec and other resources.

@mikesherov
Copy link
Author

My extremely naive take on:

  • an API which makes it easy to check if the page was ever backgrounded is the best solution

is that PerformanceEntry should have some sort of taintedReason property that describes if the value should potentially be discarded, like taintedReason: 'background' or something more elegant than I can self-bikeshed at the moment.

@tdresser
Copy link
Contributor

tdresser commented Feb 8, 2019

Yup, I think that makes a lot of sense!

@npm1
Copy link
Contributor

npm1 commented Mar 10, 2020

The current recommendation is to use PageVisibility and exclude entries that are created after the first time in which the document becomes hidden. But I do think it might be useful to add some tainting attribution to PerformanceEntry itself (that would probably go on the PerformanceTimeline specification, not this one).

@noamr
Copy link
Contributor

noamr commented Apr 2, 2020

I think we should be explicit in the spec that a document would only mark paint timing if it had been fully active and visible throughout its lifetime.

@npm1
Copy link
Contributor

npm1 commented Apr 2, 2020

I think it's an interesting idea but requires discussion because I'm not sure it's the correct solution. We're also needing a solution for other types of performance entries. See w3c/performance-timeline#105

@noamr
Copy link
Contributor

noamr commented Apr 3, 2020

I think it's an interesting idea but requires discussion because I'm not sure it's the correct solution. We're also needing a solution for other types of performance entries. See w3c/performance-timeline#105

I think that thread is relevant to visibility - but what about suspension due to the document going to session history? I think we should explicit about what happens to a document that gets suspended/restored from session history and hasn't yet reported FCP - in this case it't not so much about throttling...

@yoavweiss
Copy link
Contributor

yoavweiss commented Apr 3, 2020

I agree a restored document should also have an indication, but similarly, that doesn't seem like a unique concern for paint timing. Navigation Timing, resource timing, and user timings can similarly be impacted. It would be good to address this holistically.

@npm1
Copy link
Contributor

npm1 commented May 1, 2020

As an update, we discussed this on the call and from the email thread there seems to be support to having a PageVisibility observer, so that's the current approach we'll take to fixing this issue.

@mikesherov
Copy link
Author

@npm1 yes, it can be worked around with page visibility, but the problem is that the current results are surprising. Having a property on a Timing called taintedReason (or some other better named property) would be a signal as to why the surprising result exists.

Consider the 2 different user experiences:

  1. WITHOUT A PROPERTY: a dev implements page timing, and suddenly notices really large values coming out of it. They don't know why and have to awkwardly search for the reason, and then implement the pageVisibility workaround to get it to work.
  2. WITH A PROPERTY: a dev implements page timing, and suddenly notices really large values coming out of it. They poke around the paint timing record, and notice there's a taintedReason property. They don't know what it is, and google it, and find that they can ignore records with a falsy taintedReason value.

I'd argue that especially for telemetry tools, more info here, the better.

@yoavweiss
Copy link
Contributor

The result may be surprising, but as mentioned above, that's not unique to Paint Timing, and having a boolean value on the entry will give you less information than an observer. The impact of briefly being in the background and never being in the foreground can obviously be different on different timings. Compressing that information into a boolean seems extremely lossy.

@mikesherov
Copy link
Author

taintedReason wouldn't be a boolean. It would be a description of why it was tainted.

@yoavweiss
Copy link
Contributor

What would be potential values? How would you use it?

@mikesherov
Copy link
Author

My extremely naive take on:

  • an API which makes it easy to check if the page was ever backgrounded is the best solution

is that PerformanceEntry should have some sort of taintedReason property that describes if the value should potentially be discarded, like taintedReason: 'background' or something more elegant than I can self-bikeshed at the moment.

See above ^. My thought here is that ergonomics outweigh the workarounds with pageVisibility.

@npm1
Copy link
Contributor

npm1 commented May 4, 2020

We're not tackling 'other' reasons at this time, so it would basically be a boolean signal (backgrounded or not). Given the tradeoff of ergonomics vs flexibility, we had a thread in the web perf mailing list. People supported the observer instead of the explicit signal in the entry. Here it is:
https://lists.w3.org/Archives/Public/public-web-perf/2020Apr/0005.html

@mikesherov
Copy link
Author

I read the thread and it seemed like most thought both 1 and 2 would be useful.

I'm curious to see what the code looks like for the solution to this problem using a hypothetical buffered PageVisibilityObserver.

I'm a bit confused though with where to register the feedback. Should the issue be discussed here or on the mailing list?

@npm1
Copy link
Contributor

npm1 commented May 6, 2020

Hmm good question, I guess we did both for this particular issue. Do you disagree with the current proposal to use PageVisibilityObserver along with documentation in PaintTiming to explicitly point out this issue to users? The code would look something like this I guess:

// Calculate first time the page was backgrounded.
// Observer would need to include an entry corresponding to the initial state.
let firstBackground = -1;
new PageVisibilityObserver(entries => {
  entries.forEach(entry => {
    if (entry.visibilityState === "hidden") {
      // startTime would be the time in which state change occurred.
      firstBackground = entry.startTime;
      return;
    }
  }
}).observe(true /* buffered */);
// Paint Timing would simply use firstBackground.
performance.getEntriesByType('paint').forEach(entry => {
  if (firstBackground !== -1 && entry.startTime < firstBackground)
    continue;
  // Report entry to analytics
});

@mikesherov
Copy link
Author

Yeah, that code doesn't bad but I do think users are going to have a hard time discovering the solution, and I think the feedback in the thread about other taint reasons, like having console open, is interesting. But perhaps when and if y'all decide to address devtools tainting, than that's the time to reraise this issue.

@marcusdarmstrong
Copy link

To echo my comments from the public-web-perf list, I think the underlying problem on the discoverability front is that page visibility isn't what we're interested in observing in this case, but rather interventions* taking place that are triggered by that page visibility—accordingly, a possible solution is to provide links between affected performance entries and ReportingObserver's entries.

* Note: I would include delayed first paint due to backgrounding in an overall "intervention" bucket even though it's not currently an intervention in the way that chrome typically uses that term, but interventions like Background Tab Resource Load Throttling would also directly impact paint timings even if paint while in a backgrounded state was possible.

@npm1
Copy link
Contributor

npm1 commented May 11, 2020

Unless it's defined in an interoperable way I don't think we'd be able to add it.

@npm1
Copy link
Contributor

npm1 commented May 15, 2020

I wrote a summary of the current discussions: https://bit.ly/364z2PB

@mikesherov
Copy link
Author

Thanks so much @npm1 for your patient and thoughtful write up and for summarizing and championing the concerns well.

@npm1
Copy link
Contributor

npm1 commented Jun 15, 2020

Hey! Just wanted to give an update here. There was not agreement regarding surfacing the tainted attribute on the entry. The rationale is that this is something that is somewhat independent from the entry itself. In addition, most developers are expected to be using libraries that know how to extract the performance data rather than code this directly. Finally, it does not seem feasible to standardize all the possible reasons the browser may think that an entry is affected by some external issues. So my plan at the moment is to solve the visibility problem via an observer (to provide full visibility history) and put the tainted flag on hold for now.

@mikesherov
Copy link
Author

Thanks for considering and all the listening and advocacy you did on behalf of the concern I raised here. I'll close this issue now. Keep up the great work!

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

6 participants