Fix: Restrict block insert, move, and replace attending to allowedBlocks, locking, and child blocks #14003

jorgefilipecosta · 2019-02-21T12:20:16Z

Supersedes: https://github.com/WordPress/gutenberg/pull/7301/files
Uses the select control implemented in #13924.
This PR changes the action creators to be a generator and have all the required validations before yielding the action.

Fixes: #14568;
Fixes: #10186;
Fixes: #13099;

Missing the update to the unit tests.

How has this been tested?

Test block: 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.

packages/block-editor/src/store/actions.js

aduth · 2019-03-18T18:35:45Z

packages/block-editor/src/store/actions.js

+		first( clientIds )
+	);
+	// 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.


or if we had a block of the same type in the position of the block being replaced.

Is there an occasion of this you had in mind that led you to implement this exception? It's not clear to me for what reason we'd want it, and it adds some complexity to the implementation.

Hi @aduth,
Initially, I followed your suggestion, but on a second thought, I think the condition has some benefits for the merging of blocks.
In the following gist we see a sample block with InnerBlocks:
https://gist.github.com/jorgefilipecosta/6d8c1620b46d916d8a16a23e730a2ab6

If we have this condition the user can merge paragraphs by pressing backspace or delete. Without this condition, merge is not possible. But the same result is possible to achieve by removing a paragraph and appending its content to the content of another paragraph -- So I think merging should be possible.

In the following gist we see a sample block with InnerBlocks:
https://gist.github.com/jorgefilipecosta/6d8c1620b46d916d8a16a23e730a2ab6

This seems like an invalid use of InnerBlocks, by the fact the template contains blocks not valid per allowedBlocks. At the very least, we shouldn't optimize to anticipate this use-case.

Hi @aduth there are InnerBlocks scenarios where I expect this situation to happen.
For example, I just want to allow a maximum of 3 headings inside my InnerBlocks area. The way to implement this restriction would be remove heading from the allowed blocks if we already have 3 headings.

But I agree even this last scenario I referred is not normal and maybe something we should not optimize for.
I thought replacements would be needed to paste a block in the same position as an existing block. Paste HTML heading in a heading block, but that's not the case. I will reflect a little bit to make sure I'm not missing anything but I find don't any "normal" case I will simplify the logic.

jorgefilipecosta · 2019-03-19T18:03:23Z

This PR was updated after the PR's it depended on were merged and is ready for a new look.

aduth · 2019-03-27T18:58:19Z

Noting some overlap between this and #14594 , cc @nerrad (specifically ensureDefaultBlock refactor).

aduth · 2019-03-27T19:21:07Z

packages/block-editor/src/store/actions.js

@@ -201,16 +220,41 @@ export function toggleSelection( isSelectionEnabled = true ) {
 *
 * @param {(string|string[])} clientIds Block client ID(s) to replace.
 * @param {(Object|Object[])} blocks    Replacement block(s).
- *
- * @return {Object} Action object.


@yields is available as a valid substitute in JSDoc.

http://usejsdoc.org/tags-yields.html

aduth · 2019-03-27T19:24:01Z

packages/block-editor/src/store/actions.js

+		first( clientIds )
+	);
+	// 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.


In the following gist we see a sample block with InnerBlocks:
https://gist.github.com/jorgefilipecosta/6d8c1620b46d916d8a16a23e730a2ab6

This seems like an invalid use of InnerBlocks, by the fact the template contains blocks not valid per allowedBlocks. At the very least, we shouldn't optimize to anticipate this use-case.

aduth · 2019-03-27T19:25:58Z

packages/block-editor/src/store/actions.js

