WordPress.org
Get WordPress

Make WordPress Core

Opened 12 months ago

Closed 2 months ago

Last modified 2 months ago

#62731 closed enhancement (fixed)

Inline CSS inserted by wp_print_auto_sizes_contain_css_fix too high in HTML

Reported by: superpoincare's profile superpoincare Owned by: westonruter's profile westonruter
Milestone: 6.9 Priority: normal
Severity: minor Version: 6.7.1
Component: Media Keywords: has-patch has-test-info dev-feedback
Focuses: Cc:

Description (last modified by SergeyBiryukov)

The ticket [59435] / #62413 injects inline css in HTML. However the CSS seems too much early, even before the title element.

Although valid, injecting it somewhere below is desirable.

Attachments (1)

block-themes-title-tag-support.diff​ (1.5 KB) - added by sabernhardt 6 months ago.
adds 'title-tag' support to block themes to work with _wp_render_title_tag

Download all attachments as: .zip

Change History (23)

#1 @SergeyBiryukov
12 months ago

  • Description modified (diff)

#2 @sabernhardt
6 months ago

  • Severity changed from normal to minor

I did not find an explanation on #62413 for printing the image style instead of enqueuing it.

The style tag is added at priority 1 along with the <title> and wp_robots. Block themes print the title later because _block_template_render_title_tag ​replaces `_wp_render_title_tag`, adding the <title> at the same priority but apparently later in the order.

Twenty Twenty-Four (block theme):

<!DOCTYPE html>
<html lang="en-US">
<head>
	<meta charset="UTF-8" />
	<meta name="viewport" content="width=device-width, initial-scale=1" />
<meta name='robots' content='noindex, nofollow' />
	<style>img:is([sizes="auto" i], [sizes^="auto," i]) { contain-intrinsic-size: 3000px 1500px }</style>
	<title>Sample Page &#8211; Trunk (SVN)</title>

Twenty Twenty-One (classic theme with 'title-tag' support):

<!doctype html>
<html lang="en-US" >
<head>
	<meta charset="UTF-8" />
	<meta name="viewport" content="width=device-width, initial-scale=1.0" />
	<title>Sample Page &#8211; Trunk (SVN)</title>
<meta name='robots' content='noindex, nofollow' />
	<style>img:is([sizes="auto" i], [sizes^="auto," i]) { contain-intrinsic-size: 3000px 1500px }</style>

#3 @sabernhardt
6 months ago

