Block Hooks: Sibling insertion incompatible with constrained layout

[Bernhard Reiter]

It's been brought to my attention that insertion of e.g. a 'Like' button block after a Post Content block 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.

See ​here for an illustration of the problem.

[sabernhardt]

The GitHub image is giving me a 404 not found error; could you either upload the image on this ticket or link to a page? Also, who brought the issue to your attention (for props)?

[Bernhard Reiter]

Screenshot illustrating the issue

[Bernhard Reiter]

Screenshot illustrating the desired result

[Bernhard Reiter]

Hey @sabernhardt! I've now attached both screenshots from the GitHub PR (current state vs desired result).

The issue was brought up to me by both @timbroddin and @yansern.

WIP. Pulls together code from #5609 (Extract insert_hooked_blocks() function, and some ideas from #5811.

Importantly, this is an alternative to ​https://github.com/WordPress/wordpress-develop/pull/5811 in that it provides a possible solution for https://core.trac.wordpress.org/ticket/60126.

To demonstrate the latter, apply the following patch to the ​Like Button block code:

like-button.php

@@ -24 +24 @@
        register_block_type( __DIR__ . '/build' );
 }
 add_action( 'init', 'create_block_like_button_block_init' );
+
+function set_like_button_block_layout_attribute_based_on_adjacent_block( $hooked_block, $relative_position, $anchor_block ) {
+       // Is the hooked block adjacent to the anchor block?
+       if ( 'before' !== $relative_position && 'after' !== $relative_position ) {
+               return $hooked_block;
+       }
+
+       // Does the anchor block have a layout attribute?
+       if ( isset( $anchor_block['attrs']['layout'] ) ) {
+               // Copy the anchor block's layout attribute to the hooked block.
+               $hooked_block['attrs']['layout'] = $anchor_block['attrs']['layout'];
+       }
+
+       return $hooked_block;
+}
+add_filter( 'hooked_block_ockham/like-button', 'set_like_button_block_layout_attribute_based_on_adjacent_block', 10, 3 );

TODO:

• [ ] Figure out details for filter signature/shape of $hooked_block. Note that we currently do no pass on any inner blocks to get_comment_delimited_block_content.
• [ ] Test coverage

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

## Advantages of this approach

• Neatly separate from hooked_block_types, reasonable separation of concerns: The former allows modifying _which_ block types should be hooked, the latter is for more granular control -- e.g. to set attributes (and has access to anchor block’s).
• Using the hooked_block_{$hooked_block_type} nomenclature would be somewhat familiar from e.g. ​`render_block_{$type}`.

## Downsides of this approach
The major downside is that while it might be tempting to use this filter to use a pattern instead of a hooked block, it's not a good idea to do so.

