Allow selector ordering to ensure theme.json root selector margin takes precedence #36960

getdave · 2021-11-29T13:23:16Z

Description

Companion to Core patch https://core.trac.wordpress.org/ticket/54550.

(originally an alternative approach to #36958).

Note that as with #36958 this PR ~~also includes changes from #36957 which make it possible for you to test this PR. They will be removed prior to merge~~ requires you to test against WordPress 5.8 (that's five point EIGHT not NINE). See #36957 (comment) for reasons.

To use WP 5.8 add this to your .wp-env.override.json file:

{
    "core": "WordPress/WordPress#5.8.2"
}

Closes #36147

How has this been tested?

Follow same instructions as #36958.

But this time you should see that the body { margin: 0; } declaration is still output but is overridden by values from theme.json if they are found.

Basically the default margin: 0 is still applied but anything in theme.json will win out.

I believe this is the best approach because

It is limited in scope.
It retains the original margin: 0 on body which Themes have come to expect thus avoiding backwards compat issues.
It caters for both types of margin notation (i.e. string or object) that can be applied in theme.json.

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

getdave · 2021-11-29T13:27:05Z

lib/class-wp-theme-json-gutenberg.php

@@ -1059,7 +1076,7 @@ private function get_block_classes( $style_nodes ) {
 }

 if ( self::ROOT_BLOCK_SELECTOR === $selector ) {
- $block_rules .= 'body { margin: 0; }';


Avoid applying rule here otherwise it will end up being applied last in the selector order thereby ensuring it takes precedence over rules applied via theme.json.

getdave · 2021-11-29T13:27:39Z

lib/class-wp-theme-json-gutenberg.php

+ // `margin` in its `spacing` declaration for the `body` element then these
+ // user-generated values take precedence in the CSS cascade.
+ // See: https://github.com/WordPress/gutenberg/issues/36147.
+ if ( self::ROOT_BLOCK_SELECTOR === $selector ) {


By applying the margin: 0 rule early we ensure that any values provided from theme.json will be later in the selector order and thus take precedence over the default.

getdave · 2021-11-29T13:32:03Z

cc @overclokk as you raised the original Issue.

ntsekouras

Thanks for the PRs Dave! I tested and work as expected.

I think I prefer this PR from the conditional zeroing of margin - also in the other approach we should also take into account the format of margin:{top: '10px'} etc.

I'm just wondering if it worths to call the functions again, instead of just

if ( self::ROOT_BLOCK_SELECTOR === $selector ) {
	$block_rules .= 'body { margin: 0; }';
}

before the generation of the styles.

getdave · 2021-11-29T14:19:09Z

I'm just wondering if it worths to call the functions again, instead of just
if ( self::ROOT_BLOCK_SELECTOR === $selector ) {
	$block_rules .= 'body { margin: 0; }';
}
before the generation of the styles.

I did this because it was putting the selectors on the same line within the outputted code so I thought perhaps it was safer to reuse the function.

overclokk · 2021-11-29T15:48:10Z

I was thinking why not adding that value as default in theme.json in core, so the theme can override that vales without an if statement

getdave · 2021-11-30T09:21:00Z

In light of #36957 (comment) I've removed the fix I cherrypicked from #36957.

You now need to test this PR against WordPress 5.8.

getdave · 2021-11-30T09:37:00Z

I was thinking why not adding that value as default in theme.json in core, so the theme can override that vales without an if statement

Responded in #36958 (comment).

ntsekouras

Thanks Dave for your work here! IMO this fix can be landed and discuss afterwards a different approach that would potentially change where we apply all the root styles from theme.json, given the WP release phase we are right now.

I'd give it a bit more time before merging if anyone else wants to chime in - cc @youknowriad

lib/class-wp-theme-json-gutenberg.php

Addresses #36960 (comment)

youknowriad · 2021-12-01T07:32:26Z

lib/class-wp-theme-json-gutenberg.php

@@ -724,7 +734,6 @@ private function get_block_classes( $style_nodes ) {
 }

 if ( self::ROOT_BLOCK_SELECTOR === $selector ) {


Makes me wonder if all the content of this condition should be above but it's good regardless.

getdave · 2021-12-01T09:18:23Z

Ok then folks, it seems the majority are agreed that this PR is the approach we want to pursue.

I will close out #36996 and then make a Core patch for the changes in this PR.

Addresses #36960 (comment)

Also avoids creating failing unit tests due to a single space character changing in a snaphshot!

getdave · 2021-12-01T15:39:48Z

Companion Core patch in https://core.trac.wordpress.org/ticket/54550 cc @ntsekouras

See WordPress/wordpress-develop#1989

getdave · 2021-12-09T14:37:44Z

Just fixing the PHP unit tests prior to merging.

…es precedence (#36960) * Set default margin on body prior to generating styles from theme.json * Add additional context to comment Addresses #36960 (comment) * Restore simple version of body rule Also avoids creating failing unit tests due to a single space character changing in a snaphshot! * Update with fixes made in Core patch See WordPress/wordpress-develop#1989 * Fix output now includes new line. * Fix tests relating to spacing * Fix test to put body CSS reset first

getdave mentioned this pull request Nov 29, 2021

Only apply default margin from global styles if root body does not have margin set in theme.json #36958

Closed

7 tasks

getdave added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Nov 29, 2021

getdave self-assigned this Nov 29, 2021

getdave requested review from oandregal and ntsekouras November 29, 2021 13:25

getdave commented Nov 29, 2021

View reviewed changes

getdave marked this pull request as ready for review November 29, 2021 13:27

getdave requested review from spacedmonkey and TimothyBJacobs as code owners November 29, 2021 13:27

getdave 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 Nov 29, 2021

ntsekouras reviewed Nov 29, 2021

View reviewed changes

getdave force-pushed the fix/alt-default-theme-json-body-margin branch from ad82e53 to b3b02dc Compare November 30, 2021 09:19

getdave force-pushed the fix/alt-default-theme-json-body-margin branch from b3b02dc to a66fb10 Compare November 30, 2021 09:33

ntsekouras approved these changes Nov 30, 2021

View reviewed changes

lib/class-wp-theme-json-gutenberg.php Outdated Show resolved Hide resolved

getdave added a commit that referenced this pull request Nov 30, 2021

Add additional context to comment

f7ed117

Addresses #36960 (comment)

getdave mentioned this pull request Nov 30, 2021

Move the root body margin reset into the Core theme.json #36996

Closed

7 tasks

youknowriad reviewed Dec 1, 2021

View reviewed changes

getdave added 3 commits December 1, 2021 15:15

Set default margin on body prior to generating styles from theme.json

09c052b

Add additional context to comment

8c827ca

Addresses #36960 (comment)

Restore simple version of body rule

7c2969d

Also avoids creating failing unit tests due to a single space character changing in a snaphshot!

getdave force-pushed the fix/alt-default-theme-json-body-margin branch from 23acf16 to 7c2969d Compare December 1, 2021 15:15

getdave mentioned this pull request Dec 1, 2021

Generate global styles body reset rule prior to processes theme.json WordPress/wordpress-develop#1989

Closed

youknowriad approved these changes Dec 3, 2021

View reviewed changes

getdave added 4 commits December 9, 2021 13:58

Update with fixes made in Core patch

8132d20

See WordPress/wordpress-develop#1989

Fix output now includes new line.

6afbc51

Fix tests relating to spacing

4b7b15a

Fix test to put body CSS reset first

0a52906

getdave merged commit 30aab8a into trunk Dec 9, 2021

getdave deleted the fix/alt-default-theme-json-body-margin branch December 9, 2021 15:50

github-actions bot added this to the Gutenberg 12.2 milestone Dec 9, 2021

getdave mentioned this pull request Dec 9, 2021

The margin added at the top level styles (for body) in theme.json is override by a margin added from gutenberg #36147

Closed

noisysocks 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 Dec 13, 2021

noisysocks mentioned this pull request Dec 13, 2021

Update @wordpress packages WordPress/wordpress-develop#2048

Closed

andrewserong mentioned this pull request Sep 8, 2022

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow selector ordering to ensure theme.json root selector margin takes precedence #36960

Allow selector ordering to ensure theme.json root selector margin takes precedence #36960

getdave commented Nov 29, 2021 •

edited

Loading

getdave Nov 29, 2021

getdave Nov 29, 2021

getdave commented Nov 29, 2021

ntsekouras left a comment

getdave commented Nov 29, 2021

overclokk commented Nov 29, 2021 •

edited

Loading

getdave commented Nov 30, 2021

getdave commented Nov 30, 2021

ntsekouras left a comment

youknowriad Dec 1, 2021

getdave commented Dec 1, 2021

getdave commented Dec 1, 2021

getdave commented Dec 9, 2021

		@@ -724,7 +734,6 @@ private function get_block_classes( $style_nodes ) {
		}

		if ( self::ROOT_BLOCK_SELECTOR === $selector ) {

Allow selector ordering to ensure theme.json root selector margin takes precedence #36960

Allow selector ordering to ensure theme.json root selector margin takes precedence #36960

Conversation

getdave commented Nov 29, 2021 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

getdave Nov 29, 2021

Choose a reason for hiding this comment

getdave Nov 29, 2021

Choose a reason for hiding this comment

getdave commented Nov 29, 2021

ntsekouras left a comment

Choose a reason for hiding this comment

getdave commented Nov 29, 2021

overclokk commented Nov 29, 2021 • edited Loading

getdave commented Nov 30, 2021

getdave commented Nov 30, 2021

ntsekouras left a comment

Choose a reason for hiding this comment

youknowriad Dec 1, 2021

Choose a reason for hiding this comment

getdave commented Dec 1, 2021

getdave commented Dec 1, 2021

getdave commented Dec 9, 2021

getdave commented Nov 29, 2021 •

edited

Loading

overclokk commented Nov 29, 2021 •

edited

Loading