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.

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

Block spacing not applied when enabling blockGap via blocks.registerBlockType JS filter hook #53155

Open
webmandesign opened this issue Jul 30, 2023 · 16 comments
Labels
[Block] Details Affects the Details Block - used to display content which can be shown/hidden [Feature] Layout Layout block support, its UI controls, and style output. [Status] In discussion Used to indicate that an issue is in the process of being discussed [Type] Enhancement A suggestion for improvement.

Comments

@webmandesign
Copy link
Contributor

webmandesign commented Jul 30, 2023

Description

When I enable supports.spacing.blockGap for a block via blocks.registerBlockType JavaScript filter in my theme, the "Block spacing" control is displayed correctly in the block settings sidebar, but tweaking the control does not apply the styles on the block.

Is this a bug or should I enable something more in my theme code?
(Currently I'm using WP_HTML_Tag_Processor to add inline style via render_block PHP hook. But it feels hacky and unnecessary.)

Step-by-step reproduction instructions

  1. In Twenty Twenty-Three theme add a functions.php file with this content:
<?php

add_action( 'enqueue_block_editor_assets', function() {

	wp_add_inline_script( 'wp-hooks', "
		wp.hooks.addFilter(
			'blocks.registerBlockType',
			'twentytwentythree/block-mods',
			function( settings, name ) {
				if ( 'core/details' == name ) {
					settings.supports.spacing.blockGap = true;
				}
				return settings;
			}
		);
	" );

} );
  1. In block editor add Details block into the post/page content and try to edit newly enabled "Block spacing" option. Note there are no styling changes to Details block in the block editor, nor on front-end.

Screenshots, screen recording, code snippet

No response

Environment info

  • WP 6.3-RC2
  • Gutenberg 16.3.0

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@jordesign jordesign added [Type] Bug An existing feature does not function as intended Needs Testing Needs further testing to be confirmed. [Package] Style Engine /packages/style-engine labels Jul 31, 2023
@annezazu
Copy link
Contributor

@ryanwelcher @justintadlock could you all chime in here?

@justintadlock
Copy link
Contributor

I can confirm this doesn't work (I also tested with a couple of other blocks). But I'm not familiar enough with how this works under the hood to say why.

I'm pretty sure just adding blockGap support to even a custom block doesn't work out of the box. It may require layout support too.

@andrewserong - Pinging you in case it is related to layout support.

@andrewserong andrewserong added [Feature] Layout Layout block support, its UI controls, and style output. [Status] In discussion Used to indicate that an issue is in the process of being discussed [Type] Enhancement A suggestion for improvement. and removed [Package] Style Engine /packages/style-engine Needs Testing Needs further testing to be confirmed. [Type] Bug An existing feature does not function as intended labels Aug 2, 2023
@andrewserong
Copy link
Contributor

andrewserong commented Aug 2, 2023

Thanks for the ping!

It looks like there are a couple of questions in this issue:

Adding blockGap support to to the Details block

This isn't possible without opting the Details block in to use the layout block support. If this is something desired more generally, it might be worth opening a separate issue as a suggested feature (I'm happy to do that Edit: issue added in #53252). It looks like some CSS rules were added in #49808 so that the block uses the theme's root block gap CSS variable var(--wp--style--block-gap). That looks like a bit of a temporary situation to me, so longer-term it could be better to use the layout block support, which provides the spacing rules, and then would allow opting-in to the blockGap support.

@richtabor do you know if there was a reason we didn't use the layout block support for the Details block from the outset? Just thought I'd double check in case there'll be some stumbling blocks to implementing it in follow-ups.

blockGap depends on layout support

I'm pretty sure just adding blockGap support to even a custom block doesn't work out of the box. It may require layout support too.

Yes, blockGap only works for blocks that have opted-in to using the layout block support. Now that the layout block support has stabilised in WP 6.3, I think we can expand a bit of the blockGap documentation in the handbook to make it clearer that spacing.blockGap is only supported for blocks that use layout support. I'm happy to have a go at that, too. CC: @tellthemachines for visibility, since we've chatted a fair bit about the confusion of spacing controls and layout.


So, where does that leave us for this particular issue? In terms of being able to enable blockGap via blocks.registerBlockType JS filter hook, I don't think this is so much a bug, but more something that isn't supported. I'll adjust the labels for this issue slightly (I think the discussion here highlights more of an enhancement request than a bug), but feel free to change them if it's not quite right!

For themes wishing to adjust spacing for the Details block, in the shorter-term, they might need to provide some CSS styles if the current state of block supports doesn't meet a theme's needs.

@andrewserong andrewserong added the [Block] Details Affects the Details Block - used to display content which can be shown/hidden label Aug 2, 2023
@andrewserong
Copy link
Contributor

andrewserong commented Aug 2, 2023

Update: I've opened a separate issue in #53252 for adding layout and blockGap support to the Details block. We can use that issue for any discussion about potential blocks to opting-in to those supports.

@andrewserong
Copy link
Contributor

Another quick update: I've opened a PR in #53254 to update the dev docs slightly to clarify that blockGap depends on layout.

@webmandesign
Copy link
Contributor Author

webmandesign commented Aug 2, 2023

That's pitty that blockGap is tied up with layout. I've used Details block just as an example here. But actually I'm enabling blockGap for these blocks in my theme:

1. core/details and core/post-author-biography

Maybe both of these could benefit from enabling layout?

2. core/group

Group already has "blockGap": true, set in its JSON but I'm modifying it to blockGap: { sides: [ 'horizontal', 'vertical' ] } for more precise responsive/wrapped spacing control. I've opened an enhancement request for this at #53255

3. core/categories and core/tag-cloud

This is very specific usecase as I add "Inline Buttons" block style via my theme for these blocks, so they can be used as posts list "filter". The only thing I'm needing here is to set up proper --wp--style--block-gap value on the block HTML container, so I'm currently doing this with help of WP_HTML_Tag_Processor via render_block filter.

Especially for this last usecase I expected Gutenberg to provide inline style attribute for specific block as style="--wp--style--block-gap: ↕value ↔value;" upon enabling blockGap for the blocks. Although, now that I understand blockGap is related to layout, this would not work.

So, here is my additional request:
Could there be some mechanism in Gutenberg to provide inline style HTML attribute setting up --wp--style--block-gap value if blockGap is enabled for the block, but the block does not support layout? Please. :)

@andrewserong
Copy link
Contributor

Thanks for your further thoughts!

core/post-author-biography

Unfortunately that block likely isn't a good candidate for layout block support as it's not a wrapper for other blocks. The layout support controls spacing for containers of child blocks.

Group already has "blockGap": true, set in its JSON but I'm modifying it to blockGap: { sides: [ 'horizontal', 'vertical' ] }

It'd be great to add axial block spacing support to the group block, but it would also need to be aware of layout types. There's an earlier issue for it in #47084 that would be good to implement.

I expected Gutenberg to provide inline style attribute for specific block as style="--wp--style--block-gap: ↕value ↔value;" upon enabling blockGap for the blocks.

Block gap was originally implemented in a way that's kind of similar to that, but unfortunately due to subtle issues of inheritance when nesting blocks within other blocks (as it'd reset the value for all children), the CSS variable-based approach had to be largely deprecated. The CSS variable is still output if themes wish to use or hook into it, but so far, core blocks have largely moved away from using the variable and instead receive style values directly via the layout support.

