Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Try adding layout classnames to inner block wrapper #44600

Merged
merged 15 commits into from
Oct 26, 2022
Merged

Try adding layout classnames to inner block wrapper #44600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7e34fb5
Try adding layout support to Cover block.
tellthemachines Aug 23, 2022
a29bd4a
Try adding layout classnames to inner block wrapper.
tellthemachines Sep 30, 2022
d291f82
Make it work in the editor
tellthemachines Oct 6, 2022
1315784
Use WP_HTML_Tag_Processor to add layout classnames
tellthemachines Oct 12, 2022
0d0f24f
Use WP_HTML_Tag_Processor to identify inner wrapper classnames
tellthemachines Oct 12, 2022
1327338
Improvements to inner content classname logic
tellthemachines Oct 13, 2022
95a330b
Only add layout classnames to inner blocks
tellthemachines Oct 13, 2022
0ad7274
Manually add layout classnames to Gallery and Post Content edit
tellthemachines Oct 13, 2022
03757a2
Add comment to inner content classnames logic.
tellthemachines Oct 14, 2022
06ad2b5
Mark layoutClassnames unstable
tellthemachines Oct 24, 2022
8295cbd
Add classnames to container in a loop.
tellthemachines Oct 24, 2022
d34bb68
Remove layout support for Cover block.
tellthemachines Oct 24, 2022
e87b92b
Apply layout to outer wrapper if legacy Group
tellthemachines Oct 24, 2022
4074881
Off switch for layout classes in inner wrapper
tellthemachines Oct 25, 2022
5381d2b
Change unstable prop name
tellthemachines Oct 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 35 additions & 12 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,23 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
return '';
}

/**
* Gets classname from last tag in a string of HTML.
*
* @param string $html markup to be processed.
* @return string String of inner wrapper classnames.
*/
function gutenberg_get_classnames_from_last_tag( $html ) {
$tags = new WP_HTML_Tag_Processor( $html );
$last_classnames = '';

while ( $tags->next_tag() ) {
$last_classnames = $tags->get_attribute( 'class' );
}

return $last_classnames;
}

/**
* Renders the layout config to the block wrapper.
*
Expand Down Expand Up @@ -320,7 +337,6 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {

$class_names = array();
$layout_definitions = _wp_array_get( $global_layout_settings, array( 'definitions' ), array() );
$block_classname = wp_get_block_default_classname( $block['blockName'] );
$container_class = wp_unique_id( 'wp-container-' );
$layout_classname = '';

Expand Down Expand Up @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$used_layout,
$has_block_gap_support,
$gap_value,
Expand All @@ -412,16 +428,22 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
}
}

/*
* This assumes the hook only applies to blocks with a single wrapper.
* A limitation of this hook is that nested inner blocks wrappers are not yet supported.
*/
$content = preg_replace(
'/' . preg_quote( 'class="', '/' ) . '/',
'class="' . esc_attr( implode( ' ', $class_names ) ) . ' ',
$block_content,
1
);
/**
* The first chunk of innerContent contains the block markup up until the inner blocks start.
* We want to target the opening tag of the inner blocks wrapper, which is the last tag in that chunk.
*/
$inner_content_classnames = isset( $block['innerContent'][0] ) && 'string' === gettype( $block['innerContent'][0] ) ? gutenberg_get_classnames_from_last_tag( $block['innerContent'][0] ) : '';
Copy link
Contributor

@andrewserong andrewserong Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's exactly how it works! Good idea to add a comment, as it's not immediately obvious.


$content = new WP_HTML_Tag_Processor( $block_content );
if ( $inner_content_classnames ) {
$content->next_tag( array( 'class_name' => $inner_content_classnames ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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' )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmsnell @aristath @getdave here's a PR that adds has_class #46232

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@adamziel adamziel Dec 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WordPress/wordpress-develop#5096

foreach ( $class_names as $class_name ) {
$content->add_class( $class_name );
}
} else {
$content->next_tag();
$content->add_class( implode( ' ', $class_names ) );
}

return $content;
}
Expand All @@ -433,6 +455,7 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
'register_attribute' => 'gutenberg_register_layout_support',
)
);

if ( function_exists( 'wp_render_layout_support_flag' ) ) {
remove_filter( 'render_block', 'wp_render_layout_support_flag' );
}
Expand Down
3 changes: 2 additions & 1 deletion packages/block-editor/src/components/block-edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,12 @@ import { BlockEditContextProvider, useBlockEditContext } from './context';
export { useBlockEditContext };

