Filter wp_get_attachment_image_context context for shortcodes #4799

joemcgill · 2023-07-05T22:08:34Z

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

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

spacedmonkey · 2023-07-05T22:18:26Z

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

Example

wordpress-develop/src/wp-includes/widgets/class-wp-widget-media-gallery.php

Line 129 in ebd0369

echo gallery_shortcode( $shortcode_atts );

felixarntz · 2023-07-05T22:26:52Z

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

To me that is 6.4 material rather than 6.3, except if we feel strongly that we should restore the previous behavior (even though it's just as incorrect or potentially worse).

joemcgill · 2023-07-05T22:50:34Z

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.

felixarntz · 2023-07-05T22:55:15Z

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 · 2023-07-06T10:41:34Z

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 as it is currently. We can have another the_content filter possibly after priority 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_fold threshold. Any image that crosses this threshold will be considered below the fold.
Have a pixel_crossed variable that keeps a tab of pixels so far.
Get images in content. We can use regex, the way they did for shortcode in do_shortcode.
Get pixel of the image (width*height) and add it to pixel_crossed
Exclude the images that are already has a loading strategy, however add it to the pixel_crossed 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.
Also this will take into consideration shortcodes that were manually called but withing the_content.

Later, we may further enhance it to remove our current heuristic and create a new one that checks for image towards the end and not at the start.

spacedmonkey · 2023-07-06T11:05:54Z

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.

wordpress-develop/src/wp-includes/media.php

Line 5697 in bc526d3

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

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.

joemcgill · 2023-07-06T16:41:51Z

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.

felixarntz · 2023-07-06T17:13:45Z

@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 · 2023-07-06T17:26:26Z

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 shortcode 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 · 2023-07-06T18:20:00Z

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

felixarntz · 2023-07-06T18:38:09Z

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

felixarntz

@joemcgill Left some feedback on the code below, the overall approach is good to move forward with.

felixarntz · 2023-07-06T18:41:50Z

src/wp-includes/shortcodes.php

+		return 'do_shortcode';
+	};
+
+	add_filter( 'wp_get_attachment_image_context', $callback );


Shortcodes are sometimes nested, particularly by plugins that make more complex usage of shortcodes. We should make sure that adding the filter here and removing it below doesn't cause weird behaviors.

I think this code would be more error-proof if we did the following:

Only add the filter callback if it's not already added to the filter hook.

Below, only remove the filter callback if it was added in the same do_shortcode() execution.

To ensure that we reference the same callback throughout different do_shortcode() calls, we could either make $callback a static variable so that it always references the same closure or replace it with a private function (or rather marked as private) like _return_do_shortcode_context().

Some unit tests with nest shortcodes would be handy. Also a test for gallery and caption shortcodes, as these are used a lot in wild.

src/wp-includes/media.php

spacedmonkey · 2023-07-07T13:36:09Z

src/wp-includes/shortcodes.php

+	$callback = static function() {
+		return 'do_shortcode';
+	};


I don't love this pattern of adding static closures to hooks. It makes it impossible unhook then. They are harder to test. Can we make a private function for this.

I disagree here. Not everything that is in a hook is intended to be unhooked. While hooks are great, there are things for which unhooking will simply break expected behavior, so I find the idea of requiring every hook to be removable questionable.

IMO a private function is a less clean approach here than this closure.

I usually would agree, and was a bit on the fence about this one so I'm happy to change it if needed. For now, I've updated an inline comment (ee97a6c) where that callback is added to give more context for why it's there. I agree that the intent is not to have folks unhook it, but we could consider adding it to an earlier priority to ensure any other filters added overtake this one if you think it would help.

Closure are horrible for unit test coverage reports and for profiling tools. See this example screenshot. I think closures make sense in plugins, but as a framework, WordPress does and should always, allow you to remove filters.

Look at this profile, how can find these closures? Are they even the same closure?

I'm wary of this turning into a broader conversation about closure usage. The closure here is tiny and introducing a private function for it instead would add things to the global scope, which is usually not a good idea for things that are private.

For profiling, I'd argue seeing the parent functions below should clarify where the closure is called.

Using a closure like this may also result in the same filter being added multiple files, as _wp_filter_build_unique_id generates a new value for everytime the closure is used. This may end up adding many many keys to the callbacks property of WP_Hook.

But if you're passing the same variable referencing the same closure then spl_object_hash() returns the same hash, no? Don't wanna argue about the usefulness of closure, but pretty sure it would work.

@swissspidy That's what I was thinking as well.

@spacedmonkey I think the only reason that the hashes in your screenshots in the other thread #4799 (comment) are different is because they are not the same closure. We would need to put the closure into a static variable and only then it would be the same.

@felixarntz This screen was from a static variable like this

$callback = static function() { return 'do_shortcode'; };

I replicate it again with commit c3776ab

That‘s just a static closure but not a static variable

@spacedmonkey That's not a static variable, only the closure is a static function. For it to be a static variable, it would need to be

static $callback = static function() { return 'do_shortcode'; };

joemcgill · 2023-07-10T13:16:47Z

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 · 2023-07-10T14:29:49Z

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.

joemcgill · 2023-07-10T14:31:55Z

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

joemcgill · 2023-07-10T14:41:36Z

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.

felixarntz · 2023-07-10T14:58:26Z

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

joemcgill · 2023-07-10T16:59:53Z

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.

