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

Style engine: enqueue block supports styles in Gutenberg #42880

Merged

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Aug 2, 2022

What?

A follow up to:

What?

Let's enqueue stored block supports styles in Gutenberg, rather than in the style engine.

At the same time, we'll combine block style and layout styles into one style tag.

Why?

This PR follows up on a discussion in #42452, the substance of which is that it's probably better to give the consumer control over how styles are rendered via hooks / enqueues etc

How?

Adding a new function in script-loader.php - gutenberg_enqueue_stored_block_supports_styles() (name to be debated) that grabs block supports styles from the style engine store and enqueues the stylesheet.

Testing Instructions

Test in both classic and block themes.

Check that elements and layouts styles are enqueued.

Create a new post and insert some text links and group blocks (or any layout styles)

Here's some test HTML:

Example
<!-- wp:group {"style":{"color":{"background":"#ee6b6b"},"elements":{"link":{"color":{"text":"var:preset|color|vivid-cyan-blue"}}}},"layout":{"type":"flex","flexWrap":"nowrap","justifyContent":"space-between","orientation":"vertical"}} -->
<div class="wp-block-group has-background has-link-color" style="background-color:#ee6b6b"><!-- wp:paragraph {"style":{"elements":{"link":{"color":{"text":"var:preset|color|white"}}}}} -->
<p class="has-link-color"><a href="https://word">Test</a></p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"style":{"elements":{"link":{"color":{"text":"#eaf1f6"}}}}} -->
<p class="has-link-color"><a href="https://word">Test</a></p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

<!-- wp:group {"style":{"color":{"background":"#c4cce6"}},"layout":{"contentSize":"342px"}} -->
<div class="wp-block-group has-background" style="background-color:#c4cce6"><!-- wp:paragraph {"style":{"elements":{"link":{"color":{"text":"var:preset|color|vivid-red"}}}}} -->
<p class="has-link-color"><a href="https://word">Test</a></p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

<!-- wp:group {"style":{"color":{"background":"#9eebbc"}},"layout":{"contentSize":"342px"}} -->
<div class="wp-block-group has-background" style="background-color:#9eebbc"><!-- wp:paragraph {"style":{"elements":{"link":{"color":{"text":"#cd31d6"}}}}} -->
<p class="has-link-color"><a href="https://word">Test</a></p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

Screen Shot 2022-08-02 at 3 42 30 pm

Expected CSS output. For classic themes this style tag should appear at the bottom of the page. For block themes, in the head.

<style id='block-supports-styles-inline-css'>
.wp-elements-bfe89bcbd3fbf76f9bf93ac884528d9a a {color: var(--wp--preset--color--vivid-cyan-blue);}.wp-elements-85694c8ee6aa1fb95efca209b615f0b0 a {color: var(--wp--preset--color--white);}.wp-elements-6ac1c4acebd6a433ed64dc4e6b0eb185 a {color: #eaf1f6;}.wp-elements-469734e762a60395c96370480f3c9414 a {color: var(--wp--preset--color--vivid-red);}.wp-elements-9307c4f13980ac2b051b4fcf5f7b28fa a {color: #cd31d6;}.wp-block-group.wp-container-1 {flex-wrap: nowrap; align-items: flex-start;}.wp-block-post-content.wp-container-4 > :where(:not(.alignleft):not(.alignright):not(.alignfull)) {max-width: 840px; margin-left: auto !important; margin-right: auto !important;}.wp-block-post-content.wp-container-4 > .alignwide {max-width: 1100px;}.wp-block-group.wp-container-2 > :where(:not(.alignleft):not(.alignright):not(.alignfull)),.wp-block-group.wp-container-3 > :where(:not(.alignleft):not(.alignright):not(.alignfull)) {max-width: 342px; margin-left: auto !important; margin-right: auto !important;}.wp-block-group.wp-container-2 > .alignwide,.wp-block-group.wp-container-3 > .alignwide {max-width: 342px;}.wp-block-group.wp-container-2 .alignfull,.wp-block-group.wp-container-3 .alignfull,.wp-block-post-content.wp-container-4 .alignfull {max-width: none;}
</style>

@ramonjd ramonjd requested a review from spacedmonkey as a code owner August 2, 2022 05:44
@ramonjd ramonjd self-assigned this Aug 2, 2022
@ramonjd ramonjd added [Package] Style Engine /packages/style-engine [Type] New API New API to be used by plugin developers or package users. [Status] In Progress Tracking issues with work in progress labels Aug 2, 2022
Comment on lines 43 to 93
/**
* Fetches, processes and compiles stored block supports styles, then renders them to the page.
* Styles are stored via the style engine API. See: packages/style-engine/README.md
*/
function gutenberg_enqueue_stored_block_supports_styles() {
$styles = gutenberg_style_engine_get_stylesheet_from_store( 'block-supports' );
$styles .= gutenberg_style_engine_get_stylesheet_from_store( 'layout-block-supports' );
if ( ! empty( $styles ) ) {
$key = 'block-supports-styles';
wp_register_style( $key, false, array(), true, true );
wp_add_inline_style( $key, $styles );
wp_enqueue_style( $key );
}
}

Copy link
Member

@aristath aristath Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better if we abstract this function a bit... Instead of having a function specifically for the block-supports, we could have it for all stores. In another PR I'm converting global-styles to use the style-engine, so if we make this function specifically for block-supports, then we'd have to add a separate function for global-styles, then another one for theme-styles and any other implementation we may come up with in the future.

This works fine:

Suggested change
/**
* Fetches, processes and compiles stored block supports styles, then renders them to the page.
* Styles are stored via the style engine API. See: packages/style-engine/README.md
*/
function gutenberg_enqueue_stored_block_supports_styles() {
$styles = gutenberg_style_engine_get_stylesheet_from_store( 'block-supports' );
$styles .= gutenberg_style_engine_get_stylesheet_from_store( 'layout-block-supports' );
if ( ! empty( $styles ) ) {
$key = 'block-supports-styles';
wp_register_style( $key, false, array(), true, true );
wp_add_inline_style( $key, $styles );
wp_enqueue_style( $key );
}
}
/**
* Fetches, processes and compiles stored styles, then renders them to the page.
* Styles are stored via the style engine API. See: packages/style-engine/README.md
*/
function gutenberg_enqueue_stored_styles() {
$stores = WP_Style_Engine_CSS_Rules_Store_Gutenberg::get_stores();
foreach ( array_keys( $stores ) as $store_name ) {
$styles = gutenberg_style_engine_get_stylesheet_from_store( $store_name );
if ( ! empty( $styles ) ) {
$key = "wp-style-engine-$store_name";
wp_register_style( $key, false, array(), true, true );
wp_add_inline_style( $key, $styles );
wp_enqueue_style( $key );
}
}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair call on the name change. Much better.

I was also wondering about how we'd cater for global styles and make it backwards compatible.

I left it out of the equation so we could decide later how to handle them. Mainly because of what's going on in gutenberg_enqueue_global_styles() and the ever-changing nature of global styles.

Also, this would print out every stored store. Is that the intention?

Would there be any issue against being explicit about which styles Gutenberg prints, i.e., adding them as we need them?

Aside from the certainty/control aspect, we'll need to determine the order eventually, e.g., we definitely want block supports styles to be printed after global styles.

I can imagine it would be handy for theme authors to be able to queue styles via the style engine, but I'm unaware if it's something that they should control themselves (?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, great discussion here, I think I agree with both of you 😄 — it'd be good to come up with a combined function that works for those future use cases, rather than needing it to be tied specifically to block supports 👍

Would there be any issue against being explicit about which styles Gutenberg prints, i.e., adding them as we need them?

I agree that it'd be better to be explicit about which particular stores we're enqueueing, so that we can both determine the order, and have the function be clear about what's being enqueued (which should help with debugging).

Ideally, if something somewhere (a plugin or theme) adds another store, then maybe it shouldn't be automatically enqueued. Enqueueing styles should probably still be the responsibility of the theme to do explicitly, rather than happening automatically?

If we want the function to help themes or other use cases with the task of enqueueing, perhaps one way to do it might be to have a param that defaults to default or something like that. If it's default then we enqueue the Gutenberg stores, and if it isn't default then it looks up the store name to be enqueued? 🤷

Also, this might just be a personal preference, but it can be useful to have explicit strings for style keys where we can, so that if you're inspecting a rendered page and need to look up where that style gets generated, you can search the codebase for the string in order to find it. Not very DRY, but can sometimes help with debugging.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'd be good to come up with a combined function that works for those future use cases, rather than needing it to be tied specifically to block supports

Yeah, it'd be great to be able satisfy both causes:

  • having control over the stores that Gutenberg requires to be enqueued
  • giving theme authors the opportunity to opt-in to this behaviour

A naive version might be to filter out and enqueue Gutenberg's stores in gutenberg_enqueue_block_support_styles(), then loop over the remaining (presumably added elsewhere by themes or whatever).

It'd be better to explicitly define "equeuability" so maybe we could utilize the context and enqueue parameters so that we store all styles that specify a context, but only enqueue them if enqueue === true 🤔

The consequence is that we'd have to filter out rules from the store that shouldn't be enqueued. 🤔 🤔🤔🤔🤔🤔

return gutenberg_style_engine_get_stylesheet(
    $layout_styles,
   array(
      'context' => 'layout-block-supports', // It will be stored in the  'layout-block-supports' store.
      'enqueue' => true, // It will be enqueued.
   )
);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A naive version might be to filter out and enqueue Gutenberg's stores in gutenberg_enqueue_block_support_styles(), then loop over the remaining (presumably added elsewhere by themes or whatever).

I mean something like this:
/**
 * Fetches, processes and compiles stored core styles, then combines and renders them to the page.
 * Any other registered stores are enqueued separately after the core styles.
 * Styles are stored via the style engine API. See: packages/style-engine/README.md
 */
function gutenberg_enqueue_stored_styles() {
	$core_styles_keys    = array( 'block-supports', 'layout-block-supports' );
	$compiled_stylesheet = '';
	foreach ( $core_styles_keys as $style_key ) {
		$compiled_stylesheet .= gutenberg_style_engine_get_stylesheet_from_store( $style_key );
	}

	// Combine Core styles.
	// @TODO check for `defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG` and split them for ease of debugging.
	if ( ! empty( $styles ) ) {
		$key = 'block-supports-styles';
		wp_register_style( $key, false, array(), true, true );
		wp_add_inline_style( $key, $compiled_stylesheet );
		wp_enqueue_style( $key );
	}

	$additional_stores = WP_Style_Engine_CSS_Rules_Store_Gutenberg::get_stores();
	foreach ( array_keys( $additional_stores ) as $store_name ) {
		if ( in_array( $store_name, $core_styles_keys, true ) ) {
			continue;
		}
		$styles = gutenberg_style_engine_get_stylesheet_from_store( $store_name );
		if ( ! empty( $styles ) ) {
			$key = "wp-style-engine-$store_name";
			wp_register_style( $key, false, array(), true, true );
			wp_add_inline_style( $key, $styles );
			wp_enqueue_style( $key );
		}
	}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this would print out every stored store. Is that the intention?

Yes

Would there be any issue against being explicit about which styles Gutenberg prints, i.e., adding them as we need them?

No issue with that, but I don't think that having a separate function per-store would be the right way to approach this 😅

Aside from the certainty/control aspect, we'll need to determine the order eventually, e.g., we definitely want block supports styles to be printed after global styles.

Well... not necessarily! I agree with the "we'll need to determine the order eventually" part of the sentence, but I don't believe they should actually be enqueued separately!
Ideally, block supports and global styles would be in the same store (let's say a core store).
Block supports would be added to the store after global styles, so any CSS that needs to override global-styles will be merged and override things properly. Having these 2 in the same store would allow for further optimization of the resulting CSS by combining things where appropriate 😉

Maybe instead of a priority on enqueue it would be more appropriate to have a priority at the time we add styles to the store... This way we'd be able to sort the array based on priorities, and merge styles properly, ensuring overrides are correctly applied?

I can imagine it would be handy for theme authors to be able to queue styles via the style engine, but I'm unaware if it's something that they should control themselves (?)

They definitely should be able to! I can think of many scenarios where theme-authors would be able to take advantage of it and append their styles to the Core styles, or use the style-engine to add block-specific styles! It would certainly be a lot more efficient (for performance and sustainability) if they added their opinionated block-styles this way instead of enqueueing monolithic stylesheets.
Additionally, it's not just themes that can use it... Think block.json and styles we can add in there - both in Core blocks and 3rd-party blocks.

Ideally, if something somewhere (a plugin or theme) adds another store, then maybe it shouldn't be automatically enqueued. Enqueueing styles should probably still be the responsibility of the theme to do explicitly, rather than happening automatically?

Hmmm what if we add a prop to stores that can define if the store should be automatically enqueued or not?

Also, this might just be a personal preference, but it can be useful to have explicit strings for style keys where we can, so that if you're inspecting a rendered page and need to look up where that style gets generated, you can search the codebase for the string in order to find it. Not very DRY, but can sometimes help with debugging.

Intersting. I suppose we could do something like that if SCRIPT_DEBUG is set to true... but I don't think it should be the default behavior. The default output should always be optimized for consumers, not developers 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A naive version might be to filter out and enqueue Gutenberg's stores in gutenberg_enqueue_block_support_styles(), then loop over the remaining (presumably added elsewhere by themes or whatever).

Seems reasonable!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issue with that, but I don't think that having a separate function per-store would be the right way to approach this

Totally agree 👍

I agree with the "we'll need to determine the order eventually" part of the sentence, but I don't believe they should actually be enqueued separately!

I think we're on the same page. I was only referring to having the ability, however we do it, to respect the styles hierarchy and preserve the cascade.

So I mean Global styles -> Elements -> Block styles -> Block supports.

Ideally, block supports and global styles would be in the same store (let's say a core store).

This makes sense! I like!

Block supports would be added to the store after global styles, so any CSS that needs to override global-styles will be merged and override things properly. Having these 2 in the same store would allow for further optimization of the resulting CSS by combining things where appropriate

Oooh now I see what you mean. Sweet! Thanks for the explainer. 🍺

I'm off for a couple of days but will finish this off and do some more testing on Friday.

Thanks for all the great feedback!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A naive version might be to filter out and enqueue Gutenberg's stores in gutenberg_enqueue_block_support_styles(), then loop over the remaining (presumably added elsewhere by themes or whatever).

I've done this for now and have added test coverage for it.

Happy to refine things in follow ups!

@aristath
Copy link
Member

aristath commented Aug 2, 2022

This looks nice!

I pushed a small commit, removing the constructor and the get_instance() method from the main class. All methods in that class are static now, so we don't need to get any instances etc, and the constructor no longer makes any sense if we remove the get_instance() method.

I only have one thing that I would like to see changed in this PR, and I left a comment above 👍

@ramonjd ramonjd force-pushed the update/style-engine-enqueue-block-supports-scripts-in-gutenberg branch 3 times, most recently from 8917291 to ba6f124 Compare August 3, 2022 04:11
@ramonjd ramonjd force-pushed the update/style-engine-enqueue-block-supports-scripts-in-gutenberg branch 2 times, most recently from 2e2abce to 8d227f6 Compare August 5, 2022 00:01
@@ -646,10 +570,10 @@ function wp_style_engine_get_styles( $block_styles, $options = array() ) {
}

if ( ! empty( $parsed_styles['declarations'] ) ) {
$styles_output['css'] = $style_engine::compile_css( $parsed_styles['declarations'], $options['selector'] );
$styles_output['css'] = WP_Style_Engine::compile_css( $parsed_styles['declarations'], $options['selector'] );
$styles_output['declarations'] = $parsed_styles['declarations'];
if ( true === $options['enqueue'] ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enqueue doesn't really work as an option anymore since all the style engine is doing is "storing"

Maybe we can default the "context" to null and store wherever a non-empty context string value is passed (?)

Then we could remove enqueue altogether for now.

I'll do this in a follow up if folks think its legit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do this in a follow up if folks think its legit.

Sounds legit for a follow-up to me! Makes for a slightly simpler call to store styles, and means that the calling code doesn't need to think about what the style engine is doing.

@ramonjd ramonjd added Needs User Documentation Needs new user documentation Needs Dev Note Requires a developer note for a major WordPress release cycle labels Aug 5, 2022
@ramonjd ramonjd force-pushed the update/style-engine-enqueue-block-supports-scripts-in-gutenberg branch 2 times, most recently from 0a41d80 to ed847e3 Compare August 8, 2022 00:54
Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is testing really nicely for me @ramonjd! Thanks for all the follow-ups. Just left a few nits — the one item that we should probably address before merge is the Gallery block's usage of the gutenberg_enqueue_block_support_styles function. I think the choices are probably between updating the call in that block, or deferring introducing the _deprecated_function call to a follow-up, once we've refactored the usage.

What do you think?

@@ -646,10 +570,10 @@ function wp_style_engine_get_styles( $block_styles, $options = array() ) {
}

if ( ! empty( $parsed_styles['declarations'] ) ) {
$styles_output['css'] = $style_engine::compile_css( $parsed_styles['declarations'], $options['selector'] );
$styles_output['css'] = WP_Style_Engine::compile_css( $parsed_styles['declarations'], $options['selector'] );
$styles_output['declarations'] = $parsed_styles['declarations'];
if ( true === $options['enqueue'] ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do this in a follow up if folks think its legit.

Sounds legit for a follow-up to me! Makes for a slightly simpler call to store styles, and means that the calling code doesn't need to think about what the style engine is doing.

$compiled_core_stylesheet .= "/**\n * Core styles: $style_key\n */\n";
}
// Chain core store ids to signify what the styles contain.
$style_tag_id .= $style_tag_id ? '-' . $style_key : $style_key;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since $style_tag_id is defined as 'core' by default it looks like this ternary will always be true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! T'was an artefact from something... somewhere.

foreach ( $core_styles_keys as $style_key ) {
// Add comment to identify core styles sections in debugging.
if ( defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG ) {
$compiled_core_stylesheet .= "/**\n * Core styles: $style_key\n */\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea to aid debugging!

packages/style-engine/README.md Outdated Show resolved Hide resolved
packages/style-engine/README.md Outdated Show resolved Hide resolved
packages/style-engine/README.md Outdated Show resolved Hide resolved
* @param string $style String containing the CSS styles to be added.
* @param int $priority To set the priority for the add_action.
*/
function gutenberg_enqueue_block_support_styles( $style, $priority = 10 ) {
_deprecated_function( __FUNCTION__, '6.1' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice that with this PR applied, this is being logged once per page request:

image

It looks like it's being called from the Gallery block. I imagine we should update that call, too, otherwise we'll potentially risk flooding logs of sites running the GB plugin?

Alternately, we could leave the _deprected_function call to a follow-up if you'd prefer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deferring introducing the _deprecated_function call to a follow-u

Actually, I think this you had the best idea for now. We can deprecate when we deal with the final Gutenberg usage in the gallery.

Just in case the gallery gets hairy 😄

I've reverted this change.

@ramonjd
Copy link
Member Author

ramonjd commented Aug 8, 2022

the one item that we should probably address before merge is the Gallery block's usage of the gutenberg_enqueue_block_support_styles function

Good call. I had that buried deep on my list of TODOs. Thanks for reminding me. I'll get a PR up for that ASAP. 🙇

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the follow-up @ramonjd — this is testing well for me manually. I just noticed that I think the output is being processed twice, I've left a comment around the add_action lines. I wasn't too sure which way would be best for handling it, but happy to do more testing / exploration!

lib/compat/wordpress-6.1/script-loader.php Show resolved Hide resolved
* Fetches, processes and compiles stored core styles, then combines and renders them to the page.
* Styles are stored via the style engine API. See: packages/style-engine/README.md
*/
function gutenberg_enqueue_stored_styles() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking, the idea with using a generic name like wp_enqueue_stored_styles rather than being tied to the style engine, is that we're not exposing any style engine behaviour here? I think that sounds good to me, in that this is about enqueuing stored styles in general, (and deals with essential styles for WordPress) rather than being specific to the style engine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another tricky naming issue 😄

"Stored" indirectly implies the style engine, but yeah the motivation was to expand this function later to apply to all stores. See comment above.

If that's true, it will probably require a strategy to subsume/integrate gutenberg_enqueue_global_styles() when we get to global styles. ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that we're not exposing any style engine behaviour here?

Only pulling from style engine stores. Do you think we should be more specific in the naming? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should be more specific in the naming? 🤔

Naming's hard! For anything that's specifically style engine related (rather than the style engine being an implementation detail), I like the idea of including style_engine in the public function name so that it's all grouped together. So I'd probably lean toward wp_style_engine_enqueue_stored_styles as the function name, but I don't feel at all strongly about it if you prefer the shorter name 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds infinitely reasonable.

I don't really have a preference either 🙉

I suppose a weak argument to keep it generic is that wp_style_engine_enqueue_stored_styles might communicate to some that it's a public function of the style engine package.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose a weak argument to keep it generic is that wp_style_engine_enqueue_stored_styles might communicate to some that it's a public function of the style engine package.

Good point! Since neither of us feels strongly about it, I think that nudges us more toward keeping the shorter title — after all, it's a function defined outside of the style engine package.

ramonjd and others added 4 commits August 9, 2022 14:31
Hard code a list of core styles that we want to fetch from the store and the order in which we fetch them.
For classic themes, in the footer.
@ramonjd ramonjd force-pushed the update/style-engine-enqueue-block-supports-scripts-in-gutenberg branch from 1de71a1 to 13785f1 Compare August 9, 2022 04:39
Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the follow-up @ramonjd! This is testing nicely for me now, with the callback only firing once. I smoke tested that the output still appears to be correct, and is only being enqueued once in wp_enqueue_scripts in blocks-based themes, and once in wp_footer in Classic themes.

This LGTM, and we can continue to refine and adjust the logic within gutenberg_enqueue_stored_styles in follow-ups as we explore global styles, etc, I reckon.

Nice work! 🎉

@ramonjd ramonjd merged commit 090171d into trunk Aug 9, 2022
@ramonjd ramonjd deleted the update/style-engine-enqueue-block-supports-scripts-in-gutenberg branch August 9, 2022 22:16
@github-actions github-actions bot added this to the Gutenberg 13.9 milestone Aug 9, 2022
@oandregal oandregal added [Type] Experimental Experimental feature or API. and removed [Type] New API New API to be used by plugin developers or package users. [Status] In Progress Tracking issues with work in progress labels Aug 10, 2022
@oandregal
Copy link
Member

For the changelog, as this is a follow-up to #42452 (marked as experimental) I've labeled this as experimental as well.

@ramonjd
Copy link
Member Author

ramonjd commented Aug 10, 2022

For the changelog, as this is a follow-up to #42452 (marked as experimental) I've labeled this as experimental as well.

@oandregal Is there anything you need me to do here or is it still automated?

@oandregal
Copy link
Member

Nah, everything is fine. I was just editing titles and labels and wanted to tell people why I did.

@enodekciw
Copy link
Contributor

Is there a specific reason why this is being enqueued in wp_footer when using classic theme?

The website I'm currently working on doesn't use a FSE theme, but uses more like a hybrid approach - header and footer are loaded from header.php and footer.php but everything else is managed with Gutenberg. I suppose this is considered "a classic theme", right?

Let's say I build a hero section now:

  • Group block with specific layout sizes
  • Inside of that Group block there's a center aligned Heading block
  • center aligned Buttons block comes after the Heading block

Now what happens is that all of the layout / alignment CSS from Gutenberg is enqueued in footer and this creates a huge flash of unstyled content issue. Also, Core Web Vitals (especially, CLS) isn't happy. Basically, I have to load custom CSS in the head section just to get rid of this, atleast for a hero section.

Maybe it would be a good idea to atleast have a filter to be able to enqueue this in the head section instead of the footer for classic (more like hybrid) themes?

@ramonjd
Copy link
Member Author

ramonjd commented Aug 21, 2022

Is there a specific reason why this is being enqueued in wp_footer when using classic theme?

This PR doesn't change the functionality that's been in place all this time.

I started looking in the Git history for a specific reason. It goes way back and I believe it's for backwards compatibility with Core. This PR provides some clues:

The dynamic block styles for layout and elements should be loaded in the for block themes. While that should also be the case for classic themes, the current mechanism we use (render_block) does not allow us to do that

If you manage to go deeper and find out more it'd be great to know.

Maybe it would be a good idea to at least have a filter to be able to enqueue this in the head section instead of the footer for classic (more like hybrid) themes?

This is an interesting suggestion, thanks. Adding filters is definitely something that folks could look at, and I know, once the style engine is in Core and is stable, there'll be some in the style engine package at least.

As for a filter for enqueuing itself, it could be worth a try so long as we take into account the historical compatibility issues during testing.

@aristath
Copy link
Member

Regarding loading in header vs footer in classic themes vs block themes: it all comes down to the timing and when we parse blocks.
In block themes, we get the contents of a page and render the blocks before wp_head when rendering a page. In classic themes however, rendering happens differently and when wp_head runs, we don't actually know what blocks exist on the whole page. Blocks get rendered as we discover them in the content and the only viable option is to print their styles in the footer because the head has already been rendered.
So the main difference is that block themes get the whole template/content/etc before the head, while classic themes have "progressive" rendering.

@ramonjd
Copy link
Member Author

ramonjd commented Aug 22, 2022

Thanks for the explanation @aristath!! 🙇

It sounds like something I could add to the docs or in a code comment for posterity.

Edit: oh, there's a note related this already here:

* For classic ones, it's loaded in the body

markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Sep 19, 2022
…sts to 6.1.

This changeset backports the following changes:

- Implement [WordPress/gutenberg#42880 gutenberg#42880]: Backport script loader: enqueue stored block supports styles
- Allow a way to bypass `SCRIPT_DEBUG` in tests. See [WordPress/wordpress-develop#3259 (comment) comment] and the related [WordPress/gutenberg#44248 Gutenberg pull request]

Props ramonopoly, gziolo, bernhard-reiter, audrasjb, costdev.
See #56467.

Built from https://develop.svn.wordpress.org/trunk@54214


git-svn-id: http://core.svn.wordpress.org/trunk@53773 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Sep 19, 2022
…sts to 6.1.

This changeset backports the following changes:

- Implement [WordPress/gutenberg#42880 gutenberg#42880]: Backport script loader: enqueue stored block supports styles
- Allow a way to bypass `SCRIPT_DEBUG` in tests. See [WordPress/wordpress-develop#3259 (comment) comment] and the related [WordPress/gutenberg#44248 Gutenberg pull request]

Props ramonopoly, gziolo, bernhard-reiter, audrasjb, costdev.
See #56467.

Built from https://develop.svn.wordpress.org/trunk@54214


git-svn-id: https://core.svn.wordpress.org/trunk@53773 1a063a9b-81f0-0310-95a4-ce76da25c4cd
ootwch pushed a commit to ootwch/wordpress-develop that referenced this pull request Nov 4, 2022
…sts to 6.1.

This changeset backports the following changes:

- Implement [WordPress/gutenberg#42880 gutenberg#42880]: Backport script loader: enqueue stored block supports styles
- Allow a way to bypass `SCRIPT_DEBUG` in tests. See [WordPress#3259 (comment) comment] and the related [WordPress/gutenberg#44248 Gutenberg pull request]

Props ramonopoly, gziolo, bernhard-reiter, audrasjb, costdev.
See #56467.


git-svn-id: https://develop.svn.wordpress.org/trunk@54214 602fd350-edb4-49c9-b593-d223f7449a82
@ramonjd ramonjd removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs User Documentation Needs new user documentation [Package] Style Engine /packages/style-engine [Type] Experimental Experimental feature or API.
Projects
Status: 🏆 Done
Development

Successfully merging this pull request may close these issues.

6 participants