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 issues with section medias above the fold #1709

Open
sofiamatulis opened this issue May 18, 2022 · 7 comments
Open

Lazy loading issues with section medias above the fold #1709

sofiamatulis opened this issue May 18, 2022 · 7 comments
Labels
Category: Enhancement New feature or request Category: Tech Debt Opportunities to refactor or improve the code and/or development workflow

Comments

@sofiamatulis
Copy link
Contributor

Describe the enhancement you'd like
When the image banner section is placed above the fold by the merchant, there should be logic to update loading=lazy to auto/eager

This was noticed in the image banner, but we can check other sections with images as well since sections can be reordered

Original comment by @bacounts

#1701 (comment)

@sofiamatulis sofiamatulis added this to To be triaged in Dawn: Iterations Roadmap via automation May 18, 2022
@sofiamatulis sofiamatulis added the Category: Enhancement New feature or request label May 18, 2022
@ludoboludo
Copy link
Contributor

@bacounts this is currently an issue for any section. We don't have an easy way to tell which section is placed above the fold. So it's definitely something that'd be great to know and address but tricky as any combination of sections can exist.

We've tweaked it some places where we could tell that in most cases x or y component is going to sit at the top.
With the principles we have for the theme we didn't want to use JS to tackle it. I think one way to go about it would be to use an intersection observer on each section and remove/add the attribute where needed.
There was also some new properties that sounded very useful for those use cases, content-visibility: https://web.dev/content-visibility/
But the support for this so far is pretty low.

@bacounts
Copy link

bacounts commented May 18, 2022

I think one way to go about it would be to use an intersection observer on each section and remove/add the attribute where needed. There was also some new properties that sounded very useful for those use cases, content-visibility: https://web.dev/content-visibility/ But the support for this so far is pretty low.

@ludoboludo - The intersection observer could be an interesting way to tackle this issue globally. When I run Google Lighthouse, the loading=lazy on any section with an image above the fold negatively impacts the speed numbers.

Maybe you could rename this issue to reflect a global theme issue better?

Screen Shot 2022-05-18 at 8 22 26 AM

@ludoboludo ludoboludo changed the title Image banner lazy loading logic Lazy loading issues with section medias above the fold May 18, 2022
@bakura10
Copy link

I am not sure a JS based approach will work here: it will be already too late. To solve the issue the server should directly return the loading with eager property and preload it.

But as you said this will be a hard issue to solve as of today, and I am not sure the impact is that important.

Shopify would need to add the ability to check if the section is the first one, but as I understood it with previous talk with Thibaut, this would introduce dependencies between sections and would break one big assumption that sections are independant.

@nicklepine nicklepine added the Category: Tech Debt Opportunities to refactor or improve the code and/or development workflow label May 27, 2022
@nicklepine nicklepine moved this from To be triaged to Bugs Icebox 🐜 in Dawn: Iterations Roadmap May 27, 2022
@ludoboludo
Copy link
Contributor

Here is a PR that was adding a setting for image-banner to decide whether to load the media with eager or lazy.
Not the ideal approach but could be an option. Though it would need to be on all sections with medias and with a simpler label probably 🤔

@tyleralsbury
Copy link
Contributor

Shopify would need to add the ability to check if the section is the first one, but as I understood it with previous talk with Thibaut, this would introduce dependencies between sections and would break one big assumption that sections are independant.

That's right. All of the platform based solutions we've tossed around thus far have had this critical issue. When a section is updated in the theme editor, only that section is affected. Any solution that changes the value of something like is_first_section or section_index, etc... will always have the same problem where another section will be affected by it, meaning the assumption the editor makes that regarding which section could have changes is broken.


That said, the PR that was made that Ludo referenced above proposes the use of a setting. It would put the onus on the merchant to handle it, but does at least give them control. If we default it to lazy the experience won't get any worse.

Here's my take on what would be required to use it in Dawn: #1586 (comment)

I'm going to raise this issue with the broader Flagship themes team to see if it's something we are okay with doing. I would love to be able to crank out a bit more performance, even if it's only for merchants who are "in the know" - the language used in the setting itself can help with informing merchants of the purpose of the setting. It is a bit technical, but we can phrase it in a way that it is less so.

@bakura10
Copy link

bakura10 commented Jun 7, 2022

Good morning everyone :).

