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

Global Styles: Split root layout rules into a different function #43792

Merged
merged 13 commits into from
Sep 12, 2022

Conversation

scruffian
Copy link
Contributor

@scruffian scruffian commented Sep 1, 2022

What?

This refactors the code that generates Global Styles CSS to put the rules that relate specifically to the root block layouts into a different function.

Why?

Since this code only works for root blocks, putting it into a different function keeps concerns separate and makes it easier to follow the code.

How?

Creating a new function called get_root_layout_rules

Testing Instructions

Check that layouts still work as before.

@scruffian scruffian added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Sep 1, 2022
@scruffian scruffian self-assigned this Sep 1, 2022
@priethor priethor added the [Type] Regression Related to a regression in the latest release label Sep 2, 2022
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 opening this PR @scruffian! There's a couple of small bugs, but overall I like the idea of refactoring to a separate function for the root rules (I think @tellthemachines might have brought that up in the past, too), since we've got quite a few of them, and this does appear to fix the raised issue for me in testing:

image

Something I'm wondering about, though, is whether the logic of the root rules being rendered after block rules is the right approach — when a user intentionally enters margin rules in theme.json, should those be allowed to override the layout provided owl selector rules (I'm mostly thinking about the issue described in #43404)?

I added a comment over on the other PR about a proposed ideal order of specificity (#41689 (comment)), so I was wondering if in the ideal state the default margin rules for blocks would be lower (e.g. via a :where) rather than having the same specificity as styles that have been added at the block level by a theme developer in theme.json?

Since the goal is ultimately to fix the current bug, I wouldn't be opposed to trying to land the approach here, and us potentially revisiting how all this works (order of specificity) separately.

I'll be travelling for the next few days, but can give this a re-test next week if no-one beats me to it.


$root_block_key = array_search( static::ROOT_BLOCK_SELECTOR, array_column( $style_nodes, 'selector' ) );
$stylesheet .= $this->get_additional_root_rules( static::ROOT_BLOCK_SELECTOR, $style_nodes[ $root_block_key ] );
var_dump( $stylesheet );
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this one needs to be removed 🙂

@scruffian
Copy link
Contributor Author

Thanks @andrewserong you raise some good points. I wonder now what is the purpose of even having the margin values set on these blocks if we're just going to override them anyway. As you say, this solution actually creates a different issue - we can no longer override the margins via theme.json as the layout selector takes precedence. 🤔

@jasmussen
Copy link
Contributor

Thanks for exploring solutions, this is a tricky one!

I wonder now what is the purpose of even having the margin values set on these blocks if we're just going to override them anyway.

The Image and Embed blocks are difficult because they are wrapped in a figure element which comes with intrinsic margins, notably left and right, which are often problematic. I believe those intrinsic margins are the reason we've always had some built-in stylings for those margins. But as you say, they need to be easily overridden.

Could the margin move back to style.scss? It seems like that would provide the baseline, but still be overridden both by theme.json and owl selectors?

Alternatively, could/should theme.json provide defaults for only left and right margins, rather than all four directions?

@scruffian
Copy link
Contributor Author

Here's an alternative fix: #43813

@scruffian scruffian changed the title Block Editor: Change CSS load order Global Styles: Split root layout rules into a different function Sep 7, 2022
@scruffian
Copy link
Contributor Author

I fixed this so that it's now focussed on the refactoring element of the work and updated the title and description accordingly.

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 updating @scruffian! Overall I like the direction of the refactor, I think it'll improve readability and make it a bit easier when we need to add additional root rules.

I ran into one blocking issue, and added a comment about moving over the remaining root static::ROOT_BLOCK_SELECTOR rules from get_styles_for_block to this new function, which should help resolve a subtle regression 🤞

Happy to test further tomorrow!

$stylesheet .= $this->get_block_classes( $style_nodes );

$root_block_key = array_search( static::ROOT_BLOCK_SELECTOR, array_column( $style_nodes, 'selector' ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

This line needs to be moved to before the $stylesheet .= $this->get_root_layout_rules line above, otherwise the layout rules aren't output.

There should always be an element for the root block selector, but I was wondering if we should also add an if ( false !== $root_block_key ) check before calling get_root_layout_rules in case a match isn't found?

* @param array $block_metadata The metadata for the root block.
* @return string The additional root rules CSS.
*/
private function get_root_layout_rules( $selector, $block_metadata ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function appears to be working well for me in testing. Reading over the function, the logic looks a little split to me in the current state of the PR, given that the root layout content and wide size rules are still contained in get_styles_for_block. Would it be worth moving over the remaining lines from that function? (The block starting from around line 791 with if ( static::ROOT_BLOCK_SELECTOR === $selector ) {), here:

if ( static::ROOT_BLOCK_SELECTOR === $selector ) {
/*
* Reset default browser margin on the root body element.
* This is set on the root selector **before** generating the ruleset
* from the `theme.json`. This is to ensure that if the `theme.json` declares
* `margin` in its `spacing` declaration for the `body` element then these
* user-generated values take precedence in the CSS cascade.
* @link https://github.com/WordPress/gutenberg/issues/36147.
*/
$block_rules .= 'body { margin: 0;';
/*
* If there are content and wide widths in theme.json, output them
* as custom properties on the body element so all blocks can use them.
*/
if ( isset( $settings['layout']['contentSize'] ) || isset( $settings['layout']['wideSize'] ) ) {
$content_size = isset( $settings['layout']['contentSize'] ) ? $settings['layout']['contentSize'] : $settings['layout']['wideSize'];
$content_size = static::is_safe_css_declaration( 'max-width', $content_size ) ? $content_size : 'initial';
$wide_size = isset( $settings['layout']['wideSize'] ) ? $settings['layout']['wideSize'] : $settings['layout']['contentSize'];
$wide_size = static::is_safe_css_declaration( 'max-width', $wide_size ) ? $wide_size : 'initial';
$block_rules .= '--wp--style--global--content-size: ' . $content_size . ';';
$block_rules .= '--wp--style--global--wide-size: ' . $wide_size . ';';
}
$block_rules .= '}';
}

I think doing that would also resolve a subtle regression I just noticed that merged a while back. Ideally the body { margin: 0; } rule should be output before the spacing values for margin that can be set at the root styles.spacing.margin level in theme.json (originally fixed back in #36960 to resolve #36147).

@scruffian
Copy link
Contributor Author

Thanks for the review, you were right that does fix the issue with the body margin as well. Nice work! I've updated the PR.

@andrewserong
Copy link
Contributor

andrewserong commented Sep 8, 2022

Thanks @scruffian! Looks like there were a couple of tiny things that needed updating so I've pushed a commit in 4720b61 (hope you don't mind). I think we then just need to fix up the tests and it 🤞 should be good to go, I think. I'll see if I can squeeze in a bit more time to do more testing on this later today.

Update: I think some of the failing tests will need to be updated as they're testing get_styles_for_block directly rather than the parent get_stylesheet 🤔

@andrewserong
Copy link
Contributor

Also, just linking in a semi-related PR. @tellthemachines is looking to update the root padding rules in #43842, so we might want to land that PR before this one so that we can factor in those changes?

@scruffian scruffian force-pushed the fix/editor-css-load-order branch from 4720b61 to ac96cd5 Compare September 9, 2022 16:26
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 updates @scruffian, this is testing nicely for me now. LGTM! 🎉

@scruffian
Copy link
Contributor Author

Thanks for merging!

pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Sep 14, 2022
This change backports the following changes from Gutenberg repository:

- [WordPress/gutenberg#40875 gutenberg#40875] Layout: Use semantic classnames, centralize layout definitions, reduce duplication, and fix blockGap in theme.json
- [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544
- [WordPress/gutenberg#42087 gutenberg#42087] Theme.json: Add block support feature level selectors for blocks gutenberg#42087
- [WordPress/gutenberg#43792 gutenberg#43792] Global Styles: Split root layout rules into a different function gutenberg#43792
- [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544
- [WordPress/gutenberg#42665 gutenberg#42665] Layout: Reduce specificity of fallback blockGap styles gutenberg#42665
- [WordPress/gutenberg#42085 gutenberg#42085] Core CSS support for root padding and alignfull blocks gutenberg#42085

Note that it doesn't entirely port over PR40875 — the remaining PHP changes for that PR will be explored in a separate PR targeting `layout.php`.

Props andrewserong, aaronrobertshaw, isabel_brison.
See #56467.


git-svn-id: https://develop.svn.wordpress.org/trunk@54159 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Sep 14, 2022
This change backports the following changes from Gutenberg repository:

- [WordPress/gutenberg#40875 gutenberg#40875] Layout: Use semantic classnames, centralize layout definitions, reduce duplication, and fix blockGap in theme.json
- [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544
- [WordPress/gutenberg#42087 gutenberg#42087] Theme.json: Add block support feature level selectors for blocks gutenberg#42087
- [WordPress/gutenberg#43792 gutenberg#43792] Global Styles: Split root layout rules into a different function gutenberg#43792
- [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544
- [WordPress/gutenberg#42665 gutenberg#42665] Layout: Reduce specificity of fallback blockGap styles gutenberg#42665
- [WordPress/gutenberg#42085 gutenberg#42085] Core CSS support for root padding and alignfull blocks gutenberg#42085

Note that it doesn't entirely port over PR40875 — the remaining PHP changes for that PR will be explored in a separate PR targeting `layout.php`.

Props andrewserong, aaronrobertshaw, isabel_brison.
See #56467.

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


git-svn-id: http://core.svn.wordpress.org/trunk@53718 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to gilzow/wordpress-performance that referenced this pull request Sep 14, 2022
This change backports the following changes from Gutenberg repository:

- [WordPress/gutenberg#40875 gutenberg#40875] Layout: Use semantic classnames, centralize layout definitions, reduce duplication, and fix blockGap in theme.json
- [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544
- [WordPress/gutenberg#42087 gutenberg#42087] Theme.json: Add block support feature level selectors for blocks gutenberg#42087
- [WordPress/gutenberg#43792 gutenberg#43792] Global Styles: Split root layout rules into a different function gutenberg#43792
- [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544
- [WordPress/gutenberg#42665 gutenberg#42665] Layout: Reduce specificity of fallback blockGap styles gutenberg#42665
- [WordPress/gutenberg#42085 gutenberg#42085] Core CSS support for root padding and alignfull blocks gutenberg#42085

Note that it doesn't entirely port over PR40875 — the remaining PHP changes for that PR will be explored in a separate PR targeting `layout.php`.

Props andrewserong, aaronrobertshaw, isabel_brison.
See #56467.

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


git-svn-id: https://core.svn.wordpress.org/trunk@53718 1a063a9b-81f0-0310-95a4-ce76da25c4cd
whereiscodedude pushed a commit to whereiscodedude/wpss that referenced this pull request Sep 18, 2022
This change backports the following changes from Gutenberg repository:

- [WordPress/gutenberg#40875 gutenberg#40875] Layout: Use semantic classnames, centralize layout definitions, reduce duplication, and fix blockGap in theme.json
- [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544
- [WordPress/gutenberg#42087 gutenberg#42087] Theme.json: Add block support feature level selectors for blocks gutenberg#42087
- [WordPress/gutenberg#43792 gutenberg#43792] Global Styles: Split root layout rules into a different function gutenberg#43792
- [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544
- [WordPress/gutenberg#42665 gutenberg#42665] Layout: Reduce specificity of fallback blockGap styles gutenberg#42665
- [WordPress/gutenberg#42085 gutenberg#42085] Core CSS support for root padding and alignfull blocks gutenberg#42085

Note that it doesn't entirely port over PR40875 — the remaining PHP changes for that PR will be explored in a separate PR targeting `layout.php`.

Props andrewserong, aaronrobertshaw, isabel_brison.
See #56467.

Built from https://develop.svn.wordpress.org/trunk@54159
ootwch pushed a commit to ootwch/wordpress-develop that referenced this pull request Nov 4, 2022
This change backports the following changes from Gutenberg repository:

- [WordPress/gutenberg#40875 gutenberg#40875] Layout: Use semantic classnames, centralize layout definitions, reduce duplication, and fix blockGap in theme.json
- [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544
- [WordPress/gutenberg#42087 gutenberg#42087] Theme.json: Add block support feature level selectors for blocks gutenberg#42087
- [WordPress/gutenberg#43792 gutenberg#43792] Global Styles: Split root layout rules into a different function gutenberg#43792
- [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544
- [WordPress/gutenberg#42665 gutenberg#42665] Layout: Reduce specificity of fallback blockGap styles gutenberg#42665
- [WordPress/gutenberg#42085 gutenberg#42085] Core CSS support for root padding and alignfull blocks gutenberg#42085

Note that it doesn't entirely port over PR40875 — the remaining PHP changes for that PR will be explored in a separate PR targeting `layout.php`.

Props andrewserong, aaronrobertshaw, isabel_brison.
See #56467.


git-svn-id: https://develop.svn.wordpress.org/trunk@54159 602fd350-edb4-49c9-b593-d223f7449a82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants