GoogleChrome / web-vitals

Essential metrics for a healthy site.
https://web.dev/vitals
Apache License 2.0
7.59k stars 415 forks source link

LCP monitoring can add to click/keyboard event handler #383

Closed tunetheweb closed 1 year ago

tunetheweb commented 1 year ago

LCP is finalized when a click or keyboard event happens.

To do this we attached an event listener: https://github.com/GoogleChrome/web-vitals/blob/27db8a0644f76550fcb4ef17555b9b53f673870d/src/onLCP.ts#L90-L106

This results in the onLCP callback being fired as part of the click handler. That can extend the click handler potentially causing INP issues.

See this trace as an example: https://trace.cafe/t/T78ZsydUcb around the 4330 ms mark and the gtag function that the site sets onLCP adds 77ms to the already slow click handler:

image

We could wrap the stopListening function contents in a setTimeout or one of the new schedular APIs so the callback happens in it's own task rather than as part of the click/keyboard event handler.

philipwalton commented 1 year ago

I remember considering this when originally writing this logic, and I ultimately decided not to because it's entirely possible that the first user click is on an outbound link, in which case the use of setTimeout() would likely mean that the callback never executes.

Instead of wrapping the stopListening() function in a setTimeout(), you could potentially do it in the addEventListener() call. Something like:

['keydown', 'click'].forEach((type) => {
+  addEventListener(type, () => setTimeout(stopListening, 0), true);
-  addEventListener(type, stopListening, true);
});
tunetheweb commented 1 year ago

Would that be any less likely to fail on the link case scenario? Or just as likely and you’re thinking now that’s maybe an acceptable risk, given the potential INP benefit?

philipwalton commented 1 year ago

Sorry, I'm realizing now I didn't fully explain my idea :)

Would that be any less likely to fail on the link case scenario?

It would be equally likely to fail, but the onHidden(stopListening) logic below would catch the case of the user clicking an external link (since navigating away triggers a visibilitychange event).

If you wrapped the stopListening() logic in a setTimeout() then both would likely fail, which is what my proposal was trying to avoid.

tunetheweb commented 1 year ago

Gotcha makes sense. Will test and send in a PR.