Only finalize LCP on user events (isTrusted=true)
#635
Conversation
philipwalton
left a comment
philipwalton
left a comment
There was a problem hiding this comment.
Overall, LGTM. I just had a few minor comments.
test/e2e/onLCP-test.js
Outdated
| // Wait until all images are loaded and fully rendered. | ||
| await imagesPainted(); | ||
|
|
||
| // Pause to ensure web-vitals has a chance to collect performance metrics |
There was a problem hiding this comment.
Line 124 already has built-in waiting for collection, so that makes me wonder if this is needed for some other reason? Is there another signal we could wait for that brings the same reliability without the arbitrariness of 100ms?
There was a problem hiding this comment.
I think the racy part is DCL waits for the library to be loaded, but not for the PO to have fired before we navigate away (and then also the callbacks for run once and idle until urgent).
it seems to mostly work on Chrome but is flakey on Firefox.
And we can’t await the beacon before the navigation since it’s the navigation that triggers the beacon. I can change this to a H1 click instead of a nav to trigger the beacon and that does work. I was in two minds whether to do that anyway, but wanted to confirm the reason it was racy (and why mostly in this test).
Bumps the vendored-in web vitals library to include the changes between `5.0.2` <-> `5.1.0` from upstream #### Changes from upstream - Remove `visibilitychange` event listeners when no longer required [#627](GoogleChrome/web-vitals#627) - Register visibility-change early [#637](GoogleChrome/web-vitals#637) - Only finalize LCP on user events (isTrusted=true) [#635](GoogleChrome/web-vitals#635) - Fallback to default getSelector if custom function is null or undefined [#634](GoogleChrome/web-vitals#634) #### Our own Changes - Added `addPageListener` and `removePageListener` utilities because the upstream package changed the listeners to be added on `window` instead of `document`, so I added those utilities to avoid having to check for window every time we try to add a listener.
2 participants
Fixes #632
Note this meant the stubVisibilityChange no longer finalized LCP in the test suite so I switched most of those to a new
switchToNewTabAndBackfunction.