If we allow attributes, people might start adding a core/pattern block, and specify the pattern's slug via the slug attribute. However, it is still required to add the core/pattern block as a hooked block in the first place. Since third-party blocks cannot modify a Core block's block.json (and shouldn't resort to doing so via the [https://developer.wordpress.org/block-editor/reference-guides/filters/block-filters/#registration block_type_metadata or block_type_metadata_settings filters]), this would need to be done via the ​`hooked_block_types` filter.

For example: A Like button block uses the hooked_block_types filter to add a core/pattern block after the Post Content block, and then uses the hooked_block filter to set its slug attribute to my/like-button-wrapper.

But what if a Subscribe button block does the exact same, except setting the pattern slug to third-party/subscribe-button-wrapper?

Both blocks might assume at hooked_block filter that the core/pattern block that was added after core/post-content is "theirs"; consequently, they will both try to set the Pattern block's slug attribute. As a result, we'll likely end up with only _one_ hooked Pattern block -- and it's determined by filter order which block pattern prevails. Not ideal 😕

To avoid this from happening, we might even consider warning (or banning) Core blocks from being used as hooked blocks. (See ​https://github.com/WordPress/wordpress-develop/pull/5810 for a somewhat related PR that seeks to warn against "incompatible" _anchor_ blocks.) Then again, maybe this is not so clear-cut: After all, it makes sense to e.g. use Core's Login/out block as a hooked block that's the last child of the Navigation block.

To offer alternatives for extenders, we might want to allow passing inner blocks to the filter; ​see.

To offer alternatives for extenders, we might want to allow passing inner blocks to the filter; ​see.

Noting that there might be a bit of an inconsistency with this when used to solve https://core.trac.wordpress.org/ticket/60126, as e.g. the hooked_block filter -- when called for the Like button block -- would return a Group block (which has the Like button block as its inner block). Hopefully not a dealbreaker though 🤔

Here's a proof-of-concept (based on the example from #5837) that works with this PR to replace the Like Button block with a pattern that wraps it in a Group block:

like-button.php

@@ -22 +22 @@
  */
 function create_block_like_button_block_init() {
        register_block_type( __DIR__ . '/build' );
+
+       register_block_pattern(
+               'ockham/like-button-wrapper',
+               array(
+                       'title'       => __( 'Like Button', 'like-button' ),
+                       'description' => _x( 'A button to like content.', 'Block pattern description', 'like-button' ),
+                       'content'     => '<div class="wp-block-group"></div>',
+                       'inserter'    => false
+               )
+       );
 }
 add_action( 'init', 'create_block_like_button_block_init' );
+
+function insert_like_button_pattern_after_post_content( $hooked_block, $position, $anchor_block ) {
+       if ( 'before' !== $position && 'after' !== $position ) {
+               return $hooked_block;
+       }
+
+       if ( 'core/post-content' !== $anchor_block['blockName'] ) {
+               return $hooked_block;
+       }
+
+       // We replace the "simple" block with a pattern that contains a Group block wrapper.
+       return array(
+               'blockName' => 'core/pattern',
+               'attrs'     => array(
+                       'slug' => 'ockham/like-button-wrapper',
+               ),
+       );
+}
+add_filter( 'hooked_block_ockham/like-button', 'insert_like_button_pattern_after_post_content', 10, 3 );

This seems reasonably ergonomic. The major downside is still that it doesn't allow setting the layout attribute dynamically (and instead hard-wires it to be "type": "constrained").

This seems reasonably ergonomic. The major downside is still that it doesn't allow setting the layout attribute dynamically (and instead hard-wires it to be "type": "constrained").

I wonder if pattern overrides (formerly known as partial sync patterns) would allow to solve this problem. I'll need to research that.

Per ​https://github.com/WordPress/wordpress-develop/pull/5835/commits/b34e2cf374d4acb128f2e63ed70de3282b9cc6ff (which I just worked on with @c4rl0sbr4v0), it is now possible to pass a block that has inner blocks:

like-button.php

@@ -24 +24 @@
        register_block_type( __DIR__ . '/build' );
 }
 add_action( 'init', 'create_block_like_button_block_init' );
+
+function insert_like_button_after_post_content( $hooked_block, $position, $anchor_block ) {
+       if ( 'before' !== $position && 'after' !== $position ) {
+               return $hooked_block;
+       }
+
+       if ( 'core/post-content' !== $anchor_block['blockName'] ) {
+               return $hooked_block;
+       }
+
+       $attrs = isset( $anchor_block['attrs']['layout']['type'] )
+               ? array(
+                       'layout' => array(
+                               'type' => $anchor_block['attrs']['layout']['type']
+                       )
+               )
+               : array();
+
+       // Wrap the Like Button block in a Group block.
+       return array(
+               'blockName' => 'core/group',
+               'attrs'     => $attrs,
+               'innerBlocks' => array(
+                       array(
+                               'blockName'    => 'ockham/like-button',
+                               'attrs'        => array(),
+                               'innerBlocks'  => array(),
+                               'innerContent' => array(),
+                       ),
+               ),
+               'innerContent' => array(
+                       '<div class="wp-block-group">',
+                       null,
+                       '</div>'
+               ),
+       );
+}
+add_filter( 'hooked_block_ockham/like-button', 'insert_like_button_after_post_content', 10, 3 );

It's not exactly pretty though 😬

I've absorbed ​this comment and ​this one into the PR description to see at a glance how the new filter can be used with different techniques.

This seems reasonably ergonomic. The major downside is still that it doesn't allow setting the layout attribute dynamically (and instead hard-wires it to be "type": "constrained").

I wonder if ​pattern overrides (formerly known as partial sync patterns) would allow to solve this problem. I'll need to research that.

I've talked to folks who are working on pattern overrides, and it seems like they only work for a block's content, not for arbitrary attributes.

@gziolo suggested to allow returning null from the filter, to indicate that the hooked block shouldn't be rendered after all.

I decided against this. The reason is that we already have the hooked_block_types filter to add and remove a block from the list of hooked blocks; and while returning null from the hooked_block filter would allow us to remove it, the opposite -- adding a block via that filter -- is not possible (lack of symmetry).

Furthermore, by not allowing removal of a block via the hooked_block filter, we retain the invariant that that filter will not change the list of hooked blocks, which should make code overall more predictable and resilient. (It avoids e.g. problems if one hooked_block filter returns null, and the next one tries to access and/or modify that block.)

Opening for review. Reviewers, please take the time to read the PR description, which I've updated with the rationale behind this filter, plus some code snippets that demonstrate how to use it 🙌

FYI @tjcafferkey @nerrad @yansern @TimBroddin 🙂 Feel free to review!

From my brief time working with and understanding this API, this looks like an acceptable solution to me, I like how your solution is relatively 'simple' and readable and solves this particular problem. I do have a few comments though:

​To avoid this from happening, we might even consider warning (or banning) Core blocks from being used as hooked blocks.

I'd be against this for the reason you've outlined (using the core/loginout block) especially as Gutenberg's block library expands this will become more of a problem. It feels unharmonious to release an API in core whilst at the same time be trying to encourage people to use Gutenberg but not allow one to work with the other.

Also, the API feels quite flexible based on the use cases you've presented in the PR description. If we release something which can be used in a multitude of ways would it limit our ability to safely iterate on and improve it going forward if we need to consider so many use-cases? Just a thought.

Thanks a lot for your feedback @tjcafferkey, it's much appreciated!

From my brief time working with and understanding this API, this looks like an acceptable solution to me. I like how your solution is relatively 'simple' and readable yet solves this particular problem. I do have a few comments though:

​To avoid this from happening, we might even consider warning (or banning) Core blocks from being used as hooked blocks.

I'd be against this for the reason you've outlined (using the core/loginout block) especially as Gutenberg's block library expands this will become more of a problem. It feels unharmonious to release an API in core whilst at the same time be trying to encourage people to use Gutenberg but not allow one to work with the other.

Yeah, that's a fair point; I agree now that we shouldn't wholesale ban Core blocks 👍

Also, the API feels quite flexible based on the use cases you've presented in the PR description. If we release something which can be used in a multitude of ways would it limit our ability to safely iterate on and improve it going forward if we need to consider so many use-cases? Just a thought.

Yes, the ​technique we're about to explore in ​https://github.com/WordPress/gutenberg/pull/57754 might work with similar use cases (i.e. blocks that fetch data from a separate entity in the DB) 👍 Off the top of my head, it might carry over quite well to core/template-part; it might be a bit trickier with core/post-content (where it would end up injecting hooked blocks into the actual post, which would mean we'd actually start injecting into content, and those hooked blocks would appear in the post editor; this would be kind of a major decision to make).

