Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Break wp-block-library stylesheet apart into per-block inline styles #49927

Open
felixarntz opened this issue Apr 19, 2023 · 29 comments · May be fixed by #50087
Open

Break wp-block-library stylesheet apart into per-block inline styles #49927

felixarntz opened this issue Apr 19, 2023 · 29 comments · May be fixed by #50087
Assignees
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Package] Block library /packages/block-library [Status] In Progress Tracking issues with work in progress [Type] Performance Related to performance efforts

Comments

@felixarntz
Copy link
Member

felixarntz commented Apr 19, 2023

What problem does this address?

Block themes do a great job conditionally enqueueing only the CSS that is actually needed using inline <style> tags. This is facilitated by the benefit of block themes generating their entire markup prior to outputting it, which allows us to detect what is used in the page and then conditionally including the necessary CSS in the <head>, i.e. still before the relevant content is rendered in the page.

While block themes are the future of WordPress, classic themes are still heavily used, and at the moment they always load the entire wp-block-library stylesheet, regardless of how much of the CSS rules in there are being used. This is bad for performance, and while for classic themes it may not seem as intuitive as for block themes to only load the CSS that's needed per block, there may still be a good way to do so.

At a minimum, this will involve breaking apart the single wp-block-library stylesheet that is built in the block-library package into distinct stylesheets per block type. This is already the case in the source code here, but we'd have to probably update the build process to no longer create just a single CSS file with everything.

What is your proposed solution?

Classic themes are procedurally output, i.e. we can't enqueue something in the <head> based on the page content, since by the time we know that, we're already past rendering the <head>. However, we can still conditionally output styles per block as we encounter the blocks on the page:

  • Inline <style> tags are perfectly reasonable to be included anywhere in the page. They do not have to appear in the <head>, and in fact loading them later can even be good for performance, as applied in the best practice of inlining critical CSS and loading the remaining CSS later.
  • We can leverage that as follows: Instead of a single wp-block-library stylesheet that has basic styles for tons of blocks, all of these blocks could have their individual block stylesheet in block.json.
  • As soon as a block is rendered on the page, we inline its stylesheet right there, before the block output. For example, if a core/gallery block is encountered for the first time in a page, we include a <style> tag with the contents of that block's stylesheet (based on packages/block-library/src/gallery/style.scss) right before the block's actual output.

With the above, we will be able to load substantially less CSS, particularly as many pages wastefully load the entire wp-block-library file today even though they use only a fraction of the blocks referenced in the CSS rules there.

Important considerations

Note that using inline stylesheets and including them right where the block is rendered is crucial: If CSS for visible content gets loaded too late, there is a risk of layout shifts which make for a bad user experience, so we can't just put the CSS in the footer, as that would mean the CSS is loaded too late. It's crucial we include the CSS right before the block is rendered. Additionally, we also shouldn't use external stylesheets as that would mean potentially a lot of extra stylesheets for little CSS, at which point it makes more sense to inline the CSS rules, similarly to how it is already done for block themes.

@felixarntz felixarntz added [Type] Performance Related to performance efforts [Package] Block library /packages/block-library CSS Styling Related to editor and front end styles, CSS-specific issues. labels Apr 19, 2023
@felixarntz
Copy link
Member Author

@oandregal @spacedmonkey @gziolo I'd love to get your thoughts on this idea.

@gziolo
Copy link
Member

gziolo commented Apr 19, 2023

If I understand correctly, then it should mostly be possible today for Classic Themes to opt-in for features that would enable such setup. It’s how Block Themes work by default after @aristath implemented a set of optimizations summarized in this dev note:
https://make.wordpress.org/core/2021/07/01/block-styles-loading-enhancements-in-wordpress-5-8/

We might only need to tweak the default configuration in WordPress core slightly.

@felixarntz
Copy link
Member Author

@gziolo Oh wow, I was totally not aware this is already a thing 🤦

Reading that post however it sounds like the block-specific styles are added in the footer? I'm a bit wary about layout shifts since it means the styles are printed notably later than the relevant HTML markup. Is that why it's currently opt-in?

cc @aristath

@Mamaduka
Copy link
Member

I believe they're added to the footer only in the Classic theme; because we don't have data on blocks used on the page when the head enqueue styles action is triggered.

The block should load per-block styles in the head.

@gziolo
Copy link
Member

gziolo commented Apr 20, 2023

Oh wow, I was totally not aware this is already a thing 🤦

We have a few hidden gems that work out of the box for block themes, mainly because it was safer to roll out this way as there were no backward compatibility considerations. We could test whether it would work with all themes maintained in WP core and the themes repository's top 10-20 Classic Themes. If it works seamlessly, then maybe we could turn it on for all themes.

@aristath
Copy link
Member

aristath commented Apr 20, 2023

I believe they're added to the footer only in the Classic theme; because we don't have data on blocks used on the page when the head enqueue styles action is triggered.

Exactly.
As mentioned in the initial post here, in block themes, we render the whole page internally and then send it, so we know what blocks will be rendered beforehand. This happens in the get_the_block_template_html() function, which runs before the <head> in wp-includes/template-canvas.php.
In classic themes however, the page is rendered progressively so we don't have that information. Since blocks can exist not only in the content but also in widget-areas, they can basically be anywhere there is a widget on the page - including the footer.
As a result, the only viable option was to either print these styles in the footer, OR completely change the way classic themes work and render the page internally fully and then send it. The former solution was simpler, while the latter has the potential to break everything so we avoided it. For example, right now if there is a fatal error in a classic theme in the footer, the page will render properly and the only thing that fails will be the footer itself. If we switch to pre-rendering internally, then that site will break.
That's why we went with the safer option of just enqueueing these in the footer for classic themes 👍

We could test whether it would work with all themes maintained in WP core and the themes repository's top 10-20 Classic Themes. If it works seamlessly, then maybe we could turn it on for all themes.

That would be amazing @gziolo!

@aristath
Copy link
Member

aristath commented Apr 20, 2023

Inline <style> tags are perfectly reasonable to be included anywhere in the page. They do not have to appear in the , and in fact loading them later can even be good for performance, as applied in the best practice of inlining critical CSS and loading the remaining CSS later.

That was actually the 1st implementation I tried when working on splitting the block styles... And it worked brilliantly! I would very much prefer doing that instead of what we do now where we enqueue everything in the <head>... However, if I remember correctly not everyone liked that approach when I had some discussions in the early stages of development, so I went with enqueueing things in the <head> instead. There was less resistance and friction there, so that's what got implemented 🤷

To give more context, I basically had 4 implementations to add the styles... 🤣

1. Inline styles the 1st time a block is encountered.

This was simply hooking in render_block, and injecting the styles inline like <style>/* CSS here */</style> right before the block got rendered. This was by far the fastest method, and like you mentioned the browser sees the styles right when they're needed for rendering the content that follows. Internally I was setting a static var where I was keeping track of which blocks are rendered, so that we don't print the same styles twice.
It was a beautiful solution IMO... extremely simple and performant.

2. Use JS to add styles

I loved this method as well... Basically what this was doing, was to add a tiny wpEnqueueStyle() JS function in the <head>. That function had as arguments the same args that wp_enqueue_style() has. On render_block, instead of printing the styles inline like the previous method, it would print <script>wpEnqueueStyle( foo, bar, whatever )</script> right above the block content, and then the wpEnqueueStyle function was injecting the <link> stylesheet in the head.

3. Add <link> elements the 1st time a block is encountered inline in the content

That one was a bit controversial for some, and it was throwing errors in HTML checkers that were not updated for HTML5, so it was getting people worried. In HTML5 having <link> elements inside the <body> is perfectly valid, but it's like trying to convince people you're not an elephant. 🤷

4. Enqueue styles in the <head> or the footer depending on whether it's a block theme or a classic theme

This was the least controversial method and that's what we went with.

An early proof of concept for all 4 implementations still exists in #25118. It's unpolished and at this point too old so a lot of things have changed in our codebase, but the code is solid and it works if we want to reopen that discussion. I'd love to improve the way we handle styles...

@spacedmonkey
Copy link
Member

I put together a quick PR WordPress/wordpress-develop#4343 that output the link in the footer if block is found and in the header in the block theme.

It seems work well, but putting the styles in the footer means layout shift problems.

@felixarntz
Copy link
Member Author

@aristath Thank you for all the context!

I wonder why we went with option 4 instead of option 1. I think option 4 is perfectly reasonable for block themes, but for classic themes it is problematic. What was the pushback on option 1? From a layout shift perspective, I can with certainty say that option 1 will work better for classic themes, as putting the styles in the footer is technically too late.

For that reason, I think we should go with option 1, at least for classic themes. For block themes, either option 1 or 4 work fine.

@aristath
Copy link
Member

I wonder why we went with option 4 instead of option 1.

What was the pushback on option 1?

If I remember correctly, the argument was that WP already has a mechanism to enqueue styles using wp_enqueue_style - as well as inlining them using wp_add_inline_style, so there was less friction when we used those instead of "reinventing the wheel".
TBH I just wanted to get this merged and take the 1st step towards improving the performance there, so I went with the flow.
Now that some time has passed and we have a better idea, I think it would be great to revisit that.

I think option 4 is perfectly reasonable for block themes, but for classic themes it is problematic.

Option 4 is OK for block themes, but I still believe that option 1 would be preferable...

From a layout shift perspective, I can with certainty say that option 1 will work better for classic themes, as putting the styles in the footer is technically too late.

Agreed.

What if we try to do option 1 for classic themes, and then experiment & test performance for block themes as well? If it's not a regression for block themes, then ideally we'd use option 1 for all instances, regardless of the type of theme.

@aristath
Copy link
Member

I could whip up a PR if we want to check inlining the CSS at the time the block gets rendered so we can test things 😉

@felixarntz
Copy link
Member Author

Thanks @aristath, implementing option 1 for classic themes only sounds like a great start! From there we can see how well that works in practice.

@aristath
Copy link
Member

aristath commented Apr 24, 2023

A rough proof of concept that does what was discussed above in included in the snippet below.
We can use that as a testing ground to check the performance impact of such a change, and see if it makes sense to create a PR for it or not.

<?php

if ( ! wp_is_block_theme() ) {
	// Load separate block styles.
	add_filter( 'should_load_separate_core_block_assets', '__return_true' );
	remove_action( 'wp_footer', 'wp_maybe_inline_styles', 1 );

	// Inline block styles.
	add_filter(
		'render_block',
		/**
		 * Filters the content of a single block.
		 *
		 * @param string   $block_content The block content.
		 * @param array    $block         The full block, including name and attributes.
		 */
		function( $block_content, $block ) {
			// Bail early if not a Core block.
			if ( ! str_starts_with( $block['blockName'], 'core/' ) ) {
				return $block_content;
			}

			/* This filter is documented in wp-includes/script-loader.php */
			$total_inline_limit = apply_filters( 'styles_inline_size_limit', 20000 );

			// A static var to keep track of the total inlined CSS size.
			static $total_inline_size = 0;

			// Get the handle for this block's stylesheet.
			$handle = str_replace( 'core/', 'wp-block-', $block['blockName'] );

			// Get the WP_Styles object.
			$wp_styles = wp_styles();

			// Bail early if this block was already processed.
			if ( in_array( $handle, $wp_styles->done, true ) ) {
				return $block_content;
			}

			// Bail early if the block doesn't have a stylesheet.
			if ( empty( $wp_styles->registered[ $handle ] ) ) {
				return $block_content;
			}

			$path  = wp_styles()->get_data( $handle, 'path' );
			$after = wp_styles()->get_data( $handle, 'after' );
			if ( is_array( $after ) ) {
				$after = implode( '', $after );
			}

			// Bail early if the stylesheet doesn't have a "path" defined,
			// or if there are no inline styles attached to it.
			if ( ! $path && ! $after ) {
				return $block_content;
			}

			// Get the CSS for this block's stylesheet.
			$css = file_exists( $path ) ? file_get_contents( $path ) : '';

			// Add the "after" styles.
			$css .= $after;

			// Get the size of the CSS.
			$styles_size = strlen( $css );

			// Bail early if the CSS is empty.
			if ( 0 === $styles_size ) {
				$wp_styles->done[] = $handle;
				return $block_content;
			}

			// Check if adding these styles inline will exceed the limit.
			// If it does, then bail early.
			if ( $total_inline_size + $styles_size > $total_inline_limit ) {
				return $block_content;
			}

			$wp_styles->done[] = $handle;
			$block_content    .= '<style id="' . $handle . '-inline-css">' . $css . '</style>';

			return $block_content;
		},
		PHP_INT_MAX,
		2
	);
}

How to test the above code

  • Create a test-styles-loading.php file in wp-content/mu-plugins
  • Paste the above snippet in there.
  • Activate a classic theme

Notes

We already have a styles_inline_size_limit filter in WP-Core, so the above PR takes that into account as well. Only the first 20kb of styles will be inlined, so if there are more, the rest of them will be enqueued in the footer.
It's easy to change that by adding something like this:

add_filter( 'styles_inline_size_limit', function() {
	return MB_IN_BYTES;
} );

@spacedmonkey
Copy link
Member

I have my own version of printing inline, here styles. It needs improvement etc, but it does work.

Reminder, that in #30579, we discussed changing the output of from a link to a style tag + import. Apparently this is not needed, as link tags in the body is valid HTML5.

@felixarntz
Copy link
Member Author

@aristath Thanks for sharing this snippet, that looks like a great start to me! Two points of feedback:

  1. Regarding $handle = str_replace( 'core/', 'wp-block-', $block['blockName'] );, is that how we currently use the handle already? Just asking since I find it a bit odd to use the plain block name as an asset handle 🤔
  2. Regarding $css .= $after;, I believe $after is an array of strings rather than just a string, so this would need to be fixed here.

@spacedmonkey Looking at your branch, that's another interesting variant, but it looks like it still prints the entire wp-block-library stylesheet when just one block is used? That's certainly better than what we have today, but still not as granular as it preferably should be.

@aristath I'm still not sure I fully understand the relationship between the wp-block-library CSS asset and each individual block's styles - does every core block have its own stylesheet registered already today, which is the same as the contents of the wp-block-library stylesheet for all core blocks? 🤔

@aristath
Copy link
Member

Regarding $handle = str_replace( 'core/', 'wp-block-', $block['blockName'] );, is that how we currently use the handle already? Just asking since I find it a bit odd to use the plain block name as an asset handle 🤔

Yes, that's what Core does too. We have the generate_block_asset_handle() function we could use, but internally that's what it does as well, and for the sake of this proof-of-concept it was just a little faster to do the str_replace directly.
If we deem the implementation worthy of a PR, then we can just switch to using that function, and this way also support 3rd-party blocks 👍

Regarding $css .= $after;, I believe $after is an array of strings rather than just a string, so this would need to be fixed here.

You're absolutely right, I missed that. Fixed by imploding the values 👍

@aristath I'm still not sure I fully understand the relationship between the wp-block-library CSS asset and each individual block's styles - does every core block have its own stylesheet registered already today, which is the same as the contents of the wp-block-library stylesheet for all core blocks? 🤔

The wp-block-library stylesheet has 2 possible sources:

  • If the should_load_separate_core_block_assets filter returns false (default for classic themes), wp-block-library loads the file from block-library/src/style.scss. That file imports all styles for all blocks, and then the block-library/src/common.scss file.
  • If the should_load_separate_core_block_assets filter returns true, wp-block-library loads the common.css file.

When we add add_filter( 'should_load_separate_core_block_assets', '__return_true' );, the source of the wp-block-library stylesheet changes from style.css to common.css, and the block styles are not loaded. Instead, each block registers its own stylesheet and then on block_render these individual stylesheets get enqueued.

@felixarntz
Copy link
Member Author

