Make WordPress Core

Opened 10 months ago

Closed 9 months ago

Last modified 9 months ago

#58681 closed defect (bug) (fixed)

No lazy loading attribute on images in gallery

Reported by: spacedmonkey's profile spacedmonkey Owned by: joemcgill's profile joemcgill
Milestone: 6.3 Priority: normal
Severity: normal Version: 5.5
Component: Media Keywords: has-patch has-unit-tests commit
Focuses: performance Cc:

Description

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.

Attachments (6)

large-gallery-trunk.png (3.4 MB) - added by spacedmonkey 10 months ago.
Gallery shortcode with large images on Trunk
large-gallery-62.png (3.4 MB) - added by spacedmonkey 10 months ago.
Gallery shortcode with large images on WP 6.2
thumbnail-gallery-62.png (2.6 MB) - added by spacedmonkey 10 months ago.
Gallery shortcode with thumbnail images on WP 6.2
Screenshot 2023-06-30 at 10.51.13.png (2.6 MB) - added by spacedmonkey 10 months ago.
Gallery shortcode with thumbnail images on Trunk
gallary-large-twenty-twenty-three.png (846.8 KB) - added by thekt12 10 months ago.
Gallery shortcode with thumbnail images on Trunk, Theme - Twenty Twenty-Three ( working as expected )
gallary-large-twenty-twenty-nineteen.png (917.4 KB) - added by thekt12 10 months ago.
Gallery shortcode images on Trunk, Theme - Twenty Twenty-Nineteen ( working as expected )

Change History (49)

#1 @spacedmonkey
10 months ago

  • Owner set to flixos90
  • Status changed from new to assigned

@spacedmonkey
10 months ago

Gallery shortcode with large images on Trunk

@spacedmonkey
10 months ago

Gallery shortcode with large images on WP 6.2

@spacedmonkey
10 months ago

Gallery shortcode with thumbnail images on WP 6.2

@spacedmonkey
10 months ago

Gallery shortcode with thumbnail images on Trunk

@thekt12
10 months ago

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

@thekt12
10 months ago

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

#2 @spacedmonkey
10 months ago

@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"

#3 @flixos90
10 months ago

  • Milestone changed from Future Release to 6.3
  • Priority changed from high to normal

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

#4 @flixos90
10 months ago

  • Owner changed from flixos90 to thekt12

Reassigning to you @thekt12 per our DMs

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


10 months ago
#5

  • Keywords has-patch added

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

#6 @spacedmonkey
10 months ago

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.

kt-12 commented on PR #4794:


10 months ago
#7

@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 commented on PR #4794:


10 months ago
#8

@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.

joemcgill commented on PR #4794:


10 months ago
#9

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.

@flixos90 commented on PR #4794:


10 months ago
#10

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.

#13 @flixos90
10 months ago

  • Keywords 2nd-opinion added; has-patch removed
  • Milestone changed from 6.3 to 6.4
  • Owner changed from thekt12 to flixos90
  • Version changed from trunk to 5.5

@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.

@flixos90 commented on PR #4794:


10 months ago
#14

@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.

@flixos90 commented on PR #4799:


10 months ago
#15

@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.

#16 @joemcgill
10 months ago

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.

joemcgill commented on PR #4799:


10 months ago
#17

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.

@flixos90 commented on PR #4799:


10 months ago
#18

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.

kt-12 commented on PR #4799:


10 months ago
#19

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.

@spacedmonkey commented on PR #4799:


10 months ago
#20

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.
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/237508/7cdf8986-b626-4372-b403-3d66334a6c9e

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.

joemcgill commented on PR #4799:


10 months ago
#21

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.

@flixos90 commented on PR #4799:


10 months ago
#22

@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. 🤔

@spacedmonkey commented on PR #4799:


10 months ago
#23

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.

joemcgill commented on PR #4799:


10 months ago
#24

@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.

@flixos90 commented on PR #4799:


10 months ago
#25

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

@spacedmonkey commented on PR #4794:


9 months ago
#26

Closing in favour of #4799

joemcgill commented on PR #4799:


9 months ago
#27

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.

@spacedmonkey commented on PR #4799:


9 months ago
#28

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.
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/237508/2bb97509-22a8-41e8-90ca-ae545d4ac075

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

joemcgill commented on PR #4799:


9 months ago
#29

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

joemcgill commented on PR #4799:


9 months ago
#30

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.

@flixos90 commented on PR #4799:


9 months ago
#31

@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?

#32 @spacedmonkey
9 months ago

@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?

#33 @joemcgill
9 months ago

  • Keywords has-patch added; 2nd-opinion removed
  • Milestone changed from 6.4 to 6.3

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.

joemcgill commented on PR #4799:


9 months ago
#34

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.

@flixos90 commented on PR #4799:


9 months ago
#35

@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.

joemcgill commented on PR #4799:


9 months ago
#36

@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.

@flixos90 commented on PR #4799:


9 months ago
#37

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.

@spacedmonkey commented on PR #4799:


9 months ago
#38

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.

#39 @joemcgill
9 months ago

  • Keywords has-unit-tests commit added
  • Owner changed from flixos90 to 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.

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


9 months ago

#41 @joemcgill
9 months ago

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

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.

This ticket was mentioned in Slack in #hosting by javier. View the logs.


9 months ago

Note: See TracTickets for help on using tickets.