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

lazy-loading images above the fold should be an error #439

Open
colinbendell opened this issue Jun 9, 2021 · 8 comments
Open

lazy-loading images above the fold should be an error #439

colinbendell opened this issue Jun 9, 2021 · 8 comments
Labels
area:theme-check enhancement New feature or request

Comments

@colinbendell
Copy link

A common anti-pattern is to use lazy loading images (<img loading=lazy>) above the fold. This causes the browser to immediately discover the image and boost the HTTP priority of the resource on the network and choke out loading of critical javascript or css.

This was highlighted recently by Rick Viscomi in the #webperformance channel:
image

Ideally, theme-check should encourage <img loading=lazy for only images below the fold, but discourage above the fold.

A few potential mechanisms to determine above/below the fold:

  • assume ~100 lines of output html is above the fold
  • assume ~10KB is above the fold (unscientific number; needs validation)
  • use the hot reloading mechanism in shopify theme dev to create an analysis of the output to inform theme-check. (maybe to generate an equivalent .map file?)
  • and/or use a magical {% comment %}below-the-foled{% endcomment %} value for theme-check to look for. (maybe the hot reloading browser analysis could auto inject?)
@macournoyer
Copy link
Contributor

The check to upload is https://github.com/Shopify/theme-check/blob/master/lib/theme_check/checks/img_lazy_loading.rb.

We should try hard to keep this on the static analysis side, since that's all Theme Check does now.

Note that for LOC or bytes, we can't rely on single file, we need to compute "rendered" total. Tracing the path of include/render/section. We've done similar in checks such as NestedSnippet or UndefinedObject, but those don't take JSON templates into consideration.

@charlespwd
Copy link
Contributor

charlespwd commented Jul 14, 2021

I think this warrants a bit more research on our end. A chat with @tyleralsbury led me to believe that the network priority of the image was not affected by being above the fold when using native loading=lazy.

Things I'd like to know:

  • Does it affects how early it gets picked up by the browser? Does loading=lazy require the CSS to be loaded so that the layout can be calculated to figure out if the image is above the fold?
  • Can we even figure it out given sections everywhere? A section could equally be above or below the fold. We'd need some platform feature that tells us we're below. This could be hard and change depending on your breakpoint.

@charlespwd
Copy link
Contributor

Annnnd, this came out today: https://web.dev/lcp-lazy-loading/

Should read it :)

@tyleralsbury
Copy link

Can we even figure it out given sections everywhere? A section equally be above or below the fold. We'd need some platform feature that tells us we're below. This could be hard and change depending on your breakpoint.

This is the major complication. Pre-SE, we could make an assumption that, for example, the product information and gallery are at the top of the product page for a given theme. We could then assume that, at least the first image, would not need to be lazy loaded. But even this is questionable with Sections Everywhere.

@tyleralsbury
Copy link

This also highlights a sharp edge of the lazy-loading technique and the potential for API improvements at the platform level. For example, there's an open issue in Chromium to experiment with natively loading the first few images eagerly, similar to the fix, despite the loading attribute.

This would be very nice.

@charlespwd
Copy link
Contributor

Does loading=lazy require the CSS to be loaded so that the layout can be calculated to figure out if the image is above the fold?

Confirmed. The browser needs a render to start the download.

https://twitter.com/rick_viscomi/status/1415683309007515648?s=20

@siakaramalegos
Copy link

Maybe instead we either remove the rule entirely or instead make it a warning (not error) for the opposite, e.g., "Only lazy load images that are below the fold otherwise your performance could suffer".

We're going to work on the platform side, but this is really important to stop doing ATM since it's basically default training theme devs to lazy load everything. cc'ing @krzksz who may work on a PR for theme check

@muchisx
Copy link

muchisx commented Oct 13, 2022

Is there a way to remove this check on all themes without having to create a yml config for all root inside all themes? (Working in VSCode)

@lukeh-shopify lukeh-shopify transferred this issue from Shopify/theme-check Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:theme-check enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants