Style Engine: Include 6.1 CSS filter, ensure style engine can output CSS functions like clamp #43004

andrewserong · 2022-08-05T02:18:04Z

What?

A potential alternative to #42968, exploring @Mamaduka's idea from #42968 (comment)

The idea here is that to resolve the issue of the style engine stripping values that use clamp() we can roll out the proposed updated regex for filtering values from the Trac ticket: https://core.trac.wordpress.org/ticket/55966.

Why?

The idea behind this PR is that the style engine shouldn't have to be doing too much of its own work to determine whether or not a rule is valid. I've added an additional function just in case we need to add further special handling down the track (for display, perhaps?), but for now we're still deferring to safecss_filter_attr.

How?

Add a filter callback for safecss_filter_attr_allow_css that uses the proposed regex from https://core.trac.wordpress.org/ticket/55966 (the changes are viewable in this diff: https://core.trac.wordpress.org/attachment/ticket/55966/55966.2.diff)
Add another method to the style engine declarations class that accepts a property and value, and then defers to safecss_filter_attr

Testing Instructions

Follow the reproduction steps in 1.3.9: clamp() is no longer recognized in “contentSize” or “wideSize” in theme.json #42948 and check that this PR allows the expected clamp() values
Double-check that the copied over regex from https://core.trac.wordpress.org/ticket/55966 looks acceptable and comparable to the core implementation. To review this section, it might be helpful to compare to the existing core implementation over in: https://github.com/WordPress/wordpress-develop/blob/HEAD/src/wp-includes/kses.php#L2469
Since this filter applies for all CSS filtering in Gutenberg, rather than just the style engine, confidence check that the change looks acceptable

ramonjd · 2022-08-05T03:20:42Z

packages/style-engine/class-wp-style-engine-css-declarations.php

+			// Allow a controlled set of `display` values.
+			if (
+				'display' === $property &&
+				in_array( $value, array( 'block', 'inline', 'inline-block', 'flex', 'inline-flex', 'grid', 'inline-grid' ), true )


Any reason why we couldn't add a $attrs[] = 'display'; entry to gutenberg_safe_style_attrs_6_1() in lib/compat/wordpress-6.1/blocks.php?

If we're not confident that a core patch for display will be in before 6.1 maybe add another safe_style_css filter?

Over in WordPress/wordpress-develop#2928 (comment) we discussed the issue with display: none, in that we still want that value to be excluded, but to allow the remaining display values.

Currently in the Layout output of the Theme JSON class we have special handling for display here: https://github.com/WordPress/gutenberg/blob/trunk/lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php#L1370

For the purposes of this PR, we could always remove the display handling, since we're not yet using the style engine for global styles, and defer the issue to later.

I think the question comes down to — should WordPress in general allow display with a set number of values, or is it that the Layout / theme.json might need it for a particular use case, but it shouldn't otherwise be more broadly exposed.

All that said, in principle, I really like the idea that the style engine could "just work" and outsource CSS validation / sanitization to core functions and filters 🤔

we discussed the issue with display: none, in that we still want that value to be excluded

I've left a comment over in that discussion. It would be good to validate that claim.

For the purposes of this PR, we could always remove the display handling, since we're not yet using the style engine for global styles, and defer the issue to later.

I always say "if in doubt, chuck it out" 😄

If we're not using it, is there any benefit to having it right now or for 6.1?

Or could this be resolved in the core patch?

I think the question comes down to — should WordPress in general allow display with a set number of values, or is it that the Layout / theme.json might need it for a particular use case, but it shouldn't otherwise be more broadly exposed.

Yeah for sure. Do you think it'd be helpful to ask a wider audience before committing here?

I always say "if in doubt, chuck it out" 😄

If we're not using it, is there any benefit to having it right now or for 6.1?

Great point. For the scope of this PR, we only need to resolve the issue reported in #42948, as display is still handled in the Theme JSON class. Let's avoid dealing with display in this PR until we need to refactor the centralised Layout output.

I've removed the display handling in: b7261d8

ramonjd · 2022-08-05T03:58:16Z

This is looking good to me.

I think having filters doing the temporary workarounds is a nice way to be explicit about things, and probably easier to remove/extract during migrations.

I just had a question as to whether we could move the checks for display to the gutenberg_safe_style_attrs_6_1 filter to make it even cleaner.

Also, using the code that'll be closer to what core will have.

I tested by running some of the target properties and values through:

max-width: clamp(36.00rem, calc(32.00rem + 10.00vw), 40.00rem);
max-width: min(calc(3 * 10.00vw));
max-width: max(calc(3 * 10.00vw));
max-width: minmax(20px, auto) 1fr 1fr);