Could there be some mechanism in Gutenberg to provide inline style HTML attribute setting up --wp--style--block-gap value if blockGap is enabled for the block, but the block does not support layout?

I definitely see the value in doing that, and I believe there are other issues flagging the need for spacing controls for blocks that aren't suitable candidates for layout. For now at least, I don't think it'd be a good idea to attempt to reset the blockGap CSS variable as it's difficult to work with in a predictable way.

This use case does highlight the gap (if you'll excuse the pun!) between block spacing, that is spacing between blocks, and spacing/gap in a general sense between HTML elements. The latter doesn't currently exist as a block support, but could be very useful. I imagine if it were to be developed, it might want to be a separate block support to blockGap, which is tied to the concept of gap between blocks, to try to avoid some complexity there.

Sorry I don't have more conclusive answers! I very much see the problem of wanting to allow more spacing controls in these particular cases. I'd be curious to hear if anyone else has other ideas about how they could be implemented, or be factored into spacing controls in general.

Thanks again for the discussion here, it's very valuable to have!

@webmandesign
Copy link
Contributor Author

Block gap was originally implemented in a way that's kind of similar to that, but unfortunately due to subtle issues of inheritance when nesting blocks within other blocks (as it'd reset the value for all children), the CSS variable-based approach had to be largely deprecated.

