Add allowedBlocksMiddleware #7301

jorgefilipecosta · 2018-06-13T14:46:57Z

The PR fixes the following list of issues:

Clicking enter when non-textual blocks are selected creates a new paragraph even if not allowed. Clicking enter when non-textual blocks are selected creates a new paragraph even if not allowed. #10186, an alternative to PR Fix: when enter is pressed on title unallowed blocks may be inserted #7265.
Copy paste may insert unallowed blocks. Alternative to Fix enter and copy past behaviour not respects allowedBlocks and child blocks restrictions #7230.
Drag & drop allows moving blocks to not allowed destinations alternative to Fix: Drag & drop allows to move blocks to not allowed destinations #7212.

This PR adds a middleware to the editor store whose objective is to make sure we never get into a situation where a block should not have been added to an area.
Props to @aduth for referring the middleware as an option.

This PR's aims to simplify some existing PR's, avoid code duplication and reduce the need for allowed blocks checking in the UI. Some checks will still be required thought to provide a better user experience, e.g., don't allow drag to even start in some cases, don't show unallowed transforms, etc...

Instead of middleware, we could have tried to use a top-level editor higher order reducer ( "top level" because we need different branches of the editor state tree). I felt the higher order reducer approach would not be a good idea because to make this verification we use information from blocks store (inside canInsertBlockType selector), so we would make our reducers impure.

This middleware watches 3 actions: INSERT_BLOCKS, MOVE_BLOCK_TO_POSITION, and REPLACE_BLOCKS.
On INSERT_BLOCKS we filter the blocks to insert to make sure all of them are allowed. If at least one of the blocks we are trying to add is permitted the action is dispatched with the filtered allowed blocks if none of the blocks are allowed the action is discarded.
For the actions MOVE_BLOCK_TO_POSITION and REPLACE_BLOCKS we verify if it is possible to contain all the blocks in the root UID if it is not possible the action is discarded.

Supersedes: #7265, #7212
Simplifies: https://github.com/WordPress/gutenberg/pull/7230/files

Todo: Automated tests & extensive docs tests were not yet added, I would like to get some feedback on this approach and if we think it is something we should follow I will add them before the merge.

How has this been tested?

Test block: https://gist.github.com/jorgefilipecosta/b7194daac3ce26827522694fb4252c1c#file-testallowedblocksmiddleware-js

