Fix contentOnly temporary editing persisting on save (+ refactor some of the code) #72959
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
talldan
commented
Nov 4, 2025
| } | ||
| } | ||
|
|
||
| .is-focus-mode .block-editor-block-list__block.is-content-locked.has-child-selected, |
Contributor
Author
There was a problem hiding this comment.
Removed this selector, I'm not sure what it was used for. I tested and it doesn't seem to work in trunk.
Contributor
There was a problem hiding this comment.
is-content-locked doesn't seem to exist as a class so this was probably missed during an earlier refactor?
Contributor
There was a problem hiding this comment.
We don't appear to output is-content-locked as a classname in useBlockProps either, so this removal looks fine to me 👍
talldan
commented
Nov 4, 2025
| ); | ||
| useEffect( () => { | ||
| if ( ! isBlockOrDescendantSelected ) { | ||
| stopEditingAsBlocks( clientId ); |
Contributor
Author
There was a problem hiding this comment.
The clientId was incorrectly passed, the selector takes no params.
Contributor
There was a problem hiding this comment.
stopEditingAsBlocks did take a clientId though? if you mean this one: https://github.com/WordPress/gutenberg/pull/72959/files#diff-4d145a0c43689f5a56c00a8e2919550768ebd233e91a6c6d04fee71a44bae9f2L332
stopEditingContentOnlySection is nicer in its simplicity.
|
Size Change: -193 B (-0.01%) Total Size: 2.45 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d6c8e9a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19090176559
|
9 tasks
Contributor
tellthemachines
left a comment
There was a problem hiding this comment.
This looks like a good enhancement overall and it works as expected in testing! I just left a few niggles and questions below but nothing major.
Any reason it's still in draft mode? Happy to approve otherwise.
| } | ||
| } | ||
|
|
||
| .is-focus-mode .block-editor-block-list__block.is-content-locked.has-child-selected, |
Contributor
There was a problem hiding this comment.
is-content-locked doesn't seem to exist as a class so this was probably missed during an earlier refactor?
| isOutlineMode: outlineMode && ! isTyping(), | ||
| isFocusMode: focusMode || hasBlockSpotlight(), | ||
| temporarilyEditingAsBlocks: getTemporarilyEditingAsBlocks(), | ||
| editedContentOnlySection: getEditedContentOnlySection(), |
Contributor
There was a problem hiding this comment.
Should this be editableContentOnlySection instead? The selector is looking at whether it's in an editable state or not, isn't it?
Contributor
Author
There was a problem hiding this comment.
I'm calling it edited everywhere else as a shorthand for "the section that's being edited". It's not perfect, but I prefer to keep the naming consistent.
| ); | ||
| useEffect( () => { | ||
| if ( ! isBlockOrDescendantSelected ) { | ||
| stopEditingAsBlocks( clientId ); |
Contributor
There was a problem hiding this comment.
stopEditingAsBlocks did take a clientId though? if you mean this one: https://github.com/WordPress/gutenberg/pull/72959/files#diff-4d145a0c43689f5a56c00a8e2919550768ebd233e91a6c6d04fee71a44bae9f2L332
stopEditingContentOnlySection is nicer in its simplicity.
|
|
||
| // The implementation of content locking is mainly in this file, although the mechanism | ||
| // to stop temporarily editing as blocks when an outside block is selected is on component StopEditingAsBlocksOnOutsideSelect | ||
| // to stop editing a content only section an outside block is selected is on component StopEditingContentOnlySectionOnOutsideSelect |
Contributor
There was a problem hiding this comment.
typo: when an outside block is selected
| } | ||
|
|
||
| const showStopEditingAsBlocks = isEditingAsBlocks && ! isContentLocked; | ||
| const showDoneButton = isEditingContentOnlySection && ! isContentLocked; |
Contributor
There was a problem hiding this comment.
Much clearer name 👍
| focusModeToRevert, | ||
| }; | ||
| export function __unstableSetTemporarilyEditingAsBlocks( clientId ) { | ||
| return editContentOnlySection( clientId ); |
Contributor
There was a problem hiding this comment.
Is this still in use now the occurrences below have been deleted? Or are we keeping it because it's public?
Contributor
Author
There was a problem hiding this comment.
It's unused, I guess it should be deprecated like the selectors. I'll push a commit that does that.
ramonjd
reviewed
Nov 5, 2025
Member
ramonjd
left a comment
There was a problem hiding this comment.
Works as described.
I also tested:
✅ As far as I can see it doesn't break other focus mode behaviors, e.g., when editing template parts
✅ Nested patterns (in content only mode) behave
✅ I checked stuff like undo/redo, deleting locked blocks while modifying them
I think it'd be neat to select the block if clicking from the list item context menu, what do you reckon?
| const stopEditingAsBlockCallback = useCallback( () => { | ||
| stopEditingAsBlocks( clientId ); | ||
| }, [ clientId, stopEditingAsBlocks ] ); | ||
| stopEditingContentOnlySection( clientId ); |
Member
There was a problem hiding this comment.
Does stopEditingContentOnlySection accept any args?
| * @param {Object} state Current state. | ||
| * @param {Object} action Dispatched action. | ||
| * | ||
| * @return {Object} Updated state. |
Member
There was a problem hiding this comment.
Looking at action.clientId, can this also be a String?
Contributor
Author
There was a problem hiding this comment.
It should be a string 👍
| return action.focusModeToRevert; | ||
| export function editedContentOnlySection( state = '', action ) { | ||
| if ( action.type === 'EDIT_CONTENT_ONLY_SECTION' ) { | ||
| return action.clientId; |
Member
There was a problem hiding this comment.
Profoundly optional 😄 but related to return comment above.
Explicitly return empty string when undefined:
return action.clientId ?? '';
Contributor
Author
There was a problem hiding this comment.
'' usually refers to the root block, so I don't want to do that.
Contributor
Author
There was a problem hiding this comment.
Though it is weird that the default state is '' 🤔
Contributor
Author
There was a problem hiding this comment.
I've removed the default state value (it'll just be undefined) and fixed the types.
| <MenuItem | ||
| onClick={ () => { | ||
| modifyContentLockBlock( clientId ); | ||
| editContentOnlySection( clientId ); |
Member
There was a problem hiding this comment.
This is working well for me. One improvement would be to select the block if it's not selected when clicking on "Modify". That would cover clicks on the list view context menu click when the block isn't selected.
See the latter half of this vid:
Kapture.2025-11-05.at.11.12.34.mp4
Here's an example diff that works for me:
diff --git a/packages/block-editor/src/components/content-lock/modify-content-lock-menu-item.js b/packages/block-editor/src/components/content-lock/modify-content-lock-menu-item.js
index 9932ef9895..4cb9b27886 100644
--- a/packages/block-editor/src/components/content-lock/modify-content-lock-menu-item.js
+++ b/packages/block-editor/src/components/content-lock/modify-content-lock-menu-item.js
@@ -18,23 +18,31 @@ import { unlock } from '../../lock-unlock';
// also includes artifacts on the store (actions, reducers, and selector).
export function ModifyContentOnlySectionMenuItem( { clientId, onClose } ) {
- const { templateLock, isLockedByParent, isEditingContentOnlySection } =
- useSelect(
- ( select ) => {
- const {
- getContentLockingParent,
- getTemplateLock,
- getEditedContentOnlySection,
- } = unlock( select( blockEditorStore ) );
- return {
- templateLock: getTemplateLock( clientId ),
- isLockedByParent: !! getContentLockingParent( clientId ),
- isEditingContentOnlySection:
- getEditedContentOnlySection() === clientId,
- };
- },
- [ clientId ]
- );
+ const {
+ templateLock,
+ isLockedByParent,
+ isEditingContentOnlySection,
+ isBlockSelected,
+ } = useSelect(
+ ( select ) => {
+ const {
+ getContentLockingParent,
+ getTemplateLock,
+ getEditedContentOnlySection,
+ } = unlock( select( blockEditorStore ) );
+ const { isBlockSelected: _isBlockSelected } =
+ select( blockEditorStore );
+ return {
+ templateLock: getTemplateLock( clientId ),
+ isLockedByParent: !! getContentLockingParent( clientId ),
+ isEditingContentOnlySection:
+ getEditedContentOnlySection() === clientId,
+
+ isBlockSelected: _isBlockSelected( clientId ),
+ };
+ },
+ [ clientId ]
+ );
const blockEditorActions = useDispatch( blockEditorStore );
const isContentLocked =
! isLockedByParent && templateLock === 'contentOnly';
@@ -42,7 +50,8 @@ export function ModifyContentOnlySectionMenuItem( { clientId, onClose } ) {
return null;
}
- const { editContentOnlySection } = unlock( blockEditorActions );
+ const { editContentOnlySection, selectBlock } =
+ unlock( blockEditorActions );
const showStartEditingAsBlocks =
! isEditingContentOnlySection && isContentLocked;
@@ -50,6 +59,10 @@ export function ModifyContentOnlySectionMenuItem( { clientId, onClose } ) {
showStartEditingAsBlocks && (
<MenuItem
onClick={ () => {
+ // Select the block if it's not already selected
+ if ( ! isBlockSelected ) {
+ selectBlock( clientId );
+ }
editContentOnlySection( clientId );
onClose();
} }
andrewserong
reviewed
Nov 5, 2025
| } | ||
| } | ||
|
|
||
| .is-focus-mode .block-editor-block-list__block.is-content-locked.has-child-selected, |
Contributor
There was a problem hiding this comment.
We don't appear to output is-content-locked as a classname in useBlockProps either, so this removal looks fine to me 👍
|
|
||
| // The implementation of content locking is mainly in this file, although the mechanism | ||
| // to stop temporarily editing as blocks when an outside block is selected is on component StopEditingAsBlocksOnOutsideSelect | ||
| // to stop editing a content only section an outside block is selected is on component StopEditingContentOnlySectionOnOutsideSelect |
Contributor
There was a problem hiding this comment.
Tiniest of nits, feel free to ignore!
Suggested change
| // to stop editing a content only section an outside block is selected is on component StopEditingContentOnlySectionOnOutsideSelect | |
| // to stop editing a content only section when an outside block is selected is in the component StopEditingContentOnlySectionOnOutsideSelect |
| rootClientId | ||
| )?.templateLock; | ||
|
|
||
| // If this is a contentOnly template locked block that's being in the process |
Contributor
There was a problem hiding this comment.
Another tiny optional nit (feel free to ignore!)
Suggested change
| // If this is a contentOnly template locked block that's being in the process | |
| // If this is a contentOnly template locked block that's in the process |
Comment on lines
-3225
to
-3228
| export function __unstableGetTemporarilyEditingFocusModeToRevert( state ) { | ||
| deprecated( | ||
| "wp.data.select( 'core/block-editor' ).__unstableGetTemporarilyEditingFocusModeToRevert", | ||
| { |
Contributor
There was a problem hiding this comment.
I can't find this being used anywhere and searched https://wpdirectory.net/, so I reckon this is good to remove 👍
|
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. |
andrewserong
approved these changes
Nov 5, 2025
Contributor
andrewserong
left a comment
There was a problem hiding this comment.
LGTM after the latest updates! 🎉
I also liked @ramonjd's idea for the list view menu to select the section block after clicking Modify (#72959 (comment)), but I don't think that needs to happen in this PR necessarily.
Nice refactor 👍
Contributor
Author
The UI should eventually be replaced by #72044, so I don't want to spend too much time on it. Thanks for the reviews. I think I'll push some tests and then merge ❤️ |
Contributor
Ah, good call, thanks for the reminder 👍 |
Member
Sounds good!
🚀 |
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?
Fixes #49048
Also extracts some of the refactoring changes from #72044, so this PR can be seen as laying the groundwork for that one.
Why?
When the 'Modify' option is used on a contentOnly template locked set of blocks, those blocks are unlocked to allow them to be freely edited, but if the user saves before clicking 'Done', the unlocking is persisted.
This is because the code in trunk actually removes
templateLock: contentOnlyfrom the block.How?
In this PR we instead do the following when the user clicks modify:
getTemplateLockselector returnfalsefor the block in question.defaultfor all of the blocks that are being modified.Neither of those changes are persisted, they're in-memory only, so saving will still retain the templateLock.
In addition to this I've updated the naming of the private actions/selectors/reducers.
I've also removed the concept of 'reverting focus mode'. Instead the
isFocusModeselector will returntruewhenever the 'Modify' option is used, this allows for quite a lot of simplification. I've deleted one of the unstable (but public) selectors related to this, that might be controversial, but I think it's really not useful to anyone, so I can't see it being used.Testing Instructions
templateLock: contentOnlyattribute set, and has some inner blocks (paragraphs, headings etc.). Here's some block markup that you can paste into the editor:Screenshots or screencast
Kapture.2025-11-04.at.13.33.19.mp4