Size Change: +152 B (+0.01%)

Total Size: 1.92 MB

_{compressed-size-action}

Flaky tests detected in d011468.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17286666735
📝 Reported issues:

• [Flaky Test] Redirection after template creation #60678 in /test/e2e/specs/site-editor/site-editor-url-navigation.spec.js
• [Flaky Test] correctly applies dynamic allowed blocks restrictions #51220 in /test/e2e/specs/editor/plugins/inner-blocks-allowed-blocks.spec.js

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 contentOnly block editing mode, there's no way to then control the block list of that block and prevent insertion, moving, or removal of blocks.

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: jeryj <jeryj@git.wordpress.org>
Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
Co-authored-by: talldan <talldanwp@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

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.

That PR doesn't seem to do anything. It's only checking templateLock so it won't apply to any block without a templateLock.

That PR doesn't seem to do anything. It's only checking templateLock so it won't apply to any block without a templateLock.

I personally don't think there's a way to solve the problem with the APIs currently available. 🤔

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 allowedBlocks list.

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!

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 canInsertBlockTypeUnmemoized. It's been known for similar selectors to affect performance, since they might be called a lot.

Do we know if this is a recent regression? Can we add e2e test coverage?

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.

So the e2e issue turned out to be due to one of the conditions I inserted to make nested containers uneditable catching the main element in zoom out mode, so that it became impossible to add blocks to it.

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 main tag, so I added a check for whether the block we're trying to insert into is main or not.

Edit: looks like the unit tests are failing now, so there must be something wrong with this change too 😅

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:

• Removing ability to add new blocks inside Quote, Details and Cover blocks in write mode;
• Adding ability to add new items in List and Gallery blocks in write mode.

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 allowedBlocks to see how they work? Do we need to also check that the allowed block types are content blocks or is that already covered somehow?

With a list, you can indent list items, but it seems you can't unindent. We might need similar logic for removing/replacing blocks

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.

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?
The indented list question is interesting, because indented lists are essentially a new list block. If I'm not allowed to delete most nested blocks in write mode, should I be able to delete the indented list? Likewise if I delete all the list items, should the list be removed?

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!

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 allowedBlocks to see how they work? Do we need to also check that the allowed block types are content blocks or is that already covered somehow?

In this PR we're currently checking both whether the block has allowedBlocks and is a content role block. This is mainly to exclude the Navigation block 😄 but it also excludes Buttons for now. I guess we can add content roles here and there where we think they're needed to enable the functionality. I like using the content role because it translates well to what write mode should be. But there will probably be exceptions to whatever rules we come up with 😅

I can remove list items by deleting the text

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

there's no "delete" button in the Options which maybe there should be?

My take on it is that if you can delete using the keyboard the option should be there too.

Thanks everyone for the feedback on this PR!

Since this is such a significant improvement, I think it'd be great to get in in its current shape! I know it's a complex area, though, so do feel free to wait for another approving review if you'd like.

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 🙈

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.