I pasted the test block referred in the browser console.
I added the Product block.
I verified I cannot move using drag & drop the Buy button out of the Product block. (child block restriction.
I verified I cannot move using drag & drop a paragraph created outside of the Product block inside the product block. (allowed blocks restriction).
I verified that if I press enter in the buy button or image inside the product block, a paragraph is not created (allowed blocks restriction).
I added two galleries outside the product block, multi-selected them, and copied them ( ctrl + c ), I pasted them inside the list block inside the Product block and I verified they were not added (allowed blocks restriction).

I verified that when we have a template lock and we press enter on the title we now do not insert unallowed blocks.
Template lock code:

add_filter( 'allowed_block_types', function( $allowed_block_types, $post ) {
	if ( $post->post_type === 'post' ) {
	    return $allowed_block_types;
	}
	return [ 'core/image', 'core/button' ];
}, 10, 2 );

I verified that if we disable the paragraph, e.g., with a filter or inside the test block, pressing enter on the title or in placeholders does not add paragraph blocks. Pressing the sibling inserter also does not insert the paragraph (PR #7226).
Code to disable paragraph:

function myplugin_register_book_lock_post_type() {
    $args = array(
        'public' => true,
	  	'template_lock' => 'all',
        'label'  => 'Books Lock',
        'show_in_rest' => true,
        'template' => array(
            array( 'core/image', array(
            ) ),
            array( 'core/heading', array(
                'placeholder' => 'Add Author...',
            ) ),
            array( 'core/paragraph', array(
                'placeholder' => 'Add Description...',
            ) ),
        ),
    );
    register_post_type( 'book_lock', $args );
}
add_action( 'init', 'myplugin_register_book_lock_post_type' );

I added two image blocks, multi-selected them, copied them and tried to paste them on the list inside the test block. I checked the image blocks were not added inside the test block.

All these actions are possible in master.

Todo: If we accept this approach, end 2 end tests will be created before the merge that replicate this manual tests.

mcsf · 2018-07-09T15:33:53Z

InnerBlocks has changed quite a bit since this PR was opened. What's the status here?

jorgefilipecosta · 2018-07-09T16:16:03Z

Hi @mcsf, this PR was rebased. The problems this PR address continue to exist. The PR is waiting on a review.

mcsf

This makes sense, but I'm also not super familiar with the locking and templates domain. Right now this needs clean up around the inline comments, namely some structuring and maybe rephrasing for clarity.

mcsf · 2018-07-09T16:48:30Z

editor/utils/allowed-blocks-middleware.js

+		// on insert we allow the action if at least one of the blocks can be inserted filtering the other blocks
+		case 'INSERT_BLOCKS': {
+			const allowedBlocks =
+			filter( action.blocks, ( block ) => block && canInsertBlockType( store.getState(), block.name, action.rootUID ) );


These lines are weird. Consider:

const allowedBlocks = filter( action.blocks, ( block ) => block && canInsertBlockType( store.getState(), block.name, action.rootUID ) );

mcsf · 2018-07-09T16:49:57Z

editor/utils/allowed-blocks-middleware.js

+ */
+import { canInsertBlockType, getBlock, getBlockName, getBlockRootUID } from '../store/selectors';
+
+const allowedBlocksMiddleware = ( store ) => ( next ) => ( action ) => {


We should probably have a middleware directory somewhere.

Some high-level documentation for this middleware is warranted.

Hi @mcsf, in all the other store artifacts selectors, reducers, etc... We don't have a folder for them. We have all the artifacts inside the same file. So I updated the code an now our middleware is inside the middlewares file.

mcsf · 2018-07-09T16:51:22Z

editor/utils/allowed-blocks-middleware.js

+			}
+			return;
+		}
+		default:


Minor, but I think we tend to drop default: and just leave the closing statement (in this case, next( action )) after the switch block.

mcsf · 2018-07-09T16:51:33Z

editor/utils/allowed-blocks-middleware.js

+				const blockBeingReplaced = uidBlockBeingReplaced && getBlock( store.getState(), uidBlockBeingReplaced );
+				return blockBeingReplaced && blockBeingReplaced.name === block.name;
+			} );
+			if ( replaceIsValid	) {


Spacing before closing bracket.

mcsf · 2018-07-09T16:57:50Z

editor/utils/allowed-blocks-middleware.js

+	}
+	switch ( action.type ) {
+		// on insert we allow the action if at least one of the blocks can be inserted filtering the other blocks
+		case 'INSERT_BLOCKS': {


The braces around cases are something I'd never seen. 😅 I'm surprised the linter didn't complain, but it should be:

switch ( action.type ) { case 'INSERT_BLOCKS': foo(); return; case 'MOVE…':

Yes, they are not very typical but I like to use them in big cases like the ones here. It allows us to collapse case statements in the text editors for example and they make it easier to navigate around the statements.
But we should be consistent and given that we are not using them in other parts I removed them.

allows us to collapse case statements in the text editors

FWIW, editors should be able to use other folding methods (incl. manual and indentation-based), and these preferences shouldn't ever affect the way we write code.

They’re also useful in creating a scope for const variable definitions inside the case statement. We used them a lot on Mozilla projects, especially in switch case statements for reducers. 🤷🏻‍♂️

It seems we are also using this 'case VALUE: {' in the blockOrder reducer.

gutenberg/editor/store/reducer.js

Lines 419 to 425 in 6042f35

case 'INSERT_BLOCKS': {

const { rootUID = '', blocks } = action;

const subState = state[ rootUID ] || [];

const mappedBlocks = mapBlockOrder( blocks, rootUID );

const { index = subState.length } = action;

return {

The reason for the usage there seems to be what @tofumatt referred const scoping.
But in this case const scoping is not required, so we can probably continue without the extra {}.

Huh, I hadn't considered the scopes at all. It's true that these cases all feature local variables. D'oh.

That to me is enough to justify the block braces. I stand corrected; my apologies for the digression.

mcsf · 2018-07-09T17:13:22Z

editor/utils/allowed-blocks-middleware.js

+					return true;
+				}
+				const uidBlockBeingReplaced = action.uids[ index ];
+				const blockBeingReplaced = uidBlockBeingReplaced && getBlock( store.getState(), uidBlockBeingReplaced );


I recommend uidToReplace and blockToReplace, respectively.

mcsf · 2018-07-09T17:17:34Z

editor/utils/allowed-blocks-middleware.js

+			const rootUID = getBlockRootUID( store.getState(), first( action.uids ) );
+			// replace is valid if the new blocks can be inserted in the root block
+			// or if we had a block of the same type in the position of the block being replaced.
+			const replaceIsValid = every( action.blocks, ( block, index ) => {


I'd start the variable name with is to make its point clearer, and I might even avoid the word replace, since it's primarely a verb, suggesting a different kind of function. isOperationValid? isActionValid?

aduth · 2018-07-09T20:05:12Z

Without having yet looked closely into this, there's been an idea in my mind lately that I've been wanting to try, though perhaps you can give me your thoughts on whether you think there's merit to it. At one point in the past I expressed a desire to be able to provide an insertion context, which would provide various details including rootUID and block insertion restrictions. One of the main blockers to being able to pull this off is that these restrictions need to be made known to the global inserter, which is outside the scope of a block.

What I considered more recently is: Could we use the pattern of Slot/Fill or even just plain React portals to transport this context to the global inserter? My thinking is that this would be some combination of our Slot/Fill bubblesVirtually (to use React createPortal, which will transport context intact) and the Slot function-child variation to allow the Fill to inject an inserter context provider as an ancestor to the global inserter.

jorgefilipecosta · 2018-07-09T20:24:07Z

At one point in the past I expressed a desire to be able to provide an insertion context, which would provide various details including rootUID and block insertion restrictions. One of the main blockers to being able to pull this off is that these restrictions need to be made known to the global inserter, which is outside the scope of a block.

What I considered more recently is: Could we use the pattern of Slot/Fill or even just plain React portals to transport this context to the global inserter? My thinking is that this would be some combination of our Slot/Fill bubblesVirtually (to use React createPortal, which will transport context intact) and the Slot function-child variation to allow the Fill to inject an inserter context provider as an ancestor to the global inserter.

Hi @aduth, I think from the technical point of view using slot/fill + context for the inserter may work.

But we should decide if we should use the context or we should continue to have this information available globally from our store/data module.

In my option, I think the configuration of a nested area is a property of a block just like its attributes, and it should be available using a similar mechanism to the one we provide to query attributes.
Right now, plugins can query the available blocks. So, for example, the Drop it plugin may in the future check if it is possible to insert images in the currently selected block and if not it may show a message saying it is not possible to insert images in the currently selected block.
With a context-based approach, we don't have that possibility.

noisysocks · 2018-07-10T00:04:25Z

But we should decide if we should use the context or we should continue to have this information available globally from our store/data module.

I added select( 'core/editor' ).canInsertBlockType() with the intention that a single selector should contain all of the logic to determine whether or not a block can be placed into a given position. I like the idea of a selector as opposed to context because it's a more approachable API especially for plugin developers.

jorgefilipecosta · 2018-07-10T10:57:31Z

Hi @mcsf, thank you for your review. The points raised were addressed. The remaining part of this PR is an extensive list of test cases. I will give some time for us to think/discuss if this approach is something we want, and after I will add the test cases if that's the approach we should follow.

jorgefilipecosta · 2018-07-10T11:04:59Z

I added select( 'core/editor' ).canInsertBlockType() with the intention that a single selector should contain all of the logic to determine whether or not a block can be placed into a given position. I like the idea of a selector as opposed to context because it's a more approachable API especially for plugin developers.

Yes, I think or API is already simple to use canInsertBlockType( rootUID, blockType ) returns if the block can be inserted inside some specific block. Taking into account all restrictions (allowedBlocks, locking, available blocks etc....).
getInserterIntems( rootUID ) returns all the blocks that can be inserted also taking into account all the restrictions.

We have many checks to make on the UI e.g: show the option to insert some block or not, show the option to transform move etc... But even with context, if we want an optional UX these checks need to be made in order for us to render an option or not.
And I feel some checks like the drag&drop block move to a different parent block may be harder to perform if the information is only available in the context.

gziolo · 2019-01-31T17:56:00Z

@jorgefilipecosta what's the status of this PR, it seems to be very old. Is it something that still needs to be addressed since it didn't make to WordPress 5.0 release? I'm inclined to mark it Stale to make triaging PRs that need review easier. What's your plan regarding this PR?

jorgefilipecosta · 2019-02-19T18:00:20Z

Closing I will soon open a new PR that will address the same problem in a simpler way using controls, in a similar way to what was done in #13924.

jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label Jun 13, 2018

jorgefilipecosta self-assigned this Jun 13, 2018

jorgefilipecosta requested a review from a team June 18, 2018 09:56

jorgefilipecosta force-pushed the add/allowed-blocks-middleware branch from 54e9ebf to 4c8c9f5 Compare July 9, 2018 16:12

jorgefilipecosta force-pushed the add/allowed-blocks-middleware branch from 4c8c9f5 to 3dcb764 Compare July 9, 2018 16:18

mcsf reviewed Jul 9, 2018

View reviewed changes

jorgefilipecosta force-pushed the add/allowed-blocks-middleware branch 2 times, most recently from 70d072b to 9af1365 Compare July 10, 2018 10:47

aduth mentioned this pull request Sep 27, 2018

Clicking enter when non-textual blocks are selected creates a new paragraph even if not allowed. #10186

Closed

aduth mentioned this pull request Oct 24, 2018

Block List: Use default Inserter for sibling insertion #11018

Merged

Add allowedBlocksMiddleware

f1d4502

jorgefilipecosta force-pushed the add/allowed-blocks-middleware branch from 9af1365 to f1d4502 Compare November 15, 2018 22:20

gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jan 31, 2019

jorgefilipecosta closed this Feb 19, 2019

jorgefilipecosta deleted the add/allowed-blocks-middleware branch February 19, 2019 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add allowedBlocksMiddleware #7301

Add allowedBlocksMiddleware #7301

jorgefilipecosta commented Jun 13, 2018 •

edited

Loading

mcsf commented Jul 9, 2018

jorgefilipecosta commented Jul 9, 2018

mcsf left a comment

mcsf Jul 9, 2018

mcsf Jul 9, 2018

jorgefilipecosta Jul 10, 2018

mcsf Jul 9, 2018

mcsf Jul 9, 2018

mcsf Jul 9, 2018

jorgefilipecosta Jul 10, 2018

mcsf Jul 10, 2018

tofumatt Jul 10, 2018

jorgefilipecosta Jul 10, 2018 •

edited

Loading

mcsf Jul 10, 2018

mcsf Jul 9, 2018

mcsf Jul 9, 2018

aduth commented Jul 9, 2018

jorgefilipecosta commented Jul 9, 2018 •

edited

Loading

noisysocks commented Jul 10, 2018

jorgefilipecosta commented Jul 10, 2018

jorgefilipecosta commented Jul 10, 2018 •

edited

Loading

gziolo commented Jan 31, 2019

jorgefilipecosta commented Feb 19, 2019

	case 'INSERT_BLOCKS': {
	const { rootUID = '', blocks } = action;
	const subState = state[ rootUID ] \|\| [];
	const mappedBlocks = mapBlockOrder( blocks, rootUID );
	const { index = subState.length } = action;

	return {

Add allowedBlocksMiddleware #7301

Add allowedBlocksMiddleware #7301

Conversation

jorgefilipecosta commented Jun 13, 2018 • edited Loading

How has this been tested?

mcsf commented Jul 9, 2018

jorgefilipecosta commented Jul 9, 2018

mcsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Jul 9, 2018

jorgefilipecosta commented Jul 9, 2018 • edited Loading

noisysocks commented Jul 10, 2018

jorgefilipecosta commented Jul 10, 2018

jorgefilipecosta commented Jul 10, 2018 • edited Loading

gziolo commented Jan 31, 2019

jorgefilipecosta commented Feb 19, 2019

jorgefilipecosta commented Jun 13, 2018 •

edited

Loading

jorgefilipecosta Jul 10, 2018 •

edited

Loading

jorgefilipecosta commented Jul 9, 2018 •

edited

Loading

jorgefilipecosta commented Jul 10, 2018 •

edited

Loading