Skip to content

Refactor: Content only template locking block editing modes to reducer #67606

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.

Sign up for GitHub

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

Merged
talldan merged 4 commits into from
Sep 9, 2025
Merged

Refactor: Content only template locking block editing modes to reducer #67606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cb908e4
Update derived block editing modes to support content only template l…
talldan Dec 4, 2024
e32f963
Remove defunct code
talldan Sep 8, 2025
45aa6ee
Fix `getEnabledBlockParents` test
talldan Sep 8, 2025
1d93ad3
Fix test for explicitly set blockEditingMode
talldan Sep 8, 2025
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
2 changes: 0 additions & 2 deletions packages/block-editor/src/store/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,6 @@ export const getEnabledClientIdsTree = createRegistrySelector( ( select ) =>
state.derivedBlockEditingModes,
state.derivedNavModeBlockEditingModes,
state.blockEditingModes,
state.settings.templateLock,
state.blockListSettings,
select( STORE_NAME ).__unstableGetEditorMode( state ),
] )
);
Expand Down
92 changes: 92 additions & 0 deletions packages/block-editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2267,6 +2267,12 @@ function getDerivedBlockEditingModesForTree(
syncedPatternClientIds.push( clientId );
}
} );
const contentOnlyTemplateLockedClientIds = Object.keys(
state.blockListSettings
).filter(
( clientId ) =>
state.blockListSettings[ clientId ]?.templateLock === 'contentOnly'
);

traverseBlockTree( state, treeClientId, ( block ) => {
const { clientId, name: blockName } = block;
Expand Down Expand Up @@ -2457,6 +2463,23 @@ function getDerivedBlockEditingModesForTree(
derivedBlockEditingModes.set( clientId, 'disabled' );
}
}

// `templateLock: 'contentOnly'` derived modes.
if ( contentOnlyTemplateLockedClientIds.length ) {
const hasContentOnlyTemplateLockedParent =
!! findParentInClientIdsList(
state,
clientId,
contentOnlyTemplateLockedClientIds
);
if ( hasContentOnlyTemplateLockedParent ) {
if ( isContentBlock( blockName ) ) {
derivedBlockEditingModes.set( clientId, 'contentOnly' );
} else {
derivedBlockEditingModes.set( clientId, 'disabled' );
}
}
}
} );

