Remove usage of outputting styles tags

[spacedmonkey]

There are a number of places in core where style tags are manually built and output. Examples

• print_emoji_styles
• the_block_template_skip_link
• print_embed_styles

Instead of manually build tags like this, is not great. As these values are hard to filter and control the output of. These places should all be replaced with wp_add_inline_style or use the WP_Style enqueueing system.

This change does the following.

• Allows plugins to include these styles in css optimize.
• Allows developers to unqueue these styles.
• Makes it easier to maintain the these styles, as all styles are output in the same way.
• Adds constituency to the code.

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

[joemcgill]

In 56213:

Revert package-lock.json changes committed by mistake in [56212].

Props spacedmonkey, swissspidy.
See #58775.

[huubl]

Doesn't this also apply to the inline JavaScript? Like here: ​https://github.com/WordPress/wordpress-develop/blob/7cf05166a6b736b0303080dfb7deeaeeaf8db76f/src/wp-includes/theme-templates.php#L159-L204

Please let me know if I should create a new ticket.

[spacedmonkey]

@huubl I believe this is handled in #58664.

[spacedmonkey]

Assigned to @westonruter for review.

[westonruter]

​Reviewed.

[hlunter]

Replying to spacedmonkey:

@huubl I believe this is handled in #58664.

Great that a ticket has already been opened. I couldn't find it, thanks for sharing the link!

@westonruter I have reverted a lot of the changes and found work arounds for other changes. Can you take a look again?

[sabernhardt]

#58946 was marked as a duplicate.

@westonruter Side note, I am not seeing the syntax highlighting in PHPStorm.

Am I missing a setting?

@westonruter Side note, I am not seeing the syntax highlighting in PHPStorm.
Am I missing a setting?

Humm. I didn't enable anything. It's apparently powered by the bundled IntelliLang plugin: ​https://www.jetbrains.com/help/phpstorm/using-language-injections.html

@westonruter Side note, I am not seeing the syntax highlighting in PHPStorm.
Am I missing a setting?

Humm. I didn't enable anything. It's apparently powered by the bundled IntelliLang plugin: ​https://www.jetbrains.com/help/phpstorm/using-language-injections.html

Seems like it is PHPStorms new UI. It works in the old UI.

[azaozz]

is not great. As these values are hard to filter and control the output of.

That's not exactly true. Seems most of these are done from hooks and plugins can replace them very easily.

These places should all be replaced with wp_add_inline_style or use the WP_Style enqueueing system.

Why? I see two disadvantages to doing this:

1. It is slower and uses more server resources. Maybe not by much, but using Script Loader will make all of these slower.

2. Seems all of these were intended to work as they do now. What are the reasons to change this (other than "lets use the new stuff!"). Unless there are some good reasons and examples why these should be changed, this can be considered a "pointless refactor for the sake of refactoring". As such it generally would go against the WP best practices.

Awaiting @westonruter review here and I will commit.

Awaiting @westonruter review here and I will commit.

Please do not commit this unless it is proven useful. As it stands now this partial refactoring doesn't seem like a good idea. See: https://core.trac.wordpress.org/ticket/58775#comment:16. Also there aren't any use cases for it. Why would that be in core?

Also please keep in mind that CSS does work quite differently than JS and PHP. The "Cascading" part in the name means styles are "cascaded", i.e. depend on the underlying styles. Because of this removing or tweaking the default WP styles is generally a bad idea and can be considered backwards compatibility breach. However enabling the removal or tweaking of core's CSS seems to be the only benefit of this partial refactoring. So thinking that this should not be added to core.

@azaozz

However enabling the removal or tweaking of core's CSS seems to be the only benefit of this partial refactoring. So thinking that this should not be added to core.

That is not a benefit of this PR. You can already do that today by unhooking the functions that print it.

The "Cascading" part in the name means styles are "cascaded", i.e. depend on the underlying styles.

This is in fact a good reason for this change: The PR ensures those core styles make use of the WP_Styles API and thus can be properly depended upon, which isn't possible as of today.

