Block gap: Only render CSS variable if corresponding theme setting is enabled #35209

andrewserong · 2021-09-29T07:35:22Z

Description

In #35197 it's described that rendering the --wp--style--block-gap CSS variable globally when the block gap setting hasn't been explicitly opted in-to, could result in styling regressions on classic themes that use add_theme_support( 'experimental-link-color' ); if we're to proceed with updating Button block styling to use the new CSS variable.

In this PR, we're exploring skipping rendering the CSS variable if the corresponding setting in theme.json is set to null. The mechanism is to add a list of protected properties, and for these particular properties, only render them if their corresponding setting in theme.json is not null. This behaviour is consistent with the approach in #34491 to opt out of generating block gap styling. The extension here is to also skip rendering the CSS variable itself.

This is just one particular approach to solving the issue, so please let me know if you think it's a bad idea, or if you can think of a better way to handle it.

How has this been tested?

With TwentyTwentyOne theme enabled, open up the post editor and inspect element, and find the set of four or so style elements that sit underneath the visual editor component. Before this PR is applied, you should see that the global --wp--style--block-gap CSS variable is rendered.

With this PR applied, it should no longer render that variable with that theme active.

With TT1-Blocks active, and settings.spacing.blockGap enabled in the theme.json file, then the CSS variable should be rendered.

With TT1-Blocks active, and settings.spacing.blockGap set to null in the theme.json file, then the CSS variable should not be rendered.

Screenshots

TwentyTwentyOne theme before	TwentyTwentyOne theme after

TT1-Blocks (should remain unaffected when the settings.spacing.blockGap setting is enabled in theme.json:

Types of changes

Bug fix (non-breaking change which fixes an issue)

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. (N/A)
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).

andrewserong · 2021-09-29T07:36:12Z

Note: this still needs to have tests added, but I thought I'd get the PR up as a proof of concept.

stacimc · 2021-09-29T16:46:15Z

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

@@ -551,6 +560,12 @@ private static function compute_style_properties( $styles ) {
 		foreach ( self::PROPERTIES_METADATA as $css_property => $value_path ) {
 			$value = self::get_property_value( $styles, $value_path );

+			if ( in_array( $css_property, self::PROTECTED_PROPERTIES ) ) {
+				if ( ! _wp_array_get( $settings, $value_path, false ) ) {


I think we'll want to null check this in order to be consistent with #34491, which makes the block gap styles explicitly opt-out rather than opt-in; according to that PR setting the blockGap setting to false should turn off the block support UI but not the global gap styles.

Of course, the drawback to explicitly checking for null is that we'll end up with styling regressions on blocks themes that don't opt in to the block support, but do opt in to the global block gap styling (ie blockGap: false or omitting the setting entirely).

I think that just might be a limitation of the unified variable approach 🤔

stacimc · 2021-09-29T17:13:34Z

Thanks so much for this, @andrewserong! Great find -- what a tricky one to track down.

This is looking really good so far for me! I tested on Twenty Twenty One/Seedlet and confirmed that the variable is no longer generated, and on TT1-blocks I confirmed that it's added correctly. I just left one comment about blocks themes that set blockGap: false or omit it; because the block gap styles treat this setting as an opt-out only when set to null, the layout styles using the variable are generated but the variable is not.

I like this approach, although I wonder if needing to explicitly check for null makes it less reusable than it might otherwise be. The unified variable makes this tricky for backwards compatibility, and may mean that we'll just have to accept the tradeoff of styling regressions on blocks themes. I would be very happy to get this through and resolve the issue on classic themes!

… enabled

…alsy, add tests

andrewserong · 2021-09-30T04:11:09Z

Thanks for testing and for the suggestions @stacimc! I've updated this PR to use an explicit check for null and have added a couple of tests.

andrewserong · 2021-09-30T04:52:51Z

@youknowriad I've just added you as a reviewer since you've done a lot of the work on the layout and gap support. What do you think of this approach for ensuring that we don't render the --wp--style--block-gap CSS variable on classic themes (or theme.json themes that set blockGap to null)?

youknowriad

LGTM 👍

youknowriad · 2021-09-30T07:36:13Z

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

+	 * These properties are only rendered if a corresponding setting in the
+	 * `PROPERTIES_METADATA` path enables it via a value other than `null`.
+	 */
+	const PROTECTED_PROPERTIES = array( '--wp--style--block-gap' );


A small thing: Should we have something like 'spacing.blockGap' => 'spacing.blockGap' instead. Meaning (theme.json style property mapping to its corresponding theme.json setting key). The current logic relies on the output instead of the input and assumes the setting key is the same as the style key (which is of course true now).

…to setting key mapping

andrewserong · 2021-10-01T04:04:28Z

Thanks for reviewing and for the suggestion @youknowriad! That feels better to me explicitly describing the mapping so that we can support a variance between the style key and settings key if we need to for future properties 👍

I went for something close to what you suggested, using 'spacing.blockGap' => array( 'spacing', 'blockGap' ) so that the value is an array that plays nicely with _wp_array_get.

Since it's a fairly minor change, I'll go ahead and merge it in once tests pass, but happy to do any further changes in follow-ups if need be!

andrewserong added [Type] Enhancement A suggestion for improvement. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Sep 29, 2021

andrewserong requested a review from stacimc September 29, 2021 07:35

andrewserong self-assigned this Sep 29, 2021

andrewserong mentioned this pull request Sep 29, 2021

Block Gap css variable is loaded in the editor for some classic themes #35197

Closed

stacimc reviewed Sep 29, 2021

View reviewed changes

andrewserong added 2 commits September 30, 2021 13:50

Block gap: Only render CSS variable if corresponding theme setting is…

9e70deb

… enabled

Update protected property check to look for a null value instead of f…

afb35ef

…alsy, add tests

andrewserong force-pushed the update/block-gap-css-variable-to-only-render-with-corresponding-setting branch from 3cdcc6f to afb35ef Compare September 30, 2021 03:51

andrewserong changed the title ~~[WIP] Block gap: Only render CSS variable if corresponding theme setting is enabled~~ Block gap: Only render CSS variable if corresponding theme setting is enabled Sep 30, 2021

andrewserong added 2 commits September 30, 2021 13:59

Fix linting issues in test file

41a7197

Add inline comment to improve clarity

8ecb359

andrewserong marked this pull request as ready for review September 30, 2021 04:09

andrewserong requested a review from spacedmonkey as a code owner September 30, 2021 04:09

andrewserong requested a review from youknowriad September 30, 2021 04:51

youknowriad approved these changes Sep 30, 2021

View reviewed changes

stacimc mentioned this pull request Sep 30, 2021

[Buttons block]: Add gap and vertical margin support #34546

Merged

7 tasks

Switch PROTECTED_PROPERTIES to use an associative array of style key …

17b49e1

…to setting key mapping

Fix linting issue

8c397f8

andrewserong merged commit 0cdcd47 into trunk Oct 1, 2021

andrewserong deleted the update/block-gap-css-variable-to-only-render-with-corresponding-setting branch October 1, 2021 05:05

github-actions bot added this to the Gutenberg 11.7 milestone Oct 1, 2021

npneeraj mentioned this pull request Feb 13, 2023

[Snyk] Upgrade react-native from 0.69.4 to 0.71.1 npneeraj/gutenberg#7

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block gap: Only render CSS variable if corresponding theme setting is enabled #35209

Block gap: Only render CSS variable if corresponding theme setting is enabled #35209

andrewserong commented Sep 29, 2021 •

edited

Loading

andrewserong commented Sep 29, 2021

stacimc Sep 29, 2021

stacimc Sep 29, 2021

stacimc commented Sep 29, 2021

andrewserong commented Sep 30, 2021

andrewserong commented Sep 30, 2021

youknowriad left a comment

youknowriad Sep 30, 2021

andrewserong commented Oct 1, 2021

Block gap: Only render CSS variable if corresponding theme setting is enabled #35209

Block gap: Only render CSS variable if corresponding theme setting is enabled #35209

Conversation

andrewserong commented Sep 29, 2021 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

andrewserong commented Sep 29, 2021

stacimc Sep 29, 2021

Choose a reason for hiding this comment

stacimc Sep 29, 2021

Choose a reason for hiding this comment

stacimc commented Sep 29, 2021

andrewserong commented Sep 30, 2021

andrewserong commented Sep 30, 2021

youknowriad left a comment

Choose a reason for hiding this comment

youknowriad Sep 30, 2021

Choose a reason for hiding this comment

andrewserong commented Oct 1, 2021

andrewserong commented Sep 29, 2021 •

edited

Loading