return derivedBlockEditingModes;
Expand Down Expand Up @@ -2628,6 +2651,75 @@ export function withDerivedBlockEditingModes( reducer ) {
}
break;
}
case 'UPDATE_BLOCK_LIST_SETTINGS': {
// Handle the addition and removal of contentOnly template locked blocks.
const addedBlocks = [];
const removedClientIds = [];

const updates =
typeof action.clientId === 'string'
? { [ action.clientId ]: action.settings }
: action.clientId;

for ( const clientId in updates ) {
const isNewContentOnlyBlock =
state.blockListSettings[ clientId ]?.templateLock !==
'contentOnly' &&
nextState.blockListSettings[ clientId ]
?.templateLock === 'contentOnly';

const wasContentOnlyBlock =
state.blockListSettings[ clientId ]?.templateLock ===
'contentOnly' &&
nextState.blockListSettings[ clientId ]
?.templateLock !== 'contentOnly';

if ( isNewContentOnlyBlock ) {
addedBlocks.push(
nextState.blocks.tree.get( clientId )
);
} else if ( wasContentOnlyBlock ) {
removedClientIds.push( clientId );
}
}

if ( ! addedBlocks.length && ! removedClientIds.length ) {
break;
}

const nextDerivedBlockEditingModes =
Copy link
Member

@ramonjd ramonjd Sep 8, 2025

Choose a reason for hiding this comment

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

Do you think it's worth adding performance tests later?

I could be totally barking up the wrong tree, but let's say we have a document with 10,000 blocks, does this mean we're storing 10,000 block editing modes entries in memory?

Copy link
Member

@ramonjd ramonjd Sep 8, 2025

Choose a reason for hiding this comment

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

I did a very crude js heap comparision using window.performance.memory and I couldn't see any memory degradation comparing trunk with this PR based on a derivedNavModeBlockEditingModes count of 40. Total heap size hovered around 200-280 MB

Copy link
Contributor Author

@talldan talldan Sep 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We do already have performance tests in CI, but then I don't think they test these modes.

I could be totally barking up the wrong tree, but let's say we have a document with 10,000 blocks, does this mean we're storing 10,000 block editing modes entries in memory?

Not really, not every block gets a block editing mode. Only contentOnly or disabled blocks are given one. Most of the time blocks will just be default and then they have no mode set.

But anyway, if we have 10,000 blocks in the post, we'd be storing other state for those blocks, so why not a block editing mode? It is only a short string.

I think the issue is more about whether we update 10,000 blocks on every state change, and we don't.

Here's some of the performance related checks the reducer does:

  • Only updates subtrees of blocks - e.g. if a synced pattern is inserted only that pattern and its children have block editing modes recomputed
  • Takes care to only update on particular actions - e.g. insert blocks, remove blocks, changing templateLock and others
  • Avoids unnecessary store updates as much as possible - e.g. don't replace objects with new references if the data in those objects hasn't changed. This will prevent react renders that don't do anything.

Copy link
Member

@ramonjd ramonjd Sep 9, 2025

Choose a reason for hiding this comment

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

Thanks for the explainer! 🙇🏻

getDerivedBlockEditingModesUpdates( {
prevState: state,
nextState,
addedBlocks,
removedClientIds,
isNavMode: false,
} );
const nextDerivedNavModeBlockEditingModes =
getDerivedBlockEditingModesUpdates( {
prevState: state,
nextState,
addedBlocks,
removedClientIds,
isNavMode: true,
} );

if (
nextDerivedBlockEditingModes ||
nextDerivedNavModeBlockEditingModes
) {
return {
...nextState,
derivedBlockEditingModes:
nextDerivedBlockEditingModes ??
state.derivedBlockEditingModes,
derivedNavModeBlockEditingModes:
nextDerivedNavModeBlockEditingModes ??
state.derivedNavModeBlockEditingModes,
};
}
break;
}
case 'SET_BLOCK_EDITING_MODE':
case 'UNSET_BLOCK_EDITING_MODE':
case 'SET_HAS_CONTROLLED_INNER_BLOCKS': {
Expand Down
76 changes: 25 additions & 51 deletions packages/block-editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3076,62 +3076,36 @@ export function __unstableIsWithinBlockOverlay( state, clientId ) {
* @return {BlockEditingMode} The block editing mode. One of `'disabled'`,
* `'contentOnly'`, or `'default'`.
*/
export const getBlockEditingMode = createRegistrySelector(
( select ) =>
( state, clientId = '' ) => {
// Some selectors that call this provide `null` as the default
// rootClientId, but the default rootClientId is actually `''`.
if ( clientId === null ) {
clientId = '';
}
export function getBlockEditingMode( state, clientId = '' ) {
// Some selectors that call this provide `null` as the default
// rootClientId, but the default rootClientId is actually `''`.
if ( clientId === null ) {
clientId = '';
}

const isNavMode = isNavigationMode( state );

// If the editor is currently not in navigation mode, check if the clientId
// has an editing mode set in the regular derived map.
// There may be an editing mode set here for synced patterns or in zoomed out
// mode.
if (
! isNavMode &&
state.derivedBlockEditingModes?.has( clientId )
) {
return state.derivedBlockEditingModes.get( clientId );
}
const isNavMode = isNavigationMode( state );
Copy link
Member

@ramonjd ramonjd Sep 8, 2025

Choose a reason for hiding this comment

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

Can you refresh my memory... is navigation flow related to the former block interaction tool?

Screenshot 2025-09-08 at 4 54 38 pm

Is this for backwards compat?

Copy link
Contributor Author

@talldan talldan Sep 8, 2025

Choose a reason for hiding this comment

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

It is related and not. Write Mode has now taken that tool's place, but the API used is still the same. So navigation mode === write mode.

Copy link
Member

@ramonjd ramonjd Sep 8, 2025

Choose a reason for hiding this comment

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

My brain hurts.


// If the editor *is* in navigation mode, the block editing mode states
// are stored in the derivedNavModeBlockEditingModes map.
if (
isNavMode &&
state.derivedNavModeBlockEditingModes?.has( clientId )
) {
return state.derivedNavModeBlockEditingModes.get( clientId );
}
// If the editor is currently not in navigation mode, check if the clientId
// has an editing mode set in the regular derived map.
// There may be an editing mode set here for synced patterns or in zoomed out
// mode.
if ( ! isNavMode && state.derivedBlockEditingModes?.has( clientId ) ) {
return state.derivedBlockEditingModes.get( clientId );
}

// In normal mode, consider that an explicitly set editing mode takes over.
const blockEditingMode = state.blockEditingModes.get( clientId );
if ( blockEditingMode ) {
return blockEditingMode;
}
// If the editor *is* in navigation mode, the block editing mode states
// are stored in the derivedNavModeBlockEditingModes map.
if ( isNavMode && state.derivedNavModeBlockEditingModes?.has( clientId ) ) {
return state.derivedNavModeBlockEditingModes.get( clientId );
}
Comment on lines +3096 to +3100
Copy link
Contributor

@jeryj jeryj Aug 11, 2025

Choose a reason for hiding this comment

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

Should we only have if ( isNavMode ) here so it will return contentOnly as a fallback if state.derivedNavModeBlockEditingModes?.has( clientId ) is false? I'm concerned about edge cases where an item isn't in the derivedNavModeBlockEditingModes for some reason, and this ends up returning default.

Copy link
Contributor Author

@talldan talldan Sep 8, 2025

Choose a reason for hiding this comment

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

I guess there are no cases where default is used in Write Mode?

If so, I think what you suggest is probably a good idea, though perhaps disabled as the fallback instead of contentOnly.

Copy link
Contributor Author

@talldan talldan Sep 8, 2025

Choose a reason for hiding this comment

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

Lets handle it in a separate PR given the code for this is already in trunk.


// In normal mode, top level is default mode.
if ( clientId === '' ) {
return 'default';
}
// In normal mode, consider that an explicitly set editing mode takes over.
if ( state.blockEditingModes.has( clientId ) ) {
return state.blockEditingModes.get( clientId );
}

const rootClientId = getBlockRootClientId( state, clientId );
const templateLock = getTemplateLock( state, rootClientId );
// If the parent of the block is contentOnly locked, check whether it's a content block.
if ( templateLock === 'contentOnly' ) {
const name = getBlockName( state, clientId );
const { hasContentRoleAttribute } = unlock(
select( blocksStore )
);
const isContent = hasContentRoleAttribute( name );
return isContent ? 'contentOnly' : 'disabled';
}
return 'default';
}
Comment on lines -3121 to -3133
Copy link
Contributor

@jeryj jeryj Aug 11, 2025

Choose a reason for hiding this comment

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

I really like the refactor to move this logic to the reducer rather than spreading it out within the getter.

Copy link
Member

@ramonjd ramonjd Sep 8, 2025

Choose a reason for hiding this comment

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

Just so I understand, is the selling point that previously, every call to getBlockEditingMode would traverse the entire block tree and recalculate. Now it's computed once per state change?

Copy link
Contributor Author

@talldan talldan Sep 8, 2025

Choose a reason for hiding this comment

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

The history is that some time ago Synced Patterns didn't work properly in Write Mode.

I tried fixing that by adding extra code to the getBlockEditingMode selector, but it caused a huge performance regression (#65408 (comment)).

I had to buy some more performance in order to be able to fix that bug, and so I've done that by moving code from the selector.

I think the code is still far from good, but it's a tough problem to solve, and I think it's reflective of how complicated the features that use blockEditingModes are. We should try to simplify both the features and the code where we can.

);
return 'default';
}

/**
* Indicates if a block is ungroupable.
Expand Down
107 changes: 107 additions & 0 deletions packages/block-editor/src/store/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3575,6 +3575,7 @@ describe( 'state', () => {
blocks,
settings,
zoomLevel,
blockListSettings,
blockEditingModes,
} )
);
Expand Down Expand Up @@ -3885,6 +3886,112 @@ describe( 'state', () => {
} );
} );

describe( 'contentOnly template locking', () => {
let initialState;
beforeAll( () => {
initialState = dispatchActions(
[
{
type: 'RESET_BLOCKS',
blocks: [
{
name: 'core/group',
clientId: 'group-1',
attributes: {},
innerBlocks: [
{
name: 'core/paragraph',
clientId: 'paragraph-1',
attributes: {},
innerBlocks: [],
},
{
name: 'core/group',
clientId: 'group-2',
attributes: {},
innerBlocks: [
{
name: 'core/paragraph',
clientId: 'paragraph-2',
attributes: {},
innerBlocks: [],
},
],
},
],
},
],
},
{
type: 'UPDATE_BLOCK_LIST_SETTINGS',
clientId: 'group-1',
settings: {
templateLock: 'contentOnly',
},
},
],
testReducer,
initialState
);
} );

it( 'returns the expected block editing modes for a parent block with contentOnly template locking', () => {
// Only the parent pattern and its own children that have bindings
// are in contentOnly mode. All other blocks are disabled.
expect( initialState.derivedBlockEditingModes ).toEqual(
new Map(
Object.entries( {
'paragraph-1': 'contentOnly',
'group-2': 'disabled',
'paragraph-2': 'contentOnly',
} )
)
);
} );

it( 'removes block editing modes when template locking is removed', () => {
const { derivedBlockEditingModes } = dispatchActions(
[
{
type: 'UPDATE_BLOCK_LIST_SETTINGS',
clientId: 'group-1',
settings: {
templateLock: false,
},
},
],
testReducer,
initialState
);

expect( derivedBlockEditingModes ).toEqual( new Map() );
} );

it( 'allows explicitly set blockEditingModes to override the contentOnly template locking', () => {
const { derivedBlockEditingModes } = dispatchActions(
[
{
type: 'SET_BLOCK_EDITING_MODE',
clientId: 'paragraph-2',
mode: 'disabled',
},
],
testReducer,
initialState
);

expect( derivedBlockEditingModes ).toEqual(
new Map(
Object.entries( {
'paragraph-1': 'contentOnly',
'group-2': 'disabled',
// Paragraph 2 already has an explicit mode, so isn't set as a derived mode.
} )
)
);
} );
} );

describe( 'navigation mode', () => {
let initialState;

Expand Down
Loading
Loading