Consolidate getBlockEditingMode to return 'contentOnly' or 'disabled' while in NavMode
#71119
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…NavMode We have a lot of logic spread throughout the codebase to determine if we're in contentOnly mode for a block. We're coupling it with isNavigationMode checks too. We already have this state within useBlockEditingMode, so it should simplify by only requiring the useBlockEditingMode check. This is assuming my understaning of how Write Mode is supposed to work is correct, which I'm not confident of yet :)
|
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 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. |
jeryj
added
[Feature] Write mode
[Type] Code Quality
|
Size Change: -92 B (0%) Total Size: 1.91 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 321a478. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/16811517276
|
Contributor
|
This makes sense to me, and should mean we can clean up a lot of other places in the code. |
getdave
reviewed
Aug 8, 2025
Contributor
getdave
left a comment
getdave
left a comment
There was a problem hiding this comment.
Thanks for looking into how we can make this more efficient.
I have concerns about this approach. Previously we had a lot of checks in selectors for various states that would result in different block editing modes....etc. This caused a number of issues - unfortunately I can't remember the specifics as it was a while ago (maybe @talldan can?).
This is what @talldan tried to clean up via the creation of derived reducers...etc. We now have one place where the modes of blocks are calculated. This PR seems to break that contract.
Also, I suspect there might be scenarios where the editing mode of specific blocks might need to be changed even when in Nav mode. Forcing a hardcoded state would make that impossible.
Instead, we should ensure that getDerivedBlockEditingModesForTree properly handles all navigation mode scenarios and that the derived state is always up-to-date when the editor mode changes.
talldan
changed the title
talldan
changed the title
getBlockEditingMode to return 'contentOnly' or 'disabled' while in NavMode
getdave
reviewed
Aug 8, 2025
| // are stored in the derivedNavModeBlockEditingModes map. | ||
| return state.derivedNavModeBlockEditingModes?.has( clientId ) | ||
| ? state.derivedNavModeBlockEditingModes.get( clientId ) | ||
| : 'contentOnly'; |
Contributor
There was a problem hiding this comment.
Doing this short circuits the hierarchy / cascade which is:
- Derived state = "Based on current editor context, this block should be X",
- Explicit modes = "This block explicitly wants to be Y" (if no derived state),
- Template locks = "Based on structural constraints, this block should be Z" (if no derived or explicit mode)
- Default - the default editing mode.
Contributor
There was a problem hiding this comment.
Thanks for the ping. TBH, it's difficult to understand this selector after such a long time away (even though it's much simpler than it used to be). I've forgotten all about it at some point.
The change might be ok, but I'm not sure it'll result in much gain. I think most blocks that need contentOnly mode when in nav mode are given it in the derivedNavModeBlockEditingModes, so I expect the fallback might not do much. Hard to say without a lot of testing.
I have #67606 that I want to pick up again, I rebased it recently. I need to understand the changes again. Unfortunately it was still WIP when I left it and I didn't write a PR description. It simplifies the selector even more though by moving all the template locking code out.
talldan
reviewed
Aug 8, 2025
| const classes = clsx( 'block-editor-block-contextual-toolbar', { | ||
| 'has-parent': showParentSelector, | ||
| 'is-inverted-toolbar': isNavigationMode && ! hasFixedToolbar, | ||
| 'is-inverted-toolbar': isContentOnlyMode && ! hasFixedToolbar, |
Contributor
There was a problem hiding this comment.
contentOnly is also used outside of navigation (write) mode:
- block overrides in synced patterns
- templateLock
contentOnly - Post content blocks when editing a post with the 'Show Template'
Those all now have the inverted toolbar after this change, whereas my understanding is that it's only for navigation mode to indicate the different mode.
Before:
After:
Contributor
Author
There was a problem hiding this comment.
It was my understanding that we'd surface if a block was contentOnly with the toolbar color, but maybe I misunderstood. @richtabor, can you clarify? I could go either way on if the toolbar should turn black or not if it's contentOnly. It'd be surprising to see the black toolbar, but also nice to have a visual indicator to know why the tools are missing.
Contributor
There was a problem hiding this comment.
Ok, that's fine if so.
There might be some contrast issues to work out with the purple icon color (also not sure about the grey text color).
Contributor
Author
This is what I wasn't sure of. Should blocks be allowed to be in a different state other than disabled or contentOnly while in Write Mode? |
jeryj
commented
Aug 8, 2025
Comment on lines
+3052
to
+3058
| if ( isNavMode ) { | ||
| // If the editor *is* in navigation mode, the block editing mode states | ||
| // are stored in the derivedNavModeBlockEditingModes map. | ||
| return state.derivedNavModeBlockEditingModes.get( clientId ); | ||
| } | ||
| return state.derivedBlockEditingModes.get( clientId ); | ||
| }; |
Contributor
Author
There was a problem hiding this comment.
I was curious if everything was stored in derivedBlockEditingModes. It appears not 😅 . It looks like derivedBlockEditingModes does handle most everything though. I think we can simplify this overall to be clearer. For example, do we need the templateLock section? derivedBlockEditingMode handles a ton of this too.
Contributor
There was a problem hiding this comment.
I think derivedNavModeBlockEditingModes should almost handle everything for nav mode.
derivedBlockEditingModes handles lots for regular mode.
For the template locking code I have another PR that I was working on - #67606.
IIRC, block editing modes don't handle every type of template lock, only contentOnly. I think there are a few things block editing modes can't do, so other types of template lock use completely different APIs.
An example of something block editing modes can't do is enable a block, but prevent inner blocks from being added/moved/removed from that enabled block.
Contributor
Author
There was a problem hiding this comment.
I'll close this PR in favor of #67606. That seems like it's further ahead on the refactor.
Contributor
Author
|
I'm not sure this PR is a good idea. I think the discussion will be good for helping us understand how the system works though. |
Contributor
Author
|
Closing in favor of #67606 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Rearranges the logic to return
contentOnlyordisabledfor all blocks while in Navigation Mode (Write Mode)Why?
We have a lot of logic spread throughout the codebase to determine if we're in
contentOnlymode for a block. We're coupling it withisNavigationModewithin the components consuming thegetBlockEditingModechecks.We already have a check within
useBlockEditingModeforisNavMode, so it should consolidate the logic by moving theisNavModecheck up so if we're in Write Mode (isNavMode), we always returncontentOnlyordisabled. This is assuming my understanding of how Write Mode is supposed to work is correct, which I'm not confident of yet :)This should result in a more performant and simplified codebase and give us a better foundation for implementing WriteMode.
How?
Move
isNavModelogic return first ingetBlockEditingModeTesting Instructions
Testing Instructions for Keyboard
Screenshots or screencast