Fix: Preset controls need the preset CSS variables in scope #27119

jorgefilipecosta · 2020-11-19T19:34:45Z

This PR makes sure presets and CSS vars are available in the scope of editor UI. These styles are just defining CSS variables and specific classes they should not impact anything in the UI. But if a UI component needs a variable e.g: a gradient preset that depends on a color preset having the variable available makes the UI components look as expected.

How has this been tested?

I enabled 2021 blocks.
I added the following declaration to theme.json:

{
  "color": {
    "gradients": [
      {
        "slug": "some-gradient",
        "gradient": "linear-gradient(to bottom, transparent 20%, var(--wp--preset--color--secondary) 20%, var(--wp--preset--color--secondary) 80%, transparent 80%)"
      }
    ],
    "palette": [
      {
        "slug": "primary",
        "color": "#000000"
      },
      {
        "slug": "secondary",
        "color": "#3C8067"
      }
    ]
  }
}

I verified the gradient control circle renders as expected.

github-actions · 2020-11-19T19:43:44Z

Size Change: +102 B (0%)

Total Size: 1.19 MB

Filename	Size	Change
`build/block-editor/index.js`	128 kB	+20 B (0%)
`build/edit-site/index.js`	24.1 kB	+82 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.14 kB	0 B
`build/annotations/index.js`	3.8 kB	0 B
`build/api-fetch/index.js`	3.42 kB	0 B
`build/autop/index.js`	2.84 kB	0 B
`build/blob/index.js`	665 B	0 B
`build/block-directory/index.js`	8.72 kB	0 B
`build/block-directory/style-rtl.css`	943 B	0 B
`build/block-directory/style.css`	942 B	0 B
`build/block-editor/style-rtl.css`	11.2 kB	0 B
`build/block-editor/style.css`	11.2 kB	0 B
`build/block-library/editor-rtl.css`	8.95 kB	0 B
`build/block-library/editor.css`	8.95 kB	0 B
`build/block-library/index.js`	148 kB	0 B
`build/block-library/style-rtl.css`	8.27 kB	0 B
`build/block-library/style.css`	8.27 kB	0 B
`build/block-library/theme-rtl.css`	789 B	0 B
`build/block-library/theme.css`	790 B	0 B
`build/block-serialization-default-parser/index.js`	1.87 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.06 kB	0 B
`build/blocks/index.js`	48.1 kB	0 B
`build/components/index.js`	172 kB	0 B
`build/components/style-rtl.css`	15.3 kB	0 B
`build/components/style.css`	15.3 kB	0 B
`build/compose/index.js`	9.95 kB	0 B
`build/core-data/index.js`	14.8 kB	0 B
`build/data-controls/index.js`	828 B	0 B
`build/data/index.js`	8.98 kB	0 B
`build/date/index.js`	11.2 kB	0 B
`build/deprecated/index.js`	769 B	0 B
`build/dom-ready/index.js`	571 B	0 B
`build/dom/index.js`	4.95 kB	0 B
`build/edit-navigation/index.js`	11.1 kB	0 B
`build/edit-navigation/style-rtl.css`	881 B	0 B
`build/edit-navigation/style.css`	885 B	0 B
`build/edit-post/index.js`	306 kB	0 B
`build/edit-post/style-rtl.css`	6.42 kB	0 B
`build/edit-post/style.css`	6.4 kB	0 B
`build/edit-site/style-rtl.css`	3.92 kB	0 B
`build/edit-site/style.css`	3.92 kB	0 B
`build/edit-widgets/index.js`	26.3 kB	0 B
`build/edit-widgets/style-rtl.css`	3.13 kB	0 B
`build/edit-widgets/style.css`	3.13 kB	0 B
`build/editor/editor-styles-rtl.css`	476 B	0 B
`build/editor/editor-styles.css`	478 B	0 B
`build/editor/index.js`	43.3 kB	0 B
`build/editor/style-rtl.css`	3.85 kB	0 B
`build/editor/style.css`	3.85 kB	0 B
`build/element/index.js`	4.62 kB	0 B
`build/escape-html/index.js`	735 B	0 B
`build/format-library/index.js`	6.86 kB	0 B
`build/format-library/style-rtl.css`	547 B	0 B
`build/format-library/style.css`	548 B	0 B
`build/hooks/index.js`	2.27 kB	0 B
`build/html-entities/index.js`	622 B	0 B
`build/i18n/index.js`	3.57 kB	0 B
`build/is-shallow-equal/index.js`	698 B	0 B
`build/keyboard-shortcuts/index.js`	2.54 kB	0 B
`build/keycodes/index.js`	1.94 kB	0 B
`build/list-reusable-blocks/index.js`	3.1 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	476 B	0 B
`build/list-reusable-blocks/style.css`	476 B	0 B
`build/media-utils/index.js`	5.32 kB	0 B
`build/notices/index.js`	1.82 kB	0 B
`build/nux/index.js`	3.42 kB	0 B
`build/nux/style-rtl.css`	671 B	0 B
`build/nux/style.css`	668 B	0 B
`build/plugins/index.js`	2.56 kB	0 B
`build/primitives/index.js`	1.43 kB	0 B
`build/priority-queue/index.js`	790 B	0 B
`build/redux-routine/index.js`	2.84 kB	0 B
`build/reusable-blocks/index.js`	2.92 kB	0 B
`build/rich-text/index.js`	13.4 kB	0 B
`build/server-side-render/index.js`	2.77 kB	0 B
`build/shortcode/index.js`	1.69 kB	0 B
`build/token-list/index.js`	1.27 kB	0 B
`build/url/index.js`	4.06 kB	0 B
`build/viewport/index.js`	1.86 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.22 kB	0 B

