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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Apr 3, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
84f6fdd
Added ensure default block logic to replaceBlocks action; refactor
jorgefilipecosta Feb 26, 2019
44f7b7a
Readd logic to accept the replacement if a block of the same type exi…
jorgefilipecosta Mar 19, 2019
1252d77
Revisions
jorgefilipecosta Mar 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 128 additions & 30 deletions packages/block-editor/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { castArray } from 'lodash';
import { castArray, first } from 'lodash';

/**
* WordPress dependencies
Expand All @@ -13,6 +13,25 @@ import { getDefaultBlockName, createBlock } from '@wordpress/blocks';
*/
import { select } from './controls';

/**
* Generator which will yield a default block insert action if there
* are no other blocks at the root of the editor. This generator should be used
* in actions which may result in no blocks remaining in the editor (removal,
* replacement, etc).
*/
function* ensureDefaultBlock() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

const count = yield select(
'core/block-editor',
'getBlockCount',
);

// To avoid a focus loss when removing the last block, assure there is
// always a default block if the last of the blocks have been removed.
if ( count === 0 ) {
yield insertDefaultBlock();
}
}

/**
* Returns an action object used in signalling that blocks state should be
* reset to the specified array of blocks, taking precedence over any other
Expand Down Expand Up @@ -202,15 +221,36 @@ 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yields is available as a valid substitute in JSDoc.

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

* @yields {Object} Action object.
*/
export function replaceBlocks( clientIds, blocks ) {
return {
export function* replaceBlocks( clientIds, blocks ) {
clientIds = castArray( clientIds );
blocks = castArray( blocks );
const rootClientId = yield select(
'core/block-editor',
'getBlockRootClientId',
first( clientIds )
);
// Replace is valid if the new blocks can be inserted in the root block.
for ( let index = 0; index < blocks.length; index++ ) {
const block = blocks[ index ];
const canInsertBlock = yield select(
'core/block-editor',
'canInsertBlockType',
block.name,
rootClientId
);
if ( ! canInsertBlock ) {
return;
}
}
yield {
type: 'REPLACE_BLOCKS',
clientIds: castArray( clientIds ),
blocks: castArray( blocks ),
clientIds,
blocks,
time: Date.now(),
};
yield* ensureDefaultBlock();
}

/**
Expand Down Expand Up @@ -256,16 +296,51 @@ export const moveBlocksUp = createOnMove( 'MOVE_BLOCKS_UP' );
* @param {?string} toRootClientId Root client ID destination.
* @param {number} index The index to move the block into.
*
* @return {Object} Action object.
* @yields {Object} Action object.
*/
export function moveBlockToPosition( clientId, fromRootClientId, toRootClientId, index ) {
return {
export function* moveBlockToPosition( clientId, fromRootClientId, toRootClientId, index ) {
const templateLock = yield select(
'core/block-editor',
'getTemplateLock',
fromRootClientId
);

// If locking is equal to all on the original clientId (fromRootClientId),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// it is not possible to move the block to any other position.
if ( templateLock === 'all' ) {
return;
}

const action = {
type: 'MOVE_BLOCK_TO_POSITION',
fromRootClientId,
toRootClientId,
clientId,
index,
};
// If moving inside the same root block the move is always possible.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ( fromRootClientId === toRootClientId ) {
yield action;
return;
}

const blockName = yield select(
'core/block-editor',
'getBlockName',
clientId
);

const canInsertBlock = yield select(
'core/block-editor',
'canInsertBlockType',
blockName,
toRootClientId
);

// 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 ) {
yield action;
}
}

/**
Expand All @@ -279,8 +354,18 @@ export function moveBlockToPosition( clientId, fromRootClientId, toRootClientId,
*
* @return {Object} Action object.
*/
export function insertBlock( block, index, rootClientId, updateSelection = true ) {
return insertBlocks( [ block ], index, rootClientId, updateSelection );
export function insertBlock(
block,
index,
rootClientId,
updateSelection = true,
) {
return insertBlocks(
[ block ],
index,
rootClientId,
updateSelection
);
}

/**
Expand All @@ -292,17 +377,37 @@ export function insertBlock( block, index, rootClientId, updateSelection = true
* @param {?string} rootClientId Optional root client ID of block list on which to insert.
* @param {?boolean} updateSelection If true block selection will be updated. If false, block selection will not change. Defaults to true.
*
* @return {Object} Action object.
*/
export function insertBlocks( blocks, index, rootClientId, updateSelection = true ) {
return {
type: 'INSERT_BLOCKS',
blocks: castArray( blocks ),
index,
rootClientId,
time: Date.now(),
updateSelection,
};
* @return {Object} Action object.
*/
export function* insertBlocks(
jorgefilipecosta marked this conversation as resolved.
Show resolved Hide resolved
blocks,
index,
rootClientId,
updateSelection = true
) {
blocks = castArray( blocks );
const allowedBlocks = [];
for ( const block of blocks ) {
const isValid = yield select(
'core/block-editor',
'canInsertBlockType',
block.name,
rootClientId
);
if ( isValid ) {
allowedBlocks.push( block );
}
}
if ( allowedBlocks.length ) {
return {
type: 'INSERT_BLOCKS',
blocks: allowedBlocks,
index,
rootClientId,
time: Date.now(),
updateSelection,
};
}
}

/**
Expand Down Expand Up @@ -394,16 +499,9 @@ export function* removeBlocks( clientIds, selectPrevious = true ) {
clientIds,
};

const count = yield select(
'core/block-editor',
'getBlockCount',
);

// To avoid a focus loss when removing the last block, assure there is
// always a default block if the last of the blocks have been removed.
if ( count === 0 ) {
yield insertDefaultBlock();
}
yield* ensureDefaultBlock();
}

/**
Expand Down
22 changes: 0 additions & 22 deletions packages/block-editor/src/store/effects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,12 @@ import {
replaceBlocks,
selectBlock,
setTemplateValidity,
insertDefaultBlock,
resetBlocks,
} from './actions';
import {
getBlock,
getBlocks,
getSelectedBlockCount,
getBlockCount,
getTemplateLock,
getTemplate,
isValidTemplate,
Expand Down Expand Up @@ -60,23 +58,6 @@ export function validateBlocksToTemplate( action, store ) {
}
}

/**
* Effect handler which will return a default block insertion action if there
* are no other blocks at the root of the editor. This is expected to be used
* in actions which may result in no blocks remaining in the editor (removal,
* replacement, etc).
*
* @param {Object} action Action which had initiated the effect handler.
* @param {Object} store Store instance.
*
* @return {?Object} Default block insert action, if no other blocks exist.
*/
export function ensureDefaultBlock( action, store ) {
if ( ! getBlockCount( store.getState() ) ) {
return insertDefaultBlock();
}
}

export default {
MERGE_BLOCKS( action, store ) {
const { dispatch } = store;
Expand Down Expand Up @@ -127,9 +108,6 @@ export default {
RESET_BLOCKS: [
validateBlocksToTemplate,
],
REPLACE_BLOCKS: [
ensureDefaultBlock,
],
MULTI_SELECT: ( action, { getState } ) => {
const blockCount = getSelectedBlockCount( getState() );

Expand Down
Loading