@aristath Thank you for all the clarification, that makes sense!

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Apr 26, 2023
@aristath
Copy link
Member

As per yesterday's performance meeting, I submitted a PR on #50087

@spacedmonkey
Copy link
Member

Great work on #50087. It looks great!

So the currently implements, add inline styles to the contents of say post content. Would this be an issue if you are using WP headless, say in a react site or a mobile app. Those style tags might break styling.

@felixarntz
Copy link
Member Author

@aristath I left some feedback and performance benchmarking data on #50087, it looks great so far from a client-side performance perspective. I also really like where it's going overall. That said, it would be great if we can find ways to make the additional server-side logic more efficient.

Separately, I was just thinking maybe we could also address https://core.trac.wordpress.org/ticket/56990 with this? If we know what blocks are rendered, we could include those few styles inline as well instead of always loading this small classic-themes.css stylesheet? What do you think?

@westonruter
Copy link
Member

westonruter commented Apr 28, 2023

That was actually the 1st implementation I tried when working on splitting the block styles... And it worked brilliantly! I would very much prefer doing that instead of what we do now where we enqueue everything in the <head>... However, if I remember correctly not everyone liked that approach when I had some discussions in the early stages of development, so I went with enqueueing things in the <head> instead. There was less resistance and friction there, so that's what got implemented 🤷

I believe I was one of those who was expressing resistance here, but not because I don't love the idea! Option 1 (and Option 3) both involve marking a stylesheet as being done when render_block happens (via do_blocks and thus the_content filters). So once a block is rendered, the stylesheet is printed and won't be printed again the next time the block is rendered on the page. The problem is that a block may be rendered but not displayed, and this can cause the stylesheets to not get printed at all.

For example, in my syntax-highlighting-code-block plugin, it currently is waiting to print the stylesheets for syntax highlighting until a Code block appears on the page (using the same approaches as Options 1 & 3). The link[rel=stylesheet] tag and inline style tag are injected right after the <pre> start tag for the first Code block. However, some users were complaining about missing styles: westonruter/syntax-highlighting-code-block#286

The reason is that some plugins, like the Fixed TOC table of contents plugin, will call do_blocks() on the content beforehand (e.g. by applying the_content filters) to obtain content for obtaining the table of contents. There are a several thousand plugins that manually apply the_content filters, and hundreds of others that manually call do_blocks().

The other concern I have about inlining is it could cause the CSS selectors to also break. Let's say you have two blocks, a paragraph block and a list block:

<p>...</p>

<ol>
  <li>...</li>
</ol>

And the stylesheets for both get inlined right before each block:

<style> p {} </style>
<p>...</p>

<style> ol {} </style>
<ol>
  <li>...</li>
</ol>

Let's also say you have a stylesheet in your theme that decides you want to apply some styling to lists that appear right after paragraphs:

p + ol,
p + ul {
   /* ... */
}

The inlining of the CSS with the blocks unfortunately breaks these CSS selectors since the ol is no longer a sibling of p since style was injected between them.

To inline CSS by injecting style & link elements into the blocks, there will at least need to be education among developers. There are thousands of themes using sibling selectors that could break with such CSS inlining.

@westonruter
Copy link
Member

westonruter commented Apr 28, 2023

The only alternative I can think of which would work for classic themes would be to incorporate output buffering of the template output, which would effectively open the door to doing the same optimizations possible with the block themes (i.e. the block styles printed in the footer could be moved to the head). But I doubt this would be an attractive solution to many.

Nevertheless, with the WP_HTML_Tag_Processor it could be implemented much more elegantly than before with brute regex.

@westonruter
Copy link
Member

Actually, no string manipulation would be needed at all with output buffering. If output buffering starts at the wp_head action after wp_print_styles() runs, and the output of print_late_styles() is captured at the footer, then the output buffer callback merely has to concatenate the output of print_late_styles() with the rest of the buffer from wp_head. Here's a proof of concept plugin which turns out to be pretty straightforward: https://gist.github.com/westonruter/25187db63a7666f08b151ca53497ffb5

