Make WordPress Core

Opened 16 months ago

Closed 15 months ago

Last modified 13 months ago

#58089 closed defect (bug) (fixed)

Featured images within post content cause lazy-loading logic to apply incorrectly

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

Description (last modified by flixos90)

When including featured images within post content, the logic to omit the loading="lazy" attribute on the first x images may not be working correctly, depending on where the featured images are placed in the content.

The reason for that is that each featured image calls wp_get_loading_attr_default() with a context of the_post_thumbnail while the post content is being parsed. In other words, the_content filter triggers the logic of adding lazy-loading attributes to the output using the the the_content context, but before that happens all blocks are parsed, including those that display featured images via get_the_post_thumbnail(). These calls then trigger the logic for featured images to apply within post content, but that logic is only intended to apply when featured images are displayed by themselves, i.e. outside of post content.

I ran into this bug on my own website and was able to figure out a solution, but I haven't yet been able to replicate it on another site. That said, I am quite certain that it is a bug, but will need to investigate more closely to figure out how to replicate it with core. Potentially the only way to trigger it is using certain custom blocks.

This bug was likely introduced by the original feature #53675, and is furthermore related to #56930, although the latter is likely not the cause for it.

Change History (24)

This ticket was mentioned in PR #4300 on WordPress/wordpress-develop by @flixos90.


16 months ago
#1

  • Keywords has-patch added

This fixes the underlying bug, which I have already verified on my website where I ran into it. I am however not sure yet how to best replicate it elsewhere.

Trac ticket: https://core.trac.wordpress.org/ticket/58089

#2 @flixos90
16 months ago

  • Description modified (diff)

I have opened a PR https://github.com/WordPress/wordpress-develop/pull/4300 with the fix that resolves the bug on my own website, for early consideration. Will still have to figure out whether this is a bug that can happen from core itself or only via custom blocks that include featured images.

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


16 months ago

#4 @antpb
16 months ago

@joedolson found good reproduction steps for anyone testing this.

In Twenty Twenty Three:

1) Add the featured image block within the content of a page.
2) Edit the Page template in the site editor, and remove the featured image block.
On the resulting page, the Featured image, which is the first image on the page, retains the lazy loading attributes.

Last edited 16 months ago by antpb (previous) (diff)

#5 @flixos90
16 months ago

Thank you @joedolson @antpb!

Are you able to test if https://github.com/WordPress/wordpress-develop/pull/4300 resolves the bug?

#6 @flixos90
16 months ago

  • Milestone changed from Awaiting Review to 6.3

#7 @joedolson
16 months ago

Yes - using the reproduction method described, the lazy loading attribute is present before the PR is applied and absent after it's applied.

#8 @joedolson
16 months ago

  • Keywords needs-testing-info removed

#9 @flixos90
15 months ago

  • Keywords needs-unit-tests added
  • Owner set to flixos90
  • Status changed from new to assigned

I have enhanced the PR to apply the same approach when the wp_get_attachment_image() function is called within the_content filter. This is commonly used today by e.g. blocks that may call the function or plugins that rely on the the_content filter to inject their own content.

With the check here, it is ensured that such images are only counted once, as part of going through the entire post content (i.e. the "the_content" $context).

#10 @flixos90
15 months ago

  • Priority changed from normal to high

Addressing the remaining lazy-loading related performance issues is a high priority for the performance team for the 6.3 cycle.

@flixos90 commented on PR #4300:


15 months ago
#11

@kt-12 FYI since you're working on some of the other lazy-load related tickets.

#12 @flixos90
15 months ago

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

@flixos90 commented on PR #4300:


15 months ago
#13

@spacedmonkey I've addressed your feedback, please take another look.

#14 @flixos90
15 months ago

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

In 55825:

Media: Prevent special images within post content to skew image counts and cause lazy-loading bugs.

In order to skip lazy-loading the first few images on a page, as of WordPress 5.9 there has been logic to count images that are eligible based on certain criteria. One of those groups are images that appear within the content of a post.

This changeset fixes a bug where images created via get_the_post_thumbnail() or wp_get_attachment_image() that are injected into the post content would skew the count and therefore result in all images to be lazy-loaded, potentially hurting load time performance. This is relevant for example when those functions are called in server-side rendered blocks, or any other filter callbacks hooked into the_content.

Props flixos90, antpb, joedolson, spacedmonkey, mukesh27, thekt12, costdev, jrf.
Fixes #58089.
See #53675.

#15 @flixos90
15 months ago

  • Keywords needs-dev-note added

#17 @asafm7
14 months ago

@flixos90 I think it is the issue I'm encountering, but I want to be sure.

I have a Cover block set to display the featured image inside a Single Product block template. It is being lazy-loaded, although it shouldn't be.

I've tested, and the problem is here on media.php. 'lazy' is returned either because we're not on in the loop, or it isn't the main query:

		if ( is_admin() || ! in_the_loop() || ! is_main_query() ) {
			return 'lazy';
		}

Will your fix address this issue?

Thanks

#18 @flixos90
14 months ago

@asafm7 Your problem sounds similar to the one that was fixed here in [55825], but I can't say for sure without having more context. Are you able to test it on your end?

Basically, adding this to the beginning of the wp_get_loading_attr_default() function in wp-includes/media.php should fix it:

if ( ( 'the_post_thumbnail' === $context || 'wp_get_attachment_image' === $context ) && doing_filter( 'the_content' ) ) {
        return false;
}

#19 @asafm7
14 months ago

Thanks, @flixos90 .

It only works if I remove && doing_filter( 'the_content' ).

Probably because the context is a block template?

Do you think the fix can be modified to include this case?

#20 @flixos90
14 months ago

@asafm7 Sorry, I got a bit confused, I actually think this here is not the right ticket for the problem you're outlining. Removing doing_filter( 'the_content' ) wouldn't work as that wouldn't fix the problem outlined in this ticket.

However the fix from #58211 (commit [55847]) sounds more like it may help in your case, maybe take a look at that?

#21 @asafm7
14 months ago

Thanks @flixos90 . Going over https://core.trac.wordpress.org/changeset/55847 it seems it might solve my issue. I will wait and see.

#22 @flixos90
13 months ago

In 56164:

Media: Avoid programmatically created images within post content from incorrectly receiving fetchpriority="high".

Follow-up to [56037], as that changeset accidentally did not consider the changes made in [55825]: Images that are programmatically injected into post content (e.g. through a block, or shortcode, or any hook calling a function like wp_get_attachment_image()) must be treated as part of the whole post content blob since otherwise the heuristics for applying fetchpriority="high" and loading="lazy" are skewed by parsing certain images before others rather than sequentially parsing the entire post content. [55825] addressed that for lazy-loading, but when [56037] introduced fetchpriority support, the related refactor missed making the same consideration for that attribute.

Props flixos90, spacedmonkey, thekt12, mukesh27.
Fixes #58235.
See #58089.

#24 @flixos90
13 months ago

  • Keywords needs-dev-note removed
Note: See TracTickets for help on using tickets.