[go: nahoru, domu]

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

new_audit: lcp-lazy-loaded #12838

Merged
merged 18 commits into from
Aug 24, 2021

Conversation

milutin
Copy link
Contributor
@milutin milutin commented Jul 30, 2021

Fixes #12785

Copy link
Collaborator
@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

thanks @milutin! you pieced together the right info, nicely done :)

we'll probably discuss the exact policy a bit more in our eng sync, so a few more comments might be coming

lighthouse-core/audits/lcp-lazy-loaded.js Outdated Show resolved Hide resolved
lighthouse-core/audits/lcp-lazy-loaded.js Outdated Show resolved Hide resolved
lighthouse-core/audits/lcp-lazy-loaded.js Outdated Show resolved Hide resolved
lighthouse-core/audits/lcp-lazy-loaded.js Outdated Show resolved Hide resolved
lighthouse-core/audits/lcp-lazy-loaded.js Outdated Show resolved Hide resolved
lighthouse-core/config/default-config.js Outdated Show resolved Hide resolved
lighthouse-core/audits/lcp-lazy-loaded.js Outdated Show resolved Hide resolved
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
@milutin milutin changed the title new_audit:lcp-lazy-loaded new_audit: lcp-lazy-loaded Jul 31, 2021
@milutin
Copy link
Contributor Author
milutin commented Aug 19, 2021

Sorry for break. I am ready to finish this. I am working on tests to pass checks and on strings.

Copy link
Collaborator
@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

taking great shape @milutin thank you! :)

this would also be a great candidate for a smoketest if you'd like to add loading lazy to