[spacedmonkey]

Replying to azaozz:

is not great. As these values are hard to filter and control the output of.

That's not exactly true. Seems most of these are done from hooks and plugins can replace them very easily.

These places should all be replaced with wp_add_inline_style or use the WP_Style enqueueing system.

Why? I see two disadvantages to doing this:

1. It is slower and uses more server resources. Maybe not by much, but using Script Loader will make all of these slower.

2. Seems all of these were intended to work as they do now. What are the reasons to change this (other than "lets use the new stuff!"). Unless there are some good reasons and examples why these should be changed, this can be considered a "pointless refactor for the sake of refactoring". As such it generally would go against the WP best practices.

This issue was created because plugins like ​autoptimize, are used to optimize css rendered on the page. This does so by concatenating all enqueued css into one blob. As this css is not enqueued correctly, then this is not possible.

There is nothing wrong with using WordPress it's own API to output CSS. The only reason that it was not used before in many cases, as this inline styles did not exist.

This change does the following.

1. Allows plugins to include these styles in css optimize.
2. Allows developers to unqueue these styles.
3. Makes it easier to maintain the these styles, as all styles are output in the same way.
4. Addes constituency to the code.

This is not a refactor for the sake of refactor.

@felixarntz IDE type hinting was removed in ​https://github.com/WordPress/wordpress-develop/pull/4824/commits/5b8bc3c9d78e37a8124115b6c343e0593cfe00ad

Committed ​https://github.com/WordPress/wordpress-develop/commit/54c4de13edfaf3b9fbc1b67d6ba384618614d59e

[spacedmonkey]

Committed in ​https://github.com/WordPress/wordpress-develop/commit/54c4de13edfaf3b9fbc1b67d6ba384618614d59e

[azaozz]

@flixos90:

That is not a benefit of this PR. You can already do that today by unhooking the functions that print it.

Yes, for some of these. So if this is not the benefit, then what is the benefit of this refactoring?

The PR ensures those core styles make use of the WP_Styles API and thus can be properly depended upon, which isn't possible as of today.

Sorry but I don't understand what you are saying? This is simply not true! Using the WP_Styles API does not improve handling of these styles in the slightest. Please add examples!

At this time it seems that this change is a "pointless refactoring". There are no fixes or improvements as a result of it. Please see the WordPress philosophy in regards to doing pointless refactoring. On top of that it could bring new bugs or regressions.

In these terms I think it should be reverted until a proof that it is actually beneficial for something.

[azaozz]

Replying to spacedmonkey:

Committed in ​https://github.com/WordPress/wordpress-develop/commit/54c4de13edfaf3b9fbc1b67d6ba384618614d59e

The commit is [56682]. Full text of the commit message:

In this commit, enhancements have been made by replacing manually 
constructed style tags with calls to wp_add_inline_style. 
Previously, numerous style tags were generated and output directly 
in the header, resulting in redundant code and bypassing the 
core's style enqueueing system. This approach made it challenging 
for third-party developers to manage and control the output of 
these style tags.

To ensure backward compatibility, the following functions have been 
deprecated and replaced:

    print_embed_styles
    print_emoji_styles
    wp_admin_bar_header
    _admin_bar_bump_cb 

Backward compatibility shims have also been added, ensuring that 
if these functions were previously unhooked from there actions, 
they will continue to not output a style tag.

However, for the following functions, conversion to use inline 
styles was not feasible due to the potential disruption it 
might cause by changing the style tag IDs, potentially breaking 
JavaScript functionality for a number of plugins in the repository:

    custom-background
    wp-custom 

These changes improve code maintainability and enhance the flexibility 
and control available to developers when managing style outputs 
within WordPress core.

Props spacedmonkey, hlunter, westonruter, flixos90.
Fixes #58775.

@spacedmonkey In the future could you please use Trac references? Makes it a lot easier to follow.

The part of the commit message that doesn't match the previous comments is:

...bypassing the core's style enqueueing system. This approach made it challenging for third-party developers to manage and control the output of these style tags.