_{compressed-size-action}

oandregal · 2020-11-24T08:09:28Z

👋 I've tested this and these are my thoughts:

The fix only works in the site editor.
It regresses an issue we fixed recently at Update stylesheet generation at edit site #27065 with the HTML block.

The first issue seems solvable by following the proposal at #26511 (comment)

The second one is a bit more convoluted. On the one hand, in the site editor, we need to grab the preset styles to update their values upon user changes ― so far, we've done this by creating our own style node in the client. On the other hand, the stylesheet needs to be ready for when the HTML block (and potentially others such as classic?) grabs them to use in its iframe. I'm not sure how to balance the two needs without looking at how the HTML preview works.

I can perhaps offer some ideas about how to address this but they need some investigation:

It looks like adding presets to the settings.style key as it works now wouldn't work. Perhaps we can teach this code to avoid wrapping some styles? This would have the benefit that observers of the styles would still work as expected.
Creating a named embedded stylesheet in the server that we can grab in the client to update its values should it be necessary. It could work to fix the initial render of the HTML preview, but without looking at how the HTML preview works, I'm not sure whether it'll work for subsequent re-renders (user changes values of the theme presets).
Add a style node in the client, but in a way that's hooked up to the way the HTML preview (and other blocks?) work.

oandregal · 2020-11-24T08:24:42Z

Issues for the HTML block that may relevant to look at together with this

jorgefilipecosta · 2020-11-24T18:17:50Z

Hi @nosolosw thank you for the review and for suggesting possible paths to address the issues.

I updated this PR and it follows the path "It looks like adding presets to the settings.style key as it works now wouldn't work. Perhaps we can teach this code to avoid wrapping some styles? This would have the benefit that observers of the styles would still work as expected.".

oandregal · 2020-11-26T10:02:41Z

To avoid CSS conflicts, in the editors, we should have this:

Block styles: with wrapper.
CSS variables: without wrapper.
Classes generated by the CSS variables: with wrapper.

Code-wise:

