Render blocking CSS `classic-themes.css` unnecessarily enqueued

[adamsilverstein]

Since WordPress 6.1, a new CSS file ​classic-themes.css is now enqueued on all pages on the front end of non-block themes.

This stylesheet seems to only include styles related to the wp-block-button__link class and the stylesheet is added regardless of whether a block button is used on the page. Although the file itself is tiny, the cumulative impact of every WordPress site using a non-block theme is huge and no explanation is given in the doc block as to why the style is needed.

This change was originally added in ​https://github.com/WordPress/gutenberg/pull/44731 and ported over to core. Even in ​this comment though, the front end view already looks fine, so it is unclear why the style needs to be added manually here. The original Gutenberg patch only enqueued the styles in the editor, so it is unclear why styles were added to the front end in
this commit: ​https://github.com/WordPress/WordPress/commit/bbb40de012670768a525193639c0d5e4ea932df7.

In my testing, removing the style had no front end impact, the button looked exactly the same before and after I removed the stylesheet. The button is already styled from blocks.css.

This stylesheet adds an unnecessary render blocking stylesheet with a huge cumulative impact on WordPress sites. I suggest we remove it.

Worth noting that this enqueue was added during RC, probably something we avoid doing if we hope to avoid performance regressions. Also points to the need for better performance metrics, even something as simple as catching new front end enqueues could be useful here.

[adamsilverstein]

Monosnap New classic theme test | wpdev build 2022-11-03 16-03-50.jpg​ shows a front end view without the stylesheet enqueued.

Trac ticket:

[adamsilverstein]

cc: @bernhard-reiter for any additional insight into this stylesheet

[desrosj]

@mamaduka Do you have any insight into this on the editor side and whether we could address this in 6.1.1?

[Mamaduka]

@adamsilverstein, I can reproduce the issue using the Twenty Ten theme. See the screenshots for comparison.

Cc @scruffian

[JeffPaul]

As discussed in ​today's 6.1.1 bug scrub, it is not clear why the styles are always loaded and not included more selectively. As such, this is going to be punted to Future Release until the root cause is clear and a patch/PR can be readied to re-milestone this into a numbered release.

[adamsilverstein]

@Mamaduka can you help clear up why the required styles aren't included directly - only when the block is used? Currently in core this CSS is enqueued unconditionally for all pages, even if the button block isn't being used.

[adamsilverstein]

56990.2.diff​ takes a different approach, only enqueuing the file when a button is present in the content. In my testing, pages with buttons look good in the twenty-ten theme, and the classic-themes CSS is only enqueued when there is a button.

@Mamaduka does this look good or can you propose a better way to ensure the CSS is only enqueued on pages that actually contain a button?

Also: should we inline this CSS directly instead of loading it from an external file?

[adamsilverstein]

Since this isn't quite ready for 6.1.1, milestoning for 6.1.2. It would be good to better understand why this is needed, but in the meantime this prevents the styles from being added everywhere.

[scruffian]

Thanks for looking at this. There are a few things to consider:

