Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
10 changes: 10 additions & 0 deletions packages/block-editor/src/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -1260,6 +1260,16 @@ export const mergeBlocks =
return;
}

// If blockA has no content (e.g. an empty heading), just remove it
// and keep blockB focused. Without this check, merging would transform
// blockB into blockA's type (e.g. paragraph → heading), which is
// unexpected when the leading block is simply empty.
if ( blockAType.merge && isUnmodifiedBlock( blockA, 'content' ) ) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We probably need to check if blockA.name !== blockB.name and also it would be better to have the code below the if ( ! blockAType.merge ) { block for two reasons:

  1. We remove the blockAType.merge check
  2. Preserve the if ( isUnmodifiedDefaultBlock( blockB ) ) { block flow. Placement here changes the current behavior a bit right now and we can avoid it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ntsekouras! That actually is a better way. I made the change and tested the behaviour and It works flawlessly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Failing list tests seem that something different (or more) is needed. --cc @ellatrix for any quick insights due to the heavy work on this area.

dispatch.removeBlock( clientIdA, false );
dispatch.selectBlock( clientIdB );
return;
}

if ( isUnmodifiedDefaultBlock( blockB ) ) {
dispatch.removeBlock(
clientIdB,
Expand Down
59 changes: 59 additions & 0 deletions packages/block-editor/src/store/test/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,65 @@ describe( 'actions', () => {
} );
} );

it( 'should remove blockA and focus blockB if blockA has a merge function but empty content', () => {
registerBlockType( 'core/test-block', {
apiVersion: 3,
attributes: {
content: {
type: 'rich-text',
role: 'content',
},
},
merge( attributes, attributesToMerge ) {
return {
content:
( attributes.content || '' ) +
( attributesToMerge.content || '' ),
};
},
save: noop,
category: 'text',
title: 'test block',
} );
// blockA is empty (no content attribute set).
const blockA = deepFreeze( {
clientId: 'chicken',
name: 'core/test-block',
attributes: {},
innerBlocks: [],
} );
const blockB = deepFreeze( {
clientId: 'ribs',
name: 'core/test-block',
attributes: { content: 'ribs' },
innerBlocks: [],
} );

const select = {
getBlock: ( clientId ) =>
[ blockA, blockB ].find( ( b ) => b.clientId === clientId ),
getBlockEditingMode: () => 'default',
isBlockSelected: () => true,
};
const dispatch = Object.assign( jest.fn(), {
selectBlock: jest.fn(),
removeBlock: jest.fn(),
} );

mergeBlocks(
blockA.clientId,
blockB.clientId
)( { select, dispatch } );

// Should remove the empty blockA without selecting previous.
expect( dispatch.removeBlock ).toHaveBeenCalledWith(
'chicken',
false
);
// Should focus blockB so it remains unchanged.
expect( dispatch.selectBlock ).toHaveBeenCalledWith( 'ribs' );
} );

it( 'should only focus the blockA if the blockA has no merge function and the content of blockB is modified', () => {
registerBlockType( 'core/test-block', {
...defaultBlockSettings,
Expand Down
Loading