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

Jump to bottom

Fix Synced Patterns non-overridden content being editable in write mode #66949

Closed
wants to merge 1 commit into from
Closed

Fix Synced Patterns non-overridden content being editable in write mode #66949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
168 changes: 167 additions & 1 deletion packages/block-editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2131,6 +2131,170 @@ const combinedReducers = combineReducers( {
zoomLevel,
} );

function recurseBlocks( _blocks, callback ) {
if ( ! _blocks?.length ) {
return;
}

for ( const block of _blocks ) {
callback( block );
recurseBlocks( block?.innerBlocks, callback );
}
}

function hasParentWithName( state, clientId, name ) {
let parent = state.blocks.parents.get( clientId );
while ( parent ) {
if ( state.blocks.byClientId.get( parent ).name === name ) {
return true;
}
parent = state.blocks.parents.get( parent );
}
return false;
}

function getPatternBlockEditingModes( state ) {
if ( ! state.patternClientIds?.size ) {
return new Map();
}

const patternBlockEditingModes = new Map();
for ( const clientId of state.patternClientIds ) {
// The pattern block is a controlled block, so the inner blocks are stored
// under a special key in the tree.
// Fallback to the normal key if the special key doesn't exist, as it will
// still allow adding the editing mode of the pattern block itself.
const patternTree =
state.blocks.tree.get( `controlled||${ clientId }` ) ??
state.blocks.tree.get( clientId );

if ( hasParentWithName( state, clientId, 'core/block' ) ) {
// This is a nested pattern block, it should be set to disabled,
// along with all its child blocks.
patternBlockEditingModes.set( clientId, 'disabled' );
recurseBlocks( patternTree?.innerBlocks, ( block ) => {
patternBlockEditingModes.set( block.clientId, 'disabled' );
} );
} else {
// Set the parent pattern block to contentOnly.
patternBlockEditingModes.set( clientId, 'contentOnly' );
recurseBlocks( patternTree?.innerBlocks, ( block ) => {
// If an inner block has bindings, it should be set to contentOnly.
// Else it should be set to disabled.
if (
block?.attributes?.metadata?.bindings &&
Object.keys( block?.attributes?.metadata?.bindings ).length
) {
patternBlockEditingModes.set(
block.clientId,
'contentOnly'
);
} else {
patternBlockEditingModes.set( block.clientId, 'disabled' );
}
} );
}
}
return patternBlockEditingModes;
}

const withPatternBlockEditingModes = ( reducer ) => {
return ( state, action ) => {
const nextState = reducer( state, action );

if ( nextState === state ) {
return state;
}

nextState.patternClientIds = state?.patternClientIds ?? new Set();
nextState.patternBlockEditingModes =
state?.patternBlockEditingModes ?? new Map();

switch ( action.type ) {
case 'RESET_BLOCKS': {
nextState.patternClientIds = new Set();
recurseBlocks( action.blocks, ( block ) => {
if ( block.name === 'core/block' ) {
nextState.patternClientIds.add( block.clientId );
}
} );
nextState.patternBlockEditingModes =
getPatternBlockEditingModes( nextState );
break;
}
case 'INSERT_BLOCKS': {
recurseBlocks( action.blocks, ( block ) => {
if ( block.name === 'core/block' ) {
nextState.patternClientIds.add( block.clientId );
}
} );
nextState.patternBlockEditingModes =
getPatternBlockEditingModes( nextState );
break;
}
case 'REMOVE_BLOCKS': {
for ( const removedClientId of action.clientIds ) {
const removedTree =
state.blocks.tree.get(
`controlled||${ removedClientId }`
) ?? state.blocks.tree.get( removedClientId );
if ( removedTree ) {
recurseBlocks( [ removedTree ], ( block ) => {
nextState.patternClientIds.delete( block.clientId );
} );
}
}
nextState.patternBlockEditingModes =
getPatternBlockEditingModes( nextState );
break;
}
case 'REPLACE_INNER_BLOCKS': {
const rootTree =
state.blocks.tree.get(
`controlled||${ action.rootClientId }`
) ?? state.blocks.tree.get( action.rootClientId );

if ( rootTree ) {
recurseBlocks( rootTree?.innerBlocks, ( block ) => {
nextState.patternClientIds.delete( block.clientId );
} );
}
recurseBlocks( action.blocks, ( block ) => {
if ( block.name === 'core/block' ) {
nextState.patternClientIds.add( block.clientId );
}
} );
nextState.patternBlockEditingModes =
getPatternBlockEditingModes( nextState );
break;
}
case 'REPLACE_BLOCKS': {
for ( const removedClientId of action.clientIds ) {
const removedTree =
state.blocks.tree.get(
`controlled||${ removedClientId }`
) ?? state.blocks.tree.get( removedClientId );
if ( removedTree ) {
recurseBlocks( [ removedTree ], ( block ) => {
nextState.patternClientIds.delete( block.clientId );
} );
}
}
recurseBlocks( action.blocks, ( block ) => {
if ( block.name === 'core/block' ) {
nextState.patternClientIds.add( block.clientId );
}
} );
nextState.patternBlockEditingModes =
getPatternBlockEditingModes( nextState );
break;
}
}

return nextState;
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this is the kind of code that deserves some unit testing?

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 can do, though I think the other PR (#66919) shows that unit testing only the reducer or only the selectors isn't a great approach. That PR is a refactor, the store as a whole should be working the same, but I'll have to rewrite all the tests, which is a lot of work and could introduce some margin of error.

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 should also add some e2e tests.


function withAutomaticChangeReset( reducer ) {
return ( state, action ) => {
const nextState = reducer( state, action );
Expand Down Expand Up @@ -2184,4 +2348,6 @@ function withAutomaticChangeReset( reducer ) {
};
}

export default withAutomaticChangeReset( combinedReducers );
export default withPatternBlockEditingModes(
withAutomaticChangeReset( combinedReducers )
);
4 changes: 4 additions & 0 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 @@ -3071,6 +3071,10 @@ export const getBlockEditingMode = createRegistrySelector(
return 'disabled';
}

if ( state?.patternBlockEditingModes?.has( clientId ) ) {
return state?.patternBlockEditingModes.get( clientId );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What I find odd is that the denormalization only applies in the case of the "pattern block editing modes", why not just make a global state.blockEditingModes ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see you mentioned in the description that you have follow-up for "zoom-out" editing modes too. I think we should ideally just have the selector return directly the state value in the end, but I'm ok with going in steps.

Copy link
Contributor

Choose a reason for hiding this comment

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

One challenge is going to be that the selector depends on editorMode which is not within the reducer itself. So it's possible that in the end we'll have two values in the state for each block, a block editing mode for "write" and another one for "design" and the selector would pick the right value depending on the active mode.

Copy link
Contributor Author

@talldan talldan Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What I find odd is that the denormalization only applies in the case of the "pattern block editing modes", why not just make a global state.blockEditingModes ?

Yeah, it's a tricky one. In the case of switching from design to write mode, we don't need to recalculate every pattern block, so storing it separately is useful in that case, we can retain the info across editor mode switches.

Similarly, we need to retain back compat for the block editing modes that might be set via setBlockEditingMode, so I think those also need to be stored in separate state so that we don't accidentally clear them when switching editor modes.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yes, I mistakenly used the same name, we do need this to be separate from the original setBlockEditingMode state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One challenge is going to be that the selector depends on editorMode which is not within the reducer itself.

My other PR does handle this by dispatching an action for switching editor mode to ensure the reducer re-runs:
https://github.com/WordPress/gutenberg/pull/66919/files#diff-d1fbd3af72ee0220c5c3ab2952a24d818174aed711b9a50b57c9544218a63988R1681

but it still reads the mode from the preferences store:
https://github.com/WordPress/gutenberg/pull/66919/files#diff-3c4b969f7547d02bc04a2c554e0f7d21da07c3f779714acf02800ee0341b50d9R279-R281

I'm not sure if this is the best approach. The reducer also needs access to the blocks store to check for role: content attributes, so it's not the only place where it selects from other stores and I saw there was some precedence for this in the block editor reducer.


const editorMode = __unstableGetEditorMode( state );
if ( editorMode === 'navigation' ) {
const sectionRootClientId = getSectionRootClientId( state );
Expand Down
57 changes: 6 additions & 51 deletions packages/block-library/src/block/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import clsx from 'clsx';
* WordPress dependencies
*/
import { useSelect, useDispatch } from '@wordpress/data';
import { useRef, useMemo, useEffect } from '@wordpress/element';
import { useRef, useMemo } from '@wordpress/element';
import {
useEntityRecord,
store as coreStore,
Expand Down Expand Up @@ -37,12 +37,10 @@ import { getBlockBindingsSource } from '@wordpress/blocks';
/**
* Internal dependencies
*/
import { name as patternBlockName } from './index';
import { unlock } from '../lock-unlock';

const { useLayoutClasses } = unlock( blockEditorPrivateApis );
const { isOverridableBlock, hasOverridableBlocks } =
unlock( patternsPrivateApis );
const { hasOverridableBlocks } = unlock( patternsPrivateApis );

const fullAlignments = [ 'full', 'wide', 'left', 'right' ];

Expand Down Expand Up @@ -75,22 +73,6 @@ const useInferredLayout = ( blocks, parentLayout ) => {
}, [ blocks, parentLayout ] );
};

function setBlockEditMode( setEditMode, blocks, mode ) {
blocks.forEach( ( block ) => {
const editMode =
mode ||
( isOverridableBlock( block ) ? 'contentOnly' : 'disabled' );
setEditMode( block.clientId, editMode );

setBlockEditMode(
setEditMode,
block.innerBlocks,
// Disable editing for nested patterns.
block.name === patternBlockName ? 'disabled' : mode
);
} );
}

function RecursionWarning() {
const blockProps = useBlockProps();
return (
Expand Down Expand Up @@ -171,7 +153,6 @@ function ReusableBlockEdit( {
name,
attributes: { ref, content },
__unstableParentLayout: parentLayout,
clientId: patternClientId,
setAttributes,
} ) {
const { record, hasResolved } = useEntityRecord(
Expand All @@ -184,49 +165,24 @@ function ReusableBlockEdit( {
} );
const isMissing = hasResolved && ! record;

const { setBlockEditingMode, __unstableMarkLastChangeAsPersistent } =
const { __unstableMarkLastChangeAsPersistent } =
useDispatch( blockEditorStore );

const {
innerBlocks,
onNavigateToEntityRecord,
editingMode,
hasPatternOverridesSource,
} = useSelect(
const { onNavigateToEntityRecord, hasPatternOverridesSource } = useSelect(
( select ) => {
const { getBlocks, getSettings, getBlockEditingMode } =
select( blockEditorStore );
const { getSettings } = select( blockEditorStore );
// For editing link to the site editor if the theme and user permissions support it.
return {
innerBlocks: getBlocks( patternClientId ),
onNavigateToEntityRecord:
getSettings().onNavigateToEntityRecord,
editingMode: getBlockEditingMode( patternClientId ),
hasPatternOverridesSource: !! getBlockBindingsSource(
'core/pattern-overrides'
),
};
},
[ patternClientId ]
[]
);

// Sync the editing mode of the pattern block with the inner blocks.
useEffect( () => {
setBlockEditMode(
setBlockEditingMode,
innerBlocks,
// Disable editing if the pattern itself is disabled.
editingMode === 'disabled' || ! hasPatternOverridesSource
? 'disabled'
: undefined
);
}, [
editingMode,
innerBlocks,
setBlockEditingMode,
hasPatternOverridesSource,
] );

const canOverrideBlocks = useMemo(
() => hasPatternOverridesSource && hasOverridableBlocks( blocks ),
[ hasPatternOverridesSource, blocks ]
Expand All @@ -244,7 +200,6 @@ function ReusableBlockEdit( {
} );

const innerBlocksProps = useInnerBlocksProps( blockProps, {
templateLock: 'all',
layout,
value: blocks,
onInput: NOOP,
Expand Down
Loading