Make WordPress Core

Opened 13 months ago

Last modified 5 months ago

#56990 assigned defect (bug)

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

Reported by: adamsilverstein's profile adamsilverstein Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 6.1
Component: Themes Keywords: needs-unit-tests has-patch
Focuses: css, performance Cc:

Description

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.

Attachments (9)

Monosnap New classic theme test | wpdev build 2022-11-03 16-03-50.jpg (557.0 KB) - added by adamsilverstein 13 months ago.
56990.diff (1.9 KB) - added by adamsilverstein 13 months ago.
buttons-classic-css-loaded.png (65.0 KB) - added by Mamaduka 13 months ago.
buttons-classic-css-not-loaded.png (52.6 KB) - added by Mamaduka 13 months ago.
56990.2.diff (690 bytes) - added by adamsilverstein 13 months ago.
56990.3.diff (2.5 KB) - added by adamsilverstein 13 months ago.
56990.4.diff (3.3 KB) - added by adamsilverstein 13 months ago.
56990-inline-style.diff (2.0 KB) - added by wpgurudev 11 months ago.
Add inline style on page
Screenshot 2023-05-26 at 02.23.05.png (42.5 KB) - added by spacedmonkey 6 months ago.

Download all attachments as: .zip

Change History (63)

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


13 months ago
#2

  • Keywords has-patch added; needs-patch removed

Trac ticket:

#3 @adamsilverstein
13 months ago

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

#4 @sabernhardt
13 months ago

  • Component changed from General to Themes

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


13 months ago

#6 @JeffPaul
13 months ago

  • Keywords needs-testing added

#7 @desrosj
13 months ago

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

#8 @Mamaduka
13 months ago

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



Cc @scruffian

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


13 months ago

#10 @JeffPaul
13 months ago

  • Milestone changed from 6.1.1 to Future Release

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.

#11 @adamsilverstein
13 months ago

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

#12 @adamsilverstein
13 months ago

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?

#13 @adamsilverstein
13 months ago

  • Milestone changed from Future Release to 6.1.2

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.

#14 @scruffian
13 months ago

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.

#15 @adamsilverstein
13 months ago

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?

#16 @adamsilverstein
13 months ago

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

Last edited 13 months ago by adamsilverstein (previous) (diff)

#17 @scruffian
13 months ago

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.

#18 @sabernhardt
13 months ago

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?

#19 @scruffian
13 months ago

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.

#20 @Mamaduka
13 months ago

@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

#21 @scruffian
13 months ago

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

#22 @Mamaduka
13 months ago

@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)"
		}
	}
},

#23 @scruffian
13 months ago

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.

#24 @Mamaduka
13 months ago

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

#25 @scruffian
13 months ago

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

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


11 months ago

#27 @flixos90
11 months ago

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

#28 @scruffian
11 months ago

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
11 months ago

Add inline style on page

#29 @wpgurudev
10 months ago

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.
  1. We can see from the network tab screenshot, External stylesheet and Inline CSS that DomContentLoaded is occurring earlier with external stylesheet.
  1. When we add the inline style to the page, the overall size of the page increases from 6.1 KB to 6.4 KB
  1. 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.

#30 @adamsilverstein
10 months ago

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

#31 @scruffian
10 months ago

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

#32 @flixos90
10 months ago

  • Milestone changed from 6.1.2 to 6.2

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

#33 @adamsilverstein
10 months ago

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 ticket was mentioned in PR #4012 on WordPress/wordpress-develop by @flixos90.


10 months ago
#34

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

#35 @flixos90
10 months ago

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

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

#36 @wpgurudev
10 months ago

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

#37 @scruffian
10 months ago

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

#38 @patrickmeenan
10 months ago

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.

#39 @flixos90
10 months ago

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.

Last edited 10 months ago by sabernhardt (previous) (diff)

#40 @scruffian
10 months ago

Personally I'm happy with either approach.

#41 @wildworks
10 months ago

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

#42 @flixos90
10 months ago

  • Keywords needs-patch needs-unit-tests added; has-patch needs-testing removed
  • Milestone changed from 6.2 to 6.3

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.

@flixos90 commented on PR #4012:


10 months ago
#43

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

@flixos90 commented on PR #3561:


10 months ago
#44

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.

#45 @flixos90
9 months ago

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.

#46 @spacedmonkey
8 months ago

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

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


6 months ago
#47

  • Keywords has-patch added; needs-patch removed

#48 @spacedmonkey
6 months ago

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

#49 @flixos90
6 months ago

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

#50 @spacedmonkey
6 months ago

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.

#51 @adamsilverstein
6 months ago

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.

#52 @adamsilverstein
6 months ago

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.

@spacedmonkey commented on PR #4511:


6 months ago
#53

Committed.

#54 @flixos90
5 months ago

  • Milestone changed from 6.3 to Future Release
  • Owner flixos90 deleted

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.

Note: See TracTickets for help on using tickets.