WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 4 months ago

#50425 closed defect (bug) (fixed)

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

Reported by: flixos90 Owned by: 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 (13)

#1 follow-up: @joemcgill
6 months 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 6 months ago by joemcgill (previous) (diff)

#2 @johnbillion
6 months 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 @flixos90
5 months 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
5 months 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.


5 months ago

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

#6 @flixos90
5 months ago

  • Keywords needs-dev-note added

#7 @azaozz
5 months 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
5 months 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
5 months 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
5 months 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.


5 months ago

#12 @flixos90
5 months ago

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

#13 @desrosj
4 months 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.

Note: See TracTickets for help on using tickets.