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.

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

Allow template part editing in write mode #67372

Merged
merged 16 commits into from
Dec 24, 2024
+482 −230
Merged

Allow template part editing in write mode #67372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3616308
Allow template part editing in write mode
talldan Nov 28, 2024
e8c33f1
Update unit tests for template parts
talldan Nov 28, 2024
05542c6
Refine block editing mode calculation - allow editing of inner templa…
talldan Nov 27, 2024
0ebb6ea
Update tests for content blocks in template parts
talldan Nov 28, 2024
700f059
Add content role to site logo block
talldan Nov 28, 2024
508bd39
Consider template parts a section in navigation mode
talldan Dec 4, 2024
5e68eea
Allow content blocks to be explicitly disabled even when they have a …
talldan Dec 6, 2024
f609354
Avoid showing template parts when edting a page in Write Mode
talldan Dec 10, 2024
d6243e4
entries().some is not widely available, so switch to Array.from().some
talldan Dec 10, 2024
adde4d2
Update test to show explicitly disabled block
talldan Dec 10, 2024
0d4cf1d
Update unit tests
talldan Dec 11, 2024
673f4b0
Remove unused argument
talldan Dec 11, 2024
80d9048
Add optimization for disabled blocks calculation
talldan Dec 11, 2024
98f98cb
Avoid state updates when setting block editor modes to the same thing…
talldan Dec 11, 2024
e008553
Update DisableNonPageContentBlocks to not set block editor modes on a…
talldan Dec 11, 2024
0051caf
Try breaking DisableNonPageContentBlocks into separate useEffect calls
talldan Dec 11, 2024
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
20 changes: 15 additions & 5 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 @@ -502,13 +502,23 @@ export const getParentSectionBlock = ( state, clientId ) => {
* @return {boolean} Whether the block is a content locking parent.
*/
export function isSectionBlock( state, clientId ) {
const blockName = getBlockName( state, clientId );
if (
blockName === 'core/block' ||
getTemplateLock( state, clientId ) === 'contentOnly'
) {
return true;
}

// Template parts become sections in navigation mode.
const _isNavigationMode = isNavigationMode( state );
if ( _isNavigationMode && blockName === 'core/template-part' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do template parts and pattern have different behaviors? Should they be "sections" always?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'd love to align this more. It mostly comes down to the way synced patterns and blocks with contentOnly template lock are locked from being edited all the time.

Template Parts are still fully editable when editing a template, so they only act as these contentOnly sections in write mode.

This could be aligned more, but I think there's also a UX part to this.

Copy link
Member

Choose a reason for hiding this comment

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

Speaking only about template parts, the behaviour in this PR seems intuitive to me in that it makes template parts consistent with current "sections" on trunk: content blocks are editable in write mode but nothing else. On trunk the entire template part is inert.

As for synced patterns, which behaviour should they share?

I'm still catching up, so is it proposed that the content blocks of synced patterns in write mode should be editable?

I see how the two are related, but I see synced patterns as more discrete objects that require deliberate editing.

When editing a template, template parts at least in my brain "belong" to the template and so it's logical that their content parts should be editable.

Of course, I could not be making sense at all. 😇

return true;
}

const sectionRootClientId = getSectionRootClientId( state );
const sectionClientIds = getBlockOrder( state, sectionRootClientId );
return (
getBlockName( state, clientId ) === 'core/block' ||
getTemplateLock( state, clientId ) === 'contentOnly' ||
( isNavigationMode( state ) && sectionClientIds.includes( clientId ) )
);
return _isNavigationMode && sectionClientIds.includes( clientId );
}

/**
Expand Down
131 changes: 112 additions & 19 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 @@ -1964,8 +1964,14 @@ export function temporarilyEditingFocusModeRevert( state = '', action ) {
export function blockEditingModes( state = new Map(), action ) {
switch ( action.type ) {
case 'SET_BLOCK_EDITING_MODE':
if ( state.get( action.clientId ) === action.mode ) {
return state;
}
return new Map( state ).set( action.clientId, action.mode );
case 'UNSET_BLOCK_EDITING_MODE': {
if ( ! state.has( action.clientId ) ) {
return state;
}
const newState = new Map( state );
newState.delete( action.clientId );
return newState;
Expand Down Expand Up @@ -2186,19 +2192,19 @@ function getBlockTreeBlock( state, clientId ) {
* The callback receives the current block as its argument.
*/
function traverseBlockTree( state, clientId, callback ) {
const parentTree = getBlockTreeBlock( state, clientId );
if ( ! parentTree ) {
const tree = getBlockTreeBlock( state, clientId );
if ( ! tree ) {
return;
}

callback( parentTree );
callback( tree );

if ( ! parentTree?.innerBlocks?.length ) {
if ( ! tree?.innerBlocks?.length ) {
return;
}

for ( const block of parentTree?.innerBlocks ) {
traverseBlockTree( state, block.clientId, callback );
for ( const innerBlock of tree?.innerBlocks ) {
traverseBlockTree( state, innerBlock.clientId, callback );
}
}

Expand All @@ -2212,8 +2218,12 @@ function traverseBlockTree( state, clientId, callback ) {
* @return {string|undefined} The client ID of the parent block if found, undefined otherwise.
*/
function findParentInClientIdsList( state, clientId, clientIds ) {
if ( ! clientIds.length ) {
return;
}

let parent = state.blocks.parents.get( clientId );
while ( parent ) {
while ( parent !== undefined ) {
if ( clientIds.includes( parent ) ) {
return parent;
}
Expand Down Expand Up @@ -2258,15 +2268,65 @@ function getDerivedBlockEditingModesForTree(
// so the default block editing mode is set to disabled.
const sectionRootClientId = state.settings?.[ sectionRootClientIdKey ];
const sectionClientIds = state.blocks.order.get( sectionRootClientId );
const syncedPatternClientIds = Object.keys(
state.blocks.controlledInnerBlocks
).filter(
( clientId ) =>
state.blocks.byClientId?.get( clientId )?.name === 'core/block'
const hasDisabledBlocks = Array.from( state.blockEditingModes ).some(
( [ , mode ] ) => mode === 'disabled'
);
const templatePartClientIds = [];
const syncedPatternClientIds = [];

Object.keys( state.blocks.controlledInnerBlocks ).forEach( ( clientId ) => {
const block = state.blocks.byClientId?.get( clientId );

if ( block?.name === 'core/template-part' ) {
templatePartClientIds.push( clientId );
}

if ( block?.name === 'core/block' ) {
syncedPatternClientIds.push( clientId );
}
} );

traverseBlockTree( state, treeClientId, ( block ) => {
const { clientId, name: blockName } = block;

// If the block already has an explicit block editing mode set,
// don't override it.
if ( state.blockEditingModes.has( clientId ) ) {
return;
}
Comment on lines +2292 to +2296
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this change might need some discussion. Previously, if a block had a blockEditingMode explicitly set to something (say 'disabled' or 'contentOnly'), but you then switched to zoom out, write mode, or if the block is in a synced pattern, then those experiences would be able to override the blockEditingMode (they would set modes within the derivedBlockEditingModes state and that had precedence over blockEditingModes).

This change makes it the other way around, the explicit blockEditingModes now has precedence.

It's perhaps questionable because it allows plugins to change how those features work by calling setBlockEditingMode.

On the other hand, it's what allows Page Editing to work correctly in Write Mode, the editor package calls setBlockEditingMode in its DisableNonPageContentBlocks component to control certain aspects of the experience. The editor is really an extensibility layer over the block editor package, I haven't been able to think of a way to give it special privileges over the block editing modes without also opening up the access to plugins (other than introducing another private API).

Copy link
Contributor

Choose a reason for hiding this comment

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

This change makes it the other way around, the explicit blockEditingModes now has precedence.

I think this makes more sense, that said, I think we should work to remove/deprecate the explicit blockEditingModes entirely (not specifically here) because that is just creating confusion and moving us away from decorativeness.


// Disabled explicit block editing modes are inherited by children.
// It's an expensive calculation, so only do it if there are disabled blocks.
if ( hasDisabledBlocks ) {
// Look through parents to find one with an explicit block editing mode.
let ancestorBlockEditingMode;
let parent = state.blocks.parents.get( clientId );
while ( parent !== undefined ) {
// There's a chance we only just calculated this for the parent,
// if so we can return that value for a faster lookup.
if ( derivedBlockEditingModes.has( parent ) ) {
ancestorBlockEditingMode =
derivedBlockEditingModes.get( parent );
} else if ( state.blockEditingModes.has( parent ) ) {
// Checking the explicit block editing mode will be slower,
// as the block editing mode is more likely to be set on a
// distant ancestor.
ancestorBlockEditingMode =
state.blockEditingModes.get( parent );
}
if ( ancestorBlockEditingMode ) {
break;
}
parent = state.blocks.parents.get( parent );
}

// If the ancestor block editing mode is disabled, it's inherited by the child.
if ( ancestorBlockEditingMode === 'disabled' ) {
derivedBlockEditingModes.set( clientId, 'disabled' );
return;
}
}

if ( isZoomedOut || isNavMode ) {
// If the root block is the section root set its editing mode to contentOnly.
if ( clientId === sectionRootClientId ) {
Expand All @@ -2287,15 +2347,41 @@ function getDerivedBlockEditingModesForTree(

// If zoomed out, all blocks that aren't sections or the section root are
// disabled.
// If the tree root is not in a section, set its editing mode to disabled.
if (
isZoomedOut ||
! findParentInClientIdsList( state, clientId, sectionClientIds )
) {
if ( isZoomedOut ) {
derivedBlockEditingModes.set( clientId, 'disabled' );
return;
}

const isInSection = !! findParentInClientIdsList(
state,
clientId,
sectionClientIds
);
if ( ! isInSection ) {
if ( clientId === '' ) {
derivedBlockEditingModes.set( clientId, 'disabled' );
return;
}

// Allow selection of template parts outside of sections.
if ( blockName === 'core/template-part' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious - are we "allowed" to reference core blocks like this in the block editor package?

If not how can we pass in references to these blocks and express the custom logic here that relates to "template parts"?

I was thinking of a private block editor setting, but then I'm not sure how we'd handle the custom logic that seems to relate specifically to template parts.

I'm also thinking about how we could extend this to the Navigation block in the future.

Copy link
Contributor Author

@talldan talldan Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think not ideal to have loads of this, but it's ok for testing new concepts. I don't think it's harmful to any non-WP consumers of the block editor package as these bits of code will have no effect if they don't have these block types. It's just a bit of a code quality issue to have lots of hard to follow conditions, and it's better in the long run to try unifying some concepts behind an API or similar, but sometimes the right one takes a while to emerge.

Hopefully someday in the future template parts will be deprecated and there will only be synced patterns, and they'll work the same way.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do have a fair bit of this already in the block editor package 😅

Could we use a similar logic to get/set groupingBlockName for the template part block? Is it likely that third parties will provide other types of template part-like blocks that we might want this code to work with?

Copy link
Contributor

Choose a reason for hiding this comment

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

core/block is a special block (We could even relocate it within block-editor). Template part should be just core/block ultimately. I think we should try to avoid any other block type (maybe other than these two)

derivedBlockEditingModes.set( clientId, 'contentOnly' );
return;
}

const isInTemplatePart = !! findParentInClientIdsList(
state,
clientId,
templatePartClientIds
);
// Allow contentOnly blocks in template parts outside of sections
// to be editable. Only disable blocks that don't fit this criteria.
if ( ! isInTemplatePart && ! isContentBlock( blockName ) ) {
derivedBlockEditingModes.set( clientId, 'disabled' );
return;
}
}

// Handle synced pattern content so the inner blocks of a synced pattern are
// properly disabled.
if ( syncedPatternClientIds.length ) {
Expand Down Expand Up @@ -2560,11 +2646,16 @@ export function withDerivedBlockEditingModes( reducer ) {
}
break;
}
case 'SET_BLOCK_EDITING_MODE':
case 'UNSET_BLOCK_EDITING_MODE':
case 'SET_HAS_CONTROLLED_INNER_BLOCKS': {
const updatedBlock = nextState.blocks.tree.get(
const updatedBlock = getBlockTreeBlock(
nextState,
Comment on lines +2652 to +2653
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change here is because SET_BLOCK_EDITING_MODE is often called on the root block (with a client id of ''). When you do nextState.blocks.tree.get( '' ), it returns an object that doesn't have a clientId property and that causes some of the later code to fail. getBlockTreeBlock works around that by patching the returned object to have a clientId.

It might be better to make sure the root in the tree does have a clientId property. 😄

action.clientId
);
// The block might have been removed.

// The block might have been removed in which case it'll be
// handled by the `REMOVE_BLOCKS` action.
if ( ! updatedBlock ) {
break;
}
Expand All @@ -2573,13 +2664,15 @@ export function withDerivedBlockEditingModes( reducer ) {
getDerivedBlockEditingModesUpdates( {
prevState: state,
nextState,
removedClientIds: [ action.clientId ],
addedBlocks: [ updatedBlock ],
isNavMode: false,
} );
const nextDerivedNavModeBlockEditingModes =
getDerivedBlockEditingModesUpdates( {
prevState: state,
nextState,
removedClientIds: [ action.clientId ],
addedBlocks: [ updatedBlock ],
isNavMode: true,
} );
Expand Down
4 changes: 1 addition & 3 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 @@ -3087,9 +3087,7 @@ export const getBlockEditingMode = createRegistrySelector(
const isContent = hasContentRoleAttribute( name );
return isContent ? 'contentOnly' : 'disabled';
}
// Otherwise, check if there's an ancestor that is contentOnly
const parentMode = getBlockEditingMode( state, rootClientId );
return parentMode === 'contentOnly' ? 'default' : parentMode;
return 'default';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally found this code really confusingly written (and the comment made me more confused). It'd be easier to understand if it were written like this:

const parentMode = getBlockEditingMode( state, rootClientId );
// Child blocks inherit a 'disabled' state from their parents.
if ( parentMode === 'disabled' ) {
    return 'disabled';
}

// Other modes are not inherited, return the 'default' value.
return 'default';

That would make it clear that the part I've refactored is the if ( parentMode === 'disabled' ) block. It has now moved to the withDerivedBlockEditingModes reducer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having a hard time connecting your comment with the existing code and why the change? (Agreed it's confusing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The handling of the inherited disabled states has been moved to withDerivedBlockEditingModes, this is the new code that replaces the recursive call in the selector:

gutenberg/packages/block-editor/src/store/reducer.js

Lines 2300 to 2328 in 0cef3d4

if ( hasDisabledBlocks ) {
// Look through parents to find one with an explicit block editing mode.
let ancestorBlockEditingMode;
let parent = state.blocks.parents.get( clientId );
while ( parent !== undefined ) {
// There's a chance we only just calculated this for the parent,
// if so we can return that value for a faster lookup.
if ( derivedBlockEditingModes.has( parent ) ) {
ancestorBlockEditingMode =
derivedBlockEditingModes.get( parent );
} else if ( state.blockEditingModes.has( parent ) ) {
// Checking the explicit block editing mode will be slower,
// as the block editing mode is more likely to be set on a
// distant ancestor.
ancestorBlockEditingMode =
state.blockEditingModes.get( parent );
}
if ( ancestorBlockEditingMode ) {
break;
}
parent = state.blocks.parents.get( parent );
}
// If the ancestor block editing mode is disabled, it's inherited by the child.
if ( ancestorBlockEditingMode === 'disabled' ) {
derivedBlockEditingModes.set( clientId, 'disabled' );
return;
}
}

As for the why - it's related to what I mentioned comment about letting the explicit block editing modes override the derived ones. It would've been difficult to do that for the inherited disabled states because the code for determining that was in the selector.

}
);

Expand Down
Loading
Loading