Editor: Fix link color CSS cascade conflict for duplicate blocks with identical markup#12126
Editor: Fix link color CSS cascade conflict for duplicate blocks with identical markup#12126USERSATOSHI wants to merge 16 commits into
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
… use WP_HTML_Tag_Processor and eliminate duplication
| * | ||
| * @covers ::wp_get_elements_class_name | ||
| */ | ||
| public function test_elements_block_support_styles_with_duplicate_blocks(): void { |
There was a problem hiding this comment.
Do we need this test? Is it not the same as in Tests_Block_Supports_WpRenderElementsSupport?
There was a problem hiding this comment.
It is almost the same, but they test different outputs.
the Tests_Block_Supports_WpRenderElementsSupport one checks for the html output
while this one checks for the styles part.
so something like <p class="wp-elements-abc123"> vs .wp-elements-abc123 a { color: blue; }
|
I took this as an opportunity to fix up some typing issues on code that touches |
| if ( ! $block_type ) { | ||
| return $parsed_block; | ||
| } |
There was a problem hiding this comment.
This condition handles a possible case where the parsed block doesn't exist in the registry.
| $actual_stylesheet = wp_style_engine_get_stylesheet_from_context( 'block-supports', array( 'prettify' => false ) ); | ||
|
|
||
| // Both rules should be present with distinct class names. | ||
| $this->assertMatchesRegularExpression( | ||
| '/\.wp-elements-[a-f0-9]{32}[0-9]+ a:where\(:not\(\.wp-element-button\)\)\{color:blue;\}/', | ||
| $actual_stylesheet, | ||
| 'First block element style should be present' | ||
| ); | ||
| $this->assertMatchesRegularExpression( | ||
| '/\.wp-elements-[a-f0-9]{32}[0-9]+ a:where\(:not\(\.wp-element-button\)\)\{color:blue;\}/', | ||
| $actual_stylesheet, | ||
| 'Second block element style should also be present' | ||
| ); |
There was a problem hiding this comment.
This doesn't make sense to me. It's asserting the same thing twice. In any case, doing a pattern match on the entire CSS rule seems maybe excessive and is liable to break when the selector changes. I think this can be simplified. See f944d13.
|
@t-hamano If this looks good to you, I can commit. |
t-hamano
left a comment
There was a problem hiding this comment.
Thanks for the PR! This PR works well in my tests.
I am wondering if a hash value is necessary at all if we are using wp_unique_prefixed_id(). I am thinking if we can simply do the following:
function wp_get_elements_class_name( $parsed_block ): string {
return wp_unique_prefixed_id( 'wp-elements-' );
}Since the CSS class name is guaranteed to be unique by wp_unique_prefixed_id(), there should be no need to generate a hash value. This simplifies CSS class names to be like wp-elements-1, wp-elements-2, etc.
This should match the implementation in the editor.
What do you think?
…m/USERSATOSHI/wordpress-develop into fix/elements-class-name-collision
…m/USERSATOSHI/wordpress-develop into fix/elements-class-name-collision
|
@USERSATOSHI Why did you revert the hash removal? |
|
After adding it, All the test started to fail and reverting that hash was the only way to make the tests pass. |
|
https://github.com/WordPress/wordpress-develop/actions/runs/27536856548/job/81388480559?pr=12126 action link after that commit |
Since we changed the logic for the CSS output, it's natural that the unit tests are failing. Can we fix the tests? |
|
sure, let me check this. |
|
Hi @t-hamano @westonruter , I just realized while updating the tests that we also dont need parsed_block parameter for the function now since it doesn't really on it. so do I also update this for all the places where this function is used? |
|
I'm starting to wonder if removing the arguments is truly the best approach, as I have concerns about backward compatibility. Although this function is marked as As far as I can think of, there are four approaches. @westonruter What do you think?
function wp_get_elements_class_name( $parsed_block ): string {
$hash = md5( serialize( $parsed_block ) );
return wp_unique_prefixed_id( 'wp-elements-' . $hash );
}
function wp_get_elements_class_name( $parsed_block ): string {
return wp_unique_prefixed_id( 'wp-elements-' );
}
function wp_get_elements_class_name(): string {
return wp_unique_prefixed_id( 'wp-elements-' );
}
function wp_get_elements_class_name(): string {
if ( func_num_args() > 0 ) {
_deprecated_argument(
__FUNCTION__,
'7.1.0',
__( 'The $parsed_block parameter is no longer used.' )
);
}
return wp_unique_prefixed_id( 'wp-elements-' );
} |
|
@t-hamano Either 3 or 4 looks good to me. It doesn't hurt to pass in an argument that isn't expected. Static analysis in an editor will flag it, but it won't be any kind of runtime error. So calling Is it worth it to warn them that they're passing in an unused argument at runtime? Probably not. It would also be annoying if they have to support multiple versions of WordPress, since then they'd have to do a version check for whether to pass in the So I'd say go with:
|
|
got it, I will start with this then! |
…on and simplify class name structure
t-hamano
left a comment
There was a problem hiding this comment.
Looks good to me from my end 👍
Trac ticket: https://core.trac.wordpress.org/ticket/65435
Description
This fixes a CSS cascade bug where identical block content produces duplicate
wp-elements-*class names, causing parent block element styles (e.g. link colors) to incorrectly override child block styles.Root cause
wp_get_elements_class_name()generates CSS class names viamd5( serialize( $block ) ).When two blocks have identical parsed data (e.g. two Paragraph blocks with the same text, one standalone and one inside a Group), they produce the same class.
The Style Engine deduplicates the rule, and since the parent Group's rule renders later in DOM order, it overrides the child Paragraph's style.
A contributing factor:
elements.phpis required beforelayout.phpinwp-settings.php, sowp_render_elements_support_styles()runs beforewp_add_parent_layout_to_parsed_block().If layout ran first, nested blocks would have a
parentLayoutkey, producing different hashes.Fix
Introduces a collision tracker via
static $seen_hashesinwp_get_elements_class_name(). When a hash collision is detected, an incrementing counter is appended before re-hashing, producing a unique class name per duplicate instance. This ensures the Style Engine outputs distinct CSS rules for each block instance, preserving correct cascade order.Testing
Added Test cases for these cases.
tests/phpunit/tests/block-supports/wpRenderElementsSupportStyles.php::test_elements_block_support_styles_with_duplicate_blockstests/phpunit/tests/block-supports/wpRenderElementsSupport.php::test_elements_block_support_class_with_duplicate_blocksScreenshots
Credits
References
Use of AI Tools
AI assistance: Yes
Tool(s): OpenCode
Model(s): deepseek-v4-flash-free
Used for: I used AI only for creating the test scaffolding. Code is reviewed and written by me.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.