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

Watching for 'scroll' event may end LCP before a valid browser entry is created #75

Closed
valbooth28 opened this issue Oct 2, 2020 · 6 comments · Fixed by #84
Closed
Assignees
Milestone

Comments

@valbooth28
Copy link

TLDR; I believe web-vitals marks isFinal on all types of scroll events when only some scroll events stop largest-contentful-paint browser entries.

Background

Thanks for open sourcing this!

When we started trying to use getLCP in our code at page start, the reporter would never fire. We noticed that our component library's positioning changes caused a scroll event before any meaningful render, that the web-vitals' input handler would catch. The page hadn't rendered anything, so there weren't any LCP entries for web-vitals to report to our callback. All well and fine, we were causing a scroll event, and the docs describe marking isFinal on scroll.

However, in our case, the Web Chrome Vitals Extension later displayed an LCP value. The extension would call getLCP on tab-complete, well after the scroll that stopped our web-vitals instance and read LCP entries just fine.

Let me know if you want a repro. The code is still internal, so it may take some time to get a fiddle up.

I get the impression this library watches for scroll because it reads entries created by the browser, and the browser stops recording on scroll. Through my use case, though, I think I've found that this library stops on any scroll event when the browser only stops creating the largest-contentful-paint entries on some scroll events.

LCP logic in Chromium

Searching for LCP and scroll logic in Chromium, I think I might have found the corresponding files.

Many types of scrolls I think all create `Event.type = 'scroll':
https://github.com/chromium/chromium/blob/a4f27fdc4913d800752bd1cb9533d31357163d65/third_party/blink/renderer/core/scroll/scroll_types.h#L58

But LCP only stops recording on two types of scroll events:
https://github.com/chromium/chromium/blob/55f44515cd0b9e7739b434d1c62f4b7e321cd530/third_party/blink/renderer/core/paint/paint_timing_detector.cc#L188

Remediation?

I admittedly don't understand what kCompositorScroll is, but the library could listen to key up, key down, and wheel device events instead of scroll as a proxy for 'kUserScroll'.

If this is a valid bug and has a straightforward fix, let me know, I'm happy to open contribute.

@philipwalton
Copy link
Member

Interesting, @npm1 can you speak to Chrome's kUserScroll and kCompositorScroll usage here? Could there be some scroll events that addEventListener('scroll') would catch but wouldn't end LCP observation?

@npm1
Copy link

npm1 commented Oct 5, 2020

We should stop on scrolling caused by user, which is not the case here. Does the listener check that the event has isTrusted set?

@npm1
Copy link

npm1 commented Oct 5, 2020

Although window.scroll() does create a trusted event despite not being caused by the user so that seems insufficient. @flackr do you know if there's a reliable way to detect user-initiated scrolls?

@philipwalton
Copy link
Member

Ping @flackr

Unless we find a better way to detect only user-initiated scrolls, we'll likely have to drop this check and only listen for keypress and click. Ignoring scroll events won't affect the reliability of the results, but it will likely affect how soon tools like the Web Vitals Chrome Extension can report the metric as "final" (cc: @addyosmani)

@flackr
Copy link

flackr commented Oct 22, 2020

There is currently no reliable way to detect user-initiated scrolls. This is because the scroll event itself is an observation of the scroll position having change rather than a direct event causing it.

You could however try to observe all possible user initiated scroll sources (mousewheel, pointerdown / pointermove, down arrow / page down / space bar?) in combination with a scrollable page.

Could you explain more the rationale around no longer observing LCP after the user scrolls? This doesn't seem like it would be guaranteed IMO.

@valbooth28
Copy link
Author

Awesome, thanks for your work @philipwalton!

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.

4 participants