Allow adding "content" role blocks to containers that also have a "content" role in write mode#71232
Conversation
|
Size Change: +152 B (+0.01%) Total Size: 1.92 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d011468. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17286666735
|
|
I think Quote should allow some interaction in Write Mode. Updating the citation to the quote is an example. Updating the background of cover possibly too. I think the root cause of the bug is the same as #67408, while a parent block can be set to the There was a PR that tried to fix the issue - Fix: Block Movers appear when content block is immediate child of a section., but it caused a bug with the 'Show Template' option when editing posts/pages. I haven't checked the latest changes in that PR. |
|
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. |
That PR doesn't seem to do anything. It's only checking |
I personally don't think there's a way to solve the problem with the APIs currently available. 🤔 |
2a80f6c to
6baba67
Compare
|
I've updated this to try something else that might allow us to support inserting new block in List and Gallery and such: checking whether the block is a content block and whether it has an I'm not sure whether this is the best approach for supporting this type of blocks. I added a content role for Gallery in a place where I think it makes sense to have one (the caption) but there doesn't seem to be any good place to add one to Buttons 😄 It seems like we're very much making up the rules here as we go along, and it would be good to have a more systematic approach but because there are so many edge cases it's hard to do so. For instance, Quote might be a good candidate for allowing to add more paragraphs, but it has become a generic container block and it doesn't seem a good idea to allow it to have all sorts of blocks added to it in write mode. Anyway. Feedback much appreciated! |
| const isContentOnlyNonRootBlock = | ||
| getBlockEditingMode( state, rootClientId ?? '' ) === 'contentOnly' && | ||
| !! rootClientId; | ||
| if ( isContentOnlyNonRootBlock && ! isContentContainer ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Maybe we should convert this into a single getBlockEditingMode call, above L1697.
|
Thanks for the ping, @tellthemachines! Do we know if this is a recent regression? Can we add e2e test coverage? Just a general note of caution: We should be careful when adding more logic to |
I don't know! I just came across it while auditing blocks for #65778 Yes I'll add some tests! Also looks like the current e2e failure might be legit, I'll look into that too. |
fd39c6d to
bfbba21
Compare
|
So the e2e issue turned out to be due to one of the conditions I inserted to make nested containers uneditable catching the I'm not sure the solution I came up with is the best, but I think zoom out is only supposed to work within the Edit: looks like the unit tests are failing now, so there must be something wrong with this change too 😅 |
| // The "main" tag block is used by zoom out mode so it should be possible to insert | ||
| // blocks inside it. | ||
| const isRootBlockMain = | ||
| getBlock( state, rootClientId )?.attributes?.tagName === 'main'; |
There was a problem hiding this comment.
Would getSectionRootClientId() be useful here? It should return whatever we consider to be the top level content section that patterns get inserted into for Zoom Out
There was a problem hiding this comment.
Yes, thanks, that worked! And it fixed the unit tests too 😊
| // Check client id of selected block and see whether it matches paragraphClientId | ||
| const getSelectedBlock = async () => | ||
| await page.evaluate( () => | ||
| window.wp.data | ||
| .select( 'core/block-editor' ) | ||
| .getSelectedBlockClientId() | ||
| ); | ||
|
|
||
| expect( await getSelectedBlock() ).toEqual( paragraphClientId ); |
There was a problem hiding this comment.
I understand this is from existing tests, but I think it's better to test user-visible behavior when possible. Besides the block editor and us devs one interacts with clientIds 😄
await expect( paragraph ).toBeFocused();
// Focused is probably the same as checking the selected class.
// await expect( paragraph ).toHaveClass( /is-selected/ );
await expect( paragraph ).toHaveText( 'Something' );There was a problem hiding this comment.
That's fair 😄 I'll update it!
There was a problem hiding this comment.
It's not a blocker. I also don't mind updating the whole test suite in a follow-up.
|
Thanks for the reviews so far folks! I improved the logic a bit to be more efficient with the selectors, and added some e2e tests. Currently, this PR addresses:
I'm updating the description to reflect what it does. |
|
This tests well for me, apart from one bug that I found. With a list, you can indent list items, but it seems you can't unindent. We might need similar logic for removing/replacing blocks (I think the way it works is that it replaces a List block with a List Item block). I think I already mentioned somewhere about the Content panel not being ideal for showing the nesting of list blocks (screenshot is for a list that is indented twice): Not sure if there are any small changes we can make to improve that, maybe showing only the top level list. It can also be a separate exploration given the feature is still experimental. I think it's already a bug to some extent - e.g. if the list was added in normal mode and then the user switched to write mode. I wonder if the choice to make this work around allowedBlocks will scale well to every block. Have we checked other blocks types that use |
Actually I think the PR doesn't allow removing list items or nested lists at all, but maybe that's the expected behavior and part of the iteration. |
I'm not sure if it is 😅 I can remove list items by deleting the text, but there's no "delete" button in the Options which maybe there should be? I'm happy to iterate on this until it feels more usable, but we need to be clear on what behaviour we actually want here. Personally, I think it might work well to allow all types of create/delete actions inside the list block, but open to other thoughts! |
In this PR we're currently checking both whether the block has |
I think I see what's happening. I can delete top level list item blocks using the keyboard, but not the indented ones (even when it's only deleting the item and not the entire list). Maybe it's a similar situation to unindenting
My take on it is that if you can delete using the keyboard the option should be there too. |
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
f88c6c9 to
9843a44
Compare
|
Thanks everyone for the feedback on this PR!
If the tests still pass after the rebase I'll merge it. Given the whole feature is behind an experiment flag, it's very unlikely to break anything 🙈 |
content role from showing inserter in write mode|
Nice work landing this one! I've just updated the PR title to better match the final state of the PR, hope you don't mind (feel free to change it back / update it again of course). |
|
Thanks Andy! That's a better description. |
|
Thanks for working on this. I agree that we should rely on the content role as the guiding principle. Hopefully this API will be sufficient as a proxy for "this is a block that should be edited in write mode" (although it also has utility beyond that mode). I'll look to rebase the Nav block in Write mode PRs to pull in this change which should help with those efforts. |
What?
Addresses part of #71202.
This PR adds some logic to
canInsertBlockTypeUnmemoizedandcanRemoveBlockto:It also updates block actions to make the options to remove, duplicate, add before, etc appear more consistently (or not appear).
Also, in order to get Gallery to work with the above rules, I added a "content" role to its caption attribute (which I think makes sense to have anyway).
This doesn't address the Buttons block, which doesn't have any "content" role in its attributes. Should it?
Should the existence of a "content" role be the guiding principle here? I think it makes sense because usually the blocks we'll want to edit in write mode are content blocks. I also like the idea of using an existing, semantically appropriate, attribute to make this work, instead of adding a new one, because it will likely provide better out of the box support for third party blocks.
This PR also doesn't address the issue of any blocks being insertable at root level (whether that be the template root or the 'main' element). That problem is complicated by the close relationship between write mode and zoom out (where it is desirable to be able to insert some blocks at root level).
Testing Instructions