Fix contentOnly insertion, removal, and moving #72416
Conversation
talldan added [Type] Bug 
| if ( | ||
| blockEditingMode === 'contentOnly' && | ||
| ! isContainerInsertableToInWriteMode( state, blockName, rootClientId ) | ||
| ( isParentSectionBlock || blockEditingMode === 'contentOnly' ) && |
There was a problem hiding this comment.
If the parent is a 'section' it usually has a blockEditingMode of default (so that you can move, insert, remove the section), but I think that results in skipping the isContainerInsertableToInContentOnlyMode logic, so I've added an extra check for isParentSectionBlock here..
There was a problem hiding this comment.
Should we maybe change isContainerInsertableToInContentOnlyMode to remove the logic that allows inserting if a block is a section root? That made sense in write mode so blocks could be added to empty templates, but with the patterns approach we probably don't need to do that anymore?
There was a problem hiding this comment.
Let me try it. I guess zoomed out would be the only concern, as I think it still uses section roots. Though I don't think it uses contentOnly at all, so I don't think it should be a problem.
There was a problem hiding this comment.
It's still needed for zoomed out based on my testing.
There was a problem hiding this comment.
We could maybe look at removing it when we get to updating the zoom out logic (i.e. as part of #71807).
There was a problem hiding this comment.
Yeah, we should remove it when we simplify zoom out.
| 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. |
| } | ||
| | ||
| const blockEditingMode = getBlockEditingMode( state, rootClientId ); | ||
| const blockEditingMode = getBlockEditingMode( state, clientId ); |
There was a problem hiding this comment.
It didn't make sense to me that we check the root here, so I've changed it to check the clientId being removed.
There was a problem hiding this comment.
Iirc that's intended to match the logic for block insertion in canInsertBlockTypeUnmemoized (this line). I wanted to ensure that if a block can be inserted it can always be deleted (in some cases the "delete" option was unavailable from the options menu even when you could "duplicate" or "add after" etc..)
There was a problem hiding this comment.
A quick test of blocks like List, Buttons and Quote shows it's still possible to delete blocks inside them, so maybe it doesn't make that much of a difference.
There was a problem hiding this comment.
That's true. Maybe I should change it back to see if it causes any issues.
I do agree with the idea of making the logic similar in both selectors, but the issue I'm wondering about is the way blockEditingMode works. Usually it's possible to have a contentOnly parent block with a default (enabled) inner blocks. That's how the Post Content block works when the 'Show Template option is used.
I guess the difference is that after your recent changes, the new logic is that blockEditingMode: contentOnly now works differently for container blocks that have the content role and it enforces that the children must also have a content role and be contentOnly. 🤔
There was a problem hiding this comment.
I've updated the selectors to have matching logic, and it looks like everything still works, so that's good 😄
I think we could consider moving more of the logic related to insertion, moving, deletion for blocks within sections into a single function, perhaps repurpose isContainerInsertableToInContentOnlyMode, it seems like an easy way to keep the code always consistent across the three selectors.
I'll leave it till later on though.
| ! isContainerInsertableToInContentOnlyMode( | ||
| state, | ||
| getBlockName( state, rootClientId ), | ||
| getBlockName( state, clientId ), |
There was a problem hiding this comment.
This line looks to be a typo, because it results in isContainerInsertableToInContentOnlyMode checking whether the root block is a content block twice.
There was a problem hiding this comment.
Just checking if user-created unsynced patterns are supposed to be included? I can still delete child blocks.
I enabled the content only experiment, created an unsynced pattern, then inserted it into a post via the inserter.
The first pattern is mine, the second is a theme pattern inserted from the patterns inserter.
Kapture.2025-10-21.at.11.56.38.mp4
| const blockEditingMode = getBlockEditingMode( state, clientId ); | ||
| if ( | ||
| blockEditingMode === 'contentOnly' && | ||
| ! isContainerInsertableToInContentOnlyMode( | ||
| state, | ||
| getBlockName( state, clientId ), | ||
| rootClientId | ||
| ) | ||
| ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
To fix movers, also replicate the isContainerInsertableToInContentOnlyMode here in the canMoveBlock selector.
| I should write some tests, and I probably need to update some of the existing tests, I'll keep it a draft until I've done that. edit: all the existing tests passed 🤷 |
| Size Change: +63 B (0%) Total Size: 2.19 MB
ℹ️ View Unchanged
|
| Flaky tests detected in 63cec0b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18738363405
|
talldan added the Backport to WP 6.9 Beta/RC 
github-project-automation bot moved this to 🔎 Needs Review in WordPress 6.9 Editor Tasks 
ramonjd left a comment
ramonjd left a comment There was a problem hiding this comment.
This is working according to the PR test instructions for me, thanks.
Tested with the content only experiment on and off.
The only thing I noticed was that user-created unsynced patterns were still draggable/deletable. Is that intentional?
| ! isContainerInsertableToInContentOnlyMode( | ||
| state, | ||
| getBlockName( state, rootClientId ), | ||
| getBlockName( state, clientId ), |
There was a problem hiding this comment.
Just checking if user-created unsynced patterns are supposed to be included? I can still delete child blocks.
I enabled the content only experiment, created an unsynced pattern, then inserted it into a post via the inserter.
The first pattern is mine, the second is a theme pattern inserted from the patterns inserter.
Kapture.2025-10-21.at.11.56.38.mp4
The whole pattern should be draggable/deletable yep. For the blocks inside, some should be draggable (List Items, Buttons).
Hmm, yeah, that's a bug. Seems to happen when the blocks are at the root level. I thought I'd solved that. |
Ok, so the bug happens when an unsynced pattern is first created, but if you reload the editor then it works again. Same if you insert an existing unsynced pattern. The issue is probably related to the selector dependencies, so hopefully an easy-ish fix. |
Should we not allow this to happen? Presumably places where text lives should be able to accommodate varying amounts of it? My main concern here is that pattern authors may resort to workarounds such as using an unstyled Quote container to allow adding multiple paragraphs 😅 |
I don't think there should be any difference with how paragraphs work at the root of the pattern vs how they work in a group within a pattern. For the contentOnly experiment there's visually no difference, so it makes no sense to a user that one option allows more paragraphs to be inserted while the other doesn't. |
talldan force-pushed the fix/content-only-insertion-removal-and-moving branch from 321e63d to ed8bfd5 Compare | Object.entries( { | ||
| block1: { name: 'core/test-block-ancestor' }, | ||
| block2: { name: 'core/block' }, | ||
| block2: { name: 'core/group' }, |
There was a problem hiding this comment.
core/block is a synced pattern, which is a section, so it wouldn't allow insertion, the selector now prevents it. The block in this test needed to be changed to a different container block, one that's not a section.
This should be fixed now, @ramonjd. |
Thanks! I just retested since 124b988 by creating an unsynced pattern in the editor. I can delete blocks, but when they're inside a Cover or Media & Text block so I'm now wondering if it's block-specific.   Here's my root block (Cover) HTML:
Sorry, I ran out of time to debug further. Everything else tests as expected. 🚀 Probably best to do follow ups if it's block-specific? If so then this PR LGTM. |
| I kicked off the E2E tests, but I think they are related. At least this one is:
|
andrewserong left a comment
andrewserong left a comment There was a problem hiding this comment.
This is testing great for me, thanks for the thorough instructions!
I think it would make sense to be able to add new paragraphs anywhere where a paragraph already exists, but we haven't worked out a reliable way to do that yet.
I agree with this, I like the idea that if a paragraph exists, it's possible to add another one, and to give a little more flexibility to the content editing side of things. That said, given that the contentOnly behaviour is hidden behind an experiment, I think we could continue iterating on and trying out ideas here in subsequent PRs. And for the areas that aren't hidden behind an experiment, this appears to fix up some bugs and inconsistencies.
For now, I think this PR adds a good level of consistency and makes things feel solidly locked down, so this LGTM. But do feel free to seek another approving review if we're unsure of any of the particulars here!
| if ( | ||
| blockEditingMode === 'contentOnly' && | ||
| ! isContainerInsertableToInWriteMode( state, blockName, rootClientId ) | ||
| ( isParentSectionBlock || blockEditingMode === 'contentOnly' ) && |
There was a problem hiding this comment.
We could maybe look at removing it when we get to updating the zoom out logic (i.e. as part of #71807).
| await test.step( 'Blocks cannot be inserted before/after', async () => { | ||
| // Test block with overrides. | ||
| await editor.selectBlocks( blockWithOverrides ); | ||
| await editor.showBlockToolbar(); | ||
| | ||
| await expect( | ||
| page | ||
| .getByRole( 'toolbar', { name: 'Block tools' } ) | ||
| .getByRole( 'button', { name: 'Options' } ) | ||
| ).toBeHidden(); | ||
| | ||
| // Test block without overrides. | ||
| await editor.selectBlocks( blockWithoutOverrides ); | ||
| await editor.showBlockToolbar(); | ||
| | ||
| await expect( | ||
| page | ||
| .getByRole( 'toolbar', { name: 'Block tools' } ) | ||
| .getByRole( 'button', { name: 'Options' } ) | ||
| ).toBeHidden(); | ||
| | ||
| // Test first button. | ||
| await editor.selectBlocks( firstButton ); | ||
| await editor.showBlockToolbar(); | ||
| | ||
| await expect( | ||
| page | ||
| .getByRole( 'toolbar', { name: 'Block tools' } ) | ||
| .getByRole( 'button', { name: 'Options' } ) | ||
| ).toBeHidden(); | ||
| | ||
| // Test second button. | ||
| await editor.selectBlocks( secondButton ); | ||
| await editor.showBlockToolbar(); | ||
| | ||
| await expect( | ||
| page | ||
| .getByRole( 'toolbar', { name: 'Block tools' } ) | ||
| .getByRole( 'button', { name: 'Options' } ) | ||
| ).toBeHidden(); | ||
| } ); |
There was a problem hiding this comment.
Is this step redundant? It appears to use the same approach to check that insert before/after is not available as the duplication step, so thy could probably be combined? (I.e. in both cases, checking that Options is not available suffices.
| await test.step( 'Blocks cannot be inserted before/after', async () => { | ||
| // Test paragraph. | ||
| await editor.selectBlocks( paragraph ); | ||
| await editor.showBlockToolbar(); | ||
| | ||
| await expect( | ||
| page | ||
| .getByRole( 'toolbar', { name: 'Block tools' } ) | ||
| .getByRole( 'button', { name: 'Options' } ) | ||
| ).toBeHidden(); | ||
| | ||
| // Test first list item. | ||
| await editor.selectBlocks( firstListItem ); | ||
| await editor.showBlockToolbar(); | ||
| | ||
| await expect( | ||
| page | ||
| .getByRole( 'toolbar', { name: 'Block tools' } ) | ||
| .getByRole( 'button', { name: 'Options' } ) | ||
| ).toBeHidden(); | ||
| | ||
| // Test second list item. | ||
| await editor.selectBlocks( secondListItem ); | ||
| await editor.showBlockToolbar(); | ||
| | ||
| await expect( | ||
| page | ||
| .getByRole( 'toolbar', { name: 'Block tools' } ) | ||
| .getByRole( 'button', { name: 'Options' } ) | ||
| ).toBeHidden(); | ||
| } ); |
There was a problem hiding this comment.
Similar to the other test, is this step redundant? (It appears to test the same thing as the previous step)
There was a problem hiding this comment.
Oh, true. AI wrote the tests, and I didn't look over the results enough! The code was originally checking for the specific menu items, but then Claude realized the menu doesn't exist at all, so that's how it ended up like it.
There was a problem hiding this comment.
To be fair, the duplicate test is correct 😄
| state.settings.templateLock, | ||
| getBlockEditingMode( state, rootClientId ), | ||
| getSectionRootClientId( state ), | ||
| isSectionBlock( state, rootClientId ), |
There was a problem hiding this comment.
From looking at isSectionBlock it seems relatively simple in that it doesn't do too many lookups / calculations. But since it's being added to the list of dependencies just thought I'd double-check, do you think this has any potential performance concerns? My hunch is "no" 😄
There was a problem hiding this comment.
I think it's simple enough not to cause an issue. getParentSection is one I was concerned about though, it is used in the selector, but I haven't added it in the deps, so that might cause issues, though I didn't spot any.
| "label": { | ||
| "type": "string" | ||
| "type": "string", | ||
| "role": "content" |
There was a problem hiding this comment.
Just to mention that there's a problem in trunk that this partially solves.
In the navigation editor (in the site editor), the navigation block there is set to a contentOnly block editing mode, and it's also a content block, which means it can now only have other content blocks inserted within it.
In trunk it means you can't insert a 'Home Link' block at all because it's not a content block. This was caught by a test, so I'm fixing it here.
While I've fixed Home Link, there might be other blocks that are affected (third party blocks too). It might be a back compat break (not to mention an issue in 6.9). We might need to consider again how that editor or the contentOnly insertion thing works. Maybe some kind of exception is needed for the navigation block, or maybe we can use a different technique to make sure the Navigation block can't be removed.
cc @getdave @jeryj @scruffian to make you aware of this. I'll also make an issue.
There was a problem hiding this comment.
I've made an issue for this here - Navigation Editor: blocks without 'content' role can no longer be inserted/removed/moved
| Also retested. LGTM - great test coverage. As @andrewserong says, it makes things more consistent 👍🏻 |
…plicate or deleted
talldan force-pushed the fix/content-only-insertion-removal-and-moving branch from a6e5c38 to 63cec0b Compare github-project-automation bot moved this from 🔎 Needs Review to ✅ Done in WordPress 6.9 Editor Tasks 
github-actions bot removed the Backport to WP 6.9 Beta/RC 
* Fix confusion between clientId and rootClientId * Fix contentOnly insertion logic for blocks at root of section * Fix movers appearing in contentOnly mode * Rename selector that mentions write mode * Fix typo * Add e2e tests for synced patterns to ensure blocks cannot be moved duplicate or deleted * Update move behavior to disable movers for templateLock contentOnly * Add contentOnly templateLock tests for block duplication, insertion and moving * Add buttons example to pattern overrides test too * Fix selector memoization * Make the selectors work in a more similar fashion * Fix test * Allow moving for template lock of `insert` * Make home link a content block * Condense identical test cases ---- Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org>
github-actions bot added the Backported to WP Core | I just cherry-picked this PR to the wp/6.9 branch to get it included in the next release: 6163fc4 |
| Looks good, thanks for this. |
What?
I've noticed lately some cases where in contentOnly mode, certain block features are available that shouldn't be:
templateLock: "contentOnly"groups. I think the expectation right now is that this shouldn't be possible, but I might need confirmation on that.This PR should fix these issues, but still maintain the desired behavior from #71232
How?
Adjusts some logic in the relevant selectors. I've left some PR comments about the exact changes.
Testing Instructions
Synced patterns
Unsynced patterns
(test with the contentOnly unsynced patterns experiment active).
templateLock: contentOnly
Also worth testing
Screenshots or screencast
Content Only Template Lock
Before
After
Synced Patterns
Before
After
Unsynced Patterns
Before
After