setTimeout(() => {
const imgEl = document.createElement('img');
imgEl.src = '../dobetterweb/lighthouse-480x318.jpg';
and the audit expectations to
'largest-contentful-paint-element': {
score: null,
displayValue: '1 element found',
details: {
items: [
{
node: {
type: 'node',
nodeLabel: 'section > img',
path: '0,HTML,1,BODY,1,DIV,a,#document-fragment,0,SECTION,0,IMG',
},
},
],
},
},
:)

lighthouse-core/audits/lcp-lazy-loaded.js Outdated Show resolved Hide resolved
lighthouse-core/audits/lcp-lazy-loaded.js Outdated Show resolved Hide resolved
lighthouse-core/audits/lcp-lazy-loaded.js Outdated Show resolved Hide resolved
milutin and others added 2 commits August 23, 2021 20:06
@milutin
Copy link
Contributor Author
milutin commented Aug 23, 2021

I was looking on smoke tests, it will take me awhile to add one.
When I added lazy loading in delayed-element

imgEl.src = '../dobetterweb/lighthouse-480x318.jpg';
imgEl.loading = 'lazy';

Smoke tests failed with difference at TraceElement artifacts.

I will look later how to change expected traceElement artifacts to pass smoke test and how to add expectations of failing lcp-lazy-loaded audit.

Copy link
Collaborator
@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

excellent job @milutin!

@milutin milutin closed this Aug 24, 2021
@milutin milutin reopened this Aug 24, 2021
@milutin
Copy link
Contributor Author
milutin commented Aug 24, 2021

Great, thanks a lot for help and guidance!

@patrickhulce patrickhulce merged commit da3d838 into GoogleChrome:master Aug 24, 2021
@nosilver4u
Copy link

Hi folks, we've seen a couple folks ask about this, specifically when it looks like it IS an error it says this:
"Largest Contentful Paint image was not lazily loaded"
But from looking at this commit, it seems that's supposed to be a "success" message? It sure doesn't look like it, shows a bright red triangle that certainly looks like an error. Here's an example of one that was mentioned: https://developers.google.com/speed/pagespeed/insights/?hl=en&url=https%3A%2F%2Feldiariodemof.com%2F&tab=mobile

*To be clear, I get what the article and the recommendation are trying to tell us--that above-the-fold images should not be lazy loaded, but I think the verbiage is busted (and probably confusing to non-developers, even if it were corrected).

@cheneytsai
Copy link

Agreed with @nosilver4u here. Maybe verbiage like "Avoid lazy-loading the LCP image" may be more direct

@rsmith4321
Copy link

Hi folks, we've seen a couple folks ask about this, specifically when it looks like it IS an error it says this: "Largest Contentful Paint image was not lazily loaded"

I just assumed this was just a mistake. I think they meant to say "Largest Contentful Paint image was lazily loaded". They really need to correct this issue it is confusing.

@sethta
Copy link
sethta commented Mar 11, 2022

We have been getting conflicting results with this specific check. On every site we have tested using the Trellis WordPress theme, we have found that not lazy loading the LCP has caused our scores to go down, both on mobile and desktop. We typically see a 50% to 200% increase in performance for the LCP time when we lazy load that image.

This makes it difficult when our publishers get the "Largest Contentful Paint image was lazily loaded" diagnostic, with the large red error triangle next to it, but we have not found a single example of this recommendation speeding up the site. Even the Google Developers documentation states the following:

In Chromium, the impact of images in the initial viewport being marked with loading=lazy on Largest Contentful Paint is fairly small, with a regression of <1% at the 75th and 99th percentiles compared to eagerly loaded images.

Our interpretation of this is that there is too much emphasis placed on this audit. The potential regression is extremely minimal, and with every site we have tested this showing the exact opposite, we are worried that this perceived requirement will cause more harm than good for website owners using Trellis.

@rviscomi
Copy link
Member

@sethta what tool are you using to test and are you able to share the results?

Also note that we reassessed the performance impact of lazy-loading the LCP image and found clear evidence that it's harmful: https://web.dev/lcp-lazy-loading/

@sethta
Copy link
sethta commented Apr 12, 2022

@rviscomi

We are typically using the Lighthouse audit in the Chrome dev tools, but because most of our publishers on Trellis don't know how to access that, we also use PageSpeed Insights.

I've replicated one of our testing sites so it's publicly available, and the results still show that lazy-loading the LCP image is faster on sites using Trellis.

These two pages are completely identical with the exception that the primary image is lazy loaded:

Lazy Loaded

https://pagespeed.web.dev/report?url=https%3A%2F%2Ftrellis.alling.dev%2Flcp-lazy-loaded%2F

When we run the tests on the page with the lazy loaded image, we consistently get Performance scores of 100 for both Mobile and Desktop results.

Mobile GPSI: 100
image

Other Images:

NOT Lazy Loaded

https://pagespeed.web.dev/report?url=https%3A%2F%2Ftrellis.alling.dev%2Flcp-not-lazy-loaded%2F

When we run the tests on the page where we have removed lazy loading, we have never been able to achieve a Performance higher than 96, directly related to slow LCP, for Mobile and the LCP time was consistently slower on Desktop (0.5-0.8s compared to 0.3s).

Mobile GPSI: 94
image

Other Images:

@rviscomi
Copy link
Member

Thanks for the additional info @sethta.

It looks to me like your server might be too inconsistent for these results to be meaningful.

Here's a test of 9 runs of the lcp-not-lazy-loaded page and you can see that the backend time to first byte (TTFB) metric varies between about 500ms to 4000ms. That will greatly affect the LCP time, much more so than lazy loading.

image

Interestingly, the test of the lcp-lazy-loaded page shows that TTFB is more consistent. Is it possible that you're doing some kind of slow rewrite on the server to generate the not-lazy-loaded page?

image

Even when we pick tests with similar TTFB times to compare, the eager version still appears to render the image more quickly:

image

The lazy page has a TTFB of 572ms and the eager page has a TTFB of 620ms. However, the lazy page is reported to have an LCP of 653ms while the eager page has an LCP of 1114ms. What's happening is that your lazy version is actually fooling the LCP calculation by drawing a gray box where the image should be:

src="data:image/svg+xml,%3Csvg%20xmlns='http://www.w3.org/2000/svg'%20viewBox='0%200%20683%201024'%3E%3Crect%20width='683'%20height='1024'%20style='fill:%23e3e3e3'/%3E%3C/svg%3E"

Unbeknownst to the browser, it thinks this inline SVG image is "contentful" so it considers this very early paint time as the LCP. But to a real user, the image doesn't appear loaded until about 1300ms. This is really bad because you're optimizing for something that actually makes the UX worse, despite the metrics seeming to improve.

I'd strongly recommend removing the lazy loading behavior from your LCP images. These lab tests confirm that the image will actually render more quickly by loading the images eagerly. Even if your measured LCP seems to get slightly worse, you're making the UX better, which is what matters most. You should also assume that at some point browsers will patch the buggy behavior, so the sooner you move away from relying on it the better.

@sethta
Copy link
sethta commented Apr 15, 2022

@rviscomi Thank you so much for this!

We have been banging our heads against the wall for a couple of weeks trying to figure out why we were getting different results.

The SVG application was developed a few years ago as a way to handle lazy-loaded images through a Javascript solution before the native browser solution existed, and it has run as a fallback ever since, as it has helped with both speeds and CLS.

One of our plans is to remove that application now that Safari supports lazy loaded images, so I think you just saved us from a real headache when we get that feature adjusted.

Thanks again!

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

Successfully merging this pull request may close these issues.

New audit: LCP element was lazy-loaded
9 participants