Global styles: output :root selector for CSS custom properties #42084
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ramonjd
added
Global Styles
andrewserong
reviewed
Jul 1, 2022
Could we get some input from @jorgefilipecosta as to why we changed to |
Thanks @aristath and @getdave I wasn't aware of those PRs. 🙇 I'm happy to change it back to body if we can ditch at least one superfluous body tag with the margin reset 😄 |
ramonjd
force-pushed
the
update/global-styles-root-margin-and-selector
branch
from
e3ed5d5 to
b2283d5
Compare
|
@ramonjd Do any of the current tests actually assert on the change in this PR - namely to remove the superfluous |
Thanks for checking @getdave I had to update a few existing tests to combine the body tag declarations, where before the extra body tag was expected. For example this one: Is that what you mean? |
|
I'm happy to remove the migration to Merging the hanging body tag with the What do folks think? |
| $node = _wp_array_get( $this->theme_json, $metadata['path'], array() ); | ||
| $declarations = array_merge( static::compute_preset_vars( $node, $origins ), static::compute_theme_vars( $node ) ); | ||
|
|
||
| $stylesheet .= static::to_ruleset( $selector, $declarations ); |
There was a problem hiding this comment.
The reason we moved in #31302 from :root to body selector was to allow things like margin and padding on the root. This PR uses a different selector for the CSS variables and for the styles I guess it is ok to do that. But in that case, I guess we should have a constant close to ROOT_BLOCK_SELECTOR.
There was a problem hiding this comment.
See also #35840 which goes into detail on this one. WP_Theme_JSON::ROOT_BLOCK_SELECTOR is already in core.
I can't think of any reason why setting CSS custom properties on :root would mean that margin and padding on the body wouldn't work; the two requirements aren't related, as far as I can see.
Moving the definitions from body to :root would only raise one concern, as far as I can see: the code which assigns the definitions would have to be on :root in the front end and on .editor-styles-wrapper (NOT :root!) in the editor.
|
See #42084 (comment). |
|
Closing this as stale and not |
|
@ramonjd See #31302 (comment) for further details of why this issue remains important. |
This was referenced Aug 12, 2022
|
@markhowellsmead Thanks the for the ping! I'll reopen and retest next week. 👍 |
ramonjd
force-pushed
the
update/global-styles-root-margin-and-selector
branch
from
605647e to
1660e5e
Compare
ramonjd
changed the title
|
Size Change: +413 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
ramonjd
commented
Mar 19, 2024
| @@ -2702,11 +2701,19 @@ public function get_root_layout_rules( $selector, $block_metadata ) { | |||
| $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'; | |||
| $css .= '--wp--style--global--content-size: ' . $content_size . ';'; | |||
| $css .= '--wp--style--global--wide-size: ' . $wide_size . ';'; | |||
| $css .= static::ROOT_CSS_PROPERTIES_SELECTOR . ' { --wp--style--global--content-size: ' . $content_size . ';'; | |||
There was a problem hiding this comment.
One thing I've been playing with locally is moving the $use_root_padding logic from compute_style_properties to get_root_layout_rules.
This entails:
- deleting the
--wp--root--padding-*entries fromPROPERTIES_METADATA - in
compute_style_properties, skipping padding properties for the root block if the theme has opted into using the root padding, and removing all other padding related logic - building the rules in
get_root_layout_rules.
I haven't even considered backwards compat yet 😄
Here's a diff to illustrate what I mean:
diff --git a/lib/class-wp-theme-json-gutenberg.php b/lib/class-wp-theme-json-gutenberg.php
index e3c27ac82c..48b2493fe2 100644
--- a/lib/class-wp-theme-json-gutenberg.php
+++ b/lib/class-wp-theme-json-gutenberg.php
@@ -266,11 +266,6 @@ class WP_Theme_JSON_Gutenberg {
'padding-right' => array( 'spacing', 'padding', 'right' ),
'padding-bottom' => array( 'spacing', 'padding', 'bottom' ),
'padding-left' => array( 'spacing', 'padding', 'left' ),
- '--wp--style--root--padding' => array( 'spacing', 'padding' ),
- '--wp--style--root--padding-top' => array( 'spacing', 'padding', 'top' ),
- '--wp--style--root--padding-right' => array( 'spacing', 'padding', 'right' ),
- '--wp--style--root--padding-bottom' => array( 'spacing', 'padding', 'bottom' ),
- '--wp--style--root--padding-left' => array( 'spacing', 'padding', 'left' ),
'text-decoration' => array( 'typography', 'textDecoration' ),
'text-transform' => array( 'typography', 'textTransform' ),
'filter' => array( 'filter', 'duotone' ),
@@ -2092,6 +2087,7 @@ class WP_Theme_JSON_Gutenberg {
* @since 5.9.0 Added the `$settings` and `$properties` parameters.
* @since 6.1.0 Added `$theme_json`, `$selector`, and `$use_root_padding` parameters.
* @since 6.5.0 Passing current theme JSON settings to wp_get_typography_font_size_value().
+ * @since 6.6.0 Root padding CSS custom properties are processed in get_root_layout_rules().
*
* @param array $styles Styles to process.
* @param array $settings Theme settings.
@@ -2111,28 +2107,18 @@ class WP_Theme_JSON_Gutenberg {
return $declarations;
}
- $root_variable_duplicates = array();
-
foreach ( $properties as $css_property => $value_path ) {
$value = static::get_property_value( $styles, $value_path, $theme_json );
- if ( str_starts_with( $css_property, '--wp--style--root--' ) && ( static::ROOT_BLOCK_SELECTOR !== $selector || ! $use_root_padding ) ) {
- continue;
- }
- // Root-level padding styles don't currently support strings with CSS shorthand values.
- // This may change: https://github.com/WordPress/gutenberg/issues/40132.
- if ( '--wp--style--root--padding' === $css_property && is_string( $value ) ) {
- continue;
- }
-
- if ( str_starts_with( $css_property, '--wp--style--root--' ) && $use_root_padding ) {
- $root_variable_duplicates[] = substr( $css_property, strlen( '--wp--style--root--' ) );
- }
-
- // Look up protected properties, keyed by value path.
- // Skip protected properties that are explicitly set to `null`.
if ( is_array( $value_path ) ) {
$path_string = implode( '.', $value_path );
+ // Skip padding properties for the root block if the theme has opted into using the root padding.
+ // The CSS custom properties for root padding are processed in get_root_layout_rules() and use static::ROOT_CSS_PROPERTIES_SELECTOR selector.
+ if ( static::ROOT_BLOCK_SELECTOR === $selector && $use_root_padding && str_starts_with( $path_string, 'spacing.padding.' ) ) {
+ continue;
+ }
+ // Look up protected properties, keyed by value path.
+ // Skip protected properties that are explicitly set to `null`.
if (
isset( static::PROTECTED_PROPERTIES[ $path_string ] ) &&
_wp_array_get( $settings, static::PROTECTED_PROPERTIES[ $path_string ], null ) === null
@@ -2181,14 +2167,6 @@ class WP_Theme_JSON_Gutenberg {
);
}
- // If a variable value is added to the root, the corresponding property should be removed.
- foreach ( $root_variable_duplicates as $duplicate ) {
- $discard = array_search( $duplicate, array_column( $declarations, 'name' ), true );
- if ( is_numeric( $discard ) ) {
- array_splice( $declarations, $discard, 1 );
- }
- }
-
return $declarations;
}
@@ -2688,9 +2666,10 @@ class WP_Theme_JSON_Gutenberg {
* @return string The additional root rules CSS.
*/
public function get_root_layout_rules( $selector, $block_metadata ) {
- $css = '';
- $settings = $this->theme_json['settings'] ?? array();
- $use_root_padding = isset( $this->theme_json['settings']['useRootPaddingAwareAlignments'] ) && true === $this->theme_json['settings']['useRootPaddingAwareAlignments'];
+ $css = '';
+ $settings = $this->theme_json['settings'] ?? array();
+ $use_root_padding = isset( $this->theme_json['settings']['useRootPaddingAwareAlignments'] ) && true === $this->theme_json['settings']['useRootPaddingAwareAlignments'];
+ $root_declarations = array();
/*
* If there are content and wide widths in theme.json, output them
@@ -2701,8 +2680,37 @@ class WP_Theme_JSON_Gutenberg {
$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';
- $css .= static::ROOT_CSS_PROPERTIES_SELECTOR . ' { --wp--style--global--content-size: ' . $content_size . ';';
- $css .= '--wp--style--global--wide-size: ' . $wide_size . '; }';
+ $root_declarations[] = array(
+ 'name' => '--wp--style--global--content-size',
+ 'value' => $content_size,
+ );
+ $root_declarations[] = array(
+ 'name' => '--wp--style--global--wide-size',
+ 'value' => $wide_size,
+ );
+ }
+
+ if ( $use_root_padding ) {
+ $root_padding = array(
+ '--wp--style--root--padding-top' => static::get_property_value( $this->theme_json, array( 'styles', 'spacing', 'padding', 'top' ) ),
+ '--wp--style--root--padding-right' => static::get_property_value( $this->theme_json, array( 'styles', 'spacing', 'padding', 'right' ) ),
+ '--wp--style--root--padding-bottom' => static::get_property_value( $this->theme_json, array( 'styles', 'spacing', 'padding', 'bottom' ) ),
+ '--wp--style--root--padding-left' => static::get_property_value( $this->theme_json, array( 'styles', 'spacing', 'padding', 'left' ) ),
+ );
+
+ $root_padding = array_filter( $root_padding, 'strlen' );
+ if ( ! empty( $root_padding ) ) {
+ foreach ( $root_padding as $property => $value ) {
+ $root_declarations[] = array(
+ 'name' => $property,
+ 'value' => $value,
+ );
+ }
+ }
+ }
+
+ if ( ! empty( $root_declarations ) ) {
+ $css .= static::to_ruleset( static::ROOT_CSS_PROPERTIES_SELECTOR, $root_declarations );
}
/*There was a problem hiding this comment.
I think we should do the above, but maybe in another PR, especially if we have to test for backwards compat.
There was a problem hiding this comment.
Follow-up PR sounds like a good idea 👍
packages/block-editor/src/components/global-styles/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for tackling this one @ramonjd 👍
It's testing nicely for me so far and I haven't spotted any regressions.
✅ JS & PHP unit tests are passing (thanks for keeping these updated)
✅ Tested with themes; 2024, 2023, 2022, 2021, and 2020
✅ Custom colors added to the site editor for block themes are included under :root
✅ Duotone preset custom properties are included under :root (except scenario below)
I have a few small questions or things to note, then I'll give it a further test as needed.
This commit only affects the frontend. No editor changes have been made yet.
I take it this TODO in the PR description is out of date given the changes to use-global-styles-output.js or is there more still needed?
Also, just to confirm, the --wp--root--padding-* custom properties still occurring under body are to be ignored for the moment and addressed in a follow-up as proposed in your recent comment?
I'm still lacking a little understanding in the use case driving this change so I'm not sure how much of a potential issue this is but when we have a non-iframed editor both the general and duotone presets are added under .editor-styles-wrapper rather than :root. For example, when enabling custom fields in the post editor preferences. Is this expected?
Note: For anyone else testing this PR with the TwentTwenty theme, it hard codes some color classed for both the editor and frontend using :root .has-slug-color, which are unrelated to the changes here.
|
I appreciate the thorough review @aaronrobertshaw 🙇🏻
Exactly. I'm still looking into how to neatly extricate theme from
Great point, thanks for raising. To be honest I haven't got that far 😄 I'll look into it. I'd wager there are a few more unexpected (by me) areas that are affected by this change.
Ah yes. I'll remove. Cheers! |
tellthemachines
approved these changes
Mar 25, 2024
There was a problem hiding this comment.
Changes LGTM and everything is working well in testing, so I think we should try getting this in!
when we have a non-iframed editor both the general and duotone presets are added under .editor-styles-wrapper rather than :root.
They're added under both :root and .editor-styles-wrapper. This matches the behaviour in trunk (except in trunk of course they're under body instead of :root). I'm not 100% sure why the duplication; it might be a way of making sure none of those presets are overridden by other styles when the editor isn't iframed. Or it could be accidental 😅 but in any case it's an existing behaviour.
Thanks for testing and for double checking this @tellthemachines |
|
Thanks for the reviews and patience on this one, folks. I'll work on the padding CSS custom vars now, mentioned in #42084 (comment) |
|
It's great that this is moving forward; thanks so much for your time and involvement. |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
…ress#42084) Replacing usages of body with :root for generated CSS properties, presets and some layout settings. This is a naive replacement without much thought put into which styles should be printed under :root and which under body. I've limited the use of root to CSS vars, with the exception of `--wp--style--root--padding-X`, which are generated along with other styles as part of PROPERTIES_METADATA Using :root for CSS custom properties in the site editor and for duotone as well. Unlinked contributors: keithdevon, Inwerpsel, gyurmey2. Co-authored-by: ramonjd <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: noisysocks <[email protected]> Co-authored-by: jorgefilipecosta <[email protected]> Co-authored-by: markhowellsmead <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: tellthemachines <[email protected]> Co-authored-by: aristath <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: colorful-tones <[email protected]> Co-authored-by: webmandesign <[email protected]> Co-authored-by: cbirdsong <[email protected]> Co-authored-by: nextgenthemes <[email protected]> Co-authored-by: fabiankaegy <[email protected]> Co-authored-by: annezazu <[email protected]>
This was referenced Apr 2, 2024
Closed
cbravobernal
pushed a commit
to garridinsi/gutenberg
that referenced
this pull request
Apr 9, 2024
…ress#42084) Replacing usages of body with :root for generated CSS properties, presets and some layout settings. This is a naive replacement without much thought put into which styles should be printed under :root and which under body. I've limited the use of root to CSS vars, with the exception of `--wp--style--root--padding-X`, which are generated along with other styles as part of PROPERTIES_METADATA Using :root for CSS custom properties in the site editor and for duotone as well. Unlinked contributors: keithdevon, Inwerpsel, gyurmey2. Co-authored-by: ramonjd <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: noisysocks <[email protected]> Co-authored-by: jorgefilipecosta <[email protected]> Co-authored-by: markhowellsmead <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: tellthemachines <[email protected]> Co-authored-by: aristath <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: colorful-tones <[email protected]> Co-authored-by: webmandesign <[email protected]> Co-authored-by: cbirdsong <[email protected]> Co-authored-by: nextgenthemes <[email protected]> Co-authored-by: fabiankaegy <[email protected]> Co-authored-by: annezazu <[email protected]>
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
May 3, 2024
First commit, no tests
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
May 9, 2024
First commit, no tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What? And why?!
This PR maybe resolves #35840
What's happening is that I'm changing
bodyto:rootas the preset block selector to make it consistent with other CSS custom property blocks, example.Alternative approaches
Note
For the purposes of backwards compat, another idea is to create a theme.json settings entry to allow overwriting the CSS selector for CSS custom properties, e.g.,
"settings.cssPropertiesSelector"or something.TODO
--wp--root--padding-*) and backwards compat in another PRTesting Instructions
I'm using empty theme to test but you can use any theme!
I've tested using 2022 and also 2021, 2020, 2019 (classic themes)
Note: for classic themes, only the preset CSS rule block (the
:root { --wp--preset--*: ... }is output.Check that presets work and there are no visual regressions.
Add a few custom color palette presets and check the frontend. They should be all sitting comfortably under
:root.