From 249c0cf495ed89df95e039572974bc948fa89731 Mon Sep 17 00:00:00 2001 From: Ella <4710635+ellatrix@users.noreply.github.com> Date: Mon, 11 Nov 2024 12:51:47 +0100 Subject: [PATCH] Revert: Fix unable to remove empty blocks on merge (#65262) + alternative (#66564) Co-authored-by: ellatrix Co-authored-by: ntsekouras Co-authored-by: kspilarski Co-authored-by: talldan Co-authored-by: ndiego Co-authored-by: kevin940726 Co-authored-by: richtabor --- .../src/components/block-list/block.js | 76 +++++++------------ packages/blocks/README.md | 4 - packages/blocks/src/api/index.js | 9 --- packages/blocks/src/api/utils.js | 68 ++++------------- .../editor/various/splitting-merging.spec.js | 19 +++-- 5 files changed, 56 insertions(+), 120 deletions(-) diff --git a/packages/block-editor/src/components/block-list/block.js b/packages/block-editor/src/components/block-list/block.js index 2215625596dc5c..6d4655189d9723 100644 --- a/packages/block-editor/src/components/block-list/block.js +++ b/packages/block-editor/src/components/block-list/block.js @@ -24,8 +24,8 @@ import { isReusableBlock, getBlockDefaultClassName, hasBlockSupport, + createBlock, store as blocksStore, - privateApis as blocksPrivateApis, } from '@wordpress/blocks'; import { withFilters } from '@wordpress/components'; import { withDispatch, useDispatch, useSelect } from '@wordpress/data'; @@ -47,8 +47,6 @@ import { PrivateBlockContext } from './private-block-context'; import { unlock } from '../../lock-unlock'; -const { isUnmodifiedBlockContent } = unlock( blocksPrivateApis ); - /** * Merges wrapper props with special handling for classNames and styles. * @@ -313,6 +311,7 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { function switchToDefaultOrRemove() { const block = getBlock( clientId ); const defaultBlockName = getDefaultBlockName(); + const defaultBlockType = getBlockType( defaultBlockName ); if ( getBlockName( clientId ) !== defaultBlockName ) { const replacement = switchToBlockType( block, @@ -329,6 +328,15 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { selectBlock( nextBlockClientId ); } ); } + } else if ( defaultBlockType.merge ) { + const attributes = defaultBlockType.merge( + {}, + block.attributes + ); + replaceBlocks( + [ clientId ], + [ createBlock( defaultBlockName, attributes ) ] + ); } } @@ -342,6 +350,9 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { * to the moved block. */ function moveFirstItemUp( _clientId, changeSelection = true ) { + const wrapperBlockName = getBlockName( _clientId ); + const wrapperBlockType = getBlockType( wrapperBlockName ); + const isTextualWrapper = wrapperBlockType.category === 'text'; const targetRootClientId = getBlockRootClientId( _clientId ); const blockOrder = getBlockOrder( _clientId ); const [ firstClientId ] = blockOrder; @@ -351,50 +362,14 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { isUnmodifiedBlock( getBlock( firstClientId ) ) ) { removeBlock( _clientId ); - } else { + } else if ( isTextualWrapper ) { registry.batch( () => { - const firstBlock = getBlock( firstClientId ); - const isFirstBlockContentUnmodified = - isUnmodifiedBlockContent( firstBlock ); - const defaultBlockName = getDefaultBlockName(); - const replacement = switchToBlockType( - firstBlock, - defaultBlockName - ); - const canTransformToDefaultBlock = - !! replacement?.length && - replacement.every( ( block ) => - canInsertBlockType( block.name, _clientId ) - ); - if ( - isFirstBlockContentUnmodified && - canTransformToDefaultBlock - ) { - // Step 1: If the block is empty and can be transformed to the default block type. - replaceBlocks( - firstClientId, - replacement, - changeSelection - ); - } else if ( - isFirstBlockContentUnmodified && - firstBlock.name === defaultBlockName - ) { - // Step 2: If the block is empty and is already the default block type. - removeBlock( firstClientId ); - const nextBlockClientId = - getNextBlockClientId( clientId ); - if ( nextBlockClientId ) { - selectBlock( nextBlockClientId ); - } - } else if ( canInsertBlockType( - firstBlock.name, + getBlockName( firstClientId ), targetRootClientId ) ) { - // Step 3: If the block can be moved up. moveBlocksToPosition( [ firstClientId ], _clientId, @@ -402,17 +377,21 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { getBlockIndex( _clientId ) ); } else { - const canLiftAndTransformToDefaultBlock = - !! replacement?.length && + const replacement = switchToBlockType( + getBlock( firstClientId ), + getDefaultBlockName() + ); + + if ( + replacement && + replacement.length && replacement.every( ( block ) => canInsertBlockType( block.name, targetRootClientId ) - ); - - if ( canLiftAndTransformToDefaultBlock ) { - // Step 4: If the block can be transformed to the default block type and moved up. + ) + ) { insertBlocks( replacement, getBlockIndex( _clientId ), @@ -421,7 +400,6 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { ); removeBlock( firstClientId, false ); } else { - // Step 5: Continue the default behavior. switchToDefaultOrRemove(); } } @@ -433,6 +411,8 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { removeBlock( _clientId, false ); } } ); + } else { + switchToDefaultOrRemove(); } } diff --git a/packages/blocks/README.md b/packages/blocks/README.md index f4805e1c60b381..3e5a88a2b92c1b 100644 --- a/packages/blocks/README.md +++ b/packages/blocks/README.md @@ -503,10 +503,6 @@ _Returns_ - `Array|string`: A list of blocks or a string, depending on `handlerMode`. -### privateApis - -Undocumented declaration. - ### rawHandler Converts an HTML string to known blocks. diff --git a/packages/blocks/src/api/index.js b/packages/blocks/src/api/index.js index 0b38b8e29e68a0..3ace68be87393c 100644 --- a/packages/blocks/src/api/index.js +++ b/packages/blocks/src/api/index.js @@ -1,9 +1,3 @@ -/** - * Internal dependencies - */ -import { lock } from '../lock-unlock'; -import { isUnmodifiedBlockContent } from './utils'; - // The blocktype is the most important concept within the block API. It defines // all aspects of the block configuration and its interfaces, including `edit` // and `save`. The transforms specification allows converting one blocktype to @@ -175,6 +169,3 @@ export { __EXPERIMENTAL_ELEMENTS, __EXPERIMENTAL_PATHS_WITH_OVERRIDE, } from './constants'; - -export const privateApis = {}; -lock( privateApis, { isUnmodifiedBlockContent } ); diff --git a/packages/blocks/src/api/utils.js b/packages/blocks/src/api/utils.js index 7bace4ff84c29b..20f0f6a85ed091 100644 --- a/packages/blocks/src/api/utils.js +++ b/packages/blocks/src/api/utils.js @@ -30,30 +30,6 @@ extend( [ namesPlugin, a11yPlugin ] ); */ const ICON_COLORS = [ '#191e23', '#f8f9f9' ]; -/** - * Determines whether the block's attribute is equal to the default attribute - * which means the attribute is unmodified. - * @param {Object} attributeDefinition The attribute's definition of the block type. - * @param {*} value The attribute's value. - * @return {boolean} Whether the attribute is unmodified. - */ -function isUnmodifiedAttribute( attributeDefinition, value ) { - // Every attribute that has a default must match the default. - if ( attributeDefinition.hasOwnProperty( 'default' ) ) { - return value === attributeDefinition.default; - } - - // The rich text type is a bit different from the rest because it - // has an implicit default value of an empty RichTextData instance, - // so check the length of the value. - if ( attributeDefinition.type === 'rich-text' ) { - return ! value?.length; - } - - // Every attribute that doesn't have a default should be undefined. - return value === undefined; -} - /** * Determines whether the block's attributes are equal to the default attributes * which means the block is unmodified. @@ -67,7 +43,20 @@ export function isUnmodifiedBlock( block ) { ( [ key, definition ] ) => { const value = block.attributes[ key ]; - return isUnmodifiedAttribute( definition, value ); + // Every attribute that has a default must match the default. + if ( definition.hasOwnProperty( 'default' ) ) { + return value === definition.default; + } + + // The rich text type is a bit different from the rest because it + // has an implicit default value of an empty RichTextData instance, + // so check the length of the value. + if ( definition.type === 'rich-text' ) { + return ! value?.length; + } + + // Every attribute that doesn't have a default should be undefined. + return value === undefined; } ); } @@ -84,35 +73,6 @@ export function isUnmodifiedDefaultBlock( block ) { return block.name === getDefaultBlockName() && isUnmodifiedBlock( block ); } -/** - * Determines whether the block content is unmodified. A block content is - * considered unmodified if all the attributes that have a role of 'content' - * are equal to the default attributes (or undefined). - * If the block does not have any attributes with a role of 'content', it - * will be considered unmodified if all the attributes are equal to the default - * attributes (or undefined). - * - * @param {WPBlock} block Block Object - * @return {boolean} Whether the block content is unmodified. - */ -export function isUnmodifiedBlockContent( block ) { - const contentAttributes = getBlockAttributesNamesByRole( - block.name, - 'content' - ); - - if ( contentAttributes.length === 0 ) { - return isUnmodifiedBlock( block ); - } - - return contentAttributes.every( ( key ) => { - const definition = getBlockType( block.name )?.attributes[ key ]; - const value = block.attributes[ key ]; - - return isUnmodifiedAttribute( definition, value ); - } ); -} - /** * Function that checks if the parameter is a valid icon. * diff --git a/test/e2e/specs/editor/various/splitting-merging.spec.js b/test/e2e/specs/editor/various/splitting-merging.spec.js index eba9f1d3163fd5..146039a7c7d1bf 100644 --- a/test/e2e/specs/editor/various/splitting-merging.spec.js +++ b/test/e2e/specs/editor/various/splitting-merging.spec.js @@ -393,6 +393,11 @@ test.describe( 'splitting and merging blocks (@firefox, @webkit)', () => { attributes: { content: 'heading', level: 2 }, innerBlocks: [], }; + const paragraphWithContent = { + name: 'core/paragraph', + attributes: { content: 'heading', dropCap: false }, + innerBlocks: [], + }; const placeholderBlock = { name: 'core/separator' }; await editor.insertBlock( { name: 'core/group', @@ -407,6 +412,9 @@ test.describe( 'splitting and merging blocks (@firefox, @webkit)', () => { .getByRole( 'document', { name: 'Empty block' } ) .focus(); + // Remove the alignment. + await page.keyboard.press( 'Backspace' ); + // Remove the empty paragraph block. await page.keyboard.press( 'Backspace' ); await expect .poll( editor.getBlocks, 'Remove the default empty block' ) @@ -422,8 +430,7 @@ test.describe( 'splitting and merging blocks (@firefox, @webkit)', () => { }, ] ); - // Move the caret to the beginning of the empty heading block. - await page.keyboard.press( 'ArrowDown' ); + // Convert the heading to a default block. await page.keyboard.press( 'Backspace' ); await expect .poll( @@ -441,6 +448,9 @@ test.describe( 'splitting and merging blocks (@firefox, @webkit)', () => { ], }, ] ); + // Remove the alignment. + await page.keyboard.press( 'Backspace' ); + // Remove the empty default block. await page.keyboard.press( 'Backspace' ); await expect.poll( editor.getBlocks ).toEqual( [ { @@ -453,17 +463,16 @@ test.describe( 'splitting and merging blocks (@firefox, @webkit)', () => { }, ] ); - // Move the caret to the beginning of the "heading" heading block. - await page.keyboard.press( 'ArrowDown' ); + // Convert a non-empty non-default block to a default block. await page.keyboard.press( 'Backspace' ); await expect .poll( editor.getBlocks, 'Lift the non-empty non-default block' ) .toEqual( [ - headingWithContent, { name: 'core/group', attributes: { tagName: 'div' }, innerBlocks: [ + paragraphWithContent, expect.objectContaining( placeholderBlock ), ], },