Sorry in advance for the very long message.

First of all, I am completely against any setting exposed to the merchant. There is zero way a merchant can understand what are the implication of such a setting, whatever the wording you choose. Considering that merchants often re-order and add new sections, the risk also is that a merchant forgets to maintain the setting across their sections, resulting in images not lazyloading where they should be.

Any setting like this added to Dawn acts as a baseline for what merchants expect on premium themes ("this can be done on a free theme, why not on a $300 theme?" is a recurring catchphrase from merchants), so I would like to oppose firmly on this to be added in Dawn.

Hopefully, after giving it another thing, there is an easy way to solve this issue without introducing any complexity to the merchant, thanks to the image_tag Liquid tag. I describe below the solution.

Leveraging preload

As of today, the image_tag supports an optional preload tag that allows defining if Shopify should send a Link header to preload an image.

Shopify should leverage this attribute to define to control the loading attribute. Basically, it only makes sense to preload an image when it is not lazyloaded. So the trick here is that Shopify should change any "loading" from "lazy" to "eager", or simply leave it empty if no loading attribute is set.

This lets the developer specify what the "loading" attribute should be independently of the section position. Here is an example of a slideshow section:

{% for block in section.blocks %}
  {{ block.image | image_url: width: block.image.width | image_tag: loading: 'lazy', preload: forloop.first }}
{% endfor %}

Internally, Shopify would do the following transformation if the section is the first one:

loading value preload value outputted loading value
lazy true eager
lazy false lazy
eager true eager
eager false eager
no value true no value
no value false no value

If the section is not the first one, Shopify would ignore the loading value transformation, which would end up like this:

loading value preload value outputted loading value
lazy true lazy
lazy false lazy
eager true eager
eager false eager
no value true no value
no value false no value

This would make the code clean, without any additional settings and without exposing "is first" section attribute.

Internally, when Shopify renders the section, Shopify back end most probably knows what is the position of a section. The trick here would be for Shopify to implement it something like this (pseudo-code):

generateImageTag() {
  if (section.first && preload === true && loading == 'lazy' && accumulatedLcp <= 3) {
    loading = 'eager';
    accumulatedLcp += 1;
  }
}

The accumulatedLcp would allow to make sure that, in the accidental case a developer would set a preload for 10 images, it limits it to 3 (I think 3 is a good limit, and can take into the account of the most extreme use case that would be a slideshow section, with for instance 1 + 1 split image on desktop and 1 image on mobile).

In the case Shopify would not know, at the backend level, the position of a section, Shopify could simply use the accumulatedLcp heuristic and remove the "section.first". Of course, this would be suboptimal, as it means sections below the fold may have an image set to eager (up to 3 sections if we use a limit of 3).

Current issue with preloading

In Shopify, there are currently 3 ways to preload a resource:

  • using the "stylesheet_tag" with the preload attribute set to true
  • using the generic "preload_tag"
  • using the "image_tag" with the preload attribute set to true

Unfortunately, Shopify currently limits the number of preloadable resources to 2. It is not uncommon to use the stylesheet tag with preload to true for the main stylesheet, and use the preload tag for preloading a script, which already exhausts the limit.

To make this approach works, Shopify should segregate the limit by resource type: 2 for stylesheet_tag, 2 for preload_tag, and 3 for image_tag would be a nice start :)

Section independencies

This technique does not break anything as it does not introduce any variable exposed to Liquid. At worst, if a section is re-ordered, the second section in the theme editor (that used to be the first one) will still have a loading="eager" property instead of loading="lazy" if the whole page would have been re-rerendered. But this is just a hint, so not a big deal.

Of course, developers could abuse this and do a capture around the image tag and check if the loading attribute has been changed to detect if it is the first section, but people doing stupid things ending in stupid results is not something I would care.

Edge cases

Section rendering API

When a section is rendered using the section rendering API, because this always happen in JS and therefore never contribute to LCP, Shopify should not do any modification of the "loading" property, even if preload is set to true.

What if I want to eager loading without preloading?

This proposal does not cover this use case, but from what I understand from the LCP metric, Google recommend images contributing to the LCP to be preloading, in addition of not being lazyloading, so it does not seem like a very common use case.

Minor BC

One minor breaking change in this outlined approach would be that a previously loading="lazy" would end up in loading="eager" if this change is added:

{{ image | image_url: width: block.image.width | image_tag: loading: 'lazy', preload: true }}

Before: loading: "lazy"
After: loading: "eager" if section is the first

This means a developer doing a JS relying on this attribute:

this.querySelectorAll('[loading="lazy"]')

would have a breaking change.

Having said that, image_tag is pretty new, so I think it is a good timing to introduce such change and make it clear in the doc that the loading attribute could be overridden, and that it is just a hint for Shopify. In all cases, because a preload set to true should never have a loading=lazy from the first state, I don't think a lot of people would run into any trouble (and this would help those developers to fix a logic bug anyway).

One another idea to avoid completely this (super minor) breaking change would be to either:

  • Add a new value "auto" for loading attribute that would either be assigned "eager" or "lazy" depending on the preload value and accumulated LCP images count. However, the main issue is that if the standard later add an "auto" value for the browser, we're screwed up. I don't think this is a good idea.
  • Adding a new "lcp" boolean for the image_tag, that would handle all the logic:
{{ image | image_url: width: block.image.width | image_tag: loading: 'lazy', lcp: true }}

Setting the lcp to true would automatically change the loading attribute AND set the preload to true.
One drawback of this approach though is that we are tying an attribute to a concept (LCP) that may be removed anything if Google decide to change the way they measure performance (I think they already did remove some metrics in the past), so we would endup with concept in Shopify that would not match anything.

I therefore think leveraging preload is the best way to solve this issue, and accept the very minor breaking change.

EDIT: support for preload_media

In regards to Will comment, I think the approach I outlined would also work by simply adding a preload_media. Here is an example for the slideshow, with a mobile and desktop image as a example:

{% for block in section.blocks %}
  {{ block.settings.desktop_image | image_url: width: block.settings.desktop_image.width | image_tag: loading: 'lazy', preload: forloop.first, preload_media: '(min-width: 800px)' }}
  {{ block.settings.mobile_image | image_url: width: block.settings.mobile_image.width | image_tag: loading: 'lazy', preload: forloop.first, preload_media: '(max-width: 799px)' }}
{% endfor %}

So what would happen here, is that if it is the first section, Shopify will replace the "loading": "lazy" to eager due to the fact that preload is true on the first slide, and also output the proper preload_media to make sure the preloaded image is only sent on the device size matching the preload_media.

I have no idea on how Shopify backend is architecture, but I don't see any technical problem in implementing this, and would solve this long-standing issue very cleanly, would work with any order of sections, play well with the theme editor...

Let me know if you have any question about this proposal :)

@willbroderick
Copy link

willbroderick commented Jun 7, 2022

I would like to mention that anything related to page load metadata, like preload tags, is not required inside the theme editor. Could we have a section.final_index that is only evaluated outside of the editor?

image_tag is also missing 'media', so it's not a perfect solution yet when a theme uses different desktop/mobile images in the same section.


I propose an alternative to preloading with the image_tag filter, by registering assets as sections are built and allowing access to these in Layout/theme.liquid (only when rendered on the frontend). To give full control to the theme dev.

More

Example banner.liquid:

{% register_asset, type: 'image', image: image, data: index: section.final_index, srcset: '...', sizes="100vw" media: 'min-width: 768px' %}

Layout/theme.liquid in :

{% for asset in registered_asset %}
 {% if asset.data.index < 2 %}
  <link rel="preload" as="image" href="{{ asset.image.url }}" imagesrcset="{{ asset.data.srcset }}" imagesizes="{{ asset.data.sizes}}" media="{{ asset.data.media }}">
 {% endif %}
{% endfor %}

This would play well with how the first chunk of the HTML doc is sent very early on the load lifecycle, and that preload tags in <head> are parsed by browsers as soon as they're seen, before the doc has finished transferring (pretty much at the same time as http header preload tags, from what I can tell).

This would be very flexible and allows for media queries (different desktop/mobile hero image), picture tag media, video poster, anything.
This may also be useful for Dawn's split CSS/JS approach, with any large ATF section file dependencies able to be preloaded. All power is given to the frontend dev.

This data is not necessary in the theme customiser. 'register_asset' could just do nothing in there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Enhancement New feature or request Category: Tech Debt Opportunities to refactor or improve the code and/or development workflow
Projects
Development

No branches or pull requests

7 participants