Couple things about that:

1. This is not true. As pointed numerous times it is a bad idea for extenders to remove default WP styles. So "manage and control" is actually "doing-it-wrong" here.

2. In a previous comment @flixos90 insisted that allowing extenders to "manage and control" these styles (by replacing or removing them from PHP) is not the purpose of this change.

So, which is true and which is not? And at the end of the day, what is the purpose of this anyway?

[flixos90]

Replying to azaozz:

The PR ensures those core styles make use of the WP_Styles API and thus can be properly depended upon, which isn't possible as of today.

Sorry but I don't understand what you are saying? This is simply not true! Using the WP_Styles API does not improve handling of these styles in the slightest. Please add examples!

No need to use exclamation points. Per the discussion in ​https://wordpress.slack.com/archives/C02RQBWTW/p1695675353234609, there are primarily two benefits of this change:

• It allows plugins (e.g. optimization plugins) to filter the CSS output.
• It allows external CSS to depend on the relevant core CSS, e.g. custom WP Embed styles can now depend on wp-embed-template.

At this time it seems that this change is a "pointless refactoring". There are no fixes or improvements as a result of it. Please see the WordPress philosophy in regards to doing pointless refactoring. On top of that it could bring new bugs or regressions.

In these terms I think it should be reverted until a proof that it is actually beneficial for something.

This is not correct, the improvements were outlined several times here and in the linked Slack conversation. If there is a foundation for actual bugs this is introducing, then it should be revisited, so please present examples if you have any.

[hellofromTonya]

Updating the description to include the details @spacedmonkey shared in #comment:20.

More context about this reasoning for this ticket shared during today's scrub:

Is this a performance ticket? No, as @flixos90 ​noted in today's scrub discussion:

Performance itself should not be changed by this commit, neither to the positive nor negative.

As @joemcgill ​shared, it gives the potential for optimization by performance plugins:

My understanding is that the intent is to allow these styles to be optimized by performance optimization plugins. Currently that's not possible because these are not output using the standard WP Scripts APIs.

As @flixos90 ​shared, it allows other styles dependencies:

The other benefit of the change is that it allows for other styles to depend on those styles. For example, additional WP Embed styles in a plugin could now use something like wp_enqueue_script( $handle, $stylehsheet_file, 'wp-embed-template' ), and it would ensure they are printed in the correct order

As far as I can tell, I think this PR has introduced a regression for editor styles in the block editor.

Before this change	After this change

When looking at the markup for the post editor, there appears to now be a deprecation notice injected into __unstableResolvedAssets within the post editor's initializeEditor initial state:

[spacedmonkey]

I am removing the performance focus here, as that is unclear.

@spacedmonkey In the future could you please use Trac references? Makes it a lot easier to follow.

What do you mean here? What should I have referenced?

Using the WP_Styles API does not improve handling of these styles in the slightest. Please add examples!

See attached screenshot, of how I can now see if the emoji inline style is output or not. I can now use the emoji style as a dependency. You can now, write a plugin to collect all inline styles and minify the css ( something you would have to do on a case by case basis ).

As pointed numerous times it is a bad idea for extenders to remove default WP styles.

Is it a bad idea? Why? I don't think we get to make judgements on what developers do and it is is bad. The emoji styles can be a performance issue and are removed by a number of plugins.

For example print_emoji_styles is unhooked by a number of popular ​plugins, including Yoast SEO, W3 Total Cache, SiteGround Optimizer and WP-Optimize. All of which have over 1 million installs each. Saying no one unhooks these or it is a bad idea, I feel like is incorrect. Lots of people are doing it. Making it easier to do so makes sense.

As for it being slower, the data does not prove this. Here is the commit and the before benchmark data. Between the 1000 runs I did on a 2020 site, there is basically no difference. The 0.18ms can be seen as variant between different runs of the tool.

	[56681]	[56682]
