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

onLCP may report even page was hidden previously #325

Closed
zuoaoyuan opened this issue Mar 4, 2023 · 4 comments · Fixed by #326
Closed

onLCP may report even page was hidden previously #325

zuoaoyuan opened this issue Mar 4, 2023 · 4 comments · Fixed by #326

Comments

@zuoaoyuan
Copy link

Some metrics like LCP & FID only report if the page wasn't hidden prior to the metrics.
FID: https://github.com/GoogleChrome/web-vitals/blob/main/src/onFID.ts#L59
LCP: https://github.com/GoogleChrome/web-vitals/blob/main/src/onLCP.ts#L63-L66

However, in case ActivationStart has a non-zero value like pre-render, LCP compare with the page hidden time after subtracting the activation time . Shouldn't we compare before substract?

@zuoaoyuan
Copy link
Author

@tunetheweb Hey Barry, can you help confirm since you're the prerender expert:)

@tunetheweb
Copy link
Member

Yes I think you are correct that this should only be reported if the lastEntry.startTime was before the firstHiddenTime.

However, prerendered pages cannot currently be activated as background tabs (the prerender is currently discarded and the page is loaded as a fresh navigation if someone does right-click->open in a new tab). So this only affects if the page was prerendered AND the LCP was NOT finished by the time the prerender was activated (so the prerender wasn't completed before activation) AND the page was then hidden before the LCP was showing AND the page was then reactivated AND the LCP was shown AND this happened in a time that allowed it to report. I'd imagine that's reasonable rare.

Take these examples:

Activation = 1 second
Hidden = 1.2 second
Unhidden 2.0 seconds
LCP = 2.5 second

In that case the check would be that 1.5 seconds < 1.2 seconds, which fails, so the event would not be reported.

However in this case, the current logic is wrong:

Activation = 1 second
Hidden = 1.2 second
Unhidden 1.4 seconds
LCP = 1.9 second

In that case it would be 0.9 seconds < 1.2 seconds, which succeeds, to the event would be reported (even though it shouldn't). But it does depend on hiding and then unhiding a page really quickly.

But it's still incorrect and an easy fix so will submit a PR now.

@tunetheweb
Copy link
Member

Happy with the change in #326 ?

@zuoaoyuan
Copy link
Author

That would be a super rare case I agree.
And the fix looks great, thanks!

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

Successfully merging a pull request may close this issue.

2 participants