export default function BlockEdit( props ) {
const { name, isSelected, clientId } = props;
const { name, isSelected, clientId, __unstableLayoutClassNames } = props;
const context = {
name,
isSelected,
clientId,
__unstableLayoutClassNames,
};
return (
<BlockEditContextProvider
Expand Down
2 changes: 2 additions & 0 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ function BlockListBlock( {
isSelected,
isSelectionEnabled,
className,
__unstableLayoutClassNames: layoutClassNames,
name,
isValid,
attributes,
Expand Down Expand Up @@ -146,6 +147,7 @@ function BlockListBlock( {
clientId={ clientId }
isSelectionEnabled={ isSelectionEnabled }
toggleSelection={ toggleSelection }
__unstableLayoutClassNames={ layoutClassNames }
/>
);

Expand Down
6 changes: 5 additions & 1 deletion packages/block-editor/src/components/inner-blocks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,9 @@ const ForwardedInnerBlocks = forwardRef( ( props, ref ) => {
* @see https://github.com/WordPress/gutenberg/blob/HEAD/packages/block-editor/src/components/inner-blocks/README.md
*/
export function useInnerBlocksProps( props = {}, options = {} ) {
const { clientId } = useBlockEditContext();
const { __unstableDisableLayoutClassNames } = options;
const { clientId, __unstableLayoutClassNames: layoutClassNames = '' } =
useBlockEditContext();
const isSmallScreen = useViewportMatch( 'medium', '<' );
const { __experimentalCaptureToolbars, hasOverlay } = useSelect(
( select ) => {
Expand Down Expand Up @@ -200,12 +202,14 @@ export function useInnerBlocksProps( props = {}, options = {} ) {
innerBlocksProps.value && innerBlocksProps.onChange
? ControlledInnerBlocks
: UncontrolledInnerBlocks;

return {
...props,
ref,
className: classnames(
props.className,
'block-editor-block-list__layout',
__unstableDisableLayoutClassNames ? '' : layoutClassNames,
{
'has-overlay': hasOverlay,
}
Expand Down
16 changes: 7 additions & 9 deletions packages/block-editor/src/hooks/layout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,7 @@ import { kebabCase } from 'lodash';
*/
import { createHigherOrderComponent, useInstanceId } from '@wordpress/compose';
import { addFilter } from '@wordpress/hooks';
import {
getBlockDefaultClassName,
getBlockSupport,
hasBlockSupport,
} from '@wordpress/blocks';
import { getBlockSupport, hasBlockSupport } from '@wordpress/blocks';
import { useSelect } from '@wordpress/data';
import {
Button,
Expand Down Expand Up @@ -366,9 +362,8 @@ export const withLayoutStyles = createHigherOrderComponent(
const layoutClasses = hasLayoutBlockSupport
? useLayoutClasses( block )
: null;
const selector = `.${ getBlockDefaultClassName(
name
) }.wp-container-${ id }`;
// Higher specificity to override defaults from theme.json.
const selector = `.wp-container-${ id }.wp-container-${ id }`;
const blockGapSupport = useSetting( 'spacing.blockGap' );
const hasBlockGapSupport = blockGapSupport !== null;

Expand Down Expand Up @@ -413,7 +408,10 @@ export const withLayoutStyles = createHigherOrderComponent(
/>,
element
) }
<BlockListBlock { ...props } className={ className } />
<BlockListBlock
{ ...props }
__unstableLayoutClassNames={ className }
/>
</>
);
}
Expand Down
2 changes: 2 additions & 0 deletions packages/block-library/src/gallery/gallery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export const Gallery = ( props ) => {
mediaPlaceholder,
insertBlocksAfter,
blockProps,
__unstableLayoutClassNames: layoutClassNames,
} = props;

const { align, columns, caption, imageCrop } = attributes;
Expand All @@ -42,6 +43,7 @@ export const Gallery = ( props ) => {
{ ...innerBlocksProps }
className={ classnames(
blockProps.className,
layoutClassNames,
'blocks-gallery-grid',
{
[ `align${ align }` ]: align,
Expand Down
12 changes: 10 additions & 2 deletions packages/block-library/src/group/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,12 @@ const htmlElementMessages = {
),
};

function GroupEdit( { attributes, setAttributes, clientId } ) {
function GroupEdit( {
attributes,
setAttributes,
clientId,
__unstableLayoutClassNames: layoutClassNames,
} ) {
const { hasInnerBlocks, themeSupportsLayout } = useSelect(
( select ) => {
const { getBlock, getSettings } = select( blockEditorStore );
Expand All @@ -55,7 +60,9 @@ function GroupEdit( { attributes, setAttributes, clientId } ) {
const { type = 'default' } = usedLayout;
const layoutSupportEnabled = themeSupportsLayout || type === 'flex';

const blockProps = useBlockProps();
const blockProps = useBlockProps( {
className: ! layoutSupportEnabled ? layoutClassNames : null,
} );

const innerBlocksProps = useInnerBlocksProps(
layoutSupportEnabled
Expand All @@ -67,6 +74,7 @@ function GroupEdit( { attributes, setAttributes, clientId } ) {
? undefined
: InnerBlocks.ButtonBlockAppender,
__experimentalLayout: layoutSupportEnabled ? usedLayout : undefined,
__unstableDisableLayoutClassNames: ! layoutSupportEnabled,
}
);

Expand Down
12 changes: 8 additions & 4 deletions packages/block-library/src/post-content/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ function Content( props ) {
);
}

function Placeholder() {
const blockProps = useBlockProps();
function Placeholder( { layoutClassNames } ) {
const blockProps = useBlockProps( { className: layoutClassNames } );
return (
<div { ...blockProps }>
<p>
Expand Down Expand Up @@ -118,7 +118,11 @@ function RecursionError() {
);
}

export default function PostContentEdit( { context, attributes } ) {
export default function PostContentEdit( {
context,
attributes,
__unstableLayoutClassNames: layoutClassNames,
} ) {
const { postId: contextPostId, postType: contextPostType } = context;
const { layout = {} } = attributes;
const hasAlreadyRendered = useHasRecursion( contextPostId );
Expand All @@ -132,7 +136,7 @@ export default function PostContentEdit( { context, attributes } ) {
{ contextPostId && contextPostType ? (
<Content context={ context } layout={ layout } />
) : (
<Placeholder />
<Placeholder layoutClassNames={ layoutClassNames } />
) }
</RecursionProvider>
);
Expand Down