Block Hooks: Set hooked block's layout attribute based on anchor block's

[ockham]

This is a proof-of-concept to explore setting a hooked block's layout block support attribute to the same value as its anchor block, if the hooked block is inserted before or after the anchor block.

The motivation for this are scenarios like inserting a 'Like' button block after the Post Content block. It's been brought to my attention that this currently doesn't work well for e.g. the TT3 and TT4 Block Themes. The reason for this is that those Block Themes use layout block-support on their Post Content block and post meta Group block (i.e. the Group block that typically contains the post date, byline, categories, and sometimes also the Featured Image block) to set their layout to constrained -- which sets a certain margin and padding to center those blocks horizontally.

OTOH, an automatically inserted (hooked) Like button block -- with no attributes set -- will sit at the very left of the viewport.

To solve that issue, this fix makes it so that a hooked block that has opted into layout block support will get its layout attribute set to the same value as its anchor block's.

Before	After

Testing instructions

Use the latest version (0.6.0) of my demo Like button block plugin for testing. Try both with trunk, and with this PR. (Use the WordPress Playground for easy testing.)

Notes

It seems like the concept of different layouts (as used in this context) is somewhat incompatible with Block Hooks, as it means that a hooked block needs to have an attribute set -- or needs to be wrapped in another block with that attribute -- neither of which is currently supported by Block Hooks; otherwise it won't be displayed with the desired margins.

This is probably not going to be the final version of the fix for this issue. While it tries to be fairly cautious when setting the layout attribute, I still find that it might make too many assumptions about the hooked block.

Instead, it probably makes sense to expose the necessary information (i.e. the anchor block's attributes), and allow setting the hooked block's attributes (see #59572), so that extenders can implement the required logic in "user space", i.e. inside of the hooked_block_types filter.

Trac ticket: https://core.trac.wordpress.org/ticket/60126

---

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.

[ockham]

Note that this is only implemented for after insertion; we'll need to copy it to make_before_block_visitor to also cover before insertion.

[ockham]

FYI @yansern @TimBroddin @andrewserong @tellthemachines @gziolo @tjcafferkey

Highlighting this important bit from the PR desc:

This is probably not going to be the final version of the fix for this issue. While it tries to be fairly cautious when setting the layout attribute, I still find that it might make too many assumptions about the hooked block.

Instead, it probably makes sense to expose the necessary information (i.e. the anchor block's attributes), and allow setting the hooked block's attributes (see #59572), so that extenders can implement the required logic in "user space", i.e. inside of the hooked_block_types filter.

[yansern]

I still find that it might make too many assumptions about the hooked block.

+1.

Instead, it probably makes sense to expose the necessary information (i.e. the anchor block's attributes), and allow setting the hooked block's attributes (see #59572), so that extenders can implement the required logic in "user space", i.e. inside of the hooked_block_types filter.

I like that. I wonder if it makes sense to provide the entire $anchor_block itself, and possibly, the $parent_block to the hooked block filter. Who knows, perhaps there are other properties that the extenders might find useful from these blocks.

Side-topic: I was considering calling get_block_templates() ahead of time to scan the template structure, e.g. "does it have header, does it have footer, does it have navigation?" to determine where to best insert the block. It's probably not in this filter's interest to pass in the entire block tree, but hey, just sharing what I was thinking as an extender. :)

[andrewserong]

Thanks for the ping!

While it tries to be fairly cautious when setting the layout attribute, I still find that it might make too many assumptions about the hooked block.

That was my first thought in looking at the code — ideally this API wouldn't have to know anything in particular about the layout support or block supports in general. If it were to copy attributes from one to the other, I'd imagine it might also need to check that both blocks support layout before doing so, which would further add to the complexity, and I think most of the layout support is currently mostly contained in either layout.php or in the theme JSON class 🤔

Another idea (I imagine this won't be straightforward, either!), would it be possible for the Block Hooks API to support injecting a pattern in addition to simple blocks? If it could inject patterns then perhaps it could be up to the consumer to define a pattern with its own layout defaults included within a wrapper Group block, etc.

so that extenders can implement the required logic in "user space",

I like that, too. If there is going to be tricky logic copying particular attributes from one place to another, it sounds better to do that in the consumer / user space, rather than within the API itself, to me.

I haven't thought very much about all this, though, so that's just my first impressions in case it's at all helpful! 🙂

[tellthemachines]

Yeah I'd say it makes more sense to make the anchor block's attributes available so the hooked block can use them as needed. For layout purposes, blocks might also want to access child layout attributes within the style object, or other attributes such as padding. Setting a matching color might be needed too.

[ockham]

Thanks a lot for weighing in, folks.
Long comment ahead; tl;dr: I'm considering proceeding with this PR after all.

I'm still a bit torn on how much of the heavy lifting should be done by the Block Hooks mechanism itself, vs delegating it to user space.

To frame this a bit more: IMO, Block Hooks is a fairly "low-level" concept; the mechanism is supposed to insert a block in a given position in the block tree. This -- fairly straight-forward -- concept is arguably everything one needs to understand in order to use it (maybe with the limitation that blocks cannot be inserted as first or last children into certain blocks).

FWIW, during the early stages of development, I was considering adding both support for specifying attributes and for allowing insertion of patterns. Eventually -- and in the spirit of keeping things as simple as possible -- we didn't add either. The way that @mtias framed the problem for me was that a hooked block should be conceptually equal to one that has just been manually inserted in the editor: At that stage, no attributes have been set, nor have any inner blocks been added. This became the baseline for Block Hooks; it put the burden on block authors to choose sensible attribute default values that would work well when used with Block Hooks, which seemed reasonable. Only once a really compelling use case that required those other features had been demonstrated would we add them.

Having a hooked block aligned correctly with its sibling block(s) is certainly a compelling use case. However, in keeping with the idea of minimalism, I'd rather avoid increasing the API surface all too much. (I'm particularly skeptical of allowing pattern insertion for the sake of wrapping a third-party block in a Group block with the right attributes set to make it align properly with its sibling blocks. As for allowing attributes to be set, see #59572, and further below.)

IMO, the problem with layout block-support is that it is an (arguably) higher-level concept (as it goes beyond the abstract notion of a block tree and introduces concepts such as content width, which IIUC is owed to using certain blocks both in the post and in the site editor) that the Block Hooks mechanism -- or hooked blocks -- need to be aware of in order to be displayed correctly. (AFAIK, it's also the only such concept.)

That said, I'm glad that the problem can be at least partially solved by opting the hooked block into layout block-support. It still raises the question whose responsibility it is to set that attribute correctly: The Block Hooks mechanism's (i.e. Core's) or the hooked block's?

I'm still torn on that question. While I was indicating earlier that I was leaning towards making it the hooked block's (and having Core expose the information it needs to do so), I'm not 100% convinced it's the right choice. It feels wrong to require a block author to add a bunch of code simply to make their block "just work" as expected; it's the kind of decision that I think would lead to WP.org forum posts and SO threads sharing the same snippet over and over.

If a block author already opted their block into layout block-supports and hooked it after e.g. core/post-content, it seems like they stated their intention clearly enough; there's a point to be made that it should then be on Core to render the block the way they intended it, when there really seems to be only one sensible way of doing so. (Unless I'm missing an equally probable intention that could be expressed that way; please LMK!)

So I'm starting to consider proceeding with a solution as demonstrated in this PR's code. I'd still want to allow people to override the layout attribute that the mechanism would set, so I'll need to expose it; and since it would be arbitrary to just expose one, I'd expose them all. Thinking of adding a new filter such as

$hooked_block = apply_filters( 'hooked_block', $hooked_block, $relative_position, $anchor_block, $context );

where $hooked_block is a "parsed block array" (e.g. fields like blockName, attrs, and all that).

---

BTW I realize that if I manually insert a block with layout block-support right after another block that has layout set to constrained will currently also not set the newly inserted block's layout attribute to constrained (i.e. the Block Hooks mechanism is indeed consistent with this behavior). FWIW, I'd opt to change that, too; we have some precedent where we already carry over attributes from an existing sibling block in the editor.

[ockham]

One follow-up question that just occurred to me: Would it make sense to set the hooked block's default layout to constrained? If I apply the following patch to my Like button block, it's displayed the way it should be with Core trunk:

diff --git a/src/block.json b/src/block.json
index 99660d4..f475cf2 100644
--- a/src/block.json
+++ b/src/block.json
@@ -9,7 +9,11 @@
        "description": "Example block scaffolded with Create Block tool.",
        "supports": {
                "html": false,
-               "layout": true
+               "layout": {
+                       "default": {
+                               "type": "constrained"
+                       }
+               }
        },
        "textdomain": "like-button",
        "editorScript": "file:./index.js",

I guess the question is how confidently can a block author say that their block only makes sense to be automatically inserted into a constrained (or flow, or flex) layout? 🤔

I'm trying to come up with scenarios where we wouldn't want to set a default layout. I guess there might be themes that use the Post Content block differently (i.e. with the layout attribute set to a different value than constrained), in which case we wouldn't want the Like button block's layout to be set accordingly. Or if we'd like to use the Like button as part of a post 🤔

Maybe @tellthemachines @andrewserong can help me build a better intuition for lumping blocks into buckets where different layout attribute settings do or don't make sense, respectively? 😊🙏

[andrewserong]

Thanks for the continued thoughts @ockham, it's certainly an interesting problem!

Having a hooked block aligned correctly with its sibling block(s) is certainly a compelling use case. However, in keeping with the idea of minimalism, I'd rather avoid increasing the API surface all too much.

That's a good point to keep in mind. One concern that comes to me from looking over the code again is that I imagine that a very large number of 3rd party blocks are not going to be container blocks for other blocks, but will be individual blocks for a single use case, rather than wrapping other things. Therefore, those blocks are unlikely to be good candidates for opting-in to the layout block support, and so wouldn't line up correctly with the adjacent block.

That brings me back to thinking that the ideal state if we're attempting to match against another block that uses layout is to use a primitive for wrapping the block that we know does have layout (i.e. Group), or to see if there's a way to append as last child to the post content block.

I think my main concern is that the layout block support is already fairly complex and not suited to all blocks, so I can imagine us running into further issues if folks wind up needing to opt their 3rd party blocks in to layout to ensure they line up correctly with other blocks, which they shouldn't need to. And then we might be back to the forums and SO issue when they run into issues with the layout support when they're not using useInnerBlocksProps for non-container blocks 🤔

Would it make sense to set the hooked block's default layout to constrained? If I apply the following patch to my Like button block, it's displayed the way it should be with Core trunk:

I suppose a problem here is the assumptions we make about what a template is doing with the adjacent block. The adjacent block might be full width (default) layout, or use a custom content or wide size, so it's difficult to predict what would be needed there. If we're trying to line up an inserted block next to an existing post content block, I'm not sure we can reliably make it line up without copying the layout attributes.

I'm not sure if my comments here have been very helpful, I'm sorry! Very happy to continue discussing 🙂

[ockham]

One concern that comes to me from looking over the code again is that I imagine that a very large number of 3rd party blocks are not going to be container blocks for other blocks, but will be individual blocks for a single use case, rather than wrapping other things. Therefore, those blocks are unlikely to be good candidates for opting-in to the layout block support, and so wouldn't line up correctly with the adjacent block.

Indeed. What got me somewhat excited about opting non-container blocks into layout block support was that the Post Content block seemed to set some precedent for that. Granted, it's a bit special (as it typically does contain blocks -- the ones from the relevant post's content), but it's not an "actual" container block (i.e. with inner blocks) in the true sense of the word; plus layout block-support seemed to work pretty straight-forward for the Like button. My takeaway was that the "actual" requirement for a constrained layout to work wasn't necessarily that it was applied to a container block but to a block that had at least one direct child (HTML) element, to which the .is-layout-constrained > :where(:not(.alignleft):not(.alignright):not(.alignfull)) selector would then apply the relevant margin settings.

I'd wager that a lot of blocks already fulfill that criterion, and it might be easy enough to rewrite a block that doesn't yet, so I was wondering if we could go as far as to standardize that requirement for the constrained layout setting to work? 🤔

That brings me back to thinking that the ideal state if we're attempting to match against another block that uses layout is to use a primitive for wrapping the block that we know does have layout (i.e. Group), or to see if there's a way to append as last child to the post content block.

Yeah, and apologies for not having discussed those options more yet. FWIW, I'm not opposed to using a wrapper Group block per se, but I have some concerns about that option.

Wrapping a third-party block in a Group block is a bit tricky in terms of how we could make it work with existing Block Hooks semantics. Currently, both the block.json field and the hooked_block_types filter accept the block type of the hooked block as an argument. This shouldn't be core/group, as it would make it indistinguishable to the Block Hooks mechanism from any other hooked block that also wants to wrap itself in a Group block; it would most likely lead to only one of them being actually inserted.

I've started experimenting with two different approaches that are both somewhat promising. I've listed the pros and cons for both approaches. Neither might be a perfect fit for using a Group wrapper block, but with #5835, it might be possible. (I'll try it out these days to verify.)

As for last-child insertion into Post Content, I've also given it some more thought, and I don't think it's viable. I experimented a bit with this in WordPress/gutenberg#56972, and I think it'd have a number of serious shortcomings -- first and foremost, the hooked block wouldn't show up in the editor, which is kind of a no-go.

I think my main concern is that the layout block support is already fairly complex and not suited to all blocks, so I can imagine us running into further issues if folks wind up needing to opt their 3rd party blocks in to layout to ensure they line up correctly with other blocks, which they shouldn't need to. And then we might be back to the forums and SO issue when they run into issues with the layout support when they're not using useInnerBlocksProps for non-container blocks 🤔

This might be a detail, but FWIW, I didn't use useInnerBlockProps but __unstableLayoutClassNames in combination with useBlockProps -- as does the Post Content block.

Would it make sense to set the hooked block's default layout to constrained? If I apply the following patch to my Like button block, it's displayed the way it should be with Core trunk:

I suppose a problem here is the assumptions we make about what a template is doing with the adjacent block. The adjacent block might be full width (default) layout, or use a custom content or wide size, so it's difficult to predict what would be needed there. If we're trying to line up an inserted block next to an existing post content block, I'm not sure we can reliably make it line up without copying the layout attributes.

Yeah, assuming that we can invariably set the default to one specific layout setting seems too risky, even to me 😅

I'm not sure if my comments here have been very helpful, I'm sorry! Very happy to continue discussing 🙂

Quite the opposite! I really appreciate your thoughts, they've helped shape the potential solutions I've been working on, and have made me reconsider some of my assumptions.

---

In the spirit of the aforementioned minimalism, I'm now leaning towards #5835. This will give extenders enough flexibility and information to set the hooked block's layout attribute to match the anchor block's; and possibly also to wrap the hooked block in a Group block (TBD). I'd like to see if that solves a large enough percentage of cases, and if it doesn't, we can iterate and give even more power and information to extenders.

[andrewserong]

Glad the discussion is useful for you @ockham, I'm very much getting a lot out of it, too!

In the spirit of the aforementioned minimalism, I'm now leaning towards #5835. This will give extenders enough flexibility and information to set the hooked block's layout attribute to match the anchor block's; and possibly also to wrap the hooked block in a Group block (TBD).

Being able to handle both cases sounds like a good idea to me. There'll be some blocks that naturally work well with layout and where copying the layout attribute will work nicely, and others that won't be a suitable candidate, so I like the idea that extenders can have flexibility in how they might implement this.

I'd wager that a lot of blocks already fulfill that criterion, and it might be easy enough to rewrite a block that doesn't yet, so I was wondering if we could go as far as to standardize that requirement for the constrained layout setting to work? 🤔

It's a very good question. If you have an idea about how it should ideally work, it'd make for a good Gutenberg issue if you have time to open one? If not I'm happy to open an issue for it and link to this discussion 🙂. If we're considering including non-container blocks, then I think there'll be a few wrinkles to iron out, as there's a bit more going on than just the wrapper > child hierarchy as the UI currently assumes that it really is a container block (i.e. in the help text, etc). Also, features that are only available on the child block will be unavailable, i.e. if someone wanted the direct child to have "wide" alignment to align with a preceding constrained block where someone has used "wide" alignment, that won't be possible on a block that isn't a true container for other blocks. So if it's formally allowed for non-container blocks, there'd likely need to be a guardrail or two (or just docs) to cover the limitations. It'd also be good to get other contributors' feedback on the idea if we're considering proposing it, since there could be other considerations we mightn't have thought of. But very well worth discussing further! It could also allow more blocks to use blockGap / block spacing, which would be cool.

This might be a detail, but FWIW, I didn't use useInnerBlockProps but __unstableLayoutClassNames in combination with useBlockProps -- as does the Post Content block.

The post content block is a really interesting case as it's not a real container block, but it's doing everything it can to pretend to be one! 😄 I.e. in the editable form of the block, it uses useInnerBlocksProps here with a controlled set of blocks, and useInnerBlocksProps gets the layout classnames internally here. So it's kind of trying to do the best of both worlds and use as much of layout as it can in preview mode, and then it "really" uses it when it's in edit mode. Hopefully most 3rd party blocks won't need to jump through so many hoops to work with layout! 😅

---

From my perspective, some of the points I'm getting from the current discussion is:

• For blocks that work nicely with layout, it'd be great if the block hooks can allow extenders to copy over layout from the anchor block
• For blocks that aren't a good candidate for layout, there should be another option for extenders to use in order to get things to line up properly (e.g. the wrapping in a Group block idea, if it's possible)
• There are some blocks that could be a good candidate for some of the features of layout, and it could be worth seeing if it's possible to "partially" opt-in to layout support for non-container blocks, that are laying out content that is not separate child blocks. For this one, let's open an issue for it if we think it's worth pursuing further 🙂

[tellthemachines]

Apologies for the delay, just catching up on the latest discussion!

If a block author already opted their block into layout block-supports and hooked it after e.g. core/post-content, it seems like they stated their intention clearly enough; there's a point to be made that it should then be on Core to render the block the way they intended it, when there really seems to be only one sensible way of doing so.

If the hooked block is a container itself, it might opt into layout in order to arrange its contents in a certain way. I'm thinking it's not too farfetched that something like Query Pagination, with layout set to flex, could be a hooked block. In that case, resetting its layout attribute would break the block's internal layout.

Would it make sense to set the hooked block's default layout to constrained?

I think I'm already answering this above but yeah the wider problem is that all the blocks involved and their parents could have any type of layout: the anchor could be a flex or grid block for instance. Or the anchor's parent could be a flex block. We can't safely make any assumptions here 😅

I'm very much undecided about the possibility of opting non-containers into layout. Currently, it's technically possible to do: there's no check that a block can contain inner blocks before running the layout logic. But all layout types work on the assumption that blocks are containers in that the generated styles apply to child elements. This might not be the desired outcome for a non-container block with inner HTML, especially if we're applying the layout type of an adjacent block to it. (I'm now wondering if there should be a check in place to prevent layout from applying to non-container blocks 😬 )

Overall I'm inclined to agree with @andrewserong that it's best to let extenders decide if/how to manage layout for each hooked block.

[tellthemachines]

Not all blocks that support layout have the attribute explicitly declared; it only appears when the block has a non-default layout configuration. A more reliable check would be block_has_support( $block_type, 'layout', false ).

[ockham]

Per discussion on #5811, #5835 (comment), and #5837 (comment), I've now opened #5835 for review.

I'll address remaining feedback on this PR soon (hopefully today or tomorrow) and will subsequently close this PR.

[ockham]

I never got around to replying to all feedback in detail; apologies for that!

Since it's been almost two months and #5835 has been merged (and will be part of WP 6.5), it doesn't make sense to keep this PR open any longer.

Thanks again for the valuable discussion, folks! It helped shape the hooked_block_{$block_type} filter introduced in #5835, which doesn't make any assumptions about the presence of the layout attribute; it's up to the block author to make the relevant checks, opt their block into layout block-support, or use the filter to wrap their hooked block in a container block that has layout block-supports.