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

InnerBlocks' allowedBlockswhitelist not respected due to parent field of another block's block.json. #32436

Closed
getdave opened this issue Jun 3, 2021 · 8 comments
Labels
[Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P Needs Technical Feedback Needs testing from a developer perspective. Needs Testing Needs further testing to be confirmed. [Type] Bug An existing feature does not function as intended

Comments

@getdave
Copy link
Contributor

getdave commented Jun 3, 2021

Description

The <InnerBlocks> API enables a developer to provide a whitelist of allowedBlocks which should determine which blocks are allowed to be inserted as child blocks.

A good example of this is the core/navigation block which defines an ALLOWED_LIST as a constant.

However, currently on trunk if a child block defines a parent in its block.json then it will always be available as an "allowed block type" even if the parent block has excluded it from the allowed blocks.

The reason for this appears to be the canInsertBlockTypeUnmemozied selector

const parentBlockListSettings = getBlockListSettings( state, rootClientId );
// The parent block doesn't have settings indicating it doesn't support
// inner blocks, return false.
if ( rootClientId && parentBlockListSettings === undefined ) {
return false;
}
const parentAllowedBlocks = parentBlockListSettings?.allowedBlocks;
const hasParentAllowedBlock = checkAllowList(
parentAllowedBlocks,
blockName
);
const blockAllowedParentBlocks = blockType.parent;
const parentName = getBlockName( state, rootClientId );
const hasBlockAllowedParent = checkAllowList(
blockAllowedParentBlocks,
parentName
);
if ( hasParentAllowedBlock !== null && hasBlockAllowedParent !== null ) {
return hasParentAllowedBlock || hasBlockAllowedParent;
} else if ( hasParentAllowedBlock !== null ) {
return hasParentAllowedBlock;
} else if ( hasBlockAllowedParent !== null ) {
return hasBlockAllowedParent;
}
return true;

Essentially the current logic allows a block to be available if either:

  1. The parent has allowed it via <InnerBlock>'s allowedBlocks.
  2. The block itself has defined the parent block in the parent field of its block.json.

As a result, even if #1 is false, if #2 is true then the block will be made available despite the fact that the parent's block list settings explicitly defined that it should not be available.

For an example see the core/navigation-link block which defines core/navigation as a parent block in its block.json.

Steps to reproduce

You can test the problem for yourself.

  • Make sure you are on trunk.
  • Go to the core/navigation block's edit.js and replace

const ALLOWED_BLOCKS = [
'core/navigation-link',
'core/search',
'core/social-links',
'core/page-list',
'core/spacer',
'core/home-link',
];

with

const ALLOWED_BLOCKS = [
	// 'core/navigation-link',
	'core/search',
	// 'core/social-links',
	// 'core/page-list',
	'core/spacer',
	// 'core/home-link',
];

This should mean that only spacer and search blocks are allowed as child blocks of Navigation.

  • Build the Plugin - npm run dev.
  • Now insert a navigation block.
  • Try adding items via the block inserter.
  • You should lots of blocks available rather than the expected whitelist you defined earlier.

Expected behaviour

If a given block A defines a set of allowedBlocks and passes that to <InnerBlocks> then only those blocks should be available as "allowed blocks" even if another block B defines block A as a parent in block.json.

Actual behaviour

If a block defines another block as a parent via block.json then it will always be available even if the other block excludes it from the allowedBlocks list of <InnerBlocks>.

Screenshots or screen recording (optional)

Screen Shot 2021-06-03 at 17 15 18

Code snippet (optional)

WordPress information

  • WordPress version:
  • Gutenberg version:
  • Are all plugins except Gutenberg deactivated?
  • Are you using a default theme (e.g. Twenty Twenty-One)?

Device information

  • Device:
  • Operating system:
  • Browser:
@getdave getdave added [Type] Bug An existing feature does not function as intended [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P labels Jun 3, 2021
@getdave getdave changed the title InnerBlocks allowed blocks not respected due to parent field of another block's block.json. InnerBlocks' allowedBlockswhitelist not respected due to parent field of another block's block.json. Jun 3, 2021
@getdave getdave added Needs Technical Feedback Needs testing from a developer perspective. Needs Testing Needs further testing to be confirmed. labels Jun 3, 2021
@getdave
Copy link
Contributor Author

getdave commented Jun 3, 2021

@talldan Might you have 5mins to verify this? It would be much appreciated. I suspect this is the core reason why we are using block filters to manually force the available blocks for the Nav Editor.

@draganescu
Copy link
Contributor

In my understanding, the gist of the above is that allowed blocks should have more strength than parent.

I 100% think you're right to expect so as well, since:

  • a block specifying its allowed parent is a matter of block availability
  • a block specifying a list of allowed blocks is a matter of functionality

If a parent specification overrides the allowed blocks specification it has high chances to crash the parent. If the allowed blocks overrides the parent nothing breaks (if the only available parent of block x does not allow block x, nothing breaks, it's a small bug).

@getdave
Copy link
Contributor Author

getdave commented Jun 4, 2021

In my understanding, the gist of the above is that allowed blocks should have more strength than parent.

I believe you are correct 👍

Returns the list of allowed inserter blocks for inner blocks children

Also __experimentalGetAllowedBlocks actually appears to be specifically focused on what is allowed in the inserter.

Indeed it specifically tests for

hasBlockSupport( blockType, 'inserter', true ) ) 

In which case I think we should rename the method as I took it to mean "tell me which blocks are allowed to be inserted into the editor in general".

@draganescu
Copy link
Contributor

draganescu commented Jun 7, 2021

What is the difference between blocks allowed in the inserter and blocks allowed to be inserted in a block? I think I get the answer but we may need to spell it out. Do we need hidden blocks? If so, what is the difference between hidden and not allowed?

@talldan
Copy link
Contributor

talldan commented Jun 9, 2021

It's worth considering how plugins might work. A block might have a setting for allowedBlocks. A plugin has no way of modifying that. If a plugin wants to add a new supported inner block type to the navigation block, the only option is the parent property.

So I think the current behaviour is correct. Making allowedBlocks have precedence would severely limit what plugins can do.

It's been mentioned before that parent is a bit limited. Once it's set, the block can't be used globally, which is what Page List, Spacer, Social Links and Search all require, so they're in the allowed blocks list because they can't use parent. Blocks that have a limited context (button in buttons, social link in social links) should probably all only use parent for consistency.

Also __experimentalGetAllowedBlocks actually appears to be specifically focused on what is allowed in the inserter.

Yeah, I've mentioned before this could be renamed and stabilised, but it slips my mind now what a good name would be - #25786 (comment) (cc @draganescu).

@getdave
Copy link
Contributor Author

getdave commented Jun 9, 2021

A block might have a setting for allowedBlocks. A plugin has no way of modifying that. If a plugin wants to add a new supported inner block type to the navigation block, the only option is the parent property.

So I think the current behaviour is correct. Making allowedBlocks have precedence would severely limit what plugins can do.

I'm thinking this might be just a limitation of the block supports API (ie: it's not filterable)? It feels like we're allowing parents to take precedent purely in order to allow Plugins to modify where a block appears. To me as a developer if I set a whitelist of allowedBlocks on my InnerBlocks I'd expect that to be honoured. I certainly wouldn't expect this to be overridden because of another prop that seems (at least initially and without a lot of debugging) unrelated.

Should we instead look at if/how we can make the Block Supports API more extensible for Plugin developers? Indeed I see some overlap here with theme.json. As a theme author, I might want to limit what blocks can be added to the core/navigation block and I should be able to do that via theme.json.

@talldan
Copy link
Contributor

talldan commented Jun 10, 2021

To me as a developer if I set a whitelist of allowedBlocks on my InnerBlocks I'd expect that to be honoured.

I'm not sure, it seems a bit of an existential question—if parent is set shouldn't that also be honoured? I think of parent as the equivalent of a from transform, where a block developer shouldn't have to modify other blocks to specify a transform, they should be able to make it work from a single block specification. In the same way, if a third part developer wants to make a block for use only in the nav block, they shouldn't need to modify the nav block. So parent could be considered as adding to allowedBlocks rather than conflicting with it.

It might also be difficult to change the order of precedence now, because it'd be a breaking change.


Should we instead look at if/how we can make the Block Supports API more extensible for Plugin developers?

There have been some past issues about extending allowedBlocks, along with some other ideas, definitely would be good to explore that further:

Possibly a few duplicate issues, and worth searching for others that I didn't catch.

@getdave
Copy link
Contributor Author

getdave commented Aug 10, 2021

This isn't a question I can answer at this point so closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P Needs Technical Feedback Needs testing from a developer perspective. Needs Testing Needs further testing to be confirmed. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

3 participants