The thing is that by default, firstChild/lastChild insertion only works with container blocks (i.e. blocks that support having inner blocks), which the Navigation, Template Part, and Post Content block aren't, which is why we need to manually add support. I still think there could be some benefit in showing a warning when people attempt to insert into something like that, as I've seen a number of folks try that, and it left them confused when it didn't work. I'd love for them to have a shorter feedback loop to understand why that's happening (and to not give up on using Block Hooks 😅), and it seems easy enough to implement a warning like that.

OTOH I agree that we might continue to add support for some of these blocks in GB, and we don't want to discourage people forever from using them. Maybe we need a filterable list of unsupported blocks that the warning checks against 😅

[Bernhard Reiter]

In 57354:

Block Hooks: Introduce a new hooked_block_{$block_type} filter.

Add a new hooked_block_{$block_type} filter that allows modifying a hooked block (in parsed block format) prior to insertion, while providing read access to its anchor block (in the same format).

This allows block authors to e.g. set a hooked block's attributes, or its inner blocks; the filter can peruse information about the anchor block when doing so. As such, this filter provides a solution to both #59572 and #60126.

The new filter is designed to strike a good balance and separation of concerns with regard to the existing ​`hooked_block_types` filter, which allows addition or removal of a block to the list of hooked blocks for a given anchor block -- all of which are identified only by their block types. This new filter, on the other hand, only applies to one hooked block at a time, and allows modifying the entire (parsed) hooked block; it also gives (read) access to the parsed anchor block.

Props gziolo, tomjcafferkey, andrewserong, isabel_brison, timbroddin, yansern.
Fixes #59572, #60126.

Committed to Core in https://core.trac.wordpress.org/changeset/57354.

[Bernhard Reiter]

In 57355:

Block Hooks: Amend PHPDoc for hooked_block_{$hooked_block_type} filter.

Add missing explanation of the dynamic part of the hook name.

Follow-up [57354].
Props swissspidy.
See #59572, #60126.

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