Try adding layout classnames to inner block wrapper #44600
Conversation
tellthemachines
added
the
[Feature] Layout
|
Thanks for digging into this one! It looks like the WP_HTML_Tag_Processor class has been merged as of #42485 — would it be worth trying that out for this case, to avoid adding an additional regex? That class isn't slated to land until 6.2, so assuming that this feature is also for 6.2, I was wondering if it might be a good opportunity to see if the class holds up for this use case 🤔 |
I'll look into it! We should be able to use it for the part where we add the layout classes. I doubt if it'll work for identifying the inner wrapper though, as we're going by its position in the |
Ah, good point, yes, I forgot about the problem of flagging where the wrapper should ideally be! |
tellthemachines
force-pushed
the
try/layout-classnames-on-inner-wrapper
branch
from
f979e16
to
d358f0b
Compare
|
Update: got it working on the editor, but there's some duplication of classnames if the inner wrapper isn't the same as the outer one. I found we can't rely on InnerBlocks to add the classnames for all container blocks, because some don't use it (e.g. Gallery) and others only use it in certain views (e.g. PostContent). But I haven't found any reliable way to remove classes from the outer wrapper if they are applied to the inner already. Ideas appreciated! Still todo: look into using |
|
Size Change: +116 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
| @@ -231,7 +233,8 @@ function BlockListBlock( { | |||
| 'is-content-block': hasContentLockedParent && isContentBlock, | |||
| }, | |||
| dataAlign && themeSupportsLayout && `align${ dataAlign }`, | |||
| className | |||
| className, | |||
| layoutClassNames | |||
There was a problem hiding this comment.
We need to add these here for blocks that don't use InnerBlocks to render their children (the only examples I could find of this in Core are Gallery and PostContent when viewed in non-editable mode in the site editor, but there may be more). Unfortunately that means that if the block's inner wrapper is different from its outer, the layout classnames will be duplicated across both wrappers in the editor. There don't seem to be any consequences to this, as the blocks that currently have multiple wrappers and layout (e.g. Navigation) are already handling the styles being applied to their outer wrapper. But it would be nice to fix if anyone has ideas!
There was a problem hiding this comment.
Because the constrained layout and root padding features have rules that target the direct child of the layout classname, I think we might need to come up with a way where the layout classes are only applied to the deepest node (that is, the innerBlocks wrapper, or the outer wrapper if no inner wrapper is in use). I'm not quite sure how we ensure that, though! 😅
There was a problem hiding this comment.
The only alternative I can see here is to be deliberate about layout not supporting out of the box blocks that don't use InnerBlocks, and add some custom logic to make things work in Gallery and Post Content. I'll look into that!
tellthemachines
requested review from
spacedmonkey,
ellatrix and
ajitbohra
as code owners
lib/block-supports/layout.php
Outdated
|
|
||
| while ( true === $inner_content_chunk->next_tag() ) { | ||
| $inner_wrapper_classnames = $inner_content_chunk->get_attribute( 'class' ); | ||
| } |
There was a problem hiding this comment.
a couple thoughts on this function:
we're passing in inner blocks but then trying to get the first one. the additional list-handling here might not be helping the clarity of its purpose. if we passed in a block instead of a list of blocks, or instead of inner content, then it could focus on a more specific purpose.
function get_classes_from_last_tag( $html ) {
$tags = new WP_HTML_Tag_Processor( $html );
$last_classes = '';
while ( $tags->next_tag() ) {
$last_classes = $tags->get_attribute( 'class' ) || '';
}
return $last_classes;
}Secondly, while I think the code as-is is fine, we don't need the true === in the while condition because as a boolean value that's already implicit. No problem keeping it in there, but this isn't like functions which return a truthy value; it actually returns true or false.
Third, get_attribute( 'class' ) might return null if the last tag doesn't have a class attribute. The return type in the docblock comment says this function returns a string but unless we add a fallback it will also return null from time to time.
Fourth, a block's $inner_content is not guaranteed to contain only strings. It may contain null values where inner blocks belong. Another benefit to focusing this function on HTML processing and letting the calling code extract the HTML it wants to scan is not having to deal with this confusion here.
$inner_content_classnames = isset( $block['innerContent'][0] ) && 'string' === typeof $block['innerContent'][0]
? get_classes_from_last_tag( $block['innerContent'][0] )
: '';@adamziel I wonder; if we exposed the offset/position in the stream we could allow people to add their own "rewind" tracking and other more complicated parsing. would that be worth the risk it exposes?
$last_tag_at = null;
while ( $tags->next_tag() ) {
$last_tag_at = $tags->current_position();
}
if ( null === $last_tag_at ) {
return;
}
$tags->seek( $last_tag_at );
return $tags->get_attribute( 'class' ) || '';this could expose a lot of functionality it seems people want to do with this, as long as we don't expose the internal pointer, such that the offset points to an old version of the document, before it was changed by set_attribute() calls… 🤔
There was a problem hiding this comment.
Ooh, that's great feedback, thanks! Yes, the function is a lot neater in your example 😄
There was a problem hiding this comment.
@adamziel I wonder; if we exposed the offset/position in the stream we could allow people to add their own "rewind" tracking and other more complicated parsing. would that be worth the risk it exposes?
Hm hm hm. This worries me in the same way as exposing get_classes() worried you in the other thread. The cursor is an implementation detail and I'd rather not have people mess around with it – it would make their core fragile and severely constraint the future updates to the tag processor. For example, seek opens the following can of worms:
$tags->next_tag();
$tag_1_at = $tags->current_position();
$tags->next_tag();
$tag_2_at = $tags->current_position();
$tags->seek( $tag_1_at );
$tags->set_attribute( 'class', 'bold' );
// $tag_2_at no longer points where we expect it to
$tags->seek( $tag_2_at );
$tags->set_attribute( 'class', 'bold' );I'd rather keep an eye out for the use-cases that come up and see if we can address most of them with case-specific APIs. For example, this loop could benefit from has_next_tag():
while($tags->has_next_tag()) {
$tags->next_tag();
}
return $tags->get_attribute( 'class' );
lib/block-supports/layout.php
Outdated
| $content->add_class( esc_attr( implode( ' ', $class_names ) ) ); | ||
| } else { | ||
| $content->next_tag(); | ||
| $content->add_class( esc_attr( implode( ' ', $class_names ) ) ); |
There was a problem hiding this comment.
there's no need or benefit to running these through esc_attr() since add_class() already does this (by means of its call to set_attribute())
There was a problem hiding this comment.
Thanks, that's good to know!
|
|
||
| $content = new WP_HTML_Tag_Processor( $block_content ); | ||
| if ( $inner_content_classnames ) { | ||
| $content->next_tag( array( 'class_name' => $inner_content_classnames ) ); |
There was a problem hiding this comment.
this does what you want it to do, but it surprised me that it did, because we intended to search for individual class names and not the entire list of classes. it would be good to be aware that if we were to chop out one of the classes this would fail because the classes in $inner_content_classnames would no longer be an identical string of the same exact length as the class attribute in the searched HTML
There was a problem hiding this comment.
I think it's safe to take that risk here because we haven't changed anything in $block_content up until this point, and we don't have a better way of targeting the block's inner wrapper. Potentially, if we had a way to know ahead of time which tag is the last, we could add a classname to it instead of storing the existing ones, and then remove it after this step.
There was a problem hiding this comment.
@tellthemachines you could filter the tag openers manually:
$sought_class_names = $inner_content_classnames;
while( $content->next_tag() ) {
$tag_class_names = preg_split('/\s+/', $content->get_attribute( 'class' ));
if ( /* the two overlap */ ) {
// ...
}
}I dislike the preg_split, though. @dmsnell We already have get_attribute() in the processor, any reason not to add get_classes()?
There was a problem hiding this comment.
@adamziel my spidey-sense suggests it wouldn't be that much of an added value above get_attribute(). perhaps has_class() though would be.
what I don't want people to do is what we have in your code example. we provide semantic operations to add and remove CSS classes. if we're encouraging people to extract the raw value(s) and perform their own twiddling then we're just pushing them away from a robust framework and towards multiple ad-hoc implementations of the same thing.
it highlights the elephant in the room, which are class names with HTML entities. we may have to come back and revisit those; maybe we could find a way to detect if we think we have an entity (a character is &) and then rewind the class comparison with a cloned and decoded value.
point being is that I think still it's more ideal to focus on intended behaviors instead of internal mechanics. why do we want get_classes() when we can already query by tags containing a class name and get_attribute( 'class' )
There was a problem hiding this comment.
Such a good point @dmsnell, let's explore adding has_class. I added a note to the overview issue's description so we don't lose track of it.
There was a problem hiding this comment.
@getdave what is the good example in that bugfix link? what was the bug? I'm trying to understand how has_class() would be a help there.
taking a guess, it looks like the goal is to only seek out wp-block-navigation-item__content if there is a current-menu-item in the document, and I'm not sure where has_class() comes in there.
if we want to know if any inner block HTML contains the class, the tag processor already includes this functionality.
$inner_tags = new WP_HTML_Tag_Processor( $inner_blocks_html );
$has_current_menu_item = $inner_tags->next_tag( [ 'class_name' => 'current-menu-item' ] );Secondly, I don't know that the strpos() check is all that bad. If we're looking to make sure that we don't introduce needless slow-down, then doing a quick string search over the inner HTML for the possibility of finding the class name might be a good way to keep lots of requests fast while only resorting to proper HTML parsing when we have reason to believe the class might exist within.
There was a problem hiding this comment.
@dmsnell Using next_tag like that is very clever! And now that I think about it, it's also fully within what it was invented for, it just didn't jump out to me in the first place.
I don't know that the
strpos()check is all that bad.
Now that I think about it, the class_name (and tag_name) matchers could run that strpos check before trying to match tags. It would work well except for matching html entities.
There was a problem hiding this comment.
@adamziel apart from the HTML entities, which we currently don't match, the strpos speedup might warrant more justification before putting it in the tag processor.
on the one hand, strpos() is going to be faster if the wanted class isn't in plaintext in the document, but on the other hand, it scans up to the end of the document every time, and if we expect to find another tag "soon" after where the current pointer is, there's no need to repeatedly iterate through the document.
in terms of the general case I'd really want measured data showing that the additional complexity is worth it. my guess is that sometimes it would be faster and sometimes it would be slower, but in all cases it would add additional complexity.
There was a problem hiding this comment.
There was a problem hiding this comment.
This is looking very cool @tellthemachines, thanks for digging in! It's going to add a lot of nice flexibility to the cover block 👍
The server-rendering appeared to be working pretty well in my testing so far, but I ran into an issue with the edit view, though, because of the duplicate output of layout classnames. The constrained and root padding aware rules target the direct children of the layout classnames, and so assume that they're only going to be added on the direct wrapper of child blocks. This isn't so noticeable with the flex layout, but if we set the cover block to constrained, then in the editor view, the cover block's span winds up being constrained unexpectedly since the wp-container-$id class is added to the outer wrapper as well as the inner wrapper. On the site frontend, where the classnames are only injected on the inner wrapper, the issue isn't present:
| Editor (layout classnames are on both wrappers) | Site frontend (layout classnames are only on inner wrapper |
|---|---|
 |
 |
I've added a couple of comments, but from a quick look, I can't quite work out an alternative — from your comment about the the Gallery and PostContent blocks, it sounds like we can't rely on only outputting layoutClassNames in useInnerBlocksProps?
One of the things I was wondering about, is if there are particular blocks that have edge cases — is that something that can be solved for those individual blocks (the Post Content block is already quite an unusual one), or is it better to see if we can ensure the API for this catches all cases?
| @@ -231,7 +233,8 @@ function BlockListBlock( { | |||
| 'is-content-block': hasContentLockedParent && isContentBlock, | |||
| }, | |||
| dataAlign && themeSupportsLayout && `align${ dataAlign }`, | |||
| className | |||
| className, | |||
| layoutClassNames | |||
There was a problem hiding this comment.
Because the constrained layout and root padding features have rules that target the direct child of the layout classname, I think we might need to come up with a way where the layout classes are only applied to the deepest node (that is, the innerBlocks wrapper, or the outer wrapper if no inner wrapper is in use). I'm not quite sure how we ensure that, though! 😅
| //Dedupe layout classes | ||
| const allTheClassNames = `${ props.className } ${ layoutClassNames }`; | ||
| const classNameSet = new Set( allTheClassNames.split( ' ' ) ); | ||
| const dedupedClassNames = Array.from( classNameSet ).join( ' ' ); |
There was a problem hiding this comment.
Rather than de-duping after the fact, is there a way to ensure that layout classnames are only ever added once? I'm not quite sure how we'd do this, but ideally we'd only be outputting layoutClassNames in one place, with preference for the inner blocks wrapper where present. Since useInnerBlocksProps occurs after useBlockProps, I can't think of a clever way to do that, though 🤔
There was a problem hiding this comment.
This is related to the comment above: the duplication here happens because we're adding layout classnames to BlockListBlock. If we don't do that, this won't be a problem.
I can't think of a clever way to do that, though
Can you think of any non-clever ways? 😅
There was a problem hiding this comment.
The only non-clever thing I could come up with was to avoid useBlockProps and useInnerBlocksProps altogether, and create a separate hook that grabs layoutClassNames from the block edit context (touched on in this comment).
It would mean saying, something along the lines of, "hey, the other block supports happen automatically, but if you want to use layout, you've got to use the layout hook to grab the stuff you need, and then output it where it should go" 😅 . If it worked, I'm wondering if something like the following pseudo-code might get us in somewhere in that direction:
// layout classnames hook
function useLayoutClassNames( props ) {
const { layoutClassNames = '' } = useBlockEditContext();
let mergedClassnames = '';
// insert code here to merge props.className with layoutClassNames
return mergedClassnames;
}
// a block's edit component
function MyBlockEdit( props ) {
const blockProps = useBlockProps();
const innerBlocksProps = useInnerBlocksProps( blockProps );
// The following gets the layout class names and merges them with the classnames from the provided props
const layoutClassNames = useLayoutClassNames( innerBlocksProps );
return <div { ...blockProps }>
<div { ...innerBlocksProps } className={ layoutClassNames } />
</div>Not sure if something like that's at all viable, but it means that we'd have a hook for getting / merging layout classnames, and the block's Edit component would then be responsible for where to put them. It would mean that becomes part of the API of the layout support for blocks that want to use it, so probably not something that's all the desirable, because it wouldn't "just work" in the way that we typically expect block supports to work.
But that's all I could think of for now!
|
Just thinking out-loud, but would another potential alternative for the edit view side of things be to create a separate The downside is that layout classnames wouldn't be automatically handled by That might just wind up creating more boilerplate, though, so possibly not be a practical option 🤔 |
|
Testing this as well. Looking great so far! This is what I'm seeing: 2022-10-13.12.02.11.mp4
I noticed this as well. This is a screenshot of the frontend HTML, where the unique layout class that controls the max-width is not applied to all the cover block's children:
Does it sound too dodgy to move the unique layout class to the block container in We're already doing some magic with the inner container. I was testing it manually in the browser and it seemed to be okay. 😅 Also, if you think it's appropriate in this PR, it looks like there's another opportunity to use the Something like: $p = new WP_HTML_Tag_Processor( $content );
$p->next_tag();
$p->set_attribute( 'style', $styles ); |
For that one, I thought the server-rendered view looked a bit better, as the |
Oh good point. My only reaction was that the editor and frontend weren't same when adding a constrained content size, so either way... 😄 |
|
Thanks everyone for all the feedback and testing!
I'm starting to think it's better to optimise for blocks that use InnerBlocks, which is the canonical way to handle block nesting, and for edge case situations that require custom nesting logic, it won't be too much of a burden to handle layout manually as well.
This could be a useful tool for handling the edge cases! but I wouldn't make it the default solution. At least not if we want to move towards a Layout support that Just Works for the majority of use cases 😅 |
I added layout to Cover in this PR for testing purposes only, and plan to remove it before merging. Mainly because there's a bit of discussion going in #43140 around the best way to handle multiple layout types plus the matrix alignment tool. That sounds like a good candidate for a forthcoming Cover-specific PR though! |
Ah, we said much the same thing at the same time! 😄 Yes, I agree, it'd be good to see how far we can get with the "just works" approach, and what the edge-casey workarounds look like for the other blocks 👍. In terms of API, perhaps we can think of it as the part that "just works", and then additional tools / hooks, etc, for edge-casey blocks to use? |
|
Update: turns out the edge case workarounds were pretty easy, due to |
There was a problem hiding this comment.
This is testing really well for me @tellthemachines, thanks for honing in on the final changes, it feels nice and clean and delivers on the promise of trying to get as close as possible to a "just works" approach for attaching layout classnames and styles to the inner blocks wrapper, and falling back to the outer wrapper where there is no inner blocks wrapper 👍
It feels like a good step toward a consistent API for stabilising the layout block support, too, so I think this would be a good approach to go with.
I've left a couple of questions on the PHP side of things to make sure I'm following along correctly 🙂
Also, I'd just like to ping @youknowriad if he has time to take a look, too, as I remember many earlier discussions in the development of the layout support, and mentions of one day moving to targeting the innerBlocks wrapper. So it might be good to confidence check that the approach is consistent with the earlier wish-list of how it might work 🙂.
This is very cool, though, I'm really excited about this one! 😀
| $block_content, | ||
| 1 | ||
| ); | ||
| $inner_content_classnames = isset( $block['innerContent'][0] ) && 'string' === gettype( $block['innerContent'][0] ) ? gutenberg_get_classnames_from_last_tag( $block['innerContent'][0] ) : ''; |
There was a problem hiding this comment.
Is it worth adding a comment to explain what the 0 index of innerContent is, since it's sort of internal to the structure of the block object? E.g. something along the lines of:
// Attempt to extract classnames from last tag in block markup directly preceding first inner blocks marker.
// This identifies the first, deepest nested inner blocks wrapper.
Please correct me, though, if I've misunderstood how this is working, I just looked up the block parser class, so I think that's what it's doing, but could very well be wrong 🙂
From the comment in that class, it sounds like there's the potential for blocks to have another string between null markers (on this line of the comments). I've only ever encountered an opening string, multiple null values for each of the inner blocks, and then a closing string, so I wonder if there are any blocks that do that in practice? If so, I reckon we could look at that if and when we encounter it and then come back to the logic in this function to re-examine it, rather than worry about it now.
For now, looking at the last tag in the opening string sounds good to me!
There was a problem hiding this comment.
Yes, that's exactly how it works! Good idea to add a comment, as it's not immediately obvious.
| @@ -397,7 +413,7 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) { | |||
| $should_skip_gap_serialization = gutenberg_should_skip_block_supports_serialization( $block_type, 'spacing', 'blockGap' ); | |||
|
|
|||
| $style = gutenberg_get_layout_style( | |||
| ".$block_classname.$container_class", | |||
| ".$container_class.$container_class", | |||
There was a problem hiding this comment.
This change looks good to me. Is the reason we needed to do it because in the JS side of things we won't always have access to the block classname, is that right? Either way, I think the block classname was only there to increase specificity of this selector, so the change sounds good to me.
There was a problem hiding this comment.
This is needed because the block class is attached to the outer wrapper, and because we're now adding the container class to the inner wrapper this style block will no longer work with that combination.
tellthemachines
force-pushed
the
try/layout-classnames-on-inner-wrapper
branch
from
61305ff
to
4074881
Compare
|
Hi guys, I just discovered that this PR causes an issue for extenders. This docs page details how you can add custom classes to blocks. With this PR, any custom classes are added to both the main container as well as the inner block wrapper. |
Why?
As a prequel to #43140 we need a way to attach layout-related classnames to the inner block wrapper, instead of the outer (they are not always the same).
How?
Does some searching and matching to work out where the inner wrapper classnames are, and attach layout classes to those.
Testing Instructions
I added layout to the Cover block so we can test this is working. It doesn't have to stay as part of this PR; we can just focus on the mechanism for adding classes and then deal with block-specific stuff later. But for now this can be tested by adding a Cover block with some content and testing its layout controls.
Screenshots or screencast