Conversation
talldan
added
[Feature] Blocks
|
Size Change: +133 B (+0.01%) Total Size: 1.93 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1d93ad3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17542532945
|
talldan
force-pushed
the
refactor/template-lock-block-editing-modes-to-reducer
branch
from
f3b59c2 to
6022b63
Compare
jeryj
reviewed
Aug 11, 2025
Comment on lines
+3060
to
+3100
| // If the editor *is* in navigation mode, the block editing mode states | ||
| // are stored in the derivedNavModeBlockEditingModes map. | ||
| if ( isNavMode && state.derivedNavModeBlockEditingModes?.has( clientId ) ) { | ||
| return state.derivedNavModeBlockEditingModes.get( clientId ); | ||
| } |
Contributor
There was a problem hiding this comment.
Should we only have if ( isNavMode ) here so it will return contentOnly as a fallback if state.derivedNavModeBlockEditingModes?.has( clientId ) is false? I'm concerned about edge cases where an item isn't in the derivedNavModeBlockEditingModes for some reason, and this ends up returning default.
Contributor
Author
There was a problem hiding this comment.
I guess there are no cases where default is used in Write Mode?
If so, I think what you suggest is probably a good idea, though perhaps disabled as the fallback instead of contentOnly.
Contributor
Author
There was a problem hiding this comment.
Lets handle it in a separate PR given the code for this is already in trunk.
jeryj
reviewed
Aug 11, 2025
Comment on lines
-3085
to
-3133
| const rootClientId = getBlockRootClientId( state, clientId ); | ||
| const templateLock = getTemplateLock( state, rootClientId ); | ||
| // If the parent of the block is contentOnly locked, check whether it's a content block. | ||
| if ( templateLock === 'contentOnly' ) { | ||
| const name = getBlockName( state, clientId ); | ||
| const { hasContentRoleAttribute } = unlock( | ||
| select( blocksStore ) | ||
| ); | ||
| const isContent = hasContentRoleAttribute( name ); | ||
| return isContent ? 'contentOnly' : 'disabled'; | ||
| } | ||
| return 'default'; | ||
| } |
Contributor
There was a problem hiding this comment.
I really like the refactor to move this logic to the reducer rather than spreading it out within the getter.
Member
There was a problem hiding this comment.
Just so I understand, is the selling point that previously, every call to getBlockEditingMode would traverse the entire block tree and recalculate. Now it's computed once per state change?
Contributor
Author
There was a problem hiding this comment.
The history is that some time ago Synced Patterns didn't work properly in Write Mode.
I tried fixing that by adding extra code to the getBlockEditingMode selector, but it caused a huge performance regression (#65408 (comment)).
I had to buy some more performance in order to be able to fix that bug, and so I've done that by moving code from the selector.
I think the code is still far from good, but it's a tough problem to solve, and I think it's reflective of how complicated the features that use blockEditingModes are. We should try to simplify both the features and the code where we can.
Member
|
What do you think @talldan? Is this still something that can be achieved for 6.9? Thanks! 🍺 |
talldan
force-pushed
the
refactor/template-lock-block-editing-modes-to-reducer
branch
from
6022b63 to
516ead0
Compare
4 tasks
…ocking Move contentOnly template locking code from selector to reducer Simplify getBlockEditingMode tests and add additional cases Remove unused dependencies WIP
talldan
force-pushed
the
refactor/template-lock-block-editing-modes-to-reducer
branch
from
516ead0 to
e32f963
Compare
ramonjd
reviewed
Sep 8, 2025
Member
ramonjd
left a comment
ramonjd
left a comment
There was a problem hiding this comment.
This is awesome, thanks for reviving it, @talldan
It's working as described for me.
Pluses I see:
- separation of concerns
- great test coverage
Is there another way you'd like folks to test?
Do you think it'd help to have documentation around this somewhere?
| ) { | ||
| return state.derivedBlockEditingModes.get( clientId ); | ||
| } | ||
| const isNavMode = isNavigationMode( state ); |
Member
There was a problem hiding this comment.
Can you refresh my memory... is navigation flow related to the former block interaction tool?
Is this for backwards compat?
Contributor
Author
There was a problem hiding this comment.
It is related and not. Write Mode has now taken that tool's place, but the API used is still the same. So navigation mode === write mode.
Comment on lines
-3085
to
-3133
| const rootClientId = getBlockRootClientId( state, clientId ); | ||
| const templateLock = getTemplateLock( state, rootClientId ); | ||
| // If the parent of the block is contentOnly locked, check whether it's a content block. | ||
| if ( templateLock === 'contentOnly' ) { | ||
| const name = getBlockName( state, clientId ); | ||
| const { hasContentRoleAttribute } = unlock( | ||
| select( blocksStore ) | ||
| ); | ||
| const isContent = hasContentRoleAttribute( name ); | ||
| return isContent ? 'contentOnly' : 'disabled'; | ||
| } | ||
| return 'default'; | ||
| } |
Member
There was a problem hiding this comment.
Just so I understand, is the selling point that previously, every call to getBlockEditingMode would traverse the entire block tree and recalculate. Now it's computed once per state change?
| break; | ||
| } | ||
|
|
||
| const nextDerivedBlockEditingModes = |
Member
There was a problem hiding this comment.
Do you think it's worth adding performance tests later?
I could be totally barking up the wrong tree, but let's say we have a document with 10,000 blocks, does this mean we're storing 10,000 block editing modes entries in memory?
Member
There was a problem hiding this comment.
I did a very crude js heap comparision using window.performance.memory and I couldn't see any memory degradation comparing trunk with this PR based on a derivedNavModeBlockEditingModes count of 40. Total heap size hovered around 200-280 MB
Contributor
Author
There was a problem hiding this comment.
We do already have performance tests in CI, but then I don't think they test these modes.
I could be totally barking up the wrong tree, but let's say we have a document with 10,000 blocks, does this mean we're storing 10,000 block editing modes entries in memory?
Not really, not every block gets a block editing mode. Only contentOnly or disabled blocks are given one. Most of the time blocks will just be default and then they have no mode set.
But anyway, if we have 10,000 blocks in the post, we'd be storing other state for those blocks, so why not a block editing mode? It is only a short string.
I think the issue is more about whether we update 10,000 blocks on every state change, and we don't.
Here's some of the performance related checks the reducer does:
- Only updates subtrees of blocks - e.g. if a synced pattern is inserted only that pattern and its children have block editing modes recomputed
- Takes care to only update on particular actions - e.g. insert blocks, remove blocks, changing templateLock and others
- Avoids unnecessary store updates as much as possible - e.g. don't replace objects with new references if the data in those objects hasn't changed. This will prevent react renders that don't do anything.
|
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. |
talldan
requested review from
andrewserong,
getdave,
tellthemachines and
youknowriad
Contributor
tellthemachines
left a comment
tellthemachines
left a comment
There was a problem hiding this comment.
I've been testing this on both blocks with "templateLock":"contentOnly" and in write mode (and both together) and behaviour matches trunk, so looks good from that side!
I'm not super knowledgeable about these parts of the code but the changes seem alright too.
This was referenced Sep 9, 2025
ramonjd
approved these changes
Sep 9, 2025
Member
ramonjd
left a comment
ramonjd
left a comment
There was a problem hiding this comment.
I've tested this across the given scenarios, and can't find any regressions.
Let's get it in and see how it fares.
Thanks again for all the work on this.
talldan
deleted the
refactor/template-lock-block-editing-modes-to-reducer
branch
adamsilverstein
pushed a commit
to adamsilverstein/gutenberg
that referenced
this pull request
Sep 11, 2025
WordPress#67606) * Update derived block editing modes to support content only template locking Move contentOnly template locking code from selector to reducer Simplify getBlockEditingMode tests and add additional cases Remove unused dependencies ---- Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org> WIP * Remove defunct code * Fix `getEnabledBlockParents` test * Fix test for explicitly set blockEditingMode
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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?
Follow-up to #67026.
Fixes #71551
Refactors some of how
templateLock: contentOnlyworks.Why?
Unlike other
templateLockoptions,templateLock: contentOnlyuses theblockEditingModeAPI internally. In trunk, these block editing modes are calculated in a selector.Selectors can be called, so expensive selectors isn't a good idea. Usually the solution is memoization, but that doesn't work for all cases, like when there are a lot of dependencies or complex dependencies for the memoization.
The option chosen to resolve this in #67026 was to move the calculation of block editing modes to the block editor reducer instead.
#67026 did this for Write Mode and synced patterns. This PR does the same for
templateLock: contentOnlyHow?
The code ends up looking quite different because of the different natures of selectors and reducers.
Testing Instructions
Everything should work the same as
trunk.Here's some block markup to test
templateLock: contentOnlywith:Other things to test:
Screenshots or screencast
TBC