Add Layout support to Cover block

[tellthemachines]

What problem does this address?

In block themes, where each container block must specify its content width, Cover block has no settings to do so. This causes issues such as #37036.

An initial attempt to add Layout support to Cover (#37662) ran into trouble because of the complex markup structure of the block. The problem is similar to the Navigation block, which was addressed with a custom solution in #37473.

Additionally, the Cover block has a unique "Change content position" control in its toolbar, that allows aligning the content on a two dimensional axis that uses flexbox under the hood. If we migrate Cover to use Layout, it makes sense to replace this with the existing flex layout type.

What is your proposed solution?

We could use the same custom solution as for Navigation block. That would work well for the flex layout scenario, but we'd also like Cover to work with the default (or content width enabled) layout, and that might be problematic because the max-width values we need for content width can't be hardcoded into the block stylesheet.

(As an aside, we'll need to dust off the Layout type switcher that currently isn't in use anywhere). Design might be needed because we haven't looked at it in a while 😄

Ideally, we'd have a way to add the layout classes to the inner block wrapper (as mentioned on the original PR), so the layout styles would always work flawlessly, independent of block markup structure.

If #42485 goes forward, we could leverage a brand new Walker to do this, or we could modify the existing logic slightly, but either way it looks like we'll need some classname or other way to identify the inner block wrapper. Should we use a classname, do we want to output it in the front end? Or just use it as a temporary internal hook to identify inner block containers? What other uses could such a thing have? (I'm thinking it should be added to all inner block containers, so we can be consistent with the layout logic.)

Any further ideas and feedback welcome!

[andrewserong]

Should we use a classname, do we want to output it in the front end? Or just use it as a temporary internal hook to identify inner block containers? What other uses could such a thing have?

This probably isn't a short-term solution, but one of the thoughts I had in junction with the walker proposal in #42485 would be to see if we can use the feature level selectors idea in #42087 to specify the selector (in block.json) that targets the inner block wrapper for the layout support.

So, for the cover block, we might attempt to opt-in to the Layout support with something like the following in the block.json file:

  "supports": {
    "__experimentalLayout": {
      "__experimentalSelector": ".wp-block-cover__inner-container"
    }
  }

If no __experimentalSelector is provided, then the Layout support would use the outer wrapper as it does now.

Then, in layout.php we'd use the experimental selector to walk through the rendered markup until we find the desired selector, and use that as the point to inject the Layout classnames like is-layout-flex — because that's the main Layout classname where the rules are attached, we then wouldn't need to flag the inner wrapper with a separate classname. From the Layout support's perspective, it wouldn't care whether the block attaches is-layout-flex at the root of the block markup, or further down the hierarchy: wherever it's attached is where the layout rules begin.

That's just what I had in mind, though. Do you think there are any cases where (from the final markup on the rendered page) we'd need to explicitly flag the inner wrapper, or is it enough for block.json to tell the Layout support where to inject classnames?

[glendaviesnz]

It has been mentioned in the past that if it was possible to add a background image to the Group block then the actual Cover block could almost be deprecated and it becomes a variation of the Group block ... just putting the idea out there again in case it provides an easier way forward than adding layout to the current Cover 😃

[tellthemachines]

Thanks for the input folks!

This probably isn't a short-term solution, but one of the thoughts I had in junction with the walker proposal in #42485 would be to see if we can use the feature level selectors idea in #42087 to specify the selector (in block.json) that targets the inner block wrapper for the layout support.

That could work, though I'm more inclined towards a solution where layout Just Works 😄 for all cases, without having to specify a selector. (This hinges on the assumption that the inner block wrapper is always the element we need to attach layout classnames to, which I think is a reasonable one, even taking into account possible future layout types.)

My thinking is:

• Ideally we wouldn't need to couple layout settings with block markup. If we specify a selector for non-outward wrapper scenarios, we need to make sure that selector is unique to the inner wrapper.
• If classnames always target the inner wrapper, the logic is consistent for all blocks: less scope for things to go wrong.
• I haven't actually tried this, but we have this block filter that allows plugins to modify a block's block.json, so conceivably extenders could add layout support to any block. In this case, it makes for a nicer experience if it's not required to know what the block markup looks like in order for it to work.

It has been mentioned in the past that if it was possible to add a background image to the Group block then the actual Cover block could almost be deprecated and it becomes a variation of the Group block

There's been some experimenting with that, but nothing conclusive so far, and recent activity on #39427 and even (recent-ish) on #20193 seem to indicate Cover will remain with us for a while 😅

[andrewserong]

though I'm more inclined towards a solution where layout Just Works 😄 for all cases, without having to specify a selector.

Ah, so in that case, when the block markup is initially saved, it'd output an inner-blocks-wrapper classname of some kind (wherever that happens to sit in the DOM hierarchy), and we'd then target that consistent classname as the place where we'd inject Layout classnames in layout.php... something like that?

If we specify a selector for non-outward wrapper scenarios, we need to make sure that selector is unique to the inner wrapper.

That's a good point, too — if we start iterating over nested markup (particularly nested wrapper blocks), there could be weird edge cases if the outer block is missing a classname but there's some inner block that potentially already contains a wrapper classname? It sounds like the logic will need to be quite careful once we start moving away from just the outer wrapper 🤔

[tellthemachines]

Ah, so in that case, when the block markup is initially saved, it'd output an inner-blocks-wrapper classname of some kind (wherever that happens to sit in the DOM hierarchy), and we'd then target that consistent classname as the place where we'd inject Layout classnames in layout.php... something like that?

Yeah, I was thinking we could add either a classname or a data attribute to the inner blocks wrapper that we could use to target with the layout classes, and maybe remove it before outputting the actual markup so it doesn't fall into that classnames-as-public-API category 😅

[andrewserong]

remove it before outputting the actual markup so it doesn't fall into that classnames-as-public-API category 😅

Yeah, good point! I suppose that's one of the things I liked about the __experimentalSelector, but I think you're right in that it'd be great to be able to write a block, use the useInnerBlocksProps hook and have the Layout support magically work, rather than having to manually add a classname both there and then in block.json before the Layout support can figure out what it's doing.

I think I'd probably lean slightly toward making the block developer do the heavy lifting rather than having to mutate the saved markup before output... but then again, we are already mutating things by injecting classnames! I think you've captured the desired state of thing well, though.

[tellthemachines]

I looked into this a bit today, and I think we could add a render_block_data filter in layout.php along these lines:

function layoutz_on_inner_block_wrapper_pls( $parsed_block ) {

	// The first chunk of $parsed_block['innerContent'] is always the parent block markup up to the opening element of the inner block wrapper.
	// That's the element we want to add the layout classnames to.
        // So we find the last instance of `class` in $parsed_block['innerContent'][0] and add temp classname to it
        // This assumes the inner block wrapper always has _some_ kind of classname 😅 
	// Then in `render_layout_support_flag` we replace our temp classname with the proper layout classnames.

	return $parsed_block;
}

add_filter( 'render_block_data', 'layoutz_on_inner_block_wrapper_pls' );

Needs a bit of thinking through, especially for the possibility of the inner wrapper not having classnames at all. But this might be a way to keep the logic layout-specific and not add any extra cruft to inner block markup.

[tellthemachines]

Now that we have a first step towards this in #44600, it's probably time to start thinking about what supporting multiple layout types in Cover should look like.

As a conversation starter, I enabled layoutSwitching for the block, and it looks pretty awful:

This is easy to work around by leveraging variations like we do with Group.

Another thing to consider is if we want to replace the alignment matrix with flex layout for the inner container, as they will have slightly different effects:

• The alignment matrix works by adding flex rules to the outer block container, so the inner container gets moved around as a flex item
• Flex layout adds flex rules to the inner container, thus allowing them to apply directly to block children, an effect that currently is only possible by wrapping the children in a Group variation.

From initial experimentation I'm inclined to think the alignment matrix is not as useful as flex layout, and could be replaced by it altogether, but keen to hear what everyone thinks about this! There are probably use cases I have overlooked 😅

Cc @WordPress/gutenberg-design

[jameskoster]

I think the important thing to decide here is whether we want a generic control for switching layout types (which can be used by any container), or variations like group/row/stack as you mentioned. Those variations made sense for the Group block because adding a Row feels fairly intuitive. I'm not sure that the pattern scales that well though... we don't really want Cover Row, Cover Stack, etc... Personally I'm in favor of abstracting layout controls and reducing the number of container blocks, but keen to hear thoughts from others.

[cbirdsong]

I would hate to see the the alignment matrix go – it enables designs that would be otherwise difficult or impossible to accomplish.

For the inner block container, I can also imagine how having row layout would be useful, though it's nothing you couldn't accomplish by adding a group wrapper. Still, less unnecessary wrapper divs for layout is an upgrade!

[tellthemachines]

Thanks for the input folks! I don't really have a preference re switcher vs variations; happy to go with what makes most sense design-wise.

I would hate to see the the alignment matrix go – it enables designs that would be otherwise difficult or impossible to accomplish.

@cbirdsong do you have any examples handy to share? Even if we keep the alignment matrix, adding layout styles to the inner container might affect how it currently works. It would be good to have some pointers to avoid breaking existing behaviour!

[jasmussen]

Good discussion. Fewer tools and more DNA shared between them is good. However it's probably going to be a bit of a journey, not just for any deprecations having to be made (and sought to avoid), but in terms of each of the core blocks initially being designed to fulfill different purposes. In the case of the Cover block, it was created to be a dead-simple way to overlay text on an image.

In that light, I would agree with @cbirdsong, that the matrix control would be nice to keep around. However couldn't it be made flex aware? Center dot applies horizontal and vertical justification properties, bottom right dot provides flex-end to both? I recognize there's almost certainly more to it than this, but just thinking in terms of the flex controls being explored, an alignment matrix seems like a natural interface for it.

[tellthemachines]

However couldn't it be made flex aware? Center dot applies horizontal and vertical justification properties, bottom right dot provides flex-end to both?

Do you mean have the matrix control apply flex properties to the inner blocks, instead of their wrapper? I'm not sure we'd gain much in terms of new flex possibilities. The matrix control works well for a block of content, but it would be hard to leverage it to change to horizontal instead of vertical orientation for instance.

We could keep both the matrix, which positions the content wrapper, and also add flex layout so we can manipulate the contents. Or not! This exploration started out as a way to allow Cover contents to respect theme content width; the directly relevant layouts for that are flow/constrained. I thought flex might be a nice addition, but if there's no pressing need for it, maybe we can leave it for later 😄

If we do want to add flex, then the big question is what the layout switching UI should look like. If we don't, it's pretty straightforward to just add flow/constrained.

[jasmussen]

I do mean having the single matrix cover both vertical and horizontal. But yes, we can separate concerns for now :)

[andrewserong]

I don't really have a preference re switcher vs variations; happy to go with what makes most sense design-wise.

Just a quick thought on the switcher — I believe we'll likely want to wind up using it for the Post Template block when it comes to the layout refactor for that block (#44557) in order to enable switching between the grid/flex and flow / constrained layouts? So, if we wind up dusting off the layout type switcher for the Cover block, I'm sure we'll find lots of other uses for it pretty quickly!