Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#50425 closed defect (bug) (fixed)

Do not lazy-load images which are very likely to be in the initial viewport

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile flixos90
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.5
Component: Media Keywords: has-patch has-unit-tests has-dev-note
Focuses: Cc:

Description

Currently, loading="lazy" is added to the following images by default:

  • images in post content (context the_content)
  • images in post excerpt (context the_excerpt)
  • images in widget content (context widget_text_content)
  • avatar images (context get_avatar)
  • any image rendered via wp_get_attachment_image() (context wp_get_attachment_image)

While these defaults can be modified per context via the wp_lazy_loading_enabled filter, there are certain images in WordPress which preferably would not be lazy-loaded since they are very likely to be in the initial viewport. This ticket aims at modifying those images to use loading="eager" by default instead.

The following functions should pass loading="eager" to wp_get_attachment_image():

  • get_custom_logo()
  • get_the_post_thumbnail()

Another function to cover would be get_header_image_tag(), however that does not use wp_get_attachment_image(), so no need to update.

Change History (16)

#1 follow-up: @joemcgill
4 years ago

get_the_post_thumbnail() is used extensively in places where the featured image is not in the viewport, e.g., on archive pages or in places where some UI is being creating using a secondary query, so we might want to be more explicit in that case that it only loads when is_single() and is_main_query() are both true.

Last edited 4 years ago by joemcgill (previous) (diff)

#2 @johnbillion
4 years ago

My understanding is that modern browsers which implement native lazy loading would handle this decision making. ie. an image within the initial viewport could have the lazy attribute but the browser would still choose to load it immediately.

Is there a negative impact of including the lazy attribute on an image above the fold?

#3 in reply to: ↑ 1 ; follow-up: @flixos90
4 years ago

Replying to joemcgill:

get_the_post_thumbnail() is used extensively in places where the featured image is not in the viewport, e.g., on archive pages or in places where some UI is being creating using a secondary query, so we might want to be more explicit in that case that it only loads when is_single() and is_main_query() are both true.

That's a fair point. I'd assume in the majority of cases the image would be somewhere near the top of the page, but it's true we cannot rely on that. I think it would make most sense to provide essential support for the lazy-loading and rely on themes to manage the specifics for these kinds of images, based on where they place them. I'll post a follow up comment with more details on the proposed solution.

Replying to johnbillion:

My understanding is that modern browsers which implement native lazy loading would handle this decision making. ie. an image within the initial viewport could have the lazy attribute but the browser would still choose to load it immediately.

Is there a negative impact of including the lazy attribute on an image above the fold?

Essentially, the loading="lazy" attribute tells browsers to load the image lazily. While browsers may use some heuristics to assess priority of these images etc., having an image in the initial viewport marked with loading="lazy" will result in a slightly different behavior than having it load eagerly, which can have an impact on the largest contentful paint result for the page. Essentially the browser will typically render the page without loading the image first, which usually happens in a split second, but does technically result in a slight delay.

We should be setting sane defaults in core, and then defer to theme (and in some instances plugin) developers for tweaking with lazy-loading in a more fine-grained way based on how they use images. I'll post the proposed approach in a follow-up comment.

#4 @flixos90
4 years ago

After re-thinking the approach outlined in the original ticket description, also taking into account the comments so far, I'm proposing to handle lazy-loading in the following way to achieve a good balance between saving network resources/decreasing initial load and maintaining a solid largest contentful paint outcome:

  • Globally add support for the loading value to be specified as null, to explicitly omit the attribute. At this point, this is the recommended approach for images that should not be lazy-loaded - in other words, we should avoid using loading="eager" only to override a value of lazy.
  • Lazy-load images by default only if they are within the_content, the_excerpt, or widget_text_content, as well as get_avatar images, as these areas are primarily found below the fold.
  • Support lazy-loading images used in templates via wp_get_attachment_image, but leave the attribute off by default.
  • Document in the dev-note the default behavior and how theme developers can modify it...
    1. ...by using the wp_lazy_loading_enabled filter to generally enable or disable lazy-loading for all images for a certain context (e.g. a theme could globally enable lazy-loading for template images or globally disable lazy-loading for widget content). This could be used by a plugin or theme developer, however it is rather general, so options 2. or 3. are preferable.
    2. ...by explicitly lazy-loading certain template images via e.g. wp_get_attachment_image( $id, $size, $icon, array( 'loading' => 'lazy' ) ) or get_the_post_thumbnail( $post, $size, array( 'loading' => 'lazy' ) ). This could be used e.g. by a theme developer to lazy-load a featured image or logo used in the footer.
    3. ...by disabling lazy-loading for a specific image via the wp_img_tag_add_loading_attr filter. This allows very fine-grained modification that in certain cases _may_ make sense for a theme to do, but would likely apply more to site-specific customization e.g. by an agency.

This ticket was mentioned in PR #353 on WordPress/wordpress-develop by felixarntz.


4 years ago
#5

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

#6 @flixos90
4 years ago

  • Keywords needs-dev-note added

#7 @azaozz
4 years ago

  • Type changed from enhancement to defect (bug)
  • Version set to trunk

My understanding is that modern browsers which implement native lazy loading would handle this decision making.

Same here. Even if the browsers aren't super good at that at the moment, I'm pretty sure they will improve over time :)

Looking at https://github.com/WordPress/wordpress-develop/pull/353, most of it makes sense. The only thing that would perhaps need a confirmation if it is buggy is this: https://github.com/WordPress/wordpress-develop/pull/353/files#diff-67a565933b9d6a2850abd1d7dc5585d5R1618 (do not lazy load images that come from wp_get_attachment_image() by default).