the explosion of methods for every style use case makes me uneasy. I'd like a more compact approach but unsure how to articulate this better. Any ideas? Perhaps we can have a parametrizable get_stylesheet( $styles ) where $styles indicates what we need: all (default), only CSS vars, etc.
can we also consolidate the use of cache & CSS for link attachment to the new situation? This is minor compared to the above.

jorgefilipecosta · 2020-11-26T16:10:01Z

To avoid CSS conflicts, in the editors, we should have this:

Block styles: with wrapper.

CSS variables: without wrapper.

Classes generated by the CSS variables: with wrapper.

Code-wise:

the explosion of methods for every style use case makes me uneasy. I'd like a more compact approach but unsure how to articulate this better. Any ideas? Perhaps we can have a parametrizable get_stylesheet( $styles ) where $styles indicates what we need: all (default), only CSS vars, etc.

can we also consolidate the use of cache & CSS for link attachment to the new situation? This is minor compared to the above.

Hi @nosolosw,

Thank you for the review, all your feedback was applied 👍

oandregal · 2020-11-27T12:57:15Z

Going to take a look at the code now, but here's what I've found in testing this:

Some block contexts are duplicated in the front & post-editor => first, they are included with empty styles, then with proper styles. It seems that the CSS var stylesheet generation is messing with things.

h1 {
}

// ...

h1 {
  font-size: var(--wp--preset--font-size--gigantic);
  line-height: 1.1;
}

Other than that, it works as advertised.

oandregal · 2020-11-27T12:59:02Z

lib/class-wp-theme-json.php

 	 * @return string Stylesheet.
 	 */
-	public function get_stylesheet() {
-		return array_reduce( $this->contexts, array( $this, 'to_stylesheet' ), '' );
+	public function get_stylesheet( $type = 'all' ) {


I'd like us to keep adding tests for the new things. In this case, get_stylesheet( 'block_styles' ) and get_stylesheet( 'css_variables' ). I believe that would have caught the issue with the empty rulesets.

oandregal · 2020-11-27T13:00:26Z

lib/class-wp-theme-json.php

-		return array_reduce( $this->contexts, array( $this, 'to_stylesheet' ), '' );
+	public function get_stylesheet( $type = 'all' ) {
+		switch ( $type ) {
+			case 'all':


What if instead of using the case all we move this code to the default fallback? We'd have every code path covered.

oandregal · 2020-11-27T13:02:17Z

lib/class-wp-theme-json.php

@@ -816,13 +816,14 @@ function ( $carry, $element ) {
 	/**
 	 * Converts each context into a list of rulesets
 	 * to be appended to the stylesheet.
+	 * These rulesets contain the all css variables (custom variables and preset variables)
+	 * and all the preset classes.


This comment seems to need updating?

oandregal · 2020-11-27T13:04:01Z

lib/class-wp-theme-json.php

+			// Attach the ruleset for style and custom properties.
+			$stylesheet .= self::to_ruleset( $context['selector'], $declarations );
+		}
+
 		// Attach the rulesets for the classes.
 		self::compute_preset_classes( $stylesheet, $context );


Conceptually, I don't like that we output the preset classes here. In practical terms, I guess it's fine? Flagging in case you have thoughts.

oandregal · 2020-11-27T13:06:32Z

lib/global-styles.php

@@ -135,9 +136,9 @@ function gutenberg_experimental_global_styles_get_stylesheet( $tree ) {
 		}
 	}

-	$stylesheet = $tree->get_stylesheet();
+	$stylesheet = $tree->get_stylesheet( $type );


So, now, the resulting stylesheet can be actually 3 different stylesheets, however, we only have 1 cache. I presume that can be a source of conflicts we need to fix?

Nice catch we should only cache type all which is the one used on the front end.

jorgefilipecosta · 2020-11-27T18:09:20Z

All the feedback was addressed before the merge.

jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label Nov 19, 2020

jorgefilipecosta force-pushed the fix/preset-controls-need-the-preset-CSS-variables-in-scope branch 2 times, most recently from 5fbf985 to 71d8c39 Compare November 20, 2020 20:30

jorgefilipecosta requested a review from oandregal November 23, 2020 17:46

jorgefilipecosta force-pushed the fix/preset-controls-need-the-preset-CSS-variables-in-scope branch from 71d8c39 to fd70f5e Compare November 24, 2020 17:41

jorgefilipecosta requested review from ellatrix, spacedmonkey and TimothyBJacobs as code owners November 24, 2020 17:41

jorgefilipecosta force-pushed the fix/preset-controls-need-the-preset-CSS-variables-in-scope branch from fd70f5e to 7396e22 Compare November 24, 2020 18:15

oandregal added [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Nov 25, 2020

jorgefilipecosta force-pushed the fix/preset-controls-need-the-preset-CSS-variables-in-scope branch from 7396e22 to fd719dd Compare November 25, 2020 14:58

jorgefilipecosta force-pushed the fix/preset-controls-need-the-preset-CSS-variables-in-scope branch 3 times, most recently from 4a585ef to 210782e Compare November 26, 2020 16:08

oandregal reviewed Nov 27, 2020

View reviewed changes

Fix: Preset controls need the preset CSS variables in scope

dc1ea0f

jorgefilipecosta force-pushed the fix/preset-controls-need-the-preset-CSS-variables-in-scope branch from 210782e to dc1ea0f Compare November 27, 2020 16:44

jorgefilipecosta merged commit 8f90615 into master Nov 27, 2020

jorgefilipecosta deleted the fix/preset-controls-need-the-preset-CSS-variables-in-scope branch November 27, 2020 18:09

github-actions bot added this to the Gutenberg 9.5 milestone Nov 27, 2020

thetwopct mentioned this pull request Jan 25, 2021

Custom Gradient Picker not working with CSS variables #28254

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Preset controls need the preset CSS variables in scope #27119

Fix: Preset controls need the preset CSS variables in scope #27119

jorgefilipecosta commented Nov 19, 2020

github-actions bot commented Nov 19, 2020 •

edited

Loading

oandregal commented Nov 24, 2020

oandregal commented Nov 24, 2020

jorgefilipecosta commented Nov 24, 2020

oandregal commented Nov 26, 2020

jorgefilipecosta commented Nov 26, 2020

oandregal commented Nov 27, 2020 •

edited

Loading

oandregal Nov 27, 2020

oandregal Nov 27, 2020

oandregal Nov 27, 2020

oandregal Nov 27, 2020

oandregal Nov 27, 2020

jorgefilipecosta Nov 27, 2020

jorgefilipecosta commented Nov 27, 2020

Fix: Preset controls need the preset CSS variables in scope #27119

Fix: Preset controls need the preset CSS variables in scope #27119

Conversation

jorgefilipecosta commented Nov 19, 2020

How has this been tested?

github-actions bot commented Nov 19, 2020 • edited Loading

oandregal commented Nov 24, 2020

oandregal commented Nov 24, 2020

jorgefilipecosta commented Nov 24, 2020

oandregal commented Nov 26, 2020

jorgefilipecosta commented Nov 26, 2020

oandregal commented Nov 27, 2020 • edited Loading

oandregal Nov 27, 2020

Choose a reason for hiding this comment

oandregal Nov 27, 2020

Choose a reason for hiding this comment

oandregal Nov 27, 2020

Choose a reason for hiding this comment

oandregal Nov 27, 2020

Choose a reason for hiding this comment

oandregal Nov 27, 2020

Choose a reason for hiding this comment

jorgefilipecosta Nov 27, 2020

Choose a reason for hiding this comment

jorgefilipecosta commented Nov 27, 2020

github-actions bot commented Nov 19, 2020 •

edited

Loading

oandregal commented Nov 27, 2020 •

edited

Loading