If the title should be earlier, block themes probably could add 'title-tag' support now instead of using the _block_template_render_title_tag function. (@westonruter proposed the ​separate function, in the context of the Gutenberg plugin, and #53176 carried it over with a new name.)

Looking at some ​plugins:

  • Yoast SEO and ExpertFile remove both _wp_render_title_tag and _block_template_render_title_tag from wp_head.
  • Brizy Page Builder removes _block_template_render_title_tag because it interfered with the plugin's ​blank template.

The ​WeCodeArt theme replaces _wp_render_title_tag with _block_template_render_title_tag.

@sabernhardt
6 months ago

adds 'title-tag' support to block themes to work with _wp_render_title_tag

This ticket was mentioned in ​PR #8954 on ​WordPress/wordpress-develop by ​@westonruter.
6 months ago #4

  • Keywords has-patch added

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

Instead of manually printing the STYLE tag to fix sizes=auto issue with images, this enqueues the inline style via WP_Styles. This ensures the styles are printed with other styles as opposed to being printed very early, even before the TITLE tag. This also allows plugins to do additional processing of the stylesheet prior to it being printed. Note that array_unshift( wp_scripts()->queue, $handle ) is used instead of wp_enqueue_style( $handle ) in order to ensure that the stylesheet is printed before any others, preserving the existing cascade.

This is very similar to the change that was made to replace print_emoji_styles() with wp_enqueue_emoji_styles() in Core-58775.

Diff on rendered page (after Prettier formatting):

  • .html

    old new  
    44                <meta charset="UTF-8" />
    55                <meta name="viewport" content="width=device-width, initial-scale=1" />
    66                <meta name="robots" content="max-image-preview:large" />
    7                 <style>
    8                         img:is([sizes='auto' i], [sizes^='auto,' i]) {
    9                                 contain-intrinsic-size: 3000px 1500px;
    10                         }
    11                 </style>
    127                <title>WordPress Develop</title>
    138                <link
    149                        rel="alternate"
    … …  
    544539                                }
    545540                        )(window, document, window._wpemojiSettings);
    546541                </script>
     542                <style id="wp-img-auto-sizes-contain-inline-css">
     543                        img:is([sizes='auto' i], [sizes^='auto,' i]) {
     544                                contain-intrinsic-size: 3000px 1500px;
     545                        }
     546                </style>
    547547                <style id="wp-block-site-title-inline-css">
    548548                        .wp-block-site-title {
    549549                                box-sizing: border-box;

#5 follow-ups: @westonruter
6 months ago

  • Component changed from General to Media
  • Milestone changed from Awaiting Review to 6.9

Thank you for the ping. I've been wondering about this style and why it is printed so early.

It seems like it should have been enqueued as an inline style like was done in #58775 for the emoji styles. I've opened a ​PR to make this change. It's currently a draft because I'd like to add tests before proceeding to commit, and I want affirmation on the direction before writing tests.

@joemcgill @flixos90 Any specific reason for why the style was manually printed at wp_head?

#6 in reply to: ↑ 5 ; follow-ups: @SirLouen
6 months ago

  • Keywords needs-testing has-test-info added

For testers of this patch, the same tests apply, as in #62413

Replying to westonruter:

It seems like it should have been enqueued as an inline style like was done in #58775 for the emoji styles. I've opened a ​PR to make this change. It's currently a draft because I'd like to add tests before proceeding to commit, and I want affirmation on the direction before writing tests.

Doesn't make sense to me, deprecating such nouveau function wp_print_auto_sizes_contain_css_fix. I understand this the backwards compatibility policy, but we can be pretty confident noone is using this (and if they were using it, they might have reported this). ​So just wipe it.

Replying to sabernhardt:

If the title should be earlier, block themes probably could add 'title-tag' support now instead of using the _block_template_render_title_tag function. (@westonruter proposed the ​separate function, in the context of the Gutenberg plugin, and #53176 carried it over with a new name.)

Doesn't this seem to be a separate topic for a ticket?

#7 in reply to: ↑ 6 @westonruter
6 months ago

Replying to SirLouen:

Doesn't make sense to me, deprecating such nouveau function wp_print_auto_sizes_contain_css_fix. I understand this the backwards compatibility policy, but we can be pretty confident noone is using this (and if they were using it, they might have reported this). ​So just wipe it.

Unfortunately, WPdirectory is not up to date. As can be seen from ​searching for GUTENBERG_VERSION, the last WPdirectory was updated was around the Gutenberg v18.2.0 release, which was around April 24, 2024. The function in question here was introduced in WP v6.7.1 which was released afterward on November 21, 2024. Therefore, it is entirely expected that wp_print_auto_sizes_contain_css_fix() would not be found by searching WPdirectory.

It's safer, although noisier, to ​search GitHub. For complete backwards-compatibility, we'd leave the action on wp_head and in the enqueueing function check to see if has_action( 'wp_head', 'wp_print_auto_sizes_contain_css_fix' ) and only proceed with enqueueing the style if that is the case. This is what we did with the emoji styles.

Replying to sabernhardt:

If the title should be earlier, block themes probably could add 'title-tag' support now instead of using the _block_template_render_title_tag function. (@westonruter proposed the ​separate function, in the context of the Gutenberg plugin, and #53176 carried it over with a new name.)

Doesn't this seem to be a separate topic for a ticket?

It's an alternative approach, to change the placement of the TITLE rather than the placement of the STYLE.

#8 in reply to: ↑ 6 @sabernhardt
6 months ago

If the title should be earlier...

Now that the ticket is in the Media component, with a patch related to the media styles, that alternative approach would fit better in a separate ticket if it is a good idea.

#9 in reply to: ↑ 5 @joemcgill
6 months ago

Replying to westonruter:

@joemcgill @flixos90 Any specific reason for why the style was manually printed at wp_head?

From my memory, there is no technical reason why that CSS rule needs to be output as early or directly as it is currently. It was mainly done that way as the most direct fix in 6.7.1 to address #62413. I would welcome a more robust enqueueing solution now as long as it doesn't result in a blocking request (which is not a concern with what is being proposed).

I don't know that there is much value in changing the function name, which then requires the deprecation, but am not opposed if there is a good reason for that change as well.

The main back-compat issue I would be concerned about is if someone has forcibly removed this CSS by doing something like:

remove_action( 'wp_head', 'wp_print_auto_sizes_contain_css_fix', 1 );

I would strongly recommend against anyone using that strategy, since it could result in auto sized images being distorted (disabling the auto-sizes feature entirely would be better, using the wp_img_tag_add_auto_sizes filter), but it is possible that someone is doing this and the change proposed in ​PR#8954 would cause that CSS to be added again.

#10 follow-up: @westonruter
6 months ago

@joemcgill Thanks for that feedback. You're right that the existing function can be used to enqueue the inline style rather than to print it. The name is a bit misleading, but that's par for the core.

I've ​updated the ​PR to eliminate the function deprecation.

I think it makes sense to still try to print the CSS early to preserve the CSS cascade. If it were printed later, then it could unintentionally override another style rule that is setting the contain-intrinsic-size style.

Last edited 6 months ago by westonruter (previous) (diff)

#11 @joemcgill
6 months ago

I think it makes sense to still try to print the CSS early to preserve the CSS cascade.

Agreed. This CSS is already an override for UA styles shipped with chromium browsers, but it should also be able to be modified by any extenders who have registered their own styles to override this rule.

#12 in reply to: ↑ 10 ; follow-up: @SirLouen
6 months ago

  • Keywords changes-requested added; needs-testing removed

Replying to westonruter:

I've ​updated the ​PR to eliminate the function deprecation.

I can't see this removal, in which commit have you pushed it?

#13 in reply to: ↑ 12 ; follow-up: @westonruter
6 months ago

Replying to SirLouen:

Replying to westonruter:

I've ​updated the ​PR to eliminate the function deprecation.

I can't see this removal, in which commit have you pushed it?

I ​reverted ​it based on ​feedback from @flixos90.

Last edited 6 months ago by westonruter (previous) (diff)

#14 in reply to: ↑ 13 @SirLouen
6 months ago

Replying to westonruter:

I ​reverted ​it based on ​feedback from @flixos90.

I'm not 100% convinced, but I understand the points. But I wonder: instead of being so exaggeratedly specific about what the function is doing, I wonder why not use a simplified from minute 0 like wp_img_auto_sizes_fix. The print and the enqueue are just a supporting part of this fact.

It would have saved all this fuzz. I know that flixos is trying to find a bright side to this deprecation, but all I see is that given the BC troubles that always WP carry in the backpack, we should learn that the more specificity, the more risk of these things happening. If this was a unit test that doesn't have BC risks most of the time, you can do a function summarizing the function with two dozen words (funny thing because then most of the functions have no docs, which is paradoxical).

But for core, doing this appears to be problematic.

The original name has been sealed. Ok: now you have a second opportunity to fix this and simplify the naming.

This ticket was mentioned in ​Slack in #core by wildworks. ​View the logs.
2 months ago

#16 @wildworks
2 months ago

This ticket was featured on today's 6.9 Bug Scrub. What would it take to move this ticket forward?

​@westonruter commented on ​PR #8954:
2 months ago #17

I just merged in the latest from trunk. Note there is now a sourceURL comment present:

<style id='wp-img-auto-sizes-contain-inline-css'>
img:is([sizes="auto" i], [sizes^="auto," i]) { contain-intrinsic-size: 3000px 1500px }
/*# sourceURL=wp-img-auto-sizes-contain-inline-css */
</style>

#18 @westonruter
2 months ago

  • Keywords dev-feedback added; changes-requested removed

I've refreshed the patch by merging in the latest from trunk.

I believe the current approach addresses the concern by @flixos90 by deprecating the existing wp_print_auto_sizes_contain_css_fix() function and introducing a new wp_enqueue_img_auto_sizes_contain_css_fix(). Both the deprecated function and the new function both retain the same wp_img_tag_add_auto_sizes filter for preventing the style from being output.

#19 @westonruter
2 months ago

  • Owner set to westonruter
  • Status changed from new to accepted

​@westonruter commented on ​PR #8954:
2 months ago #20

In 67d527c I've addressed the concerns with backwards-compatibility with themes/plugins that have intended to prevent this style from being printed by unhooking the action via:

remove_action( 'wp_head', 'wp_print_auto_sizes_contain_css_fix', 1 );

In looking at this further, I realize I was mistaken in thinking that the wp_img_tag_add_auto_sizes was how this style was actually supposed to be omitted. In reality, this filter does much more, as it prevents sizes=auto from being added to all images as well. Therefore, I was wrong to suggest the proper way to simply omit this style was to do add_filter( 'wp_img_tag_add_auto_sizes', '__return_false' ).

In order to maintain backwards-compatibility, the now-deprecated wp_print_auto_sizes_contain_css_fix() function now remains hooked at wp_head with a priority 1, and now the wp_enqueue_img_auto_sizes_contain_css_fix() function is hooked at wp_head with priority 0, just before wp_print_auto_sizes_contain_css_fix() is slated to run. The first thing that wp_print_auto_sizes_contain_css_fix() does is it checks to see if has_action( 'wp_head', 'wp_print_auto_sizes_contain_css_fix' ). If not, then it short-circuits. Otherwise, it goes ahead and removes the action to prevent the deprecation notice, while also proceeding with enqueueing the CSS fix.

I've added tests for these various scenarios.

#21 @westonruter
2 months ago

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

In 60910:

Media: Switch to enqueueing contain-intrinsic-size CSS fix for IMG tags having sizes=auto.

This deprecates the wp_print_auto_sizes_contain_css_fix() function running at wp_head priority 1, in favor of a wp_enqueue_img_auto_sizes_contain_css_fix() function which runs just before at wp_head priority 0. The latter function unhooks the former while also enqueueing an inline style to be printed with all other styles but up front to preserve the cascade. This eliminates directly printing the STYLE tag, which was a change done similarly before for the emoji styles. See #58775.

For backwards compatibility, the CSS can still be prevented from being enqueued/printed via:

remove_action( 'wp_head', 'wp_print_auto_sizes_contain_css_fix', 1 );

This change ensures that all styles are printed together using the correct API for emitting styles.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/8954.

Follow-up to [59435].

Props westonruter, sabernhardt, SirLouen, flixos90, joemcgill, SergeyBiryukov, superpoincare.
See #62413.
Fixes #62731.

#22 @westonruter
2 months ago

In 60911:

Media: Add test case for removing "img_auto_sizes_contain_css_fix" by dequeueing handle at wp_enqueue_scripts.

From 6.9 and beyond, dequeueing the wp-img-auto-sizes-contain style handle would be preferable to removing the action at wp_head.

Follow-up to [60910], [59435].

See #62731, #62413.

Note: See TracTickets for help on using tickets.