Elements: Add styles to the footer before the block is rendered #37728

scruffian · 2022-01-05T15:24:35Z

Related #37582
An alternative to #37593

What

This uses the pre_render hook of the block to render its styles, so the parent styles are enqueued before the children's.

Why

See #37582 for details. Essentially, this fixes an use case where both the parent and the inner block set the link color, and the expectation is that the child link color takes precedence.

How to test

See #37582 for comprehensive instructions

mikachan

This looks like a great alternative to #37593, just because it seems to be addressing how the styles are rendered directly.

I'm seeing the below error though for $content. I guess this should be returning the block content (with the stylesheet?), similar to gutenberg_render_elements_support?

lib/block-supports/elements.php

mikachan

I was having trouble with this until I realised that pre_render_block will only run on nested blocks from WP 5.9 😅 There's more info in this trac ticket for anyone else testing.

On 5.9, this works great for me and fixes the original issue. Here's a post with different link colors using:

a paragraph on its own
a nested paragraph in a nested group, in a group
a nested paragraph in a group
another nested paragraph in a group

This is using emptytheme. It also matches what I'm seeing in the editor.

I'm approving based on the above, but it would be great if someone else could review this as well.

youknowriad · 2022-01-07T13:22:04Z

Hi there! Thanks for the PR. I'm not familiar with this particular issue but is this something specific to the "elements" styles. What about the "layout" block support styles which use a similar technique? Do we need some kind of common helper to use for all the situations where we need to inject styles from blocks?

scruffian · 2022-01-07T13:45:57Z

From what I can see the styles in layout aren't expected to inherit in the same way as these ones, so the problem isn't evident there. However I can imagine one day we might expect some of those rules to inherit (maybe block gap for example) so it might make sense to use the same approach in both places.

andrewserong

Thanks for the ping @scruffian, and good find digging into the cause behind the inheritance issue! I don't think we'll have the same issue with block gap now that we've switched away from using CSS variables for it. I've just left a few questions about generating the class name and whether we can make it a common helper function that all the block supports can use, but Riad might have a better idea of where we're headed in the future in regards to managing generated class names.

andrewserong · 2022-01-10T05:28:01Z

lib/block-supports/elements.php

+ * @param  array  $block         Block object.
+ * @return string                The unique class name.
+ */
+function gutenberg_get_elements_class_name ( $block ) {


I like the idea of programatically generating the class name based on values within the block object, and I think it helps get us a little closer to some of the ideas @youknowriad raised in the discussion on improving saving/rendering of block styles (here, it's a good example of sharing a class name between two separate functions).

Since this function will be exposed in the global namespace, is this a good opportunity to create a generic gutenberg_get_block_support_class_name function that the other block supports that currently use uniqid could also use? We might then have a first parameter of a string prefix, possibly, so each support could add its own wp-elements- or wp-container- prefix as needed.

With using the entire block object for the MD5, we might only need to use part of the block attributes (e.g. the style attribute) rather than the whole thing. That probably doesn't change the implementation here, but it could mean that we could pass this helper function any arbitrary data for generating the unique class name rather than it necessarily being a block object?

I was also wondering about the md5 string, which at 32 characters is pretty long for a class name. Would it cause any issues to use a sub string and just grab the first 16 - 24 characters?

I like the idea of having a unique class per block but I think basing it on attributes or content is not necessarily, what we really want is just a unique class for this block any block support can use.

Good point, yes I agree, the objective is for a unique id for the instance of the block (in the context of rendering the supports), rather than verifying data integrity of the attributes/content. I wonder how else we might share an id / class name across function calls 🤔

@andrewserong could be possible to generate a unique id in the $block object in the server maybe?

oandregal · 2022-01-12T09:59:52Z

The approach here seems sensible to me, can't think of a scenario where it wouldn't work.

Just noting that it doesn't fix the issue completely, as it's twofold: it affects the front (fixed by this PR) and the editor where element rendering is not deterministic (needs a different PR).

lib/block-supports/elements.php

aristath · 2022-01-18T12:32:52Z

I see this one is on the 5.9 must-haves board.
Should it be merged and back ported today? Or moved to 5.9.1?

noisysocks · 2022-01-18T23:14:52Z

Let's aim for 5.9.1.

glendaviesnz · 2022-02-10T02:35:23Z

This tests well for me, and with multiple nesting levels:

Have rebased and pushed a fix for failing unit tests 🤞.

oandregal · 2022-02-10T10:47:31Z

Can we add a unit test for this behavior to make sure there are no regressions?

adamziel · 2022-04-04T13:20:39Z

👋 @scruffian I noticed this PR is on the WordPress 5.9.x board. Do you this could still land until Wednesday to be included in WordPress 6.0 release?

scruffian · 2022-04-13T16:00:47Z

lib/block-supports/elements.php

@@ -115,7 +115,8 @@ function gutenberg_render_elements_support_footer( $pre_render, $block ) {
 	$style = ".$class_name a{" . $link_color_declaration . ';}';

 	gutenberg_enqueue_block_support_styles( $style );
-	return $style;
+
+	return null;


I did that for the sake of the test...

I removed the test I suggested we should have. As you said, in this case, the value would lie in a e2e test. It'd be great to have it :)

oandregal

I've rebased and pushed a couple of fixes: do not output styles if skip serialization is true, do not short-circuit block render.

This works well for me and can't think of a scenario where it'd break, so I'm approving.

oandregal · 2022-04-13T16:08:30Z

I've also edited the issue description so #37582 is not closed. Note that the issue is twofold:

In the frontend, the paragraph's link color is overwritten by the group's link color always. Fixed by this PR.
Applying the link color is not deterministic in the editor. This needs a follow-up PR. Test the following: 1) apply the link color to the paragraph and then to the group and 2) apply the link color to the group and then to the paragraph. While both use cases should render the same results, atm, the last link color set wins.

youknowriad · 2022-04-14T08:19:16Z

lib/block-supports/elements.php

 }

 // Remove WordPress core filter to avoid rendering duplicate elements stylesheet.
 remove_filter( 'render_block', 'wp_render_elements_support', 10, 2 );
 add_filter( 'render_block', 'gutenberg_render_elements_support', 10, 2 );
+add_filter( 'pre_render_block', 'gutenberg_render_elements_support_styles', 10, 2 );


For me this PR raises an interesting question about the styles order. Is there any place where we want the block styles to be generated "child" first then "parent" later? In other words, this PR does invert the order for the "elements" styles but why not invert it for all kind of styles.

It seems it's something worth thinking about for the future in regards of the style engine work. cc @andrewserong @ramonjd

Is there any place where we want the block styles to be generated "child" first then "parent" later?

Thanks for the ping.

I suppose depending on how we output styles we might want to look at a mix of specificity and style rendering order.

Anyway, I've added a note to look at a complete inheritance modal over at the style engine tracking issue. 🙇

I believe it should be like this for all styles, to follow the pattern of CSS inheritance.

ndiego · 2022-04-25T10:55:23Z

@youknowriad and @oandregal is this PR still something we want to get into 6.0? If so great, I'll add this to the list of needed backports, but noticed the recent discussions. Let me know!

oandregal · 2022-04-25T11:19:58Z

@ndiego Yeah, it's a bugfix that should land in 6.0.

gziolo · 2022-04-25T13:20:10Z

Do we have a PR opened for changes in WordPress core?

oandregal · 2022-04-25T14:31:10Z

Do we have a PR opened for changes in WordPress core?

I've looked and haven't found one. I can help with this.

gziolo · 2022-04-25T14:54:40Z

This is going to be handled in WordPress/wordpress-develop#2624 for WordPress 6.0 Beta 3.

oandregal · 2022-04-25T14:57:58Z

Backport PR at WordPress/wordpress-develop#2624 and follow-up for Gutenberg at #40594

This fixes an issue by which link color behaves differently in the editor and front end. Related Gutenberg PR: WordPress/gutenberg#37728. Props oandregal. See #55567. git-svn-id: https://develop.svn.wordpress.org/trunk@53260 602fd350-edb4-49c9-b593-d223f7449a82

This fixes an issue by which link color behaves differently in the editor and front end. Related Gutenberg PR: WordPress/gutenberg#37728. Props oandregal. See #55567. Built from https://develop.svn.wordpress.org/trunk@53260 git-svn-id: http://core.svn.wordpress.org/trunk@52849 1a063a9b-81f0-0310-95a4-ce76da25c4cd

This fixes an issue by which link color behaves differently in the editor and front end. Related Gutenberg PR: WordPress/gutenberg#37728. Props oandregal. See #55567. Built from https://develop.svn.wordpress.org/trunk@53260 git-svn-id: https://core.svn.wordpress.org/trunk@52849 1a063a9b-81f0-0310-95a4-ce76da25c4cd

scruffian added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Jan 5, 2022

scruffian requested review from oandregal, mikachan and jorgefilipecosta January 5, 2022 15:24

scruffian requested review from spacedmonkey and TimothyBJacobs as code owners January 5, 2022 15:24

scruffian self-assigned this Jan 5, 2022

scruffian mentioned this pull request Jan 5, 2022

Change the priority of the block supports styles #37593

Closed

3 tasks

mikachan reviewed Jan 6, 2022

View reviewed changes

lib/block-supports/elements.php Outdated Show resolved Hide resolved

mikachan approved these changes Jan 7, 2022

View reviewed changes