1. Does has_block( 'button' ) work for buttons in templates as well as in post content?
2. There is no need for a totally different file here. Originally this CSS was added to the blocks css that Gutenberg generates, but we had trouble with the unit tests, which is why we went this route (see ​https://github.com/WordPress/wordpress-develop/pull/3481).

I'm sure there's a lot we can do to improve this. Thanks to everyone for their help.

[adamsilverstein]

Hey @scruffian - thanks for the feedback!

Does has_block( 'button' ) work for buttons in templates as well as in post content?

No idea (sounds like no?), but would a button in a template even be possible in a non-block theme (which is the context we are discussing here)?

There is no need for a totally different file here. Originally this CSS was added to the blocks css that Gutenberg generates, but we had trouble with the unit tests, which is why we went this route (see ​https://github.com/WordPress/wordpress-develop/pull/3481).

Thank you so much for providing this link, I tried to figure out why this CSS file was included in the first place and couldn't figure out why. Even reading that though, I don't quite understand why including the CSS in "the block CSS that Gutenberg Generates" - which feels like the best solution - would cause the unit tests to fail. Do you have know why they failed?

I'm going to review the commit history on that PR to see if I can move the styles into the block in core - or is this something we need to do in Gutenberg?

[adamsilverstein]

Based on the thread, it looks like this stylesheet was added only to address an issue with displaying the buttons in the twentyten theme.

In 56990.3.diff​ I moved the styles directly into twentyten and removed them from default_scripts.php. I tested all core themes back to twentyten and my post with buttons looked fine.

Appreciate feedback on this proposed change @scruffian & @Mamaduka.

[scruffian]

Thanks @adamsilverstein, this won't work. There are many other themes out there that rely on these fallback styles, so we need to provide them to all themes in case they haven't got their own styles.

[sabernhardt]

Some themes we do not control probably need these default styles. Otherwise, Twenty Sixteen would need to update the font-size property in blocks.css in addition to the Twenty Ten fixes.

The has_block function checks the post content, but blocks can be in widget areas for 'classic' themes.

If the test problems cannot be resolved, could this be an inline script added to the wp-block-library stylesheet handle?

[scruffian]

These style originally existed in the button block itself. They were moved to the block.json of the button block, which meant that they were no longer output for classic themes, hence the need for this additional CSS. I think it needs to be added to all classic themes when a button block is present in either the content or elsewhere on the page. I don't have any opinion about how the CSS is added.

[Mamaduka]

@scruffian, can we move the style definitions to the core's theme.json file? I think we can consider these button styles as the default preset for themes.

Cc @oandregal

[scruffian]

@Mamaduka they are already there for elements. The problem is that we don't load style from the core theme.json for classic themes, hence the need for this in the first place.

[Mamaduka]

@scruffian, the core data or presets are available for classic and block themes. I just tested this with the Twenty Ten theme and added the following code to the wp-includes/theme.json style config.

"blocks": {
	"core/button": {
		"color": {
			"text": "#ffffff",
			"background": "#9b51e0"
		},
		"spacing": {
			"padding": "calc(0.667em + 2px) calc(1.333em + 2px)"
		}
	}
},

[scruffian]

What happens if you add it to elements?

I would rather not add it to the block definition because that would mean it would always override the element styles, so then themes would always have to declare the styles twice - once for elements and once for blocks.

[Mamaduka]

It looks like button element styles are already defined, but it doesn't seem to generate any preset. At least in my tests. See ​https://github.com/WordPress/wordpress-develop/blob/6.1/src/wp-includes/theme.json#L391-L409

[scruffian]

@Mamaduka that's right. We did have some attempts at getting the core theme.json to output CSS for elements but they all ended up causing other problems because we want to limit the uses of theme.json in classic themes. That's why we ended up with this solution!

[flixos90]

@adamsilverstein @Mamaduka @scruffian Chiming in here after a while: It seems those styles are indeed needed, that makes sense. However, the performance concern here is really the additional request.

Those few lines of CSS do not justify loading a separate CSS file, they should rather be printed in an inline <style> tag IMO. Using an inline style tag would pretty much alleviate the performance concerns, as the CSS itself is extremely little to parse. WDYT?

If you agree, it should be fairly straightforward to do that in a PR.

[scruffian]

Absolutely. The original intention was to add this to another CSS file that is built by Gutenberg. This is how I tried to do it at first with ​https://github.com/WordPress/wordpress-develop/pull/3481, but we were having problems loading the built CSS file in core, and we were running short of time, so we went for the separate CSS file as a quick solution for the release to avoid breaking everyone's buttons. If we can just change it to an inline style that would be fine IMO.

Related there's also this performance enhancement: ​https://github.com/WordPress/gutenberg/pull/45063

[wpgurudev]

Add inline style on page

[wpgurudev]

I have tested with TwentyTen theme with (attached are the links to WebPageTests)

1.​External CSS File

2.​Inline CSS

1. It has been observed that DomContentLoaded event is occurring earlier when external script is used.

2. We can see from the network tab screenshot, ​External stylesheet and ​Inline CSS that DomContentLoaded is occurring earlier with external stylesheet.

3. When we add the inline style to the page, the overall size of the page increases from 6.1 KB to 6.4 KB

4. However; in the external request, size is only 391 Bytes.

From the above results it looks like the external file is producing better results compared to inline style.

[adamsilverstein]

These style originally existed in the button block itself. They were moved to the block.json of the button block, which meant that they were no longer output for classic themes, hence the need for this additional CSS. I think it needs to be added to all classic themes when a button block is present in either the content or elsewhere on the page. I don't have any opinion about how the CSS is added.

@scruffian Adding the styles "to all classic themes when a button block is present" would be fine - the problem I raised here is that the CSS is added even when there are no buttons used on the page.

These style originally existed in the button block itself. They were moved to the block.json of the button block, which meant that they were no longer output for classic themes, hence the need for this additional CSS

Do you have any more details about why this change was made (or a link to the issue)? In my mind it would make more sense for the styles for the buttons to live in the button block so they are only present when a button is used (I guess I'm missing something though).

I have tested with TwentyTen theme with (attached are the links to WebPageTests)

Thanks for testing the performance impact @wpgurudev. I'm guessing the effect is very minor, my main concern here is this is now loaded on all classic themes, even when not required (no button block used). So although the per site impact is tiny, the cumulative impact is huge.

It seems those styles are indeed needed, that makes sense. However, the performance concern here is really the additional request.

These styles are only needed if you use the button block, yet the styles are added unconditionally to all classic themes. The concern I have is about the additional (unnecessary) CSS added to all sites multiplied by the number of sites that use classic themes and have no buttons (ie. they don't need it).

[scruffian]

@scruffian Adding the styles "to all classic themes when a button block is present" would be fine - the problem I raised here is that the CSS is added even when there are no buttons used on the page.

Absolutely, that would be a great improvement.

Do you have any more details about why this change was made (or a link to the issue)? In my mind it would make more sense for the styles for the buttons to live in the button block so they are only present when a button is used (I guess I'm missing something though).

This is the original issue: ​https://github.com/WordPress/gutenberg/pull/40260

The CSS for block themes does need to be output more generally because it's not just used for button blocks, but all buttons that contain buttons. However that's not the case for classic themes, so we should be able to load the CSS conditionally based on the presence of a button block, as you suggested above.

[flixos90]

Moving this to 6.2 for now since there is still no plans for a 6.1.2, and we should find a solution for this problem soon to get it into 6.2 (can wait until after Beta 1 though).

[adamsilverstein]

The CSS for block themes does need to be output more generally because it's not just used for button blocks, but all buttons that contain buttons.

I think you mean "all pages that contain buttons", right?

I guess block themes can do this because they "pre" parse the content; classic themes don't do this though so I wonder how or if we could introduce the conditional logic there.

This is a simpler solution to the overhead of enqueuing the classic-themes.css file on every page load (in classic themes).

• It keeps using the enqueue logic, which makes this backward compatible (e.g. allows other stylesheets to depend on it and keeps the styles in the exact same place in the dependency chain), therefore making this a safer change than https://core.trac.wordpress.org/attachment/ticket/56990/56990-inline-style.diff.
• It limits the changes to simply avoiding the external request which is the main caveat from the Trac ticket. The CSS itself is so little that it adds little overhead, it's just the external file request itself that is problematic for performance. #3561 doesn't address this adequately as it simply removes the styles and adds them to the Twenty Ten theme, which isn't a proper fix.
• Note that this PR also indirectly fixes the $version parameter used before, which was incorrectly set to true (not a valid value for the parameter). This caused the ?ver= query arg to be always set to 1. With the inline style, this is no longer relevant anyway, but just pointing out that it was incorrect before.

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

[flixos90]

@scruffian @adamsilverstein @wpgurudev I have opened ​https://github.com/WordPress/wordpress-develop/pull/4012, which I believe is a simpler fix to the most prevalent performance issue here, and it is a backward compatible change, which I think makes this a better solution than the previous patches. See the PR description for more context. Can you please take a look?

I acknowledge that the perfect outcome here would be to not even load this CSS if no buttons are being used on the page, but as you are already saying @adamsilverstein, this is challenging to reliable determine in a classic theme. Since the amount of CSS here is tiny though, I don't really see that itself as a performance concern. The main problem is that the CSS is loaded via an external file, which adds unnecessary overhead that we can avoid by inlining it.

[wpgurudev]

@flixos90 I have conducted the performance impact analysis when we inline css vs when we load via external file. Added the results in this comment. According to this, there's more adverse effect on performance when we add inline style.

We may need to conduct some more tests to analyse whether we are really benefiting in terms of performance by adding inline style.

[scruffian]

I spent some more time thinking about this. Here are the options as I see them:

1. We use an external file and only load it for themes that need it. This has the advantage of being cached, so although it will require extra bytes on the first load, subsequent loads will be much faster.
2. We inline the styles. This might result in a performance improvement the first time because it eliminates a network request, but these rules will need to be loaded on every subsequent load, so I believe the overall impact will be greater.

I put together a PR with a different approach, but I don't think it's any better:
​https://github.com/WordPress/wordpress-develop/pull/4022

[patrickmeenan]

As far as I can tell, from the WebPageTest linked tests, inlining the CSS increased the HTML size from 5.7K to 5.9K (~200 bytes) and the external file was ~200 bytes (both compressed sizes).

Inlining it will cause it to increase the size of every HTML page and may have a bit more overhead on the serving side but I'd expect the actual performance impact in either case to be pretty much equivalent (probably skewing towards inlined being slightly faster).

As an external resource, there is some non-zero amount of overhead in getting the resource out of cache that is likely much higher than the transfer time of the additional 200 bytes in the existing stream. An external resource is also more likely to cause delays from scheduling contention with other requests depending on how the origin supports prioritization.

My recommendation would be to do what makes the most sense for the platform in general and not try to base it on a performance difference.

If there is not an existing practice of inlining styles then starting to do it just for this is probably a bad precedent. If the pattern you'd like to encourage is to conditionally add styles to the main styles css then it's worth the effort to figure out why that was breaking tests.

If inlining small styles either in the head or at the first point of use is a pattern you want to encourage then it makes sense to do it here.

[flixos90]

Thanks @patrickmeenan for the additional feedback! There isn't really a precedent here, but one thing we cannot do is incorporate those styles in the main stylesheet, as it is externally controlled by whichever WordPress theme is currently active. So it needs to be separate unfortunately.

@wpgurudev @scruffian Based on the above feedback, while the performance difference is negligible, I'd still lean towards inlining it for slightly more benefit. The performance test from comment:29 doesn't show a clear benefit in either direction, as we have to consider the variance between test runs is considerable.

That being said, it's also safe to say it's not a high priority issue to address in terms of performance.

[scruffian]

Personally I'm happy with either approach.

[wildworks]

As with the button block, there is a problem with the loss of backward-compatibility style in the file block buttons as well. Therefore, I've submitted a ticket to add some styles to classic-theme.css.

https://core.trac.wordpress.org/ticket/57688

[flixos90]

I have conducted another performance benchmark for this locally, using ​this CLI script to measure FCP, LCP, and TTFB. Running locally is better for benchmarks than using WebPageTest (or generally requests to external sites) due to the higher variance in those scenarios.

I ran 20 requests each to WP 6.2 Beta 2 with vs without the change from ​https://github.com/WordPress/wordpress-develop/pull/4012 applied.

The results were inconclusive, so then I thought it may be useful to add another metric "LCP - TTFB", essentially compute that for every request and get the median (which, just to clarify, is not the same as subtracting median TTFB from median LCP!). This metric is more effective to assess the performance impact here, since the server response time (TTFB) should be ignored for this change, as what we are changing affects only client-side performance, if anything.

The additional metric did nothing but confirm that there is really no clear benefit from either approach. Sometimes the inline style was faster, other times the external file. See the data for "LCP - TTFB" below.

1. Comparison (20 requests each): 133ms (inline style) vs 137.5ms (external stylesheet)
2. Comparison (20 requests each): 134.65ms (inline style) vs 130.65ms (external stylesheet)

This additional benchmark confirms what was mentioned above: It doesn't really make a difference whether we have an inline style or external stylesheet for this. For that reason, what remains here is a more appropriate fix, which would be to somehow only load this stylesheet when at least one of those blocks is used on the page in a classic theme. This is a whole lot more complex to figure out though (if even reasonably possible), so I'm removing this from the 6.2 milestone as such a change would have the chance of introducing other problems, which we are too late for now.

Closing this as it's not worth moving forward, see https://core.trac.wordpress.org/ticket/56990#comment:42.

Closing this as it is not a feasible solution to the problem; we need to find a solution in core that somehow only enqueues this file if any of the relevant blocks is present on the page. Also see https://core.trac.wordpress.org/ticket/56990#comment:42.

[flixos90]

In 55417:

Editor: Fix invalid parameters being passed to wp_register_style().

Boolean true is not a valid value for the $ver or $media parameters of wp_register_style().

Props sabernhardt, flixos90, hellofromtonya.
Fixes #57771. See #56990, #57688.

[spacedmonkey]

I wonder why this style is not covered into an inline style with wp_maybe_inline_styles

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

[spacedmonkey]

@adamsilverstein @flixos90 I have a super simple PR at ​#4511. This add the path data to the style registered. Means that this style is read by wp_maybe_inline_styles and inlines the style. See screenshot.

[flixos90]

@spacedmonkey I like your suggestion in the PR ​https://github.com/WordPress/wordpress-develop/pull/4511, however I think that PR should be for a new ticket. It doesn't address the main problem from this ticket, which is that the styles are enqueued regardless of whether they are needed for the markup on the page or not.

Could you open a separate ticket for your PR? I think it's worth including in 6.3.

[spacedmonkey]

The issue here is that the css makes a blocking request. In-line the css, fixes that problem. The css, is very small. I understand the desire to not want to render the css at all. But I would see this as a breaking change. You never know how someone is using this css.

My fix is simple and uses exist functionity. It also side steps the backwards compatibility conversation.

[adamsilverstein]

Could you open a separate ticket for your PR? I think it's worth including in 6.3.

I opened https://core.trac.wordpress.org/ticket/58480 as a follow up to inline the styles, making them non blocking.

Regarding the performance benefit, the biggest difference will be when network conditions are poor and the browser has to wait for the external http request to complete.

[adamsilverstein]

As an external resource, there is some non-zero amount of overhead in getting the resource out of cache that is likely much higher than the transfer time of the additional 200 bytes in the existing stream. An external resource is also more likely to cause delays from scheduling contention with other requests depending on how the origin supports prioritization.

right, that makes sense @patrickmeenan - inlining will provide a negligible boost usually, but in some cases the impact could be more significant. There is also the resource impact of an additional request to the server to consider. It is only one request, however it is one request for every page load for every (non-block theme ie. 99% of) WordPress site.

If there is not an existing practice of inlining styles then starting to do it just for this is probably a bad precedent.

We are in fact inlining block styles now and this are block styles, so inlining follows current proactive.

Committed.

[flixos90]

While a fix for the related #58480 was committed, no progress has been made on the root cause this ticket is for, so I'll punt.