Skip to content
This repository has been archived by the owner on Mar 14, 2024. It is now read-only.

Clarify LCP 'caution' aside regarding tabs loaded in background #8475

Merged
merged 3 commits into from
Aug 12, 2022

Conversation

vanderhoop
Copy link
Contributor

@vanderhoop vanderhoop commented Aug 7, 2022

Hello! This change removes (what I believe to be) an outdated 'caution' aside on /lcp regarding tabs loaded in the background, as the web-vitals library ignores paint metrics for tabs loaded in the background. This is reflected in the library's README.

The aside on /lcp was initially added in August of 2019, and the web-vitals commit linked above landed in March of 2020, so I'm guessing the web.dev update probably just fell off the radar.

  - the web-vitals README states that "FCP, FID, and LCP are not reported if the page was loaded in the background."
    - this change updates the web.dev/lcp page to align with the README guidance above
@google-cla
Copy link

google-cla bot commented Aug 7, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@chrome-devrel-review-bot
Copy link
Collaborator

Hello! This is an automated review by our custom reviewbot. It updates automatically when code or GitHub comments in this pull request are created or updated.

Requested changes

If there are any common problems with the content files you created or modified, they will be listed here.

src/site/content/en/metrics/lcp/index.md

  • Please change your usage of the following words:
    • Do not use master. Affected lines: 183, 268, 337

1 similar comment
@chrome-devrel-review-bot
Copy link
Collaborator

Hello! This is an automated review by our custom reviewbot. It updates automatically when code or GitHub comments in this pull request are created or updated.

Requested changes

If there are any common problems with the content files you created or modified, they will be listed here.

src/site/content/en/metrics/lcp/index.md

  • Please change your usage of the following words:
    • Do not use master. Affected lines: 183, 268, 337

@vanderhoop
Copy link
Contributor Author

vanderhoop commented Aug 7, 2022

CLA is signed, and I've updated two of the references of master to main that were flagged by the dev-rel bot, but there is also a flagged reference to https://support.google.com/webmasters/answer/9205520 due to the infixed 'master' in the url, for which there isn't an obvious update/replacement.

@tunetheweb
Copy link
Member

The fact that web-vitals has to make extra allowances is why that caution is there as far as I understand it. If you just call PerformanceEntry you will get the LCP entry, but that timing may not be what you want to report since it will be artificially high compared to the page load and you may want to ignore it, like web-vitals.js does.

@philipwalton thoughts?

@philipwalton
Copy link
Member

@tunetheweb yeah, I think we want to keep the caution aside, but perhaps we can update it to say something like:

"Google tools that measure LCP do not report this metric if the page was loaded in the background, as it does not reflect the user-perceived load time."

WDYT?

@tunetheweb
Copy link
Member

SGTM @philipwalton

@vanderhoop can you revert your change and add that extra sentence? Sadly I can't suggest changes on deleted lines.

Or let me know if you want me to close this and take care of it.

@vanderhoop
Copy link
Contributor Author

vanderhoop commented Aug 10, 2022

Hey @tunetheweb + @philipwalton, thanks for the quick responses and clarification. I'm happy to make that change and push it up. Will try to get to that later today.

One Core Web Vitals maintenance question, though:

The fact that web-vitals has to make extra allowances is why that caution is there as far as I understand it. If you just call PerformanceEntry you will get the LCP entry.

Given that Google is driving the Core Web Vitals project, and the LCP performance entries are only currently (I believe) in Chromium browsers, wouldn't it make sense to stop emitting the 'largest-contentful-paint' events at a lower level when these circumstances (background loading, et cetera) arise, rather than relying on JS devs to handle these cases downstream (in web-vitals.js and other libraries that are destined to follow)?

I imagine this has been considered before, so if there's a conversation you can point me to, I'd gobble it up!

@tunetheweb tunetheweb changed the title Remove outdated LCP 'caution' aside regarding tabs loaded in background Clarify LCP 'caution' aside regarding tabs loaded in background Aug 11, 2022
Copy link
Member

@tunetheweb tunetheweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change LGTM.

Will leave @philipwalton to answer the question as to why the browser API still emits LCP in these cases and whether it should in the future.

@philipwalton
Copy link
Member

Will leave @philipwalton to answer the question as to why the browser API still emits LCP in these cases and whether it should in the future.

I believe this is primarily for historical reasons (first affecting paint timing). I think the Paint Timing API was created and shipped and then once people started using it in the real world, they noticed this issue; and other APIs matched the original behavior for consistency. See some historical discussion about it (as well as potential solutions):

@tunetheweb tunetheweb requested a review from malchata August 12, 2022 11:04
Copy link
Member

@malchata malchata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tunetheweb tunetheweb merged commit 09adb5a into GoogleChrome:main Aug 12, 2022
@tunetheweb
Copy link
Member

Thanks @vanderhoop !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants