Revert: Fix unable to remove empty blocks on merge (WordPress#65262) …

…+ alternative (WordPress#66564) Co-authored-by: ellatrix <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: kspilarski <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: ndiego <[email protected]> Co-authored-by: kevin940726 <[email protected]> Co-authored-by: richtabor <[email protected]>
karthick-murugan · Nov 11, 2024 · 249c0cf · 249c0cf
1 parent c9e4eab
commit 249c0cf
Show file tree

Hide file tree

Showing 5 changed files with 56 additions and 120 deletions.
diff --git 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,68 +362,36 @@ 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,
 								targetRootClientId,
 								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
@@ -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
@@ -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
@@ -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
@@ -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 ),
 					],
 				},