Fix shortcode output not receiving image (and other) optimizations

[flixos90]

This ticket aims to properly address the bug that was discovered and initially discussed in #58681, starting with this comment: As of today, HTML from shortcodes does not receive the optimizations from wp_filter_content_tags() like other content does. This has been due to an unfortunate oversight from when the function was initially introduced in WordPress 5.5, as it used the default hook priority of 10 on the the_content filter, while do_shortcode uses hook priority 11.

Similarly, when wp_filter_content_tags() was used in other places like parsing of block templates or block template parts, it was also implemented in the same incorrect order.

#58681 implemented a hacky workaround to at least get the loading optimization attributes from wp_get_loading_optimization_attributes() added to shortcodes, yet even that workaround does not work reliably since images using it are parsed separately from the rest of the content, which can lead to LCP problems, e.g. when a shortcode is on top of the page, as it would still receive loading="lazy" when it shouldn't.

[flixos90]

To fix this bug, I am proposing that we reverse the order that wp_filter_content_tags() and do_shortcode() are called. Technically that is a very simple change, but I acknowledge that we have to explore how wp_filter_content_tags() is used in the ecosystem to assess feasibility of the change, especially when it comes to the particular usage on the the_content filter. For example, are there plugins that call remove_filter( 'the_content', 'wp_filter_content_tags' )?

[flixos90]

I did a search for wp_filter_content_tags (function and filter) through the plugin directory and put the results in ​this spreadsheet. Here's a summary:

• Out of all plugins with 10,000+ installs, only 28 plugins use wp_filter_content_tags in any way. I took a closer look at them:
  • 10 plugins call the function on custom content without calling do_shortcode() on the same content at all. So there is no problem here.
  • 14 plugins call the function on custom content and then call do_shortcode() on the same content. So effectively they are doing the same thing wrong as core. If we reverse the order in core, such plugins should be recommended to reverse the order as well, that said there would be no backward compatibility implications on them if core made the change.
  • 4 plugins call the function on custom content after calling do_shortcode() before. So those plugins are effectively already doing the right thing, even though core arguably does not. So there's obviously no problem here either.
  • Only a single plugin uses wp_filter_content_tags in any way that is related to the the_content filter, and even that is only a check for has_filter( 'the_content', 'wp_filter_content_tags' ). In other words, even if core changed the hook priority of the filter, that code would still work the same as before.
