Post Featured Image: Shadows not applied to block instances #62207

t-hamano · 2024-06-02T12:52:53Z

Follow up #59616

What?

This PR fixes an issue where the shadow is not applied to the Post Featured block under the following conditions:

Apply shadow to block instance
On the front end

Why?

In the current implementation, CSS is generated via the style engine in the render callback function.

However, the value returned from the style engine is as follows and does not contain css key:

array(1) {
  ["declarations"]=>
  array(1) {
    ["box-shadow"]=>
    string(65) "6px 6px 0px -3px rgba(255, 255, 255, 1), 6px 6px rgba(0, 0, 0, 1)"
  }
}

How?

Shadow is just a CSS value, and there is no class like has-shadow. In this case, I don't think there is any need to use the style engine.

Testing Instructions

Insert a Post Featured Image block and apply a shadow.
The shadow will be applied on the front end.
Bonus: In the global style, apply a shadow to the Post Featured Image block. That shadow should be overwritten by the block instance shadow.

github-actions · 2024-06-02T12:53:53Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: t-hamano <[email protected]>
Co-authored-by: andrewserong <[email protected]>
Co-authored-by: ramonjd <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-06-02T12:54:09Z

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ packages/block-library/src/post-featured-image/index.php

github-actions · 2024-06-02T13:17:34Z

Flaky tests detected in b06dfbb.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9338411881
📝 Reported issues:

[Flaky Test] User Mention: should allow newlines after multiple mention completions #48537 in /test/e2e/specs/editor/various/autocomplete-and-mentions.spec.js

andrewserong

Thanks for looking into this — unfortunately I'm not able to reproduce the bug on trunk. When setting an individual featured image block to use a shadow (either one of the default shadows, or a custom shadow defined in the site editor's global styles), I can see the shadow output in the site frontend, and the css key is returned by the style engine.

CSS key:

Site frontend

Bundled shadow	Custom shadow in global styles

I'm testing with GB trunk in a test site running WP 6.5.3, and with TT4 theme. Do you have any additional testing instructions that might help us debug this?

Rather than merge this PR, I'd be keen for us to figure out in which conditions the CSS key isn't being returned by this style engine call, in case this fix masks another issue, and since I haven't been able to reproduce the problem so far.

CC: @ramonjd for visibility since this is style engine related 😅

ramonjd · 2024-06-03T04:01:55Z

Thanks for the ping, and for the PR @t-hamano

I can replicate with a hardcoded value, e.g.,

		$shadow_styles = wp_style_engine_get_styles( array( 'shadow' => '6px 6px 0px -3px rgba(255, 255, 255, 1), 6px 6px rgba(0, 0, 0, 1)' ) );

Rather than merge this PR, I'd be keen for us to figure out in which conditions the CSS key isn't being returned by this style engine call, in case this fix masks another issue, and since I haven't been able to reproduce the problem so far.

Good call.

It seems to be rather an issue with safecss_filter_attr()

It doesn't pass the following test:

			/*
			 * Allow CSS functions like var(), calc(), etc. by removing them from the test string.
			 * Nested functions and parentheses are also removed, so long as the parentheses are balanced.
			 */
			$css_test_string = preg_replace(
				'/\b(?:var|calc|min|max|minmax|clamp|repeat)(\((?:[^()]|(?1))*\))/',
				'',
				$css_test_string
			);

			/*
			 * Disallow CSS containing \ ( & } = or comments, except for within url(), var(), calc(), etc.
			 * which were removed from the test string above.
			 */
			$allow_css = ! preg_match( '%[\\\(&=}]|/\*%', $css_test_string );

See: https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/kses.php#L2606

From looking, it's because WordPress doesn't allow rgba()

I think WordPress should allow rgba() values:

it already does for gradients.
rgba has been widely supported for many years https://caniuse.com/?search=rgba

So I reckon, rather than hardcode a fix only for one block, let's fix it every where by adding a hook on safecss_filter_attr_allow_css and creating a Core patch.

ramonjd · 2024-06-03T04:17:33Z

Ah, I think it's because it's matching \( in this regex check:

$allow_css = ! preg_match( '%[\\\(&=}]|/\*%', $css_test_string );

andrewserong · 2024-06-03T04:37:35Z

ramonjd · 2024-06-03T04:41:35Z

So something like this might work:

Just allowing RGB(A)

if ( ! function_exists( 'gutenberg_safecss_filter_attr_allow_rgb' ) ) {
	/**
	 * Filters the check for unsafe CSS in `safecss_filter_attr` to allow `rgba()`.
	 *
	 * @param bool   $allow_css       Whether the CSS in the test string is considered safe.
	 * @param string $css_test_string The CSS string to test.
	 */
	function gutenberg_safecss_filter_attr_allow_rgb( $allow_css, $css_test_string ) {
		/*
		* Allow CSS functions like var(), calc(), etc. by removing them from the test string.
		* Nested functions and parentheses are also removed, so long as the parentheses are balanced.
		*/
		$css_test_string = preg_replace(
			// SINGLE CHANGE HERE: adding `|rgb[a]?`
			'/\b(?:var|calc|min|max|minmax|clamp|repeat|rgb[a]?)(\((?:[^()]|(?1))*\))/',
			'',
			$css_test_string
		);
		/*
		 * Disallow CSS containing \ ( & } = or comments, except for within url(), var(), calc(), etc.
		 * which were removed from the test string above.
		 */
		return ! preg_match( '%[\\\(&=}]|/\*%', $css_test_string );
	}
}

add_filter( 'safecss_filter_attr_allow_css', 'gutenberg_safecss_filter_attr_allow_rgb', 10, 2 );

It will allow rgb[a] values everywhere, e.g., color: rgba(0, 0, 0, 0.1) so it's pretty greedy.

Therefore we should check whether we need something stricter for rgba values, e.g., that they are indeed numeric.

With extra checks for RGB(A)

if ( ! function_exists( 'gutenberg_safecss_filter_attr_allow_rgb' ) ) {
	/**
	 * Filters the check for unsafe CSS in `safecss_filter_attr` to allow `rgba()`.
	 *
	 * @param bool   $allow_css       Whether the CSS in the test string is considered safe.
	 * @param string $css_test_string The CSS string to test.
	 */
	function gutenberg_safecss_filter_attr_allow_rgb( $allow_css, $css_test_string ) {
		$parts     = explode( ':', $css_test_string, 2 );
		$css_value = trim( $parts[1] );
		$has_rgb   = str_contains( $css_value, 'rgba(' ) || str_contains( $css_value, 'rgb(' );
		
		// Check that rgb(a) values are digits.
		if ( $has_rgb ) {
			$regex = '/rgba?\(\s*\d{1,3},\s*\d{1,3},\s*\d{1,3}(,\s*\d(\.\d+)?)?\s*\)/';
			if ( preg_match_all( $regex, $css_value ) ) {
				$css_test_string = str_replace( $css_value, '', $css_test_string );
			}
		}

		/*
		 * The code below is taken from Core: wp-includes/kses.php.
		 * It does not need backporting.
		 */

		/*
		 * Allow CSS functions like var(), calc(), etc. by removing them from the test string.
		 * Nested functions and parentheses are also removed, so long as the parentheses are balanced.
		 */
		$css_test_string = preg_replace(
			'/\b(?:var|calc|min|max|minmax|clamp|repeat)(\((?:[^()]|(?1))*\))/',
			'',
			$css_test_string
		);

		/*
		 * Disallow CSS containing \ ( & } = or comments, except for within url(), var(), calc(), etc.
		 * which were removed from the test string above.
		 */
		$allow_css = ! preg_match( '%[\\\(&=}]|/\*%', $css_test_string );
	}
}

add_filter( 'safecss_filter_attr_allow_css', 'gutenberg_safecss_filter_attr_allow_rgb', 10, 2 );

ramonjd · 2024-06-03T04:51:26Z

I haven't tested it, but would it work to add rgb and rgba to the list of functions a bit higher up

Yeah, I was just testing that

It's probably good enough.

It'll also open up rgb values for all properties. I think this is fine.

But if we want to add an extra layer of security, then the rgb value can also be checked, e.g.,

		// Check that rgb(a) values are digits.
		if ( $has_rgb ) {
			$regex = '/rgba?\(\s*\d{1,3},\s*\d{1,3},\s*\d{1,3}(,\s*\d(\.\d+)?)?\s*\)/';
			if ( preg_match_all( $regex, $css_value ) ) {
				$css_test_string = str_replace( $css_value, '', $css_test_string );
			}
		}

t-hamano · 2024-06-03T09:45:26Z

Thanks for the detailed advice! I should have looked deeper 😅

I looked into why this issue was difficult to reproduce and found that normally the shadow preset is applied with CSS variables just like other presets:

<!-- wp:post-featured-image {"style":{"shadow":"var:preset|shadow|outlined"}} /-->

However, after updating the shadow preset or applying the shadow several times, I found that somehow it changed to a hardcoded value with rgba values like the following (maybe I should investigate this issue more deeply):

<!-- wp:post-featured-image {"style":{"shadow":"6px 6px 0px rgba(0, 0, 0, 0.2)"}} /-->

I agree with you that a more fundamental approach is needed as you suggested.

While looking through Trac, I found a changeset that allows rgba for background colors: https://core.trac.wordpress.org/ticket/48376

In light of this approach, I am considering submitting a core patch limited to certain CSS properties. For example, the following code:

Patch Example

diff --git a/wp-includes/kses.php b/wp-includes/kses.php
index 8ab9afd5..99b049c6 100644
--- a/wp-includes/kses.php
+++ b/wp-includes/kses.php
@@ -2513,6 +2513,14 @@ function safecss_filter_attr( $css, $deprecated = '' ) {
                'background-image',
        );
 
+       /*
+        * CSS attributes that accept rgba data types.
+        *
+        */
+       $css_rgba_data_types = array(
+               'box-shadow'
+       );
+
        if ( empty( $allowed_attr ) ) {
                return $css;
        }
@@ -2528,6 +2536,7 @@ function safecss_filter_attr( $css, $deprecated = '' ) {
                $found           = false;
                $url_attr        = false;
                $gradient_attr   = false;
+               $rgba_attr       = false;
                $is_custom_var   = false;
 
                if ( ! str_contains( $css_item, ':' ) ) {
@@ -2546,6 +2555,7 @@ function safecss_filter_attr( $css, $deprecated = '' ) {
                                $found         = true;
                                $url_attr      = in_array( $css_selector, $css_url_data_types, true );
                                $gradient_attr = in_array( $css_selector, $css_gradient_data_types, true );
+                               $rgba_attr     = in_array( $css_selector, $css_rgba_data_types, true );
                        }
 
                        if ( $is_custom_var ) {
@@ -2588,6 +2598,14 @@ function safecss_filter_attr( $css, $deprecated = '' ) {
                        }
                }
 
+               if ( $found && $rgba_attr ) {
+                       $css_value = trim( $parts[1] );
+                       if ( preg_match( '/rgb(a)?\([^)]+\)/', $css_value ) ) {
+                               // Remove the whole `rgba` bit that was matched above from the CSS.
+                               $css_test_string = str_replace( $css_value, '', $css_test_string );
+                       }
+               }
+
                if ( $found ) {

Maybe it would be better to add an equivalent filter in Gutenberg after the core patch is comitted.

What do you think?

ramonjd · 2024-06-03T23:35:32Z

Thanks @t-hamano !!

In my opinion, a Core patch would be the way to go then we can decide if a filter in Gutenberg is necessary.

I'm happy to help out, or test when it's ready.

In light of this approach, I am considering submitting a core patch limited to certain CSS properties.

What do you think about allowing rgb values for all CSS properties, not just box-shadow? They can be used anywhere a color can be used.

andrewserong · 2024-06-03T23:58:21Z

What do you think about allowing rgb values for all CSS properties, not just box-shadow? They can be used anywhere a color can be used.

I think allowing everywhere would be the way to go. My understanding is that the safe CSS filters are about avoiding dangerous CSS values, not about validating CSS, so I think allowing rgb() or rgba() anywhere could be a simpler and pragmatic way to go.

t-hamano · 2024-06-09T04:43:00Z

I discovered there was a ticket (opened 11 years ago!) proposing to allow rgb(a): https://core.trac.wordpress.org/ticket/24157

I proposed to move the ticket forward, so I'd like to close this PR for now. If rgb(a) values are allowed in the core, I'll submit a PR to backport it to Gutenberg.

Thanks everyone for the advice!

ramonjd · 2024-06-10T22:28:17Z

I discovered there was a ticket (opened 11 years ago!) proposing to allow rgb(a): core.trac.wordpress.org/ticket/24157

Now that folks have had 11 years to think about it, I think the time is right!

Thanks for pursuing this @t-hamano

Post Featured Image: Shadows not applied to block instances

b06dfbb

t-hamano added [Type] Bug An existing feature does not function as intended Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta [Block] Post Featured Image Affects the Post Featured Image Block labels Jun 2, 2024

t-hamano self-assigned this Jun 2, 2024

t-hamano marked this pull request as ready for review June 2, 2024 12:53

t-hamano requested a review from ajitbohra as a code owner June 2, 2024 12:53

t-hamano mentioned this pull request Jun 2, 2024

Add Box Shadow support for featured image #59616

Merged

t-hamano requested review from andrewserong and aaronrobertshaw June 2, 2024 12:58

andrewserong reviewed Jun 3, 2024

View reviewed changes

t-hamano closed this Jun 9, 2024

ellatrix removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jun 18, 2024

ramonjd mentioned this pull request Jun 20, 2024

WIP Experiment: combine gradient and background image into background-image #60739

Draft

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Post Featured Image: Shadows not applied to block instances #62207

Post Featured Image: Shadows not applied to block instances #62207

t-hamano commented Jun 2, 2024 •

edited

Loading

github-actions bot commented Jun 2, 2024 •

edited

Loading

github-actions bot commented Jun 2, 2024

github-actions bot commented Jun 2, 2024

andrewserong left a comment

ramonjd commented Jun 3, 2024

ramonjd commented Jun 3, 2024

andrewserong commented Jun 3, 2024 •

edited

Loading

ramonjd commented Jun 3, 2024 •

edited

Loading

ramonjd commented Jun 3, 2024

t-hamano commented Jun 3, 2024

ramonjd commented Jun 3, 2024 •

edited

Loading

andrewserong commented Jun 3, 2024

t-hamano commented Jun 9, 2024

ramonjd commented Jun 10, 2024

Post Featured Image: Shadows not applied to block instances #62207

Post Featured Image: Shadows not applied to block instances #62207

Conversation

t-hamano commented Jun 2, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

github-actions bot commented Jun 2, 2024 • edited Loading

github-actions bot commented Jun 2, 2024

github-actions bot commented Jun 2, 2024

andrewserong left a comment

Choose a reason for hiding this comment

ramonjd commented Jun 3, 2024

ramonjd commented Jun 3, 2024

andrewserong commented Jun 3, 2024 • edited Loading

ramonjd commented Jun 3, 2024 • edited Loading

ramonjd commented Jun 3, 2024

t-hamano commented Jun 3, 2024

ramonjd commented Jun 3, 2024 • edited Loading

andrewserong commented Jun 3, 2024

t-hamano commented Jun 9, 2024

ramonjd commented Jun 10, 2024

t-hamano commented Jun 2, 2024 •

edited

Loading

github-actions bot commented Jun 2, 2024 •

edited

Loading

andrewserong commented Jun 3, 2024 •

edited

Loading

ramonjd commented Jun 3, 2024 •

edited

Loading

ramonjd commented Jun 3, 2024 •

edited

Loading