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.

Sign up for GitHub

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

Theme JSON: use block global style for block gap where no gap value exists in layout.php #39870

Closed
wants to merge 4 commits into from
Closed

Theme JSON: use block global style for block gap where no gap value exists in layout.php #39870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2bf7d9b
In the theme JSON class, only skip top-level style properties when co…
ramonjd Mar 30, 2022
b703a73
In the theme JSON class, linting
ramonjd Mar 31, 2022
aac2cef
Updated tests to reflect the changed situation:
ramonjd Mar 31, 2022
88d1f54
A test to check whether this solution will solve the columns regressi…
ramonjd May 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,17 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {

$class_name = wp_unique_id( 'wp-container-' );
$gap_value = _wp_array_get( $block, array( 'attrs', 'style', 'spacing', 'blockGap' ) );

// If there is no block-level value for blockGap,
// but a global styles value available for blockGap,
// use the latter.
if ( empty( $gap_value ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed? isn't the fallback supposed to happen using the CSS variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

(And we can only set the CSS variable at the global level theme.json)

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is this needed?

Good question.

Logically it's not.

I only put it there so we could skip over trying to fetch the global value via gutenberg_get_global_styles if a gap value set in the block's local styles attribute is found, which would take precedence anyway.

isn't the fallback supposed to happen using the CSS variable?

It will either way, unless I'm missing something, which could be the case 😇

My thinking went along of these lines: if the block's local styles attribute does not contain a gap value, we'll try the block's global styles.

Either could still be null by the time we call gutenberg_get_layout_style() in which case the fallback would be --wp--style--block-gap, which is set at the root.

Does that sound right?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does feel like duplication to me, since if the first one is null, the fallback will be the CSS variable (which corresponds to the root value).

Or in other words, if we do the fallback in php, why do we have the CSS variable in the first place.

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 does feel like duplication to me, since if the first one is null, the fallback will be the CSS variable (which corresponds to the root value).

Thanks for clarifying.

Just so I understand your concerns, are you seeing somewhere where the root block gap value, as opposed to the block-level global style value, is returned from

$spacing_global_styles = gutenberg_get_global_styles( array( 'blocks', $block['blockName'], 'spacing' ) );

on this line?

If so, yes, that would be a mistake.

The intention of this PR is to get gap working for block-level global styles by:

  1. filtering out block-level global styles for blockGap, but NOT the root, in the style sheet. See this line
  2. using the block-level global styles value, which still exists in the theme json tree, as a fallback ONLY IF the block attributes do not contain a style.spacing.blockGap value. If the former doesn't exist, we'll use the root value.

The effect of point 1 is that a user sets the value of, say, the Group block's gap in global styles, .wp-block-group { --wp--style--block-gap: '{someCustomValue}px} won't be rendered:

<style id='global-styles-inline-css'>
    body {
            /* this will still be here! */
	    --wp--style--block-gap: 1.5rem;
    }
    /* block-level global styles rules will not be printed */
</style>

The consequence of point 2 – at least what I'm attempting - is that if a user has set the value of Group block's gap in global styles it will be used when an individual block's attributes do not contain a style.spacing.blockGap value.

So the priority should still be:

  • try to use block attributes first
  • then the block's global styles second
  • finally the root value

If things aren't working that way in this PR it means I've done something wrong! 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

You raise good points, thanks for clarifying.

This basically changes how the "style engine" of layout works, instead of being styleObject => CSS, it's now ( styleObject, theme.json ) => CSS. It changes the mental model and will have an impact on the style engine work.

Yeah, I see what you mean. We might run into trouble if we mix block support style attributes with global styles.

we're planning to add "theme.json" like support to each block (containers)

I wasn't aware of Style Partials (theme.json) so I agree that a change like the one in this PR might come back to bite us. 👍

I was having a discussion with @tellthemachines about something similar over at Try using variable for root padding value #39926

I was wondering about an alternative: a const VALID_TOP_LEVEL_STYLES (or whatever) in WP_Theme_JSON_Gutenberg that explicitly defines unique, root-level-only CSS vars.

body {
    --wp--root--style--block-gap: 2px;
    --wp--root--style--padding: 10px;
     /* etc ... */
}

I haven't really thought this through, but I was venturing towards checking VALID_TOP_LEVEL_STYLES when we assemble the styles in compute_style_properties and appending --wp--root-style-- for ROOT_BLOCK_SELECTOR styles.

Or there might be a tricky way to assemble them in sanitize() via $schema['styles'], $schema['styles']['elements'] and $schema['styles']['blocks'].

Uniquely identifying global CSS vars has a benefit, aside from explicitly stating their purpose, in that their values can be used for top-level layout calculation.

For example, in #39926, full-width blocks can use a root padding CSS var set a negative margins so that they stretch to the "full-width" of the screen.

Ultimately, we need to know how to treat blockGap when it is a root or a block/element style value and output something different accordingly.

I can chat about it with @andrewserong and see what we can cook up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great questions.

This basically changes how the "style engine" of layout works, instead of being styleObject => CSS, it's now ( styleObject, theme.json ) => CSS. It changes the mental model and will have an impact on the style engine work.

Totally, I think it reveals some pragmatic challenges surrounding how we output styles, particularly for the layout support, which as @ramonjd mentions further down, is a little different to the other block supports.

In the case of this PR (and the current implementation of the layout support), everything happens at the individual block level when it's rendered, so out of necessity, to factor in global styles, we've kinda got to "look up" to the parents in order to deal with those values (or use CSS variables to stand-in for it).

In an ideal circumstance, with the style engine, how differently might we be doing things? The model for the style engine is a lot clearer for supports like padding, margin, typography, etc, where we'd like to consolidate the approach at the individual block level and global styles, and using a block's classname is an appropriate hook for it. In some of the explorations (like #39374) it looks like we'll be able to move more to global styles, but the unique block gap values was still a bit of mystery to me as to how best to handle it, and neatly declare values that get passed down / or cascade down correctly.

With block gap, because the value is used directly in different circumstances (rendering flow vs flex gap), it does create this additional challenge. I think because the use case is so different to each of the other block supports, I'm wondering if the approach in this PR would be an acceptable way to unblock getting the global styles support in, and if we could treat it as an implementation detail we could potentially swap out if and when partials in #39281 is explored.

From my perspective, it could be worthwhile proceeding with this change, so long as:

  • We're happy that folks will set blockGap values at the block level within theme.json and via the global styles interface in the site editor.
  • So long as we're happy with the shape of the data as it's stored, then theoretically, we could change the implementation after the fact, once we have a clearer idea of how we'd like to handle outputting the layout styles in the style engine.

From some of the explorations we've looked at recently, given that a fair bit of the logic winds up being moved to the Theme JSON class anyway, I don't think that the current change here would preclude further work, but very happy to be proved wrong, as I definitely don't want to nudge us into a decision that's difficult to reverse!

Copy link
Contributor

Choose a reason for hiding this comment

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

With block gap, because the value is used directly in different circumstances (rendering flow vs flex gap), it does create this additional challenge.

One alternative could be to keep the current logic (selectors specific styles but output something like):

.selector.is-layout-flex {
   gap: something;
}

.selector.is-layout-default > * {
   margin-top: something;
}

To be honest, I didn't think much about this, it's just a random idea for now, but it's an alternative that allows us to avoid that dependency to parent global style.

From some of the explorations we've looked at recently, given that a fair bit of the logic winds up being moved to the Theme JSON class anyway, I don't think that the current change here would preclude further work, but very happy to be proved wrong

I don't know 🤷 to be honest, personally I don't have a full confidence here but I trust your judgement.

Copy link
Contributor

Choose a reason for hiding this comment

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

One alternative could be to keep the current logic (selectors specific styles but output something like):

I really like that approach — I'd like it if we can output something like that in the longer term. I might be overthinking it, but I think that approach slightly opens up the can-of-worms surrounding rendering out semantic classnames, which we're in more of the proposal / exploratory phase of (since we'd need to adjust the logic for how the theme JSON class outputs styles, and then the layout support to inject the classnames).

I don't know 🤷 to be honest, personally I don't have a full confidence here but I trust your judgement.

Thank you, much appreciated! Since I was one of the folks to propose the idea of looking up global styles, I think I'd feel more comfortable with getting a couple more opinions if you're not totally sold on the idea, to confidence check before proceeding. I think my main interest is to see how viable we think a short-term fix is (so that we can fix this bug in WP 6.0) and then aim to have the class-based approach like in your code snippet in time for WP 6.1.

Copy link
Contributor

@youknowriad youknowriad Apr 5, 2022 •
edited by oandregal
Loading

Choose a reason for hiding this comment

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

cc @jorgefilipecosta @oandregal @mcsf if you have any opinions here.

$spacing_global_styles = gutenberg_get_global_styles( array( 'blocks', $block['blockName'], 'spacing' ) );
if ( isset( $spacing_global_styles['blockGap'] ) && ! empty( $spacing_global_styles['blockGap'] ) ) {
$gap_value = $spacing_global_styles['blockGap'];
}
}

// Skip if gap value contains unsupported characters.
// Regex for CSS value borrowed from `safecss_filter_attr`, and used here
// because we only want to match against the value, not the CSS attribute.
Expand Down
124 changes: 123 additions & 1 deletion lib/compat/wordpress-6.0/class-wp-theme-json-6-0.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,65 @@ class WP_Theme_JSON_6_0 extends WP_Theme_JSON_5_9 {
),
);

/**
* Sanitizes the input according to the schemas.
*
* @param array $input Structure to sanitize.
* @param array $valid_block_names List of valid block names.
* @param array $valid_element_names List of valid element names.
* @return array The sanitized output.
*/
protected static function sanitize( $input, $valid_block_names, $valid_element_names ) {
$output = array();

if ( ! is_array( $input ) ) {
return $output;
}

$output = array_intersect_key( $input, array_flip( static::VALID_TOP_LEVEL_KEYS ) );

// Build the schema based on valid block & element names.
$schema = array();
$schema_styles_elements = array();
foreach ( $valid_element_names as $element ) {
$schema_styles_elements[ $element ] = static::VALID_STYLES;
}
$schema_styles_blocks = array();
$schema_settings_blocks = array();
foreach ( $valid_block_names as $block ) {
$schema_settings_blocks[ $block ] = static::VALID_SETTINGS;
$schema_styles_blocks[ $block ] = static::VALID_STYLES;
$schema_styles_blocks[ $block ]['elements'] = $schema_styles_elements;
}
$schema['styles'] = static::VALID_STYLES;
$schema['styles']['blocks'] = $schema_styles_blocks;
$schema['styles']['elements'] = $schema_styles_elements;
$schema['settings'] = static::VALID_SETTINGS;
$schema['settings']['blocks'] = $schema_settings_blocks;

// Remove anything that's not present in the schema.
foreach ( array( 'styles', 'settings' ) as $subtree ) {
if ( ! isset( $input[ $subtree ] ) ) {
continue;
}

if ( ! is_array( $input[ $subtree ] ) ) {
unset( $output[ $subtree ] );
continue;
}

$result = static::remove_keys_not_in_schema( $input[ $subtree ], $schema[ $subtree ] );

if ( empty( $result ) ) {
unset( $output[ $subtree ] );
} else {
$output[ $subtree ] = $result;
}
}

return $output;
}

/**
* Returns the current theme's wanted patterns(slugs) to be
* registered from Pattern Directory.
Expand Down Expand Up @@ -328,7 +387,7 @@ protected function get_block_classes( $style_nodes ) {
$node = _wp_array_get( $this->theme_json, $metadata['path'], array() );
$selector = $metadata['selector'];
$settings = _wp_array_get( $this->theme_json, array( 'settings' ) );
$declarations = static::compute_style_properties( $node, $settings );
$declarations = static::compute_style_properties( $node, $settings, null, $metadata['selector'] );

// 1. Separate the ones who use the general selector
// and the ones who use the duotone selector.
Expand Down Expand Up @@ -377,6 +436,69 @@ protected function get_block_classes( $style_nodes ) {
return $block_rules;
}

/**
* Given a styles array, it extracts the style properties
* and adds them to the $declarations array following the format:
*
* ```php
* array(
* 'name' => 'property_name',
* 'value' => 'property_value,
* )
* ```
*
* @param array $styles Styles to process.
* @param array $settings Theme settings.
* @param array $properties Properties metadata.
* @param string|null $selector Current selector.
* @return array Returns the modified $declarations.
*/
protected static function compute_style_properties( $styles, $settings = array(), $properties = null, $selector = null ) {
if ( null === $properties ) {
$properties = static::PROPERTIES_METADATA;
}

$declarations = array();
if ( empty( $styles ) ) {
return $declarations;
}

foreach ( $properties as $css_property => $value_path ) {
// Some styles such as blockGap are only meant to be available at the top level (ROOT_BLOCK_SELECTOR),
// hence we only output styles at the top level.
if ( 'top' === _wp_array_get( self::VALID_STYLES, array( $value_path[0], $value_path[1] ), null ) && static::ROOT_BLOCK_SELECTOR !== $selector ) {
continue;
}

$value = static::get_property_value( $styles, $value_path );

// 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 );
if (
array_key_exists( $path_string, static::PROTECTED_PROPERTIES ) &&
_wp_array_get( $settings, static::PROTECTED_PROPERTIES[ $path_string ], null ) === null
) {
continue;
}
}

// Skip if empty and not "0" or value represents array of longhand values.
$has_missing_value = empty( $value ) && ! is_numeric( $value );
if ( $has_missing_value || is_array( $value ) ) {
continue;
}

$declarations[] = array(
'name' => $css_property,
'value' => $value,
);
}

return $declarations;
}

/**
* Merge new incoming data.
*
Expand Down
11 changes: 9 additions & 2 deletions lib/compat/wordpress-6.0/theme.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@
"text": true
},
"spacing": {
"blockGap": null,
"blockGap": true,
"margin": false,
"padding": false,
"units": [ "px", "em", "rem", "vh", "vw", "%" ]
Expand Down Expand Up @@ -240,6 +240,13 @@
}
},
"styles": {
"spacing": { "blockGap": "24px" }
"spacing": { "blockGap": "24px" },
"blocks": {
"core/columns": {
"spacing": {
"blockGap": "2em"
}
}
}
}
}
9 changes: 2 additions & 7 deletions packages/edit-site/src/components/global-styles/dimensions-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,8 @@ function useHasMargin( name ) {
function useHasGap( name ) {
const supports = getSupportedGlobalStylesPanels( name );
const [ settings ] = useSetting( 'spacing.blockGap', name );
// Do not show the gap control panel for block-level global styles
// as they do not work on the frontend.
// See: https://github.com/WordPress/gutenberg/pull/39845.
// We can revert this condition when they're working again.
return !! name
? false
: settings && supports.includes( '--wp--style--block-gap' );

return settings && supports.includes( '--wp--style--block-gap' );
}

function filterValuesBySides( values, sides ) {
Expand Down
37 changes: 35 additions & 2 deletions phpunit/class-wp-theme-json-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,38 @@ function test_get_stylesheet_skips_disabled_protected_properties() {
$this->assertEquals( $expected, $theme_json->get_stylesheet( array( 'styles' ) ) );
}

function test_get_stylesheet_skips_top_level_properties_at_block_level() {
$theme_json = new WP_Theme_JSON_Gutenberg(
array(
'version' => WP_Theme_JSON_Gutenberg::LATEST_SCHEMA,
'settings' => array(
'spacing' => array(
'padding' => true,
'blockGap' => true,
),
),
'styles' => array(
'spacing' => array(
'padding' => '1vw',
'blockGap' => '1em',
),
'blocks' => array(
'core/columns' => array(
'spacing' => array(
'padding' => '0.5rem',
'blockGap' => '24px',
),
),
),
),
)
);

$expected = 'body { margin: 0; }body{padding: 1vw;--wp--style--block-gap: 1em;}.wp-site-blocks > .alignleft { float: left; margin-right: 2em; }.wp-site-blocks > .alignright { float: right; margin-left: 2em; }.wp-site-blocks > .aligncenter { justify-content: center; margin-left: auto; margin-right: auto; }.wp-site-blocks > * { margin-block-start: 0; margin-block-end: 0; }.wp-site-blocks > * + * { margin-block-start: var( --wp--style--block-gap ); }.wp-block-columns{padding: 0.5rem;}';
$this->assertEquals( $expected, $theme_json->get_stylesheet() );
$this->assertEquals( $expected, $theme_json->get_stylesheet( array( 'styles' ) ) );
}

function test_get_stylesheet_renders_enabled_protected_properties() {
$theme_json = new WP_Theme_JSON_Gutenberg(
array(
Expand Down Expand Up @@ -2253,7 +2285,7 @@ function test_sanitization() {
'core/group' => array(
'spacing' => array(
'margin' => 'valid value',
'blockGap' => 'invalid value',
'blockGap' => 'valid value',
),
),
),
Expand All @@ -2271,7 +2303,8 @@ function test_sanitization() {
'blocks' => array(
'core/group' => array(
'spacing' => array(
'margin' => 'valid value',
'margin' => 'valid value',
'blockGap' => 'valid value',
),
),
),
Expand Down