Allow template part editing in write mode #67372
Conversation
talldan
added
[Type] Enhancement
|
Size Change: +279 B (+0.02%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 00bc8ae. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12270164427
|
| } | ||
|
|
||
| // Allow selection of template parts outside of sections. | ||
| if ( blockName === 'core/template-part' ) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
|
Thank you so much for all your work here! Some quick feedback after going through it today with some folks:
|
We actually do. When a template is rendered when in “Design” mode, you can select the header and see “Edit”. But in this scenario I’m referring to, the “Edit” button opens the template part in an isolated view. I propose we do the same here, treating them the same. |
That might be from this bug in
So retain the 'Edit' button? Synced patterns are also another case, 'Edit original' still shows in Write Mode when they're used as sections. Either way, the change is small enough that it could be considered for a follow-up that adjusts how this works across a wider range of blocks. |
I'm not sure what @richtabor means here but I can relay the feedback I've heard in chatting with a few folks last week that the "edit" button didn't make sense here in write mode in the block toolbar for template parts. I think as part of simplifying the site editing experience, we should remove the "edit" option when in Write mode in order to keep someone on the main canvas. It's an odd option to have listed when so few options are available and too prominent as is. |
It was pretty easy to git blame this, but I still didn't find a clear answer. The
I'd forgotten the first one exists. I don't think I've seen a use case for it. In terms of answering your question, my hunch is:
|
talldan
force-pushed
the
add/write-mode-section-editing-to-template-parts
branch
from
1fd035e to
b15f5e3
Compare
|
I think there's one more blocker issue for this, which is that when editing a Page in the Site Editor (in Template Locked rendering mode), template parts are now editable (including the inner blocks). Steps to Repro
In this PR: Template parts and their inner blocks can be edited I'll be meditating on how to solve this! It's challenging because the Zoom Out / Write mode block editing modes currently take priority over the Template Locked block editing modes. Zoom Out and Write Mode are also implemented in the block editor package, where there's no knowledge that Template Locked rendering mode exists, as that's a WordPress concept, so there's no easy option for coordinating between the two. I'm wondering if the block editing mode priorities should change so that it's still possible to call |
talldan
force-pushed
the
add/write-mode-section-editing-to-template-parts
branch
2 times, most recently
from
46a9999 to
cfbf48a
Compare
…te part content blocks
…derived mode First iteration of handling disabled blocks as derived block editing modes
talldan
force-pushed
the
add/write-mode-section-editing-to-template-parts
branch
from
cfbf48a to
631ac3a
Compare
|
I think I've got this working as is expected now, but I'm not so happy with the implementation and the performance tests don't seem happy, so I'll need to revise a few things. In terms of the implementation, I don't like how |
| // Otherwise, check if there's an ancestor that is contentOnly | ||
| const parentMode = getBlockEditingMode( state, rootClientId ); | ||
| return parentMode === 'contentOnly' ? 'default' : parentMode; | ||
| return 'default'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm having a hard time connecting your comment with the existing code and why the change? (Agreed it's confusing)
There was a problem hiding this comment.
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
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.
| if ( hasDisabledBlocks ) { | ||
| const ancestorBlockEditingMode = getAncestorBlockEditingMode( | ||
| state, | ||
| clientId | ||
| ); | ||
| if ( ancestorBlockEditingMode === 'disabled' ) { | ||
| derivedBlockEditingModes.set( clientId, 'disabled' ); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the part I've refactored from the getBlockEditingModes selector.
After this change, if a parent (ancestor) block has a blockEditingMode of 'disabled', the children will have a disabled derivedBlockEditingMode to reflect the way the mode is inherited by children.
The selector already has code to check derivedBlockEditingModes and return the value from it.
| // If the block already has an explicit block editing mode set, | ||
| // don't override it. | ||
| if ( state.blockEditingModes.has( clientId ) ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| derivedBlockEditingModes: new Map( [ | ||
| [ '6cf70164-9097-4460-bcbf-200560546988', 'disabled' ], | ||
| [ 'ef45d5fd-5234-4fd5-ac4f-c3736c7f9337', 'disabled' ], | ||
| [ 'b26fc763-417d-4f01-b81c-2ec61e14a972', 'disabled' ], | ||
| [ '9b9c5c3f-2e46-4f02-9e14-9fe9515b958f', 'disabled' ], | ||
| [ 'b3247f75-fd94-4fef-97f9-5bfd162cc416', 'disabled' ], | ||
| [ 'e178812d-ce5e-48c7-a945-8ae4ffcbbb7c', 'disabled' ], | ||
| ] ), |
There was a problem hiding this comment.
Now we have to list out all the blocks that inherit the disabled mode from the parent so that's mostly what the test changes are.
| @@ -433,7 +473,7 @@ describe( 'private selectors', () => { | |||
| ], | |||
| [ | |||
| 'e178812d-ce5e-48c7-a945-8ae4ffcbbb7c', | |||
| '9b9c5c3f-2e46-4f02-9e14-9fe9515b958f', | |||
| 'ef45d5fd-5234-4fd5-ac4f-c3736c7f9337', | |||
There was a problem hiding this comment.
This test had some mistakes in it. We can see from order that the parent of 'e178812d-ce5e-48c7-a945-8ae4ffcbbb7c' is 'ef45d5fd-5234-4fd5-ac4f-c3736c7f9337'.
It meant changing a bunch of stuff about the test.
| @@ -12,11 +12,13 @@ | |||
| }, | |||
| "isLink": { | |||
| "type": "boolean", | |||
| "default": false | |||
| "default": false, | |||
| "role": "content" | |||
There was a problem hiding this comment.
I think I might move these changes to the post blocks to another PR, as they're really not relevant to the topic of the PR.
| if ( ! isNavigationMode ) { | ||
| for ( const clientId of templateParts ) { | ||
| setBlockEditingMode( clientId, 'contentOnly' ); | ||
| } |
There was a problem hiding this comment.
When editing a page (with 'Show template' on) in write mode, template parts are now 'disabled' (because they're not set to 'contentOnly').
There was a problem hiding this comment.
Oh, I was just wondering about this, and what navigationMode even means in this context.
So in this third useEffect, it sets template part blocks to content-only mode if not in navigation mode (which seems to be another term for write mode)?
| const updatedBlock = getBlockTreeBlock( | ||
| nextState, |
There was a problem hiding this comment.
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. 😄
To expand on this, for the site editor the 'first block' test is showing a drop of around 30-40%. This is a test that edits a page in the site editor, so the changes to how disabled blocks work in this PR is going to be the problem. I'd expect this if there are lots of actions being triggered on editor load that cause the derived block editing modes to be recalculated, which there are.
Also, the way that component finds template part client ids involves looking through every single block, and it'll recalculate whenever blocks are inserted/removed. A hack for this could be to find template parts from the list of controlled blocks which is shorter and less likely to change. Block syncing for template parts also seems to trigger a lot of this. It replaces inner blocks for template parts multiple times, giving them new clientIds in the process. Some of that might be down to the way react triggers effects multiple times in dev mode. |
|
Think I've solved the performance issues now, it mostly seems to have come down to some small changes to |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @xavier-lc. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
I've noticed this testing on related work. It's really quite bad and we should consider revising it asap. |
|
|
||
| // Template parts become sections in navigation mode. | ||
| const _isNavigationMode = isNavigationMode( state ); | ||
| if ( _isNavigationMode && blockName === 'core/template-part' ) { |
There was a problem hiding this comment.
Why do template parts and pattern have different behaviors? Should they be "sections" always?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 😇
| for ( const clientId of disabledIds ) { | ||
| unsetBlockEditingMode( clientId ); | ||
| } | ||
| } ); | ||
| }; | ||
| }, [ templateParts, contentOnlyIds, disabledIds, registry ] ); | ||
| }, [ disabledIds, registry ] ); | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
What would it take to move away from this component entirely? The code here is so confusing.
What happens if we passed a list of block types as settings to the block editor: editableBlockTypes: [ 'core/post-content', 'core/post-title' ] or something like that, would that be a valid solution?
There was a problem hiding this comment.
Yeah, it is confusing! There's already a list like this - https://github.com/WordPress/gutenberg/blob/trunk/packages/editor/src/components/provider/use-post-content-blocks.js
I think your idea works for most of the blocks, apart from the template part where the block itself is enabled but inner blocks are disabled. This is buggy anyway, as disabling the inner blocks like that doesn't prevent insertions, so at the moment you can still drag things into the template part block when editing a page. 😓
I personally think it'd be best to remove the way template parts are selectable in this mode. There's a parallel discussion on it here - #67621 (comment).
There was a problem hiding this comment.
Another thing is that in the long run, I think it'd be interesting to determine these blocks via their bindings.
E.g. if the user is editing a page with the id 12, then any blocks with bindings to that entity become editable.
That's a bit far away, but it makes me thing that rather than editableBlockTypes we should instead pass editableClientIds as the editor setting.
Blocks with a post meta binding could already be made to work like this if they're in the template.
There was a problem hiding this comment.
I think all these exceptions we're introducing to please some specific UX affordance are really not helping. We keep constructing and deconstructing. I'm not sure how to solve it personally but I think we should consider scaling back UX wise maybe and trying to systemize these changes.
It's really not clear to me why we're treating template parts differently than reusable blocks. We should (at least in our minds) consider these two things the same.
There was a problem hiding this comment.
Ok, I'll look into the template part / synced pattern disparity in a separate PR so design folk can be brought into a more focused conversation. My feeling is it might also take some improvements to both blocks to land in a happy place, but we'll see.
I'm happy to discuss possibilities around DisableNonPageContentBlocks, but that doesn't really seem like a blocker to this PR, as it's barely any different to trunk. Maybe there's someone else who'd also be interested in looking into this, I seem to be getting ten new things to do with each single PR I work on 😄
There was a problem hiding this comment.
Ok, I'll look into the template part / synced pattern disparity in a separate PR
#68042 is the PR.
Actually it does highlight something. Giving Site Title/Logo the role: content attribute isn't enough. In a pattern a paragraph would have this role, but it still isn't editable unless it is marked as a pattern override.
We likely need some additional mechanic to make this work.
|
Noting that with write mode, we often have to click to highlight which blocks to edit and double clicking then leads to the isolated editing mode. More than a few times, I landed there by accident! I think in Write mode, we need to turn off isolated editing in general, both with the "edit" option in the toolbar and the double click to edit. |
What?
Fixes #66463
Allows editing template parts in Write Mode. Also makes blocks like Site Title and Site Logo editable.
Background
Before I describe changes in the PR, I'll describe how the whole things works in trunk.
The block editor has the concept of
blockEditingModes(described more deeply in #67856). Blocks can be set into one of three editing modes:default- the block behaves normally, it can be selected and edited as it usually can with all block controls available (in the inspector and toolbar)disabled- the block can't be selected or edited. All child blocks are also disabled (unless they have one of the other two block editing modes set), and they can't be removed, inserted or reordered.contentOnly- makes the block only partially editable and selectable. The block's inspector controls and some other controls are hidden. The user should be able to update content of the block, but not the design. The block can still itself be moved, and any inner blocks are also free to be moved, deleted or new blocks inserted.Features like Synced Patterns, Zoom Out, Write Mode, and the 'Show Template' option when editing a post are heavily dependent on this. When you use those features you may notice some blocks are disabled and some are contentOnly, others might just be normally editable.
Here's a brief description of each one:
childblocksdisabled. When changing the design, particular blocks in the pattern can be enabled as pattern overrides or given bindings. If you insert a pattern instance that contains any of those kinds of blocks, the blocks are madecontentOnlyand allow content changes.<main>tag. So any direct children of<main>are sections. The main element and the sections are incontentOnlymode, but all other blocks aredisabled(including Synced Patterns).navigationModein the codebase) - Write mode has the same kind of section editing as Zoom Out. Additionally child blocks of sections that are considered 'content' blocks are incontentOnlymode so that their content can be updated. That's determined currently by whether the block has at least one attribute withrole: content. Synced patterns have some special treatment to ensure users can't change the design of a pattern in write mode, they work very similarly regardless of whether write mode is active or not.The coordination of the block editing modes for those three features is managed in the codebase within the
withDerivedBlockEditingModesreducer in the block editor package (more specifically,getDerivedBlockEditingModesForTreecontains the code that determines the mode for an individualclientId). It updates two properties in the block editor store whenever particular actions are dispatched that require changes to the block editing modes (e.g. enabling zoom out, inserting a new section or synced pattern):derivedBlockEditingModes- The block editing modes for outside of write modederivedNavModeBlockEditingModes- The block editing modes for write modeThere are two separate properties because the block editor store can't determine whether it's in write mode as that data isn't within the block editor store (it's a preference).
They're called 'derived' block editing modes because they haven't been explicitly set, they're calculated based on things like which block are sections, which blocks are synced patterns, and which have the
contentrole, bindings, or pattern overrides.There's one additional piece, which is that some block types are conferred 'section' status by the
isSectionBlockselector. The main thing this does is show a 'Content' panel in the block's Inspector Controls.The the 'Show Template' option was also mentioned. This is when a user is viewing a post and the 'Show Template' option is active. In this mode, the template is shown but the majority of blocks in the template are
disabledand only page content is editable (title, featured image, any blocks within the Post Content). From the template, Post Title and Featured Image arecontentOnlyand the inner blocks of Post Content are set todefault. Template Parts are also selectable, but can't be updated.This 'Show Template' feature works a little differently. The block editor package doesn't know whether a page/post/pattern or something else entirely is being shown so it can't coordinate this. It's handled by the Editor package in the
DisableNonPageContentBlockscomponent. That component calls thesetBlockEditingMode.When
setBlockEditingModeis called it updates a different property within the block editor store calledblockEditingModes(these are sometimes referred to as 'explicit' block editing modes because they've been explicitly set). This is kept separate toderivedBlockEditingModesso that if the user switches to zoom out and back, theblockEditingModesaren't removed.The
derivedNavModeBlockEditingModes,derivedBlockEditingModesand theblockEditingModesare returned by the same selectorgetBlockEditingModes. Intrunkif the block has one of the derived modes it takes precedence over the explicitblockEditingMode.How?
role: contentadded to their attributes so that in Write Mode they'll be set tocontentOnly.isSectionBlockhas been updated to consider the template part block a section. It shows the 'Content' panel in the block inspector.getDerivedBlockEditingModesForTreehas been updated to set the correct block editing modes for template parts in Write Mode.The main challenge was the 'Show Template' option since it uses the explicit block editing modes, not the derived ones. The changes I described above alone made template parts editable when editing a page in Write Mode with 'Show Template' on. To solve this, I've made the explicit block editing modes take priority over the derived ones and added some code to
DisableNonPageContentBlocksto not enable template parts when write mode is active.There were also some performance issues caused by
DisableNonPageContentBlocks, which I've partially solved by splitting it up into multipleuseEffectcalls.Problems to solve
I think the PR is almost there, but in my own testing:
"role":"content". (another follow-up issue)Testing Instructions
(I find it's best to use Twenty Twenty Four for testing, as that theme has lots of sections out of the box)
Template editing
Page editing
Screenshots or screencast
Kapture.2024-11-29.at.12.38.36.mp4