No lazy loading attribute on images in gallery

[spacedmonkey]

Images in gallery shortcode no longer have lazy loading attribute. In WordPress 6.2 images in the gallery did have lazy loading attribute and they are now missing in trunk.

Behaviours seem a little different between image sizes.
With gallery shortcode with image size large, I see fetch priority and source set, but not lazy load attribute. With thumbnail size, I see no fetch priority or lazy loading attributes. But have decoding async, meaning they have been processed at some point.

[spacedmonkey]

Gallery shortcode with large images on Trunk

[spacedmonkey]

Gallery shortcode with large images on WP 6.2

[spacedmonkey]

Gallery shortcode with thumbnail images on WP 6.2

[spacedmonkey]

Gallery shortcode with thumbnail images on Trunk

[thekt12]

Gallery shortcode with thumbnail images on Trunk, Theme - Twenty Twenty-Three ( working as expected )

[thekt12]

Gallery shortcode images on Trunk, Theme - Twenty Twenty-Nineteen ( working as expected )

[spacedmonkey]

@thekt12 Your screenshot shows a gallery block and not a gallery shortcode. Please activate the classic editor plugin and insert a gallery shortcode using "insert media"

[flixos90]

Adding this to the 6.3 milestone since it's a follow up bug to fix. Probably not necessarily high priority though.

[flixos90]

Reassigning to you @thekt12 per our DMs

Change context of wp_get_attachment_image_context in gallery shortcode to fix lazy loading and fetch priority.

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

[spacedmonkey]

I put together a quick POC for a fix for this ​https://github.com/WordPress/wordpress-develop/pull/4794 . If @thekt12 could take a look.

@spacedmonkey I liked the idea of modifying context of gallery shortcode. However, I was wondering if there is any chance of breaking or not executing any other logic that depends on the_post_thumbnail context either within the core or outside of the core. What do you feel?

@spacedmonkey I liked the idea of modifying context of gallery shortcode. However, I was wondering if there is any chance of breaking or not executing any other logic that depends on the_post_thumbnail context either within the core or outside of the core. What do you feel?

I am not sure what you mean here? The context is change to gallery_shortcode and the back to the default with the removal of the filter. For images that are the featured image then it would set to the_post_thumbnail. I don't see how this would break anything.

As this is your ticket, I am going to leave this PR with you, see if you think it is a good part forward and maybe add some unit tests.

While this fixes the specific issue of the old gallery short code not being processed correctly, the bigger issue that this has made visible is that any images that are dynamically added to post content through a shortcode will also exhibit this bug. Rather than creating new context for any of these situations, I think we should take a look into why the rendered content isn't being processed correctly as content images. This is potentially a problem with the order in which shortcodes are processed compared with block content, but would need some investiagion.

Looking into this as well, I agree with @joemcgill's feedback. I believe the fetchpriority refactor broke something that https://core.trac.wordpress.org/changeset/55825 initially fixed. Currently testing to verify whether that is indeed the case.

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

@joemcgill One issue with this, is what if you call the gallery shortcode manually?

Example
​https://github.com/WordPress/wordpress-develop/blob/ebd03692e64d63803ad3fed2676dfb3cd4631f26/src/wp-includes/widgets/class-wp-widget-media-gallery.php#L129

[flixos90]

@spacedmonkey @thekt12 @joemcgill I have researched this bug further. I think the problem is not the context used, and it's also not caused by [55825]: While [55825] technically caused this to seem broken, the implementation was already flawed before. [56037] also included a minor unintended change that broke what [55825] solved. Let me break it down because this is quite complex and there are several issues here.

• The biggest problem is that do_shortcode() runs on the_content with hook priority 11, which is after wp_filter_content_tags() (default priority 10). This is problematic because it means the intended function to parse images in post content runs too early to affect images added via shortcodes. This has been wrong all along and really is the root of the problem.
• [55825] ensured that programmatically included images within post content were only modified when considered as part of the whole content blob, i.e. the the_content context. Yet, that didn't consider images from shortcodes, which technically caused this to seem broken. However, this was broken all along since even before [55825] images in a shortcode would be treated separately from other in-content images, which is problematic e.g. when the shortcode is at the beginning of the post content: Any other images in the post content would be counted before images in the shortcode were counted, meaning that the shortcode images would all get loading="lazy" even if they shouldn't.
• [56037] introduced a very small bug that partially reverted [55825]: Images in the_post_thumbnail or wp_get_attachment_image contexts shouldn't be handled when the the_content filter is running (i.e. not modify/add any attributes), but after [56037] they may now add fetchpriority="high".

Out of the above, the only clear and trivial course of action is to fix the regression that [56037] reintroduced after [55825] addressed it. That's a one-line change in the doing_filter( 'the_content' ) clause. I'm going to address that shortly.

For a holistic fix, I could see us doing one of two things:

• Either we change the hook priority of wp_filter_content_tags() on the the_content filter to something later, e.g. 12. That would resolve the issue holistically. However, it may have unintended side effects on the plugin ecosystem so should be approached with caution.
• Or we decide that for shortcodes it is their own responsibility of handling loading optimization attributes.

Either of the above are changes that don't fit into the scope of 6.3, both because they are too broad and because the root issue here was introduced as long ago as when wp_filter_content_tags() was added (WP 5.5).

Other than that, what we could do is ensure that the doing_filter( 'the_content' ) clause is only entered if the current hook priority ($GLOBALS['wp_filter']['the_content']->current_priority()) is anything before 10, i.e. before wp_filter_content_tags(). That would revert the behavior to what it was before, but it wouldn't make it any less faulty. Since lazy-loading too much can have more severe implications on load time performance / LCP than lazy-loading too little, I don't think we should reintroduce that bug. Either way, none of those two options would be a proper fix. Another reason to not reintroduce the previous behavior would be that both of the two more holistic proposals above would require that programmatically injected images in post content are never touched outside of the the_content context.

@spacedmonkey @joemcgill I left a comment outlining the issues I found in https://core.trac.wordpress.org/ticket/58681#comment:13

TL;DR even before this behavior changed, it was incorrect. The root problem is that shortcodes are only parsed after wp_filter_content_tags() has run. We could use approaches like this PR of #4799 to restore the previous behavior, but that's not really better than the way it behaves now.

@joemcgill I like the idea here, potentially. Please see my comment in https://core.trac.wordpress.org/ticket/58681#comment:13 for the root issue. Depending on what we decide to address it, maybe the approach here could be a starting point, but I think it requires some more holistic thought on how we want to go about the problem.

[joemcgill]

Thanks @flixos90. I've been looking into this today and came to the same conclusion as you just described above. The shortcodes are always being processed after content images are filtered by wp_filter_content_tags. Previously, dynamic images created as part of the content were always having lazy applied, which had it's own problems, as you described.

I'm curious if something like ​PR #4799 would be a simple workaround for shortcodes? Generally, I think finding a way of processing all content images at once, regardless of how they're created, would be the most robust strategy, since currently we can't determine the order in which a dynamic image added via a shortcode will fall in the markup when compared with image blocks or classic editor content.

I've left a comment on the ticket, but in general, I came to the same conclusion as you, @felixarntz. The changes introduced to the optimization heuristics in this cycle have exposed a bug that already existed with how images created via shortcodes were being handled. I'm not confident in this being a viable solution either, but it does at least ensure that shortcodes will be excluded from the logic that skips over dynamic images generated in content.

I don't know how common it is for content images to be generated from shortcodes, but it does seem like something we need to account for. I don't think making each shortcode handle it's own image optimization is the right path forward.

@spacedmonkey re:

One issue with this, is what if you call the gallery shortcode manually?

I think we only need this filter to be in place when doing the_content, so calling the shortcode manually outside of content or as part of a block render callback will probably be fine.

I'm not confident in this being a viable solution either, but it does at least ensure that shortcodes will be excluded from the logic that skips over dynamic images generated in content.

Right. Excluding shortcodes from that logic will result in shortcode images always receiving loading="lazy", while keeping as is will result in shortcode images not receiving any of the wp_get_loading_optimization_attributes() attributes. So the question here is what's the better default?

If shortcodes are primarily used for content below the fold, probably this PR is a good solution. But for shortcode usage above the fold it would potentially worsen the LCP problems caused by "excessive" lazy-loading that we've been working on fixing in this release.

I'm not confident in this being a viable solution either, but it does at least ensure that shortcodes will be excluded from the logic that skips over dynamic images generated in content.

Right. Excluding shortcodes from that logic will result in shortcode images always receiving loading="lazy", while keeping as is will result in shortcode images not receiving any of the wp_get_loading_optimization_attributes() attributes. So the question here is what's the better default?

If shortcodes are primarily used for content below the fold, probably this PR is a good solution. But for shortcode usage above the fold it would potentially worsen the LCP problems caused by "excessive" lazy-loading that we've been working on fixing in this release.

In this case It may even worsen CLS. However excluding the logic will result in equally worst experience.

This is a long short but what if we exclude all shortcodes at it is currently. We can have a the_content filter possibly after 12 or anything higher. There we can get the img tags which we can run through a simple threshold heuristic.
Heuristic will be such that.

• Have a minimum pixel threshold value. This will be considered as above_the_folder threshold. Any image that crosses this threshold will be considered bellow the fold.
• Get images in content. We can use regex, the way they did for shortcode in do_shortcode. Keep a track of img pixels from start.
• Exclude the images that are already has a loading strategy, however add it to the pixel count.
• If the img crosses the threshold we can set loading='lazy'.

This way we will not exclude lazy-loading or blindly add lazy-loading to shortcode images.

Welcome @joemcgill . This seems to fix the issue and is a nice workaround for this issue. One thing I noticed was that fetchpriority was not being added. To fix this a change the following line.

​https://github.com/WordPress/wordpress-develop/blob/bc526d31e6ecaad344a4a9f4c9343c0f3071a879/src/wp-includes/media.php#L5697

I changed it to.

if ( 'do_shortcode' === $context || 'the_content' === $context || 'the_post_thumbnail' === $context ) {

This seems to work.

This feels like a workaround, so it might work add a comment / todo in the code to fix this in a cleaner way in a future release.

Right. Excluding shortcodes from that logic will result in shortcode images always receiving loading="lazy", while keeping as is will result in shortcode images not receiving any of the wp_get_loading_optimization_attributes() attributes. So the question here is what's the better default?

If shortcodes are primarily used for content below the fold, probably this PR is a good solution. But for shortcode usage above the fold it would potentially worsen the LCP problems caused by "excessive" lazy-loading that we've been working on fixing in this release.

If we use this type of workaround, I think we could update the logic so that the do_shortcode context is treated exactly like the_content (as @spacedmonkey showed in the example above). The limitation is that all markup images processed with the_content context will be processed prior to any shortcodes and throw off the counters for when fetchpriority or lazy-loading is applied. I think in both cases, that is likely an ok tradeoff for now, as it will only mean that shortcode images that are part of the LCP will be loaded non-optimally. I think ensuring that shortcode images are lazy-loaded is a likely better than leaving things as they are now, so I'd advocate for something like this approach.

I've updated the PR with that change for consideration. I think trying to ensure shortcodes are processed prior to applying image optimizations is a bigger challenge that should be looked into separately.

@joemcgill Those are some good points. While this is surely not a proper solution, having the do_shortcode context increase image counts is _probably_ okay as a trade-off, specifically since it only runs after wp_filter_content_tags(), so it already "gave a chance" to the more reliable logic to do things the right way. For example, if you have some regular in-content images and a gallery shortcode further down the page, this would work fine.

The main only issue here is really when the shortcode is at the beginning of the post content. In that case, it'll still only be parsed after all the rest of the content, and if the rest of the content already includes images, the shortcode images will all be lazy-loaded rather than potentially receiving fetchpriority="high". This will negatively affect performance.

It really is a tradeoff: Do we avoid lazy-loading benefits for shortcode images entirely, or do we risk LCP regression due to shortcode images? I believe the latter is a much smaller surface area, but at the same time more harmful to performance than not lazy-loading.

I am inclined to say this fix here is _okay_, but it'll still have notable flaws, and part of me thinks it would be better to just leave as is and try to _properly_ fix for 6.4. 🤔

I am inclined to say this fix here is okay, but it'll still have notable flaws, and part of me thinks it would be better to just leave as is and try to properly fix for 6.4. 🤔

In my testing, I saw the lazy loading being added to the gallery images, which fixes the issues at hand. I think I would for commit this change to fix this issue for now and work on a large refactor of the code in WP 6.4 to solve this once and for all.

@felixarntz

It really is a tradeoff: Do we avoid lazy-loading benefits for shortcode images entirely, or do we risk LCP regression due to shortcode images? I believe the latter is a much smaller surface area, but at the same time more harmful to performance than not lazy-loading.

Agree that it's a tradeoff. I would think that the probability of lazy loading the LCP image is likely and edge case compared with not lazy-loading gallery images, or other images created as part of a shortcode callback. Given that these images were previously being lazy-loaded, I'd call the removal of that behavior a regression and commit something like this for the time being.

Thanks @joemcgill @spacedmonkey, that works for me. Let's make sure to add code comments to clarify why the decisions here were being taken.

Closing in favour of #4799

Hmmm....these unit tests were passing in isolation, so it seems like there is state that's not properly being reset between tests, which could have odd effects on all of these unit test cases for optimization. I'm going to look into this today.

I did some testing.

Created a new shortcode.

function div_html($atts, $content = null) {

	extract( shortcode_atts( array(
		'class' => '',
	), $atts ) );

	$class = $class ? " class=\"$class\"" : NULL;

	return "<div$class>".do_shortcode( $content ) ."</div>";
}
add_shortcode('div', 'div_html');

Then inserted a gallery shortcode + div shortcode like this.

[div][gallery ids="761,760,759,758,757"][/div]

Lazy loading attributes.

One thing that is interesting, even through this image is the second on the page, has lazy loading attributes.

Thanks, @spacedmonkey I'll give it a look.

After doing some more looking into why the shortcode tests were failing when run in the whole group, but not when run in isolation, I discovered that the places where we are setting the main query using $this->set_main_query() was polluting any following tests that rely on logic that checks the main query. To fix this, I've added a line to the set_up() routine for these tests that resets the main query to the global $wp_query object, so each test has a clean main query object.

@joemcgill

After doing some more looking into why the shortcode tests were failing when run in the whole group, but not when run in isolation, I discovered that the places where we are setting the main query using $this->set_main_query() was polluting any following tests that rely on logic that checks the main query. To fix this, I've added a line to the set_up() routine for these tests that resets the main query to the global $wp_query object, so each test has a clean main query object.

Are you sure that's the issue? It baffles me reading this because the core test suite resets all globals after each test, or at least it should. That's why generally cleanup of global changes in tests are unnecessary. If this global is not reset, then maybe there's a broader problem somewhere?

[spacedmonkey]

@joemcgill @flixos90 This was moved to WordPress 6.4. But I feel like this is a bug and should resolved in WP 6.3. The PR feels like it is close to be able to be committed. Are we working on this now so we can commit now or should we wait for a while for when trunk opens for 6.4?

[joemcgill]

Thanks for flagging, @spacedmonkey. I've moved it back to this milestone and have the PR very close to commit, pending approvals on the PR.

I've added an additional test case for the nested shortcode use, using the example code from @spacedmonkey in ​this comment. While reviewing the logic for this use case, it looks like everything is happening as expected.

@spacedmonkey I assume that the demo you set up to test this included enough content images after the shortcode that by the time the shortcodes were processed, the threshold for omitting loading="lazy" had already been reached, which is why that image is lazily loaded even though it is in the second position. This is a known limitation of the current way shortcode logic is processed and something that will require a bigger refactor to fully fix, which is why it wasn't accounted for in the scope of this approach.

@joemcgill While your new test looks good to me and passes as expected, I'd like to circle back to what I mentioned in ​https://github.com/WordPress/wordpress-develop/pull/4799/files#r1254798014: Adding the filter in every call of do_shortcode() works, but feels somewhat wasteful and potentially error-prone to me. In your test for example, the same closure will be added and removed twice. Because it's a closure, that doesn't cause _real_ problems because the closure is initialized every time that do_shortcode() is called. But if it was a regular function, the exact same construct would be added as a filter callback multiple times, which is part of why I consider that approach error-prone. Currently, it doesn't break anything, but I also don't think it's a good idea to have two closures with the same logic hooked into wp_get_attachment_image_context only to override the value.

I would advise to use a conditional and only add/remove the filter in the outermost do_shortcode() call.

@felixarntz

Are you sure that's the issue?

I was surprised by this as well, so I did some more investigation and I can confirm that the global $wp_the_query value that is set in test_the_excerpt_does_not_affect_omit_lazy_loading_logic remains in place in subsequent tests, so that global must not be getting reset after each test. the WP_UnitTestCase_Base::tear_down() method resets the global $wp_query, but not $wp_the_query, so any test that modifies the main query needs to reset it afterward.

Really, all of the resetting that we are doing should happen after each test rather than before, so I've updated things a bit in ​c809901 so that the main query gets reset to an empty query after each test, and all of the other resets happen during the tear_down() as well. This should make things more consistent generally.

Thanks @joemcgill. While resetting after the tests here specifically works well as a short-term fix, I opened #4823 for consideration of a holistic fix to this problem. The global should really be reset in the same place where the other WP query loop related globals are being reset, similar to how wp-settings.php sets them. The fact it's not happening now seems like an oversight to me.

This change looks good. I have had a chance to do a full review. But you can commit if you want to. If not committed in the morning, I will do a full review and test.

[joemcgill]

Waiting to see if the fix for #58776 is committed first, so I can remove the fix from the specific test class for this ticket before committing. Otherwise, this is ready to go.

[joemcgill]

In 56214:

Media: Optimize images created in shortcodes.

This fixes an issue where images dynamically created during shortcode rendering (e.g., shortcode image galleries), were not getting image optimizations like loading="lazy" or fetchpriority="hight" applied. Note that even after this commit, shortcodes are processed after the main content images, which can affect the order in which optimizations are applied in content areas.

Follow-up to [56037].

Props spacedmonkey, flixos90, thekt12, swissspidy, joemcgill.
Fixes #58681.

Committed in https://core.trac.wordpress.org/changeset/56214.