In that terms thinking the PR should be added to trunk now except the above change. If it is confirmed as a bug during beta testing, lets add that too (changing ticket type to bug as that's what it is, essentially).

#8 @adamsilverstein
4 years ago

I tend to agree that the browsers should be deciding exactly how and what to lazy load, ie. images in the viewport at load should load immediately regardless of a lazy attribute. It would be interesting to see some data about how impactful this is.

I tested out the code in https://github.com/WordPress/wordpress-develop/pull/353 with some recent core themes.

In general for single posts, things worked as expected, only the featured image at the top of the page excluded the loading="lazy" attribute, inline content images all included it.

I noticed a potential issue however: when I set my home page to show recent blog post summaries, the themes included the featured image for each post: these images did lot include the lazy loading attribute. Similarly, visiting category archive pages showed a list of posts and these images were not lazy loaded - so I had a long page with images and none were lazy loaded - not ideal.

After testing I am inclined to agree with @azaozz and feel wp_get_attachment_image should also default to lazy loading. Then we can clearly document for developers how they can opt out if they need a different behavior with the wp_lazy_loading_enabled filter.

#9 @flixos90
4 years ago

Thanks for the feedback @azaozz and @adamsilverstein! I will update the PR for now as mentioned, so that wp_get_attachment_image keeps lazy-loading on by default. The improved support for opting out (if opting out, the recommendation is to use null or false, but not 'eager') plus the extra test coverage is useful to be merged already, regardless of the final decision here.

I'll keep this ticket open so we can continue testing and discussing as needed.

#10 @flixos90
4 years ago

In 48272:

Media: Improve support for opting out of lazy-loading for template images.

With this changeset, in addition to the already present wp_lazy_loading_enabled filter, developers can now opt out of lazy-loading template images via wp_get_attachment_image() by passing a loading attribute with boolean value false. This can be used e.g. by theme developers on images which are very likely to be in the initial viewport.

This changeset also improves related test coverage.

Props adamsilverstein, azaozz, joemcgill, johnbillion.
See #50425, #44427.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


4 years ago

#12 @flixos90
4 years ago

  • Keywords has-dev-note added; needs-dev-note removed

#13 @desrosj
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

I'm going to close this one out in favor of new, more specific bug report tickets. If there are still tasks required that I missed, feel free to reopen.

#14 in reply to: ↑ 3 @westonruter
4 years ago

Replying to flixos90:

Replying to joemcgill:

get_the_post_thumbnail() is used extensively in places where the featured image is not in the viewport, e.g., on archive pages or in places where some UI is being creating using a secondary query, so we might want to be more explicit in that case that it only loads when is_single() and is_main_query() are both true.

That's a fair point. I'd assume in the majority of cases the image would be somewhere near the top of the page, but it's true we cannot rely on that. I think it would make most sense to provide essential support for the lazy-loading and rely on themes to manage the specifics for these kinds of images, based on where they place them. I'll post a follow up comment with more details on the proposed solution.

I noticed that in three most recent core themes (at least) the first featured image is still getting loading=lazy even though the image appears in the first viewport. So the themes aren't managing the specifics as they should be.

I think that whenever a featured image is being printed (via the_post_thumbnail()) that the resulting image should not get loading=lazy if the image being printed is:

  • The featured image for the queried object on a singular template, or
  • The featured image for the first post in the main query.

I put together a prototype of how I think this can be addressed (though it does require a bit of a action/filter dance): https://gist.github.com/westonruter/e8d5778843005b7e0d6ce4049b3ec29d

Thoughts?

#15 @westonruter
4 years ago

Correction: Twenty Twenty-One does explicitly omit loading=lazy for the featured image, but only on the singular template per #52139. It should also do so for the first post in The Loop for an archive template.

#16 @flixos90
3 years ago

In 52065:

Media: Refine the heuristics to exclude certain images and iframes from being lazy-loaded to improve performance.

This changeset implements the refined lazy-loading behavior outlined in https://make.wordpress.org/core/2021/07/15/refining-wordpress-cores-lazy-loading-implementation/ in order to improve the Largest Contentful Paint metric, which can see a regression from images or iframes above the fold being lazy-loaded. Adjusting this so far has been possible for developers via filters and still is, however this enhancement brings a more accurate behavior out of the box for the majority of themes.

Specifically, this changeset skips the very first "content image or iframe" on the page from being lazy-loaded. "Content image or iframe" denotes any image or iframe that is found within content of any post in the current main query loop as well as any featured image of such a post. This applies both to "singular" as well as "archive" content: On a "singular" page the first image/iframe of the post is not lazy-loaded, while on an "archive" page the first image/iframe of the _first_ post in the query is not lazy-loaded.

This approach refines the lazy-loading behavior correctly for the majority of themes, which use a single-column layout for post content. For themes with multi-column layouts, a new wp_omit_loading_attr_threshold filter can be used to change how many of the first images/iframes are being skipped from lazy-loaded (default is 1). For example, a theme using a three-column grid of latest posts for archives could use the filter to override the threshold to 3 on archive pages, so that the first three content images/iframes would not be lazy-loaded.

Props adamsilverstein, azaozz, flixos90, hellofromtonya, jonoaldersonwp, mte90, rviscomi, tweetythierry, westonruter.
Fixes #53675. See #50425.

Note: See TracTickets for help on using tickets.