.wp-testing-display-properties-block {
	display: block;
}
.wp-testing-display-properties-inline {
	display: inline;
}
.wp-testing-display-properties-inline-block {
	display: inline-block;
}
.wp-testing-display-properties-flex {
	display: flex;
}
.wp-testing-display-properties-inline-flex {
	display: inline-flex;
}
.wp-testing-display-properties-grid {
	display: grid;
}
.wp-testing-display-properties-inline-grid {
	display: inline-grid;
}

ramonjd

Can confirm that display is not longer supported.

Looks like a good approach to me. Keeps the style engine clean, and has a clear path to migration.

Thanks for the tests.

ramonjd · 2022-08-05T04:31:02Z

packages/style-engine/class-wp-style-engine-css-declarations.php

@@ -130,7 +146,7 @@ public function get_declarations_string( $should_prettify = false, $indent_count

 		foreach ( $declarations_array as $property => $value ) {
 			$spacer               = $should_prettify ? ' ' : '';
-			$filtered_declaration = esc_html( safecss_filter_attr( "{$property}:{$spacer}{$value}" ) );
+			$filtered_declaration = esc_html( static::filter_declaration( $property, $value, $spacer ) );


For a follow up: could we abstract all sanitization and move the esc_html too?

Good question — depending on the output context, I wasn't too sure if esc_html() is always desired so left it inline for the time being. Happy to play around with it in a follow-up if this PR looks viable (I'd like to better understand when and why we need to escape the html characters, too — I can imagine some circumstances, but I don't feel I fully get it yet 🙂).

I think this is worth investigating separately. The safecss_filter_attr is probably enough here.

Looking at other usages of safecss_filter_attr in the plugin, I don't see any of them wrapped in the esc_* method.

could we abstract all sanitization and move the esc_html too?

esc_html causes issues with values that contain quotes - like for example URLs in background:url("example.com/image.jpg"). That's why over in #42968 I was introducing a sanitize_value method 😉

I did some tests, and now I'm 99% sure we don't need esc_html here.

Sorry for the ping, @peterwilsoncc. Can you confirm that values returned by safecss_filter_attr don't need escaping?

$value = safecss_filter_attr( $css ); // VS $value = esc_html( safecss_filter_attr( $css ) );

I think we can just remove esc_html
It's not used anywhere in Core when printing CSS, and the safecss call takes care of most (all?) cases I can think of that could potentially cause issues.

Thanks for the discussion, folks! I've updated this PR to remove the call to esc_html and updated the tests accordingly.

packages/style-engine/phpunit/class-wp-style-engine-css-declarations-test.php

Mamaduka · 2022-08-05T08:07:16Z

There're new flaky test reports 43007...43011. Let's double check code for 500 errors and close those.

andrewserong · 2022-08-08T00:50:05Z

packages/style-engine/phpunit/class-wp-style-engine-css-declarations-test.php

@@ -149,14 +149,37 @@ public function test_generate_prettified_with_more_indents_css_declarations_stri
 	 */
 	public function test_remove_unsafe_properties_and_values() {
 		$input_declarations = array(
-			'color'        => '<red/>',
+			'color'        => 'url("https://wordpress.org")',


I've changed this one to be a value that safecss_filter_attr strips, instead, since this PR now removes the call to esc_html so we no longer need to test whether or not HTML characters will be escaped.

andrewserong · 2022-08-08T00:51:23Z

There're new flaky test reports 43007...43011. Let's double check code for 500 errors and close those.

Thanks for the heads-up @Mamaduka, these 500 errors occurred while wordpress.org was down, so it was most likely caused by failures fetching from there. I'll close out those issues.

…CSS functions like clamp

packages/style-engine/class-wp-style-engine-css-declarations.php

andrewserong · 2022-08-09T02:22:49Z

I think this one's ready now for a final review. @aristath, @Mamaduka and @ramonjd are there any objections to proceeding with this approach? I don't mind too much whether we go with this PR or another one, but it'd be great to try to get a fix merged before the 13.9 RC gets pushed tomorrow if we can.

ramonjd · 2022-08-09T03:49:37Z

LGTM 👍

Retested. The CSS functions are coming through as expected

<style id='block-supports-inline-css'>
.barnacle-bill {
	max-width: clamp(36.00rem, calc(32.00rem + 10.00vw), 40.00rem);
}
.barnacle-bob {
	font-size: min(calc(3 * 10.00vw));
}
.barnacle-barry {
	height: max(calc(3 * 10.00vw));
}
.barnacle-brad {
	padding: minmax(20px, auto) 1fr 1fr);
}
</style>

Mamaduka · 2022-08-09T08:17:16Z

Thanks for landing this, folks!

@andrewserong, do you mind leaving a comment on https://core.trac.wordpress.org/ticket/55966, saying that we adopted regex from the patch? It might help move the ticket forward for committing.

andrewserong · 2022-08-10T01:06:54Z

Good idea! I've added a comment in https://core.trac.wordpress.org/ticket/55966#comment:6

andrewserong requested a review from spacedmonkey as a code owner August 5, 2022 02:18

andrewserong self-assigned this Aug 5, 2022

andrewserong added [Package] Style Engine /packages/style-engine [Type] Bug An existing feature does not function as intended labels Aug 5, 2022

andrewserong mentioned this pull request Aug 5, 2022

fix sanitization for style-engine declarations #42968

Closed

andrewserong requested review from ramonjd and aristath August 5, 2022 02:23

ramonjd reviewed Aug 5, 2022

View reviewed changes

ramonjd approved these changes Aug 5, 2022

View reviewed changes

ramonjd requested a review from Mamaduka August 5, 2022 04:53

aristath reviewed Aug 5, 2022

View reviewed changes

packages/style-engine/phpunit/class-wp-style-engine-css-declarations-test.php Outdated Show resolved Hide resolved

andrewserong commented Aug 8, 2022

View reviewed changes

andrewserong added 3 commits August 8, 2022 12:09

Style Engine: Include 6.1 CSS filter, ensure style engine can output …

166bfd0

…CSS functions like clamp

Remove display handling

4100d20

Remove esc_html call

45f4c41

andrewserong force-pushed the try/allow-css-functions-and-var-fallback branch from 62444ec to 45f4c41 Compare August 8, 2022 02:09

aristath reviewed Aug 8, 2022

View reviewed changes

packages/style-engine/class-wp-style-engine-css-declarations.php Outdated Show resolved Hide resolved

Remove spacer check

2260deb

ramonjd mentioned this pull request Aug 9, 2022

Style engine: permit wp custom CSS properties #43071

Merged

aristath approved these changes Aug 9, 2022

View reviewed changes

aristath merged commit 60adc79 into trunk Aug 9, 2022

aristath deleted the try/allow-css-functions-and-var-fallback branch August 9, 2022 08:01

github-actions bot added this to the Gutenberg 13.9 milestone Aug 9, 2022

This was referenced Aug 15, 2022

Style Engine: Tweak Declarations filtering logic slightly #43215

Merged

Tracking: extend layout options in Gutenberg #39336

Closed

ockham mentioned this pull request Aug 26, 2022

Plugin: Backport PHP changes for WordPress 6.1 release #43440

Closed

37 tasks

ramonjd mentioned this pull request Oct 14, 2022

CSS: backport the 6.1 Core kses regex changes to Gutenberg for safecss_filter_attr_allow_css #44961

Closed

andrewserong mentioned this pull request Oct 14, 2022

Update 6.1 CSS filter to match core #44962

Merged

ramonjd mentioned this pull request Sep 17, 2023

Footnotes: Fix writingMode (text orientation setting) on the front #54345

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Style Engine: Include 6.1 CSS filter, ensure style engine can output CSS functions like clamp #43004

Style Engine: Include 6.1 CSS filter, ensure style engine can output CSS functions like clamp #43004

andrewserong commented Aug 5, 2022 •

edited

Loading

ramonjd Aug 5, 2022

andrewserong Aug 5, 2022

andrewserong Aug 5, 2022

ramonjd Aug 5, 2022 •

edited

Loading

andrewserong Aug 5, 2022 •

edited

Loading

ramonjd commented Aug 5, 2022 •

edited

Loading

ramonjd left a comment

ramonjd Aug 5, 2022

andrewserong Aug 5, 2022

Mamaduka Aug 5, 2022

aristath Aug 5, 2022 •

edited

Loading

Mamaduka Aug 5, 2022

aristath Aug 5, 2022

andrewserong Aug 8, 2022

Mamaduka commented Aug 5, 2022

andrewserong Aug 8, 2022

andrewserong commented Aug 8, 2022 •

edited

Loading

andrewserong commented Aug 9, 2022

ramonjd commented Aug 9, 2022

Mamaduka commented Aug 9, 2022

andrewserong commented Aug 10, 2022

Style Engine: Include 6.1 CSS filter, ensure style engine can output CSS functions like clamp #43004

Style Engine: Include 6.1 CSS filter, ensure style engine can output CSS functions like clamp #43004

Conversation

andrewserong commented Aug 5, 2022 • edited Loading

What?

Why?

How?

Testing Instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonjd Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

andrewserong Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

ramonjd commented Aug 5, 2022 • edited Loading

ramonjd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aristath Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mamaduka commented Aug 5, 2022

Choose a reason for hiding this comment

andrewserong commented Aug 8, 2022 • edited Loading

andrewserong commented Aug 9, 2022

ramonjd commented Aug 9, 2022

Mamaduka commented Aug 9, 2022

andrewserong commented Aug 10, 2022

andrewserong commented Aug 5, 2022 •

edited

Loading

ramonjd Aug 5, 2022 •

edited

Loading

andrewserong Aug 5, 2022 •

edited

Loading

ramonjd commented Aug 5, 2022 •

edited

Loading

aristath Aug 5, 2022 •

edited

Loading

andrewserong commented Aug 8, 2022 •

edited

Loading