Make WordPress Core

Opened 10 months ago

Closed 7 months ago

Last modified 3 months ago

#58775 closed enhancement (fixed)

Remove usage of outputting styles tags

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.4 Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: 2nd-opinion has-patch needs-dev-note
Focuses: Cc:

Description (last modified by hellofromTonya)

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.

Attachments (1)

Screenshot 2023-09-26 at 09.03.09.png (478.6 KB) - added by spacedmonkey 7 months ago.

Download all attachments as: .zip

Change History (48)

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


10 months ago
#1

  • Keywords has-patch added

#2 @joemcgill
10 months ago

In 56213:

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

Props spacedmonkey, swissspidy.
See #58775.

#3 @spacedmonkey
9 months ago

  • Milestone changed from Awaiting Review to 6.4

#4 @huubl
8 months ago

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.

This ticket was mentioned in Slack in #core-performance by spacedmonkey. View the logs.


8 months ago

#6 follow-up: @spacedmonkey
8 months ago

@huubl I believe this is handled in #58664.

#7 @spacedmonkey
8 months ago

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

Assigned to @westonruter for review.

#8 @westonruter
8 months ago

  • Owner changed from westonruter to spacedmonkey

#9 in reply to: ↑ 6 @hlunter
8 months ago

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!

@spacedmonkey commented on PR #4824:


7 months ago
#10

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

#11 @sabernhardt
7 months ago

#58946 was marked as a duplicate.

@spacedmonkey commented on PR #4824:


7 months ago
#12

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

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/237508/dea1f8f7-1efd-461f-b1f7-91f500ebd240

Am I missing a setting?

#13 @spacedmonkey
7 months ago

  • Keywords needs-dev-note added

@westonruter commented on PR #4824:


7 months ago
#14

@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

@spacedmonkey commented on PR #4824:


7 months ago
#15

@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.
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/237508/5881f09d-e285-4ba8-9050-9d0a8104e03d

#16 follow-up: @azaozz
7 months ago

  • Keywords close 2nd-opinion added; needs-dev-note removed

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.
  1. 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.
Last edited 7 months ago by azaozz (previous) (diff)

@spacedmonkey commented on PR #4824:


7 months ago
#17

Awaiting @westonruter review here and I will commit.

@azaozz commented on PR #4824:


7 months ago
#18

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.

@flixos90 commented on PR #4824:


7 months ago
#19

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

#20 in reply to: ↑ 16 @spacedmonkey
7 months ago

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

#21 @spacedmonkey
7 months ago

  • Focuses performance added

This ticket was mentioned in Slack in #core-performance by flixos90. View the logs.


7 months ago

#26 @spacedmonkey
7 months ago

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

#27 follow-up: @azaozz
7 months ago

  • Keywords revert added; has-patch close removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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

Last edited 7 months ago by azaozz (previous) (diff)

#28 in reply to: ↑ 24 @azaozz
7 months ago

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.
  1. 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?

Last edited 7 months ago by azaozz (previous) (diff)

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


7 months ago

#30 in reply to: ↑ 27 @flixos90
7 months ago

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.

#31 @hellofromTonya
7 months ago

  • Description modified (diff)

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

@andrewserong commented on PR #4824:


7 months ago
#32

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
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/14988353/fca6e855-9c6f-437b-a4a1-03efac81f8b4 https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/14988353/cbdc866a-9a96-4ffc-b609-5184a8f95cc1

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:

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/14988353/7c54c9f9-496c-4205-a2ae-a3e0f610d19c

#33 @spacedmonkey
7 months ago

  • Focuses performance removed

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.

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


7 months ago
#34

  • Keywords has-patch added

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

## Screenshot
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/240569/f21ae7b6-de72-4e03-bb0e-1f1f6df7ceac

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


7 months ago
#35

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

## Screenshot
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/240569/f21ae7b6-de72-4e03-bb0e-1f1f6df7ceac

#36 @Mamaduka
7 months ago

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.

@Mamaduka commented on PR #5305:


7 months ago
#37

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.

@spacedmonkey commented on PR #5305:


7 months ago
#38

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

@hellofromTonya commented on PR #5305:


7 months ago
#39

Testing it now. Give me a few minutes.

@hellofromTonya commented on PR #5305:


7 months ago
#40

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

#42 @flixos90
7 months ago

  • Keywords revert removed
  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#43 @spacedmonkey
7 months ago

  • Keywords needs-dev-note added

#44 @spacedmonkey
6 months ago

Dev note in progress here.

#45 @westonruter
6 months ago

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.

#46 @jorbin
3 months ago

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.

#47 @jorbin
3 months ago

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.

Note: See TracTickets for help on using tickets.