Response Time (median)	104.01	104.18
wp-load-alloptions-query (median)	0.7	0.7
wp-before-template (median)	21.48	21.34
wp-template (median)	76.89	76.68
wp-total (median)	98.9	98.56

One thing I don't understand here, is the Style API is used throughout core. It is good enough to use in all these places, why not here? If the style api is slow and should not be used in this context, then should it be removed everywhere?

At the end of the day, core should be dogfooding it's own APIs, like the style api and encouraging all developers to do the same.

PR removes the recently deprecated print_emoji_styles handler when generating the block editor's iframe assets.

## Why

Deprecation notice triggered by the callback breaks editor styles when WP_DEBUG is set to true.

## Testing Instructions.

1. Enable debugging:

define( 'WP_DEBUG', true );
define( 'WP_DEBUG_LOG', false );
define( 'WP_DEBUG_DISPLAY', true );

2. Open a post or page.
3. Insert a group block.
4. Placeholder styles shouldn't be broken.
5. Run the following script on the DevTools console - wp.data.select( 'core/block-editor' ).getSettings().__unstableResolvedAssets.styles
6. Returned value shouldn't include a deprecation message.

## Screenshot

PR removes the recently deprecated print_emoji_styles handler when generating the block editor's iframe assets.

## Why

Deprecation notice triggered by the callback breaks editor styles when WP_DEBUG is set to true.

## Testing Instructions.

1. Enable debugging:

define( 'WP_DEBUG', true );
define( 'WP_DEBUG_LOG', false );
define( 'WP_DEBUG_DISPLAY', true );

2. Open a post or page.
3. Insert a group block.
4. Placeholder styles shouldn't be broken.
5. Run the following script on the DevTools console - wp.data.select( 'core/block-editor' ).getSettings().__unstableResolvedAssets.styles
6. Returned value shouldn't include a deprecation message.

## Screenshot

[Mamaduka]

Created PR that fixed block editor assets conflict.

It doesn't need to be merged if you decide to revert the print_emoji_styles deprecation.

Note: Fix here uses conditional check before removing and adding the callback. This differs from a fix that landed in the Gutenberg repo this morning. I'll sync changes back if this gets committed.

@hellofromtonya Do you want to review as well. I plan to commit once the unit tests pass.

Testing it now. Give me a few minutes.

@spacedmonkey LGTM for commit. Thank you everyone for resolving this issue!

Committed ​https://github.com/WordPress/wordpress-develop/commit/6a0678659792ede933dbacb3b75c34e93a967a30

[flixos90]

Closing this ticket for now as the follow up bug has been fixed and we're about to release Beta 1.

If there are any new concerns, it can always be reopened as needed.

[spacedmonkey]

Dev note in progress ​here.

[westonruter]

In 56932:

Script Loader: Enqueue inline style for block template skip link in head instead of footer.

• Introduce wp_enqueue_block_template_skip_link() to replace the_block_template_skip_link(). Add to wp_enqueue_scripts action instead of wp_footer.
• Keep inline script for skip link in footer.
• Restore original the_block_template_skip_link() from 6.3 and move to deprecated.php.
• Preserve back-compat for unhooking skip-link by removing the_block_template_skip_link from wp_footer action.

Follow-up to [56682] and [56687].

Props sabernhardt, plugindevs, westonruter, spacedmonkey.
Fixes #59505.
See #58775.
See #58664.

[jorbin]

In 57306:

Embeds: Ensure the deprecated function print_emoji_styles isn't used

Ensure that the proper new function wp_enqueue_emoji_styles is used in embeds.

Follow-up to: [56194].

Props peterwilsoncc, bobbingwide, hellofromTonya.
Fixes #59892. See: #58775.

[jorbin]

In 57347:

Embeds: Ensure the deprecated function print_emoji_styles isn't used

Ensure that the proper new function wp_enqueue_emoji_styles is used in embeds.

Follow-up to: [56194].

Reviewed by davidbaumwald.
Merges [57306] to the 6.4 branch.

Props peterwilsoncc, bobbingwide, hellofromTonya.
Fixes #59892. See: #58775.