scruffian requested review from youknowriad and andrewserong January 7, 2022 13:04

youknowriad requested review from aristath and ellatrix January 7, 2022 13:18

youknowriad requested a review from gziolo January 7, 2022 13:22

andrewserong reviewed Jan 10, 2022

View reviewed changes

oandregal reviewed Jan 12, 2022

View reviewed changes

lib/block-supports/elements.php Outdated Show resolved Hide resolved

scruffian force-pushed the fix/37582-again branch from 6d6b142 to 5d55dc2 Compare January 13, 2022 17:10

noisysocks added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Jan 18, 2022

glendaviesnz force-pushed the fix/37582-again branch from 711d8f5 to 06788a7 Compare February 10, 2022 02:33

glendaviesnz force-pushed the fix/37582-again branch from 3028707 to f8399f4 Compare February 10, 2022 04:03

scruffian and others added 6 commits April 13, 2022 17:19

consolidate code from rebase

4c65449

Try adding a unit test

4b8f820

fix test

653aede

Do not short-circuit block rendering

92cc1ab

Rename for clarity

e524d1d

Do not output styles if skipSerialization is true

f67438c

oandregal force-pushed the fix/37582-again branch from 58d8251 to f67438c Compare April 13, 2022 15:57

scruffian commented Apr 13, 2022

View reviewed changes

oandregal approved these changes Apr 13, 2022

View reviewed changes

Remove unnecessary test

6aca09e

oandregal added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Apr 13, 2022

scruffian merged commit 4e31a43 into trunk Apr 13, 2022

scruffian deleted the fix/37582-again branch April 13, 2022 22:47

github-actions bot added this to the Gutenberg 13.1 milestone Apr 13, 2022

youknowriad reviewed Apr 14, 2022

View reviewed changes

gziolo removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Apr 25, 2022

oandregal mentioned this pull request Apr 25, 2022

Fix styles for nested elements (link color) WordPress/wordpress-develop#2624

Closed

gziolo removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Apr 25, 2022

oandregal mentioned this pull request Apr 25, 2022

Element styles: prevent the WordPress filter callback from being executed #40594

Merged

ockham mentioned this pull request Aug 26, 2022

Plugin: Backport PHP changes for WordPress 6.1 release #43440

Closed

37 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Elements: Add styles to the footer before the block is rendered #37728

Elements: Add styles to the footer before the block is rendered #37728

scruffian commented Jan 5, 2022 •

edited by oandregal

Loading

mikachan left a comment

mikachan left a comment

youknowriad commented Jan 7, 2022

scruffian commented Jan 7, 2022

andrewserong left a comment

andrewserong Jan 10, 2022

youknowriad Jan 10, 2022

andrewserong Jan 10, 2022

youknowriad Jan 11, 2022

oandregal commented Jan 12, 2022

aristath commented Jan 18, 2022 •

edited

Loading

noisysocks commented Jan 18, 2022

glendaviesnz commented Feb 10, 2022

oandregal commented Feb 10, 2022

adamziel commented Apr 4, 2022

scruffian Apr 13, 2022

oandregal Apr 13, 2022

oandregal left a comment

oandregal commented Apr 13, 2022

youknowriad Apr 14, 2022

ramonjd Apr 18, 2022

scruffian Apr 20, 2022

ndiego commented Apr 25, 2022

oandregal commented Apr 25, 2022

gziolo commented Apr 25, 2022

oandregal commented Apr 25, 2022

gziolo commented Apr 25, 2022 •

edited

Loading

oandregal commented Apr 25, 2022

Elements: Add styles to the footer before the block is rendered #37728

Elements: Add styles to the footer before the block is rendered #37728

Conversation

scruffian commented Jan 5, 2022 • edited by oandregal Loading

What

Why

How to test

mikachan left a comment

Choose a reason for hiding this comment

mikachan left a comment

Choose a reason for hiding this comment

youknowriad commented Jan 7, 2022

scruffian commented Jan 7, 2022

andrewserong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal commented Jan 12, 2022

aristath commented Jan 18, 2022 • edited Loading

noisysocks commented Jan 18, 2022

glendaviesnz commented Feb 10, 2022

oandregal commented Feb 10, 2022

adamziel commented Apr 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal left a comment

Choose a reason for hiding this comment

oandregal commented Apr 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndiego commented Apr 25, 2022

oandregal commented Apr 25, 2022

gziolo commented Apr 25, 2022

oandregal commented Apr 25, 2022

gziolo commented Apr 25, 2022 • edited Loading

oandregal commented Apr 25, 2022

scruffian commented Jan 5, 2022 •

edited by oandregal

Loading

aristath commented Jan 18, 2022 •

edited

Loading

gziolo commented Apr 25, 2022 •

edited

Loading