• In the entire plugin directory, there are zero calls to remove_filter( 'the_content', 'wp_filter_content_tags'.

Based on this research, I think it is reasonable to change the hook priority of wp_filter_content_tags on the_content, basically to anything later than 11, which is used by do_shortcode. We can discuss whether we want to use 12 or a much higher number. Even though the function has existed since WordPress 5.5, in WordPress that's relatively new, and the low usage across plugins confirms that. For example, changing the hook priority of do_shortcode on the_content would not be possible because it's heavily used with the hook priority of 11 in plugins (even a short glimpse at the plugins analyzed above confirmed that).

[flixos90]

It is worth noting that even in core, the wp_filter_content_tags() function is only called before do_shortcode() in 3 places:

• on the_content filter
• on widget_text_content filter
• on widget_block_content filter

For block templates, wp_filter_content_tags() is already called after do_shortcode(), and for block template parts, do_shortcode() is not called at all.

Based on that, I am proposing the following fix for this ticket:

• Change the hook priority of wp_filter_content_tags to a higher value (>=12) on the the_content, widget_text_content, and widget_block_content filters.
• Remove the workaround added in [56214].
• Instead, add do_shortcode to the list of contexts in wp_get_loading_optimization_attributes() where the clause returns early if doing_filter( 'the_content' ).

This will ensure that tags from shortcodes will be processed together with the other content, in the same way.

cc @joemcgill @spacedmonkey

[joemcgill]

One other use case that we need to add a test case for is when do_shortcode() is called manually to render a shortcode early in the rendering process, and not via the filter callback.

For example, if some custom code like a dynamic block's render callback is written like this (simplified):

<?php
function render_custom_block() {
    return do_shortcode( '[shortcode-slug]' );
}

The shortcode will be processed and (assuming it results in image markup) will have optimizations applied prior to the wp_filter_content_tags() callback on the_content. This currently throws off the counts used to determine when optimizations should be applied.

[flixos90]

@joemcgill In your example, I think this wouldn't be a separate problem actually, since blocks (including dynamic blocks) are still parsed as part of the_content filter, so the shortcode in a block would have the same implications as a shortcode that is directly within the post content. So I don't think this would need to be specifically considered, it would be fixed by doing the same as I outlined in my above comment for shortcodes in general. Basically it is currently wrong as well, because the do_shortcode context is not among the contexts checked for around the early return if doing_filter( 'the_content' ).

We can definitely add a test for it to make sure it's also covered.

[joemcgill]

Replying to flixos90:

@joemcgill In your example, I think this wouldn't be a separate problem actually, since blocks (including dynamic blocks) are still parsed as part of the_content filter, so the shortcode in a block would have the same implications as a shortcode that is directly within the post content.

I more detailed example would likely clarify the issue, but what I'm anticipating is that all blocks are being rendered prior to optimizations being applied to rendered markup, so the logic that tries to make assumptions about the optimizations based on the position in the markup could be wrong. For example, if a page included a large image block followed by a shortcode generated image (in my previous example), we would want to ensure that fetchpriority="high" was added to the image block and not the shortcode generated image.

Currently, I suspect the shortcode generated image would actually be the one that receives the fetchpriority attribute.

[flixos90]

Replying to joemcgill:

For example, if a page included a large image block followed by a shortcode generated image (in my previous example), we would want to ensure that fetchpriority="high" was added to the image block and not the shortcode generated image.

Currently, I suspect the shortcode generated image would actually be the one that receives the fetchpriority attribute.

Since blocks are parsed before the wp_filter_content_tags(), and the wp_get_attachment_image context that images in blocks would typically be using is explicitly not treated during the_content filter, so the block would work as expected I think. The problem is that the shortcode would currently be parsed after wp_filter_content_tags(), which realistically for this example would not cause any functional problem because it would happen after the block image anyway.

It's actually the reverse example I think which would be broken right now: A large shortcode generated image followed by a large image block would currently result in the 2nd image receiving fetchpriority="high" because only that image would be handled as part of wp_filter_content_tags(). That's because the shortcode is parsed later than that, i.e. too late.

By parsing shortcodes before wp_filter_content_tags() and ensuring that images from the do_shortcode context are ignored during the_content filter, it should be ensured that both kinds of images are handled only within wp_filter_content_tags().

• Move wp_filter_content_tags to a later priority than do_shortcode() is at (>11, for now 12 was chosen)
• Bail early when encountering a shortcode that appears within content blobs like the_content, widget_text_content, widget_block_content so that images within those shortcodes are only handled once as part of the overall content blob
• Add test coverage for a shortcode within post content, as well as a shortcode within a block within post content
• Remove test cases which aren't relevant anymore given that shortcodes should be treated within their content blobs rather than individually

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

• Move wp_filter_content_tags to a later priority than do_shortcode() is at (>11, for now 12 was chosen)
• Bail early when encountering a shortcode that appears within content blobs like the_content, widget_text_content, widget_block_content so that images within those shortcodes are only handled once as part of the overall content blob
• Add test coverage for a shortcode within post content, as well as a shortcode within a block within post content
• Remove test cases which aren't relevant anymore given that shortcodes should be treated within their content blobs rather than individually

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

@joemcgill Friendly ping, could you please take a look at this one? :)

Noting here that this PR will need to be refreshed if #5197 is committed first, or vice-versa if this one is committed first. cc @mukeshpanchal27

@joemcgill @spacedmonkey Would you mind giving this another review please? I have addressed your feedback and refreshed the PR now that https://core.trac.wordpress.org/changeset/56612 has been committed.

Per the discussion in ​https://wordpress.slack.com/archives/C02KGN5K076/p1695659619574269, it's safer to keep the priority change to 12 rather than using a later priority.

[flixos90]

In 56693:

Media: Ensure images within shortcodes are correctly considered for loading optimization attributes.

Prior to this change, images added in shortcodes would be considered separately from all other images within post content, which led to incorrect application of the loading optimization attributes loading="lazy" and fetchpriority="high".

This changeset changes the filter priority of wp_filter_content_tags() from the default 10 to 12 on the various content filters it is hooked in, in order to run that function after parsing shortcodes. While this may technically be considered a backward compatibility break, substantial research and lack of any relevant usage led to the assessment that the change is acceptable given its benefits.

An additional related fix included is that now the duplicate processing of images is prevented not only for post content blobs (the_content filter), but also for widget content blobs (widget_text_content and widget_block_content filters).

Props joemcgill, mukesh27, costdev, spacedmonkey, flixos90.
Fixes #58853.

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

[codente]

@flixos90

Is this something for the email to plugin authors which is being collated in Marcomms?

[flixos90]

Dev note published: ​https://make.wordpress.org/core/2023/10/18/image-loading-optimization-enhancements-in-6-4/