@spacedmonkey
Copy link
Member

So at the moment, we have two places where style / scripts can be output on the page, header and footer. If you enqueue a style after header are output, it fallback to outputting in the footer. What if we add a new inline styles and scripts, after all filter that have do_blocks applied to it ( so the_content and widget_block_content). This would mean, that once the blocks are processed, styles / scripts could be output inline spot. This would mean however, that styles are output a little after the markup for the block is output, meaning maybe some layout shift.

Another idea, would be to loop through posts in the loop and then see if they blocks and output styles for only blocks used in the loops post content. For blocks in widgets we could add styles before the sidebar is output. For other blocks on the page like other WP_Query, this would fallback to outputting in the footer.

None of these solutions feel great. Any solution would give theme developers control of the markup that is generated. That control, might mean we need to make this opt-in.

@westonruter
Copy link
Member

This would mean, that once the blocks are processed, styles / scripts could be output inline spot. This would mean however, that styles are output a little after the markup for the block is output, meaning maybe some layout shift.

Yeah, this is essentially the same situation as we have today, where the styles are printed in the footer. It would just mean the styles are printed a bit higher up, but there is still the possibility for FOUC.

Another idea, would be to loop through posts in the loop and then see if they blocks and output styles for only blocks used in the loops post content. For blocks in widgets we could add styles before the sidebar is output. For other blocks on the page like other WP_Query, this would fallback to outputting in the footer.

One issue with this is that even though a post may be in the loop, this doesn't mean that each post's content will actually be rendered on the page. Consider the case of archive templates that use the_excerpt() instead of the_content(). If the template is using the_excerpt(), then this approach could result in many more styles being printed than are actually needed.

IMO, the only/best way to solve this problem is to utilize output buffering. I'm going to do some more research in that area.

@spacedmonkey
Copy link
Member

So if should_load_separate_core_block_assets filter is set to true, then all the block styles are split. This seems like amazing functionality and not well promoted.

If we have should_load_separate_core_block_assets filter, why does this default to false? In block themes it is set to true here.

I would love to see all themes have this functionality, is this something we could look into changing for all themes? Thoughts @felixarntz

@westonruter
Copy link
Member

@spacedmonkey The reason it is disabled by default for non-block themes is because the split up styles end up getting printed in the footer, thus causing the possibility of FOUC. This isn't an issue for block themes since the entire page is rendered in memory before being sent to the client, and thus the split-up styles can still be inserted into the head. I was able to achieve the same for non-block themes above in #49927 (comment) by using output buffering.

@spacedmonkey
Copy link
Member

I put together a little a POC plugin. https://gist.github.com/spacedmonkey/29c2af943da9d729138eb47ff7a18cff.

This does two things

  1. forces should_load_separate_core_block_assets filter to be true.
  2. Get all posts in the current loop and all the block widgets, and enqueus there styles.

To be clear, that is not all blocks that could be on a page, blocks could come from other places, like custom WP_Query. But it will get all the likely blocks. Any other blocks styles will render in the footer. Not perfect, but could be workable.

@roambiz
Copy link

roambiz commented Feb 10, 2024

As a user, I understand that I cannot directly participate in your conversation, but I'm glad to see someone else who shares my concern about performance issues. In an ideal scenario, Gutenberg could be designed to allow users to create their own style packs?

Choose the desired blocks (already available).
Package the default styles into one bundle (currently, I use SCSS, but it's not consistent).
Choose to use an external stylesheet or inline styles (let us make the choice). This way, if there are many blocks used, we can use an external stylesheet, and if there are only a few blocks, we can inline all the styles. If a page only uses one block, only that block's styles should be loaded.
Currently, this can be achieved by disabling the default styles and manually printing the required styles using SASS.

Our idea is that no matter how it is designed, full compatibility cannot be achieved because there are too many scenarios. Sometimes, we want to use the classic editor and the block editor together.

Would it be too challenging to develop an automated style pack generation feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Package] Block library /packages/block-library [Status] In Progress Tracking issues with work in progress [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants