Ensure layout classnames are applied to the inner blocks wrapper and not to its siblings#77408
Conversation
…not to its siblings
|
Oh, hey, this is clever! Good find. Just to make sure I'm following correctly, this accumulates a stack of unclosed elements that should help us more reliably identify the inner blocks wrapper by attempting to grab the very last unclosed tag and walk our way back up. Is it possible to add some simple PHP tests to cover the cases that were breaking? I think it might be simply adding other cases to |
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Yes, we want to ensure the classnames are added to the last unclosed tag because that points to it being the actual inner block wrapper.
Good idea! |
|
Flaky tests detected in f795b39. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/24642763241
|
| * innerContent entry alongside the inner blocks, which makes it the | ||
| * inner-block container. Elements that open and close within this chunk | ||
| * are siblings that precede the inner blocks and should be ignored. | ||
| * The last unclosed element with a class attribute is the best candidate |
There was a problem hiding this comment.
Is this is the "last" as in last in a list, or "Innermost"? Just getting my head around things 😄
There was a problem hiding this comment.
It's the last open tag in the string of markup that is $block['innerContent'][0].
| * for the inner-block wrapper. | ||
| */ | ||
| $tag_stack = array(); | ||
| $html_void_elements = array( 'area', 'base', 'br', 'col', 'embed', 'hr', 'img', 'input', 'link', 'meta', 'param', 'source', 'track', 'wbr' ); |
There was a problem hiding this comment.
Anyway to use WP_HTML_Processor::is_void here? Or is it not the right use case?
There was a problem hiding this comment.
Oh I didn't know about that one! It should work.
There was a problem hiding this comment.
Updated! Much better.
andrewserong
left a comment
There was a problem hiding this comment.
Looking good! Thanks for adding the test, and this is smoke testing well with the Details block hacked in to add a class name to the summary element:
Tested with a bunch of different configurations of layouts including accordions, cover block, etc, and all working fine.
LGTM 🚀
…not to its siblings (#77408) Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: coreyworrell <coreyw@git.wordpress.org>
| while ( $first_chunk_processor->next_tag( array( 'tag_closers' => 'visit' ) ) ) { | ||
| if ( $first_chunk_processor->is_tag_closer() ) { | ||
| array_pop( $tag_stack ); | ||
| } elseif ( ! WP_HTML_Processor::is_void( $first_chunk_processor->get_tag() ) ) { |
There was a problem hiding this comment.
we also have “special atomic elements” like SCRIPT and STYLE and others which have no closing tag. they wouldn’t be likely, but some could show up, such as TEXTAREA.
a more proper solution would use the WP_HTML_Processor->expects_closer() method to see if a closing tag is meant to appear. that will handle all of the wild edge cases, including knowing that IMG is a void element in HTML, but not if it appears within an SVG or MATH element.
we can use depth to track which elements are open and closed and limit that to elements with a class because the depth increases before visiting and opening tag and decreases before visiting a closing tag.
$first_chunk_processor = WP_HTML_Processor::create_fragment( $first_chunk );
$open_classes = array();
$open_depths = array();
while ( $first_chunk_processor->next_token() ) {
if ( '#tag' !== $first_chunk_processor->get_token_type() ) {
continue;
}
$depth = $first_chunk_processor->get_current_depth();
if ( $first_chunk_processor->is_tag_closer() ) {
if ( $depth < ( end( $open_depths ) ?? 0 ) ) {
array_pop( $open_depths );
}
continue;
}
if ( null === ( $classes = $first_chunk_processor->class_list() ) ) {
continue;
}
$open_classes[] = iterator_to_array( $classes );
$open_depths[] = $depth;
}
$inner_block_wrapper_classes = end( $open_classes );this highlights another existing issue, which is that we’re looking for exact string matches of the class attribute. in this snippet I’ve tossed in a change which gives us the ability to match any element containing all of the class names in the class attribute, not just exact matches.
this could be further refined by limiting these to names that start with some prefix, like wp-
What?
Fixes #57242.
When there's an element inside a block container which is a sibling to its inner blocks, and that element has a classname, and the block has layout support, we need to ensure that the layout classnames are applied to the container and not to the inner block sibling. The logic in trunk finds the last element with a classname in the first chunk of
innerContent, which is the element right before the inner blocks start. But if this element has a closing tag in that first chunk, it's likely to be a sibling and not the container (it can still be the container if there are no inner blocks, in which case there should only be one chunk ofinnerContent, but in that case we have no way of distinguishing it from a sibling element so it's safer to keep the layout classes in the outer wrapper. I was also looking for a solution to #76235 but can't think of a reliable one that doesn't mess with dynamic content).This PR tries to check for a closing tag in the first chunk of
innerContentand not apply the layout classnames if it exists.Testing Instructions
This may be easier to test by checking out the branch locally and making changes to the Details block so that its
summaryelement has a classname.Check that when
summaryhas a classname, the layout classes don't get attached to it but stay on the outer wrapper.Use of AI Tools
Code generated with copilot; reviewed and tested by myself.