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.

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 unable to remove empty blocks on merge #65262

Merged
merged 3 commits into from
Sep 27, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
58 changes: 47 additions & 11 deletions packages/block-editor/src/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
getBlockDefaultClassName,
hasBlockSupport,
store as blocksStore,
privateApis as blocksPrivateApis,
} from '@wordpress/blocks';
import { withFilters } from '@wordpress/components';
import { withDispatch, useDispatch, useSelect } from '@wordpress/data';
Expand All @@ -46,6 +47,8 @@ import { PrivateBlockContext } from './private-block-context';

import { unlock } from '../../lock-unlock';

const { isBlockContentUnmodified } = unlock( blocksPrivateApis );

/**
* Merges wrapper props with special handling for classNames and styles.
*
Expand Down Expand Up @@ -350,34 +353,66 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => {
removeBlock( _clientId );
} else {
registry.batch( () => {
const firstBlock = getBlock( firstClientId );
const isFirstBlockContentUnmodified =
isBlockContentUnmodified( 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(
getBlockName( firstClientId ),
firstBlock.name,
targetRootClientId
)
) {
// Step 3: If the block can be moved up.
moveBlocksToPosition(
[ firstClientId ],
_clientId,
targetRootClientId,
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 ),
Expand All @@ -386,6 +421,7 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => {
);
removeBlock( firstClientId, false );
} else {
// Step 5: Continue the default behavior.
switchToDefaultOrRemove();
}
}
Expand Down
2 changes: 2 additions & 0 deletions packages/blocks/src/api/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
getBlockBindingsSource,
getBlockBindingsSources,
} from './registration';
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`
Expand Down Expand Up @@ -183,4 +184,5 @@ lock( privateApis, {
unregisterBlockBindingsSource,
getBlockBindingsSource,
getBlockBindingsSources,
isBlockContentUnmodified,
} );
68 changes: 54 additions & 14 deletions packages/blocks/src/api/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
function isAttributeUnmodified( attributeDefinition, value ) {
kevin940726 marked this conversation as resolved.
Show resolved Hide resolved
// 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.
Expand All @@ -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 );
}
);
}
Expand All @@ -73,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 ) {
kevin940726 marked this conversation as resolved.
Show resolved Hide resolved
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.
*
Expand Down
97 changes: 97 additions & 0 deletions test/e2e/specs/editor/various/splitting-merging.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 with empty contents', 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 = [
{
Expand Down
Loading