+
+	// If moving to other parent block, the move is possible if we can insert a block of the same type inside the new parent block.
+	if ( canInsertBlock ) {
+		return action;


Should we yield, not return? Unclear to me what behavior is expected from redux-routine for the return.

packages/block-editor/src/store/actions.js

aduth · 2019-03-27T19:28:25Z

packages/block-editor/src/store/actions.js

+	blocks = castArray( blocks );
+	const allowedBlocks = [];
+	for ( const block of blocks ) {
+		if ( block ) {


Why are there falsey entries in blocks ?

I guess it is never falsey, I updated the code to remove this check.

jorgefilipecosta · 2019-03-28T20:07:04Z

Hi @aduth thank you for the review, I updated the code to follow your suggestions.

aduth

A few questions, but functionally this works well in my testing.

Your "fixes" syntax from the original comment will not auto-close all the related issues. It would need to be separate individual "Fixes" one-per-line.

aduth · 2019-04-02T19:39:46Z

packages/block-editor/src/store/actions.js

 		type: 'MOVE_BLOCK_TO_POSITION',
 		fromRootClientId,
 		toRootClientId,
 		clientId,
 		index,
 	};
+	// If moving inside the same root block the move is always possible.


This too seems contrary in the same sense as #14003 (comment) , and it could produce simpler code to avoid specialized conditions. I could see an argument being made that this helps as an optimized majority case.

Hi @aduth although this could be seen as optimization as the majority of cases will return here, this is also required.
The case this covers is locking=insert, in that case, we can not insert blocks but we can move the blocks, canInsertBlockType will return false but the move is in fact possible. Without this condition when locking = insert we would not be able to move the blocks as expected.

aduth · 2019-04-02T19:41:06Z

packages/block-editor/src/store/actions.js

+		fromRootClientId
+	);
+
+	// If locking is equal to all on the original clientId (fromRootClientId),


Locking is checked in canInsertBlockType, considered later in the function. Do we need to duplicate the check here as well, or could this condition be removed and it work just as well?

Yes, we need this condition -- If the user is moving the block from a parent with locking=all to a block without locking, canInsertBlockType will return true, but the move is not possible.

aduth · 2019-04-02T19:51:07Z

packages/block-editor/src/store/actions.js

+ * in actions which may result in no blocks remaining in the editor (removal,
+ * replacement, etc).
+ */
+function* ensureDefaultBlock() {


Given #14754 and the possibility we may want to try to find a different way to "ensure" the default block, I think it's wise to not expose this as an exported action (i.e. as-is).

…sted in a given position

jorgefilipecosta · 2019-04-03T13:49:56Z

Thank you for the reviews, if there is anything I missed feel free to comment and I will iterate on this.

…cks, locking, and child blocks (#14003) Supersedes: https://github.com/WordPress/gutenberg/pull/7301/files Uses the select control implemented in #13924. This PR changes the action creators to be a generator and have all the required validations before yielding the action. Fixes: #14568; Fixes: #10186; Fixes: #13099; Missing the update to the unit tests. ## How has this been tested? Test block: [gist.github.com/jorgefilipecosta/b7194daac3ce26827522694fb4252c1c#file-testallowedblocksmiddleware-js](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.

jorgefilipecosta force-pushed the fix/correctly-restrict-block-insert-move-and-replace-attending-to-allowed-blocks-locking-and-child-blocks branch from 98a3794 to 16bd0e7 Compare February 26, 2019 22:19

jorgefilipecosta marked this pull request as ready for review February 26, 2019 22:20

jorgefilipecosta force-pushed the fix/correctly-restrict-block-insert-move-and-replace-attending-to-allowed-blocks-locking-and-child-blocks branch from 16bd0e7 to 7cd4cac Compare February 27, 2019 09:03

jorgefilipecosta requested review from ajitbohra, chrisvanpatten, gziolo, mkaz, oandregal, notnownikki and youknowriad as code owners February 27, 2019 09:03

jorgefilipecosta added the [Feature] Inserter The main way to insert blocks using the + button in the editing interface label Feb 27, 2019

jorgefilipecosta force-pushed the fix/correctly-restrict-block-insert-move-and-replace-attending-to-allowed-blocks-locking-and-child-blocks branch from 7cd4cac to fd4a3b1 Compare February 27, 2019 12:07

jorgefilipecosta requested review from noisysocks and talldan as code owners February 27, 2019 12:07

jorgefilipecosta force-pushed the fix/correctly-restrict-block-insert-move-and-replace-attending-to-allowed-blocks-locking-and-child-blocks branch 3 times, most recently from eff4d12 to 73cb181 Compare March 4, 2019 13:36

jorgefilipecosta requested a review from a team March 5, 2019 09:14

gziolo reviewed Mar 5, 2019

View reviewed changes

packages/block-editor/src/store/actions.js Show resolved Hide resolved

jorgefilipecosta mentioned this pull request Mar 7, 2019

Return inside Titel creates new block despite template_lock set on CPT #14307

Closed

gziolo added this to the 5.3 (Gutenberg) milestone Mar 11, 2019

jorgefilipecosta mentioned this pull request Mar 14, 2019

Fix: Drag & drop allows to move blocks to not allowed destinations #7212

Closed

youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019

jorgefilipecosta force-pushed the fix/correctly-restrict-block-insert-move-and-replace-attending-to-allowed-blocks-locking-and-child-blocks branch from 73cb181 to e113599 Compare March 18, 2019 18:19

aduth reviewed Mar 18, 2019

View reviewed changes

jorgefilipecosta force-pushed the fix/correctly-restrict-block-insert-move-and-replace-attending-to-allowed-blocks-locking-and-child-blocks branch from e113599 to 0f15475 Compare March 19, 2019 10:22

jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Mar 19, 2019

jorgefilipecosta force-pushed the fix/correctly-restrict-block-insert-move-and-replace-attending-to-allowed-blocks-locking-and-child-blocks branch from 0f15475 to f30c84a Compare March 19, 2019 17:39

jorgefilipecosta removed the [Status] In Progress Tracking issues with work in progress label Mar 19, 2019

This was referenced Mar 20, 2019

Fix enter and copy past behaviour not respects allowedBlocks and child blocks restrictions #7230

Closed

Pressing enter in page title ignores page level template lock and creates paragraph block. #14568

Closed

aduth mentioned this pull request Mar 27, 2019

Copy / Paste error core/paragraph using InnerBlocks #12774

Closed

aduth reviewed Mar 27, 2019

View reviewed changes

jorgefilipecosta force-pushed the fix/correctly-restrict-block-insert-move-and-replace-attending-to-allowed-blocks-locking-and-child-blocks branch from f30c84a to cb87da3 Compare March 28, 2019 13:12

aduth approved these changes Apr 2, 2019

View reviewed changes

jorgefilipecosta added 3 commits April 3, 2019 14:29

Added ensure default block logic to replaceBlocks action; refactor

84f6fdd

Readd logic to accept the replacement if a block of the same type exi…

44f7b7a

…sted in a given position

Revisions

1252d77

jorgefilipecosta force-pushed the fix/correctly-restrict-block-insert-move-and-replace-attending-to-allowed-blocks-locking-and-child-blocks branch from cb87da3 to 1252d77 Compare April 3, 2019 13:31

jorgefilipecosta requested a review from ellatrix as a code owner April 3, 2019 13:31

jorgefilipecosta closed this Apr 3, 2019

jorgefilipecosta reopened this Apr 3, 2019

jorgefilipecosta merged commit 2205f00 into master Apr 3, 2019

jorgefilipecosta deleted the fix/correctly-restrict-block-insert-move-and-replace-attending-to-allowed-blocks-locking-and-child-blocks branch April 3, 2019 14:44

aduth mentioned this pull request Apr 4, 2019

Refactor resetBlocks to action-generators #14594

Closed

8 tasks

jorgefilipecosta mentioned this pull request Apr 11, 2019

Fix move block to position bug; Add test cases; #14924

Merged

youknowriad added the [Type] Bug An existing feature does not function as intended label Apr 12, 2019

youknowriad added this to the 5.5 (Gutenberg) milestone Apr 12, 2019

talldan mentioned this pull request Apr 15, 2019

Bump version number to 5.5.0-rc.1 #14981

Merged

aduth mentioned this pull request Apr 15, 2019

Plugin: Backport bug fixes from Gutenberg 5.5.0 to wp/trunk (WordPress 5.2 RC) #14987

Merged

This was referenced Apr 17, 2019

Bump/5.5 #15011

Closed

Release/5.5 #15012

Closed

Bump plugin version to 5.5 #15013

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Restrict block insert, move, and replace attending to allowedBlocks, locking, and child blocks #14003

Fix: Restrict block insert, move, and replace attending to allowedBlocks, locking, and child blocks #14003

jorgefilipecosta commented Feb 21, 2019 •

edited

Loading

aduth Mar 18, 2019

jorgefilipecosta Mar 19, 2019

aduth Mar 27, 2019

jorgefilipecosta Mar 27, 2019

jorgefilipecosta Mar 27, 2019

jorgefilipecosta commented Mar 19, 2019

aduth commented Mar 27, 2019 •

edited

Loading

aduth Mar 27, 2019

aduth Mar 27, 2019

aduth Mar 27, 2019

aduth Mar 27, 2019

jorgefilipecosta Mar 28, 2019

jorgefilipecosta commented Mar 28, 2019

aduth left a comment

aduth Apr 2, 2019

jorgefilipecosta Apr 3, 2019

aduth Apr 2, 2019

jorgefilipecosta Apr 3, 2019

aduth Apr 2, 2019

jorgefilipecosta commented Apr 3, 2019

Fix: Restrict block insert, move, and replace attending to allowedBlocks, locking, and child blocks #14003

Fix: Restrict block insert, move, and replace attending to allowedBlocks, locking, and child blocks #14003

Conversation

jorgefilipecosta commented Feb 21, 2019 • edited Loading

How has this been tested?

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 commented Mar 19, 2019

aduth commented Mar 27, 2019 • 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

Choose a reason for hiding this comment

jorgefilipecosta commented Mar 28, 2019

aduth 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

jorgefilipecosta commented Apr 3, 2019

jorgefilipecosta commented Feb 21, 2019 •

edited

Loading

aduth commented Mar 27, 2019 •

edited

Loading