Indeed, inheritance is the issue here. That's why I'm setting the blockGap only for specific usecases where there are no child blocks. I totally understand the decision here.

This use case does highlight the gap (if you'll excuse the pun!) between block spacing, that is spacing between blocks, and spacing/gap in a general sense between HTML elements.

Good point! Yes, I'm using blockGap to set gap between inner HTML elements, not actual blocks. I guess I thought about it in CSS kind of way (a gap style applied on the specific block) but now I see your point of gap separating inner blocks.

This is tricky to have separate control for HTML elements gap, indeed. But maybe the control and naming could be the same and just generated styles would differ depending on whether a block has layout support?

@andrewserong
Copy link
Contributor

But maybe the control and naming could be the same and just generated styles would differ depending on whether a block has layout support?

That's a very compelling idea! It'd be worth hacking around with to see how it might work 🤔

@justintadlock
Copy link
Contributor

Thanks for diving right into this and addressing it @andrewserong!


On using blockGap for blocks without inner blocks and layout support: I've actually run into a few use cases for this in some custom blocks and on core blocks (the Term Description block seems to come to mind, which can have multiple inner <p> tags).

@richtabor
Copy link
Member

@richtabor do you know if there was a reason we didn't use the layout block support for the Details block from the outset?

To keep it as simple as other text/writing blocks (paragraph, heading, quotes, lists, etc).

We don't need layout tools for the details block—but one could add a group within it, then assign spacing to that, if you wanted to extend the visual.

@tellthemachines
Copy link
Contributor

We don't need layout tools for the details block

Fortunately we can add layout support in order to enable block spacing without exposing any layout tools!

I've tried it for the Details block in #53282. Let me know what you think!

@webmandesign
Copy link
Contributor Author

@tellthemachines Thanks for the code. Does this work for you on website front-end too?

I've ported the code to my theme (modified Details block supports via blocks.registerBlockType filter and updated CSS styles) and it works fine in block editor, but does not work on front-end.

I'm on WC6.3-RC3 and Gutenberg 16.3.0.

@webmandesign
Copy link
Contributor Author

OK, after modifying Details block supports also via block_type_metadata_settings PHP filter, everything works perfectly fine on front-end too. Thanks for the solution @tellthemachines!

@webmandesign
Copy link
Contributor Author

BTW, the point 3 from my previous comment is not resolved yet.
It would really be great if Gutenberg applies inline style for --wp--style--block-gap value if block has no layout support (as discussed in subsequent comments).

@andrewserong
Copy link
Contributor

It would really be great if Gutenberg applies inline style for --wp--style--block-gap value if block has no layout support (as discussed in subsequent comments).

Would it be worth opening a separate issue for this one? I think it might potentially become fairly complex in terms of designing it as a reliable API. For example, outputting the CSS variable --wp--style--block-gap wouldn't really be viable as a block support feature because the control wouldn't have any effect on the block unless the block's styling supports the CSS variable. We could output gap directly, but that would also mean the block itself needs to use flex or grid.

There's probably a few more intricacies to figure out there before attempting to build it out as a feature, but perhaps the general concept would be: what sort of block support (if any) should be available for gap/spacing for blocks that don't use layout?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Details Affects the Details Block - used to display content which can be shown/hidden [Feature] Layout Layout block support, its UI controls, and style output. [Status] In discussion Used to indicate that an issue is in the process of being discussed [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

7 participants