felixarntz · 2023-07-10T17:10:55Z

@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

To clarify what I mean in #4799 (comment), here's what I'm suggesting:

src/wp-includes/shortcodes.php

tests/phpunit/tests/media.php

joemcgill · 2023-07-10T19:44:29Z

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

felixarntz · 2023-07-10T20:26:28Z

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.

joemcgill

Fix up debugging leftovers and spacing.

tests/phpunit/tests/media.php

src/wp-includes/shortcodes.php

felixarntz

Thanks @joemcgill, LGTM! Just one non-blocking comment.

tests/phpunit/tests/media.php

spacedmonkey · 2023-07-10T21:38:49Z

tests/phpunit/tests/media.php

+
+	public function data_wp_get_loading_optimization_attributes_in_shortcodes() {
+		return array(
+			'default' => array(


These keys normally describe the test case a little.

Improved in b9a8698

spacedmonkey · 2023-07-10T21:45:26Z

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.

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

spacedmonkey

Looks good to me.

joemcgill · 2023-07-11T13:58:25Z

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

Filter wp_get_attachment_image_context context for shortcodes

e8f56fd

felixarntz mentioned this pull request Jul 5, 2023

Change the context of wp_get_attachment_image_context #4794

Closed

Apply the_content logic to shortcode images

2ea498a

felixarntz reviewed Jul 6, 2023

View reviewed changes

spacedmonkey reviewed Jul 7, 2023

View reviewed changes

joemcgill added 4 commits July 9, 2023 14:51

Add inline comment to static hook callback

ee97a6c

Add unit tests

87d12ca

Fix CS errors

6c15784

More CS fixes

09172d6

Fix unit tests

556a8fe

Reset main query before each test

b957f5c

joemcgill added 2 commits July 10, 2023 10:52

Add test for nested shortcode

4716e20

Fix unit test CS issues

c559445

felixarntz reviewed Jul 10, 2023

View reviewed changes

src/wp-includes/shortcodes.php Outdated Show resolved Hide resolved

src/wp-includes/shortcodes.php Outdated Show resolved Hide resolved

src/wp-includes/shortcodes.php Outdated Show resolved Hide resolved

swissspidy reviewed Jul 10, 2023

View reviewed changes

tests/phpunit/tests/media.php Outdated Show resolved Hide resolved

Improve resetting state

c809901

Use assertStringContainsString

c3776ab

felixarntz mentioned this pull request Jul 10, 2023

Ensure WP_UnitTestCase foundation resets main query global #4823

Closed

joemcgill added 2 commits July 10, 2023 15:31

Improve shortcode context filtering

851d603

Fix unit tests

4eac942

joemcgill commented Jul 10, 2023

View reviewed changes

tests/phpunit/tests/media.php Outdated Show resolved Hide resolved

tests/phpunit/tests/media.php Outdated Show resolved Hide resolved

Self CR fixes

755b713

spacedmonkey reviewed Jul 10, 2023

View reviewed changes

src/wp-includes/shortcodes.php Show resolved Hide resolved

felixarntz approved these changes Jul 10, 2023

View reviewed changes

tests/phpunit/tests/media.php Outdated Show resolved Hide resolved

spacedmonkey marked this pull request as ready for review July 10, 2023 21:37

spacedmonkey reviewed Jul 10, 2023

View reviewed changes

joemcgill and others added 2 commits July 10, 2023 19:43

Set main query to the global $wp_query object

e0b52a3

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

More descriptive data provider keys

b9a8698

spacedmonkey approved these changes Jul 11, 2023

View reviewed changes

joemcgill added 2 commits July 11, 2023 08:32

Remove redundant reset of the main query object

6f533b9

Merge branch 'trunk' into fix/58681-image-optimization-in-shortcodes

6d2ebed

joemcgill closed this Jul 11, 2023

Filter wp_get_attachment_image_context context for shortcodes #4799

Filter wp_get_attachment_image_context context for shortcodes #4799

Conversation

joemcgill commented Jul 5, 2023

spacedmonkey commented Jul 5, 2023

felixarntz commented Jul 5, 2023 • edited

joemcgill commented Jul 5, 2023

felixarntz commented Jul 5, 2023

kt-12 commented Jul 6, 2023 • edited

spacedmonkey commented Jul 6, 2023

joemcgill commented Jul 6, 2023

felixarntz commented Jul 6, 2023

spacedmonkey commented Jul 6, 2023 • edited

joemcgill commented Jul 6, 2023

felixarntz commented Jul 6, 2023

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz Jul 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joemcgill commented Jul 10, 2023

spacedmonkey commented Jul 10, 2023

joemcgill commented Jul 10, 2023

joemcgill commented Jul 10, 2023

felixarntz commented Jul 10, 2023

joemcgill commented Jul 10, 2023

felixarntz commented Jul 10, 2023

felixarntz left a comment

Choose a reason for hiding this comment

joemcgill commented Jul 10, 2023

felixarntz commented Jul 10, 2023

joemcgill left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacedmonkey commented Jul 10, 2023

spacedmonkey left a comment

Choose a reason for hiding this comment

joemcgill commented Jul 11, 2023

felixarntz commented Jul 5, 2023 •

edited

kt-12 commented Jul 6, 2023 •

edited

spacedmonkey commented Jul 6, 2023 •

edited

felixarntz Jul 10, 2023 •

edited