Skip to content

Commit

Permalink
Revert: Fix unable to remove empty blocks on merge (#65262) + alterna…
Browse files Browse the repository at this point in the history 
…tive (#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]>
  • Loading branch information
@ellatrix @ntsekouras
@talldan @ndiego @kevin940726 @richtabor
8 people authored Nov 11, 2024
1 parent c9e4eab commit 249c0cf
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 120 deletions.
76 changes: 28 additions & 48 deletions packages/block-editor/src/components/block-list/block.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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.
*
Expand Down Expand Up @@ -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,
Expand All @@ -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 ) ]
);
}
}

Expand All @@ -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;
Expand All @@ -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 ),
Expand All @@ -421,7 +400,6 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => {
);
removeBlock( firstClientId, false );
} else {
// Step 5: Continue the default behavior.
switchToDefaultOrRemove();
}
}
Expand All @@ -433,6 +411,8 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => {
removeBlock( _clientId, false );
}
} );
} else {
switchToDefaultOrRemove();
}
}

Expand Down
4 changes: 0 additions & 4 deletions packages/blocks/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
9 changes: 0 additions & 9 deletions packages/blocks/src/api/index.js
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -175,6 +169,3 @@ export {
__EXPERIMENTAL_ELEMENTS,
__EXPERIMENTAL_PATHS_WITH_OVERRIDE,
} from './constants';

export const privateApis = {};
lock( privateApis, { isUnmodifiedBlockContent } );
68 changes: 14 additions & 54 deletions packages/blocks/src/api/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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;
}
);
}
Expand All @@ -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.
*
Expand Down
19 changes: 14 additions & 5 deletions test/e2e/specs/editor/various/splitting-merging.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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' )
Expand All @@ -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(
Expand All @@ -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( [
{
Expand All @@ -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 ),
],
},
Expand Down

1 comment on commit 249c0cf

@github-actions
Copy link

Choose a reason for hiding this comment

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

Flaky tests detected in 249c0cf.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11777971896
📝 Reported issues:

Please sign in to comment.