Add ability to prevent editing blocks using useBlockEditingMode() #50643
Conversation
noisysocks
added
[Type] Enhancement
|
WDYT of this alternative approach @youknowriad @talldan? If we like it I can add tests and docs. |
| 'is-editing-disabled': | ||
| blockEditingMode === 'disabled' || | ||
| ( hasContentLockedParent && ! isContentBlock ), |
There was a problem hiding this comment.
Something I'm not sure about is whether or not to have getEditingMode contain the template lock logic as well so that components only need to call getBlockEditingMode instead of both getBlockEditingMode and getTemplateLock.
It makes the selector logic more complicated, but means that blocks/consumers only have to worry about calling one method and adjusting the UI accordingly.
There was a problem hiding this comment.
It makes the selector logic more complicated, but means that blocks/consumers only have to worry about calling one method and adjusting the UI accordingly.
If the API goes public my vote is for this. Maybe it'll be make it easier to maintain too if the conditions are all in one spot?
There was a problem hiding this comment.
Also to note - if I remember correctly, there's also the potential for getBlockEditingMode to take over the work of (or even replace) __unstableGetContentLockingParent?
The while loops look very similar.
From what I can see __unstableGetContentLockingParent is exclusively used to check if a block is locked by its "inert" parent - the parent's clientId is never used.
In my mind, it would be handy to know whether a block is disabled with one call.
but maybe we should avoid over-thinking/optimization altogether 😄
It's a tricky one. I'm also thinking, is there nothing a few meaningfully-named functions or properties couldn't tell us?
isBlockEditingDisabled(); // blockEditingMode === 'disabled'
isLockedByParent(); // !! __unstableGetContentLockingParent( clientId )
isContentEditingEnabled(); // blockEditingMode === 'contentOnly' || templateLock === 'contentOnly'See useBlockLock (packages/block-editor/src/components/block-lock/use-block-lock.js), that returns these sorts of things.
I'm not sure if it's related, but I'm wondering also if we need to taken into account any "lock" instructions on the block's attributes. See canEditBlock (packages/block-editor/src/store/selectors.js).
There was a problem hiding this comment.
I've put the contentOnly template lock logic into getBlockEditingMode in one of my more recent commits. I think it's definitely nice having only one selector to call, and hopefully the JSDoc comments I help demystify it. What do you think?
Respecting canEditBlock is an interesting idea. I'll investigate. From memory canEditBlock is a bit weird and we only use it in the navigation block.
There was a problem hiding this comment.
OK so you can test what canEditBlock does yourself by selecting a Navigation block, open the settings menu, selecting Lock, then enabling Lock editing.
When you do this you'll see that you can select the Navigation block but that there's an overlay meaning you can't select its children or edit anything. This is different from either 'disabled' which prevents section altogether or 'contentOnly' which allows editing but hides toolbars, etc.
I suppose we could add a fourth mode called e.g. 'overlay' to getBlockEditingMode which encapsulates this behaviour. I'm not really sure there's any benefit to doing this though. getBlockEditingMode is a private API so it's something we can change easily in the future if there's a need.
noisysocks
removed
the
Needs Dev Note
|
Size Change: +914 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 25f3e58. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5052708088
|
There was a problem hiding this comment.
If this gives us the right solution, in terms of behavior, then I'm sold. I like this API and I can even see ourselves making it public.
Some thoughts/questions:
- Will this have any impact on performance?
- I know that we can't use
inert/useDisabledfor the "contentOnly" mode but maybe we can still consolidate this with__unstableHasActiveBlockOverlayActive(make the latter use the former maybe) and when the mode is "disabled", rely onuseDisabled? (It doesn't really have to be here, but curious for your thoughts) - I like how this also replaces the "contentOnly" lock behavior (so not really a new mode or something)
|
Thanks @ramonjd for jumping on a call with me to review this PR. Notes from that call:
Will double check but should be okay so long as the
@ramonjd and I spoke about this. We could put
To be clear: I'll address all this feedback and hopefully have this ready for review/merge early next week. |
packages/block-editor/src/components/block-editing-mode/index.js
Outdated
Show resolved
Hide resolved
| 'is-editing-disabled': | ||
| blockEditingMode === 'disabled' || | ||
| ( hasContentLockedParent && ! isContentBlock ), |
There was a problem hiding this comment.
Also to note - if I remember correctly, there's also the potential for getBlockEditingMode to take over the work of (or even replace) __unstableGetContentLockingParent?
The while loops look very similar.
From what I can see __unstableGetContentLockingParent is exclusively used to check if a block is locked by its "inert" parent - the parent's clientId is never used.
In my mind, it would be handy to know whether a block is disabled with one call.
but maybe we should avoid over-thinking/optimization altogether 😄
It's a tricky one. I'm also thinking, is there nothing a few meaningfully-named functions or properties couldn't tell us?
isBlockEditingDisabled(); // blockEditingMode === 'disabled'
isLockedByParent(); // !! __unstableGetContentLockingParent( clientId )
isContentEditingEnabled(); // blockEditingMode === 'contentOnly' || templateLock === 'contentOnly'See useBlockLock (packages/block-editor/src/components/block-lock/use-block-lock.js), that returns these sorts of things.
I'm not sure if it's related, but I'm wondering also if we need to taken into account any "lock" instructions on the block's attributes. See canEditBlock (packages/block-editor/src/store/selectors.js).
|
I tested content locking in the site and post editor for the following blocks (which activate
Child content is locked as I expected with no regressions. The only change I can see, which is an added bonus, is the addition of locked symbols on the child block items in list view.
|
packages/block-editor/src/components/block-list/use-in-between-inserter.js
Outdated
Show resolved
Hide resolved
|
@noisysocks Random thought, I wonder whether it might be worth consolidating visual/html mode into this API. At the moment there's a very similar API for that ( I could imagine |
bph
added
the
Needs Dev Note
Curation / Governance is a big topic right now and this change caught my attention, as there is a need to add extensibility to parts of the site editor to enable more granular governance. It seems this API could be made public and delight a few agency and product developers. Maybe it's too early, hard for me to say, though. It might be worth a discussion, especially if this wouldn't be a hard lift. |
| const removeDisabledBlocks = ( tree ) => { | ||
| return tree.flatMap( ( { clientId, innerBlocks, ...rest } ) => { | ||
| if ( getBlockEditingMode( clientId ) === 'disabled' ) { | ||
| return removeDisabledBlocks( innerBlocks ); | ||
| } | ||
| return [ | ||
| { | ||
| clientId, | ||
| innerBlocks: removeDisabledBlocks( innerBlocks ), | ||
| ...rest, | ||
| }, | ||
| ]; | ||
| } ); | ||
| }; |
There was a problem hiding this comment.
This will return a new array reference on each call and cause the whole ListView to be rendered, even if the client IDs tree is the same.
Should we extract this into a new private memoized selector?
There was a problem hiding this comment.
I tested something out by adding the following selector to packages/block-editor/src/store/private-selectors.js.
const EMPTY_ARRAY = [];
export const getEnabledBlocks = createSelector(
( state, blocks = EMPTY_ARRAY ) =>
blocks.flatMap( ( { clientId, innerBlocks, ...rest } ) => {
if ( getBlockEditingMode( state, clientId ) === 'disabled' ) {
return getEnabledBlocks( state, innerBlocks );
}
return [
{
clientId,
innerBlocks: getEnabledBlocks( state, innerBlocks ),
...rest,
},
];
} ),
( state ) => [ state.blockEditingModes, state.blocks.parents ]
);Used in packages/block-editor/src/components/list-view/use-list-view-client-ids.js
...
const {
getDraggedBlockClientIds,
getSelectedBlockClientIds,
__unstableGetClientIdsTree,
getEnabledBlocks,
} = unlock( select( blockEditorStore ) );
...
clientIdsTree: getEnabledBlocks(
blocks ?? __unstableGetClientIdsTree( rootClientId )
),
...And it does prevent entire tree rerenders, which might be helpful for posts with lots of blocks:
Before
2023-05-31.14.46.03.mp4
After
2023-05-31.14.44.52.mp4
I'm just not sure about the dependency array [ state.blockEditingModes, state.blocks.parents ], which I copied from getExplicitBlockEditingMode() since that function is called in getEnabledBlocks() anyway.
| parents: getBlockParents( selectedBlockClientId ).filter( | ||
| ( parentClientId ) => | ||
| getBlockEditingMode( parentClientId ) !== 'disabled' | ||
| ), |
noisysocks
added
[Feature] Page Content Focus
What, why, how
Alternative to #50082.
Required to implement #50857.
Part of #44461.
This PR adds a new API to
block-editorthat lets one restrict the editing UI of a block and its children. It's based off of the suggestion by @youknowriad in #50082 (comment).A new hook is added called
useBlockEditingMode( clientId, mode ):A block can call this in its
editfunction (or anywhere, really) withmodeset to one of the three options:'disabled'- This prevents editing the block entirely. The block cannot be selected or interacted with. It uses the samepointer-events: disabledmechanism that is used whentemplateLockis set to'contentOnly'.'contentOnly'- This allows editing the block but removes most UI shown in the block toolbar so that only content related buttons (e.g. formatting) are shown. The block cannot be moved, deleted, etc. It's similar to the treatment received by a content block whentemplateLockis set to'contentOnly'.'default'- Default. Block is fully editable, moveable, removable, etc.The block editing mode is inherited from the parent all the way up to
rootBlockEditingModewhich is specified in the editor settings. This lets one disable the entire editor except for certain blocks which is what is needed to implement #49980.The same hook can be used to return the current editing mode:
This hook is a convenience wrapper for the
getBlockEditingMode( clientId )selector andsetBlockEditingMode( clientId, mode )action. Both exist in theblock-editorstore.Alternatives considered
See #50082 for my first approach at this.
The original idea suggested by @youknowriad in #50082 (comment) was to wrap blocks in a component that uses context to disable its children e.g.
<DisableBlocksProvider>. This is not sufficient for our needs though as we need to be able to customise the UI inBlockToolsandBlockToolbarwhich sits outside of theBlockListBlockcomponent tree. We therefore need to be able to look up a block's editing mode by its client ID. Because the editing mode is inherited from the block's parent, this map needs to exist within the block editor store where we have access to the block tree.Testing Instructions
pbpaste | git apply.To test the existing
contentOnlyfunctionality for regressions: