From 968e72e91b190ebd72af79e7b4857e4f6a2e96c7 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Thu, 12 Sep 2024 14:31:20 +0800 Subject: [PATCH 1/3] Fix unable to remove empty blocks on merge --- .../src/components/block-list/block.js | 77 ++++++++++++--- packages/blocks/src/api/index.js | 2 + packages/blocks/src/api/utils.js | 39 +++++--- .../editor/various/splitting-merging.spec.js | 97 +++++++++++++++++++ 4 files changed, 189 insertions(+), 26 deletions(-) diff --git a/packages/block-editor/src/components/block-list/block.js b/packages/block-editor/src/components/block-list/block.js index 90c39649319dc8..7b987a3608e034 100644 --- a/packages/block-editor/src/components/block-list/block.js +++ b/packages/block-editor/src/components/block-list/block.js @@ -24,7 +24,9 @@ import { isReusableBlock, getBlockDefaultClassName, hasBlockSupport, + __experimentalGetBlockAttributesNamesByRole, store as blocksStore, + privateApis as blocksPrivateApis, } from '@wordpress/blocks'; import { withFilters } from '@wordpress/components'; import { withDispatch, useDispatch, useSelect } from '@wordpress/data'; @@ -46,6 +48,8 @@ import { PrivateBlockContext } from './private-block-context'; import { unlock } from '../../lock-unlock'; +const { isAttributeUnmodified } = unlock( blocksPrivateApis ); + /** * Merges wrapper props with special handling for classNames and styles. * @@ -307,6 +311,26 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { canInsertBlockType, } = registry.select( blockEditorStore ); + /** + * A block is "empty" if all of its content attributes are unmodified. + * + * @param {WPBlock} block The block to check. + * @return {boolean} Whether the block is empty. + */ + function isBlockEmpty( block ) { + const blockType = getBlockType( block.name ); + const contentAttributes = + __experimentalGetBlockAttributesNamesByRole( + block.name, + 'content' + ); + return contentAttributes.every( ( attribute ) => { + const definition = blockType.attributes[ attribute ]; + const value = block.attributes[ attribute ]; + return isAttributeUnmodified( definition, value ); + } ); + } + function switchToDefaultOrRemove() { const block = getBlock( clientId ); const defaultBlockName = getDefaultBlockName(); @@ -350,12 +374,44 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { removeBlock( _clientId ); } else { registry.batch( () => { - if ( + const firstBlock = getBlock( firstClientId ); + const isFirstBlockEmpty = isBlockEmpty( firstBlock ); + const defaultBlockName = getDefaultBlockName(); + const replacement = switchToBlockType( + firstBlock, + defaultBlockName + ); + const canTransformToDefaultBlock = + !! replacement?.length && + replacement.every( ( block ) => + canInsertBlockType( block.name, _clientId ) + ); + + if ( isFirstBlockEmpty && canTransformToDefaultBlock ) { + // Step 1: If the block is empty and can be transformed to the default block type. + replaceBlocks( + firstClientId, + replacement, + changeSelection + ); + } else if ( + isFirstBlockEmpty && + 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( - getBlockName( firstClientId ), + firstBlock.name, targetRootClientId ) ) { + // Step 3: If the block can be moved up. moveBlocksToPosition( [ firstClientId ], _clientId, @@ -363,21 +419,17 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { getBlockIndex( _clientId ) ); } else { - const replacement = switchToBlockType( - getBlock( firstClientId ), - getDefaultBlockName() - ); - - if ( - replacement && - replacement.length && + const canLiftAndTransformToDefaultBlock = + !! 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 ), @@ -386,6 +438,7 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { ); removeBlock( firstClientId, false ); } else { + // Step 5: Continue the default behavior. switchToDefaultOrRemove(); } } diff --git a/packages/blocks/src/api/index.js b/packages/blocks/src/api/index.js index e7ab69af71103a..6e9e2d7becc751 100644 --- a/packages/blocks/src/api/index.js +++ b/packages/blocks/src/api/index.js @@ -8,6 +8,7 @@ import { getBlockBindingsSource, getBlockBindingsSources, } from './registration'; +import { isAttributeUnmodified } 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` @@ -183,4 +184,5 @@ lock( privateApis, { unregisterBlockBindingsSource, getBlockBindingsSource, getBlockBindingsSources, + isAttributeUnmodified, } ); diff --git a/packages/blocks/src/api/utils.js b/packages/blocks/src/api/utils.js index 20f0f6a85ed091..8f7092ece2f033 100644 --- a/packages/blocks/src/api/utils.js +++ b/packages/blocks/src/api/utils.js @@ -30,6 +30,30 @@ 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. + */ +export function isAttributeUnmodified( 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. @@ -43,20 +67,7 @@ export function isUnmodifiedBlock( block ) { ( [ key, definition ] ) => { const value = block.attributes[ key ]; - // 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; + return isAttributeUnmodified( definition, value ); } ); } diff --git a/test/e2e/specs/editor/various/splitting-merging.spec.js b/test/e2e/specs/editor/various/splitting-merging.spec.js index 29e7e5d64522c9..81b83288fbb3be 100644 --- a/test/e2e/specs/editor/various/splitting-merging.spec.js +++ b/test/e2e/specs/editor/various/splitting-merging.spec.js @@ -373,6 +373,103 @@ test.describe( 'splitting and merging blocks (@firefox, @webkit)', () => { ); } ); + // Fix for https://github.com/WordPress/gutenberg/issues/65174. + test( 'should handle unwrapping and merging blocks', async ( { + editor, + page, + } ) => { + const emptyAlignedParagraph = { + name: 'core/paragraph', + attributes: { content: '', align: 'center', dropCap: false }, + innerBlocks: [], + }; + const emptyAlignedHeading = { + name: 'core/heading', + attributes: { content: '', textAlign: 'center', level: 2 }, + innerBlocks: [], + }; + const headingWithContent = { + name: 'core/heading', + attributes: { content: 'heading', level: 2 }, + innerBlocks: [], + }; + const placeholderBlock = { name: 'core/separator' }; + await editor.insertBlock( { + name: 'core/group', + innerBlocks: [ + emptyAlignedParagraph, + emptyAlignedHeading, + headingWithContent, + placeholderBlock, + ], + } ); + await editor.canvas + .getByRole( 'document', { name: 'Empty block' } ) + .focus(); + + await page.keyboard.press( 'Backspace' ); + await expect + .poll( editor.getBlocks, 'Remove the default empty block' ) + .toEqual( [ + { + name: 'core/group', + attributes: { tagName: 'div' }, + innerBlocks: [ + emptyAlignedHeading, + headingWithContent, + expect.objectContaining( placeholderBlock ), + ], + }, + ] ); + + // Move the caret to the beginning of the empty heading block. + await page.keyboard.press( 'ArrowDown' ); + await page.keyboard.press( 'Backspace' ); + await expect + .poll( + editor.getBlocks, + 'Convert the non-default empty block to a default block' + ) + .toEqual( [ + { + name: 'core/group', + attributes: { tagName: 'div' }, + innerBlocks: [ + emptyAlignedParagraph, + headingWithContent, + expect.objectContaining( placeholderBlock ), + ], + }, + ] ); + await page.keyboard.press( 'Backspace' ); + await expect.poll( editor.getBlocks ).toEqual( [ + { + name: 'core/group', + attributes: { tagName: 'div' }, + innerBlocks: [ + headingWithContent, + expect.objectContaining( placeholderBlock ), + ], + }, + ] ); + + // Move the caret to the beginning of the "heading" heading block. + await page.keyboard.press( 'ArrowDown' ); + 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: [ + expect.objectContaining( placeholderBlock ), + ], + }, + ] ); + } ); + test.describe( 'test restore selection when merge produces more than one block', () => { const snap1 = [ { From 47b223b56cdc03f9a5b61ff6b4d38752797517a7 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Thu, 26 Sep 2024 15:14:16 +0800 Subject: [PATCH 2/3] Update to the stabilized API --- .../src/components/block-list/block.js | 33 +++++-------------- packages/blocks/src/api/index.js | 4 +-- packages/blocks/src/api/utils.js | 31 ++++++++++++++++- .../editor/various/splitting-merging.spec.js | 2 +- 4 files changed, 41 insertions(+), 29 deletions(-) diff --git a/packages/block-editor/src/components/block-list/block.js b/packages/block-editor/src/components/block-list/block.js index 7b987a3608e034..17eb272371f675 100644 --- a/packages/block-editor/src/components/block-list/block.js +++ b/packages/block-editor/src/components/block-list/block.js @@ -24,7 +24,6 @@ import { isReusableBlock, getBlockDefaultClassName, hasBlockSupport, - __experimentalGetBlockAttributesNamesByRole, store as blocksStore, privateApis as blocksPrivateApis, } from '@wordpress/blocks'; @@ -48,7 +47,7 @@ import { PrivateBlockContext } from './private-block-context'; import { unlock } from '../../lock-unlock'; -const { isAttributeUnmodified } = unlock( blocksPrivateApis ); +const { isBlockContentUnmodified } = unlock( blocksPrivateApis ); /** * Merges wrapper props with special handling for classNames and styles. @@ -311,26 +310,6 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { canInsertBlockType, } = registry.select( blockEditorStore ); - /** - * A block is "empty" if all of its content attributes are unmodified. - * - * @param {WPBlock} block The block to check. - * @return {boolean} Whether the block is empty. - */ - function isBlockEmpty( block ) { - const blockType = getBlockType( block.name ); - const contentAttributes = - __experimentalGetBlockAttributesNamesByRole( - block.name, - 'content' - ); - return contentAttributes.every( ( attribute ) => { - const definition = blockType.attributes[ attribute ]; - const value = block.attributes[ attribute ]; - return isAttributeUnmodified( definition, value ); - } ); - } - function switchToDefaultOrRemove() { const block = getBlock( clientId ); const defaultBlockName = getDefaultBlockName(); @@ -375,7 +354,8 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { } else { registry.batch( () => { const firstBlock = getBlock( firstClientId ); - const isFirstBlockEmpty = isBlockEmpty( firstBlock ); + const isFirstBlockContentUnmodified = + isBlockContentUnmodified( firstBlock ); const defaultBlockName = getDefaultBlockName(); const replacement = switchToBlockType( firstBlock, @@ -387,7 +367,10 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { canInsertBlockType( block.name, _clientId ) ); - if ( isFirstBlockEmpty && canTransformToDefaultBlock ) { + if ( + isFirstBlockContentUnmodified && + canTransformToDefaultBlock + ) { // Step 1: If the block is empty and can be transformed to the default block type. replaceBlocks( firstClientId, @@ -395,7 +378,7 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { changeSelection ); } else if ( - isFirstBlockEmpty && + isFirstBlockContentUnmodified && firstBlock.name === defaultBlockName ) { // Step 2: If the block is empty and is already the default block type. diff --git a/packages/blocks/src/api/index.js b/packages/blocks/src/api/index.js index 6e9e2d7becc751..72f7ca18704d89 100644 --- a/packages/blocks/src/api/index.js +++ b/packages/blocks/src/api/index.js @@ -8,7 +8,7 @@ import { getBlockBindingsSource, getBlockBindingsSources, } from './registration'; -import { isAttributeUnmodified } from './utils'; +import { isBlockContentUnmodified } 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` @@ -184,5 +184,5 @@ lock( privateApis, { unregisterBlockBindingsSource, getBlockBindingsSource, getBlockBindingsSources, - isAttributeUnmodified, + isBlockContentUnmodified, } ); diff --git a/packages/blocks/src/api/utils.js b/packages/blocks/src/api/utils.js index 8f7092ece2f033..25458fde05225c 100644 --- a/packages/blocks/src/api/utils.js +++ b/packages/blocks/src/api/utils.js @@ -37,7 +37,7 @@ const ICON_COLORS = [ '#191e23', '#f8f9f9' ]; * @param {*} value The attribute's value. * @return {boolean} Whether the attribute is unmodified. */ -export function isAttributeUnmodified( attributeDefinition, value ) { +function isAttributeUnmodified( attributeDefinition, value ) { // Every attribute that has a default must match the default. if ( attributeDefinition.hasOwnProperty( 'default' ) ) { return value === attributeDefinition.default; @@ -84,6 +84,35 @@ 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 isBlockContentUnmodified( 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 isAttributeUnmodified( 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 81b83288fbb3be..eba9f1d3163fd5 100644 --- a/test/e2e/specs/editor/various/splitting-merging.spec.js +++ b/test/e2e/specs/editor/various/splitting-merging.spec.js @@ -374,7 +374,7 @@ test.describe( 'splitting and merging blocks (@firefox, @webkit)', () => { } ); // Fix for https://github.com/WordPress/gutenberg/issues/65174. - test( 'should handle unwrapping and merging blocks', async ( { + test( 'should handle unwrapping and merging blocks with empty contents', async ( { editor, page, } ) => { From 74f10dc0fc4b374c64984e2399d6897ea123f288 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Fri, 27 Sep 2024 10:22:15 +0800 Subject: [PATCH 3/3] Rename the utils --- packages/block-editor/src/components/block-list/block.js | 4 ++-- packages/blocks/src/api/index.js | 4 ++-- packages/blocks/src/api/utils.js | 8 ++++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/block-editor/src/components/block-list/block.js b/packages/block-editor/src/components/block-list/block.js index 17eb272371f675..2cecd941dfa3bb 100644 --- a/packages/block-editor/src/components/block-list/block.js +++ b/packages/block-editor/src/components/block-list/block.js @@ -47,7 +47,7 @@ import { PrivateBlockContext } from './private-block-context'; import { unlock } from '../../lock-unlock'; -const { isBlockContentUnmodified } = unlock( blocksPrivateApis ); +const { isUnmodifiedBlockContent } = unlock( blocksPrivateApis ); /** * Merges wrapper props with special handling for classNames and styles. @@ -355,7 +355,7 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => { registry.batch( () => { const firstBlock = getBlock( firstClientId ); const isFirstBlockContentUnmodified = - isBlockContentUnmodified( firstBlock ); + isUnmodifiedBlockContent( firstBlock ); const defaultBlockName = getDefaultBlockName(); const replacement = switchToBlockType( firstBlock, diff --git a/packages/blocks/src/api/index.js b/packages/blocks/src/api/index.js index 72f7ca18704d89..e23f347fe4fee8 100644 --- a/packages/blocks/src/api/index.js +++ b/packages/blocks/src/api/index.js @@ -8,7 +8,7 @@ import { getBlockBindingsSource, getBlockBindingsSources, } from './registration'; -import { isBlockContentUnmodified } from './utils'; +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` @@ -184,5 +184,5 @@ lock( privateApis, { unregisterBlockBindingsSource, getBlockBindingsSource, getBlockBindingsSources, - isBlockContentUnmodified, + isUnmodifiedBlockContent, } ); diff --git a/packages/blocks/src/api/utils.js b/packages/blocks/src/api/utils.js index 25458fde05225c..7bace4ff84c29b 100644 --- a/packages/blocks/src/api/utils.js +++ b/packages/blocks/src/api/utils.js @@ -37,7 +37,7 @@ const ICON_COLORS = [ '#191e23', '#f8f9f9' ]; * @param {*} value The attribute's value. * @return {boolean} Whether the attribute is unmodified. */ -function isAttributeUnmodified( attributeDefinition, value ) { +function isUnmodifiedAttribute( attributeDefinition, value ) { // Every attribute that has a default must match the default. if ( attributeDefinition.hasOwnProperty( 'default' ) ) { return value === attributeDefinition.default; @@ -67,7 +67,7 @@ export function isUnmodifiedBlock( block ) { ( [ key, definition ] ) => { const value = block.attributes[ key ]; - return isAttributeUnmodified( definition, value ); + return isUnmodifiedAttribute( definition, value ); } ); } @@ -95,7 +95,7 @@ export function isUnmodifiedDefaultBlock( block ) { * @param {WPBlock} block Block Object * @return {boolean} Whether the block content is unmodified. */ -export function isBlockContentUnmodified( block ) { +export function isUnmodifiedBlockContent( block ) { const contentAttributes = getBlockAttributesNamesByRole( block.name, 'content' @@ -109,7 +109,7 @@ export function isBlockContentUnmodified( block ) { const definition = getBlockType( block.name )?.attributes[ key ]; const value = block.attributes[ key ]; - return isAttributeUnmodified( definition, value ); + return isUnmodifiedAttribute( definition, value ); } ); }