Quote & List v2: Deleting empty list item should list block

[ellatrix]

What?

Fixes #40979.

Why?

It's not possible to delete an empty quote or list block in one go.

How?

A generalised solution: When deleting all inner blocks, check if the block is otherwise empty (unmodified) and delete the whole block if so.

Testing Instructions

Create a quote block with a paragraph. Delete the paragraph through the UI. The quote block should be deleted too.

Screenshots or screencast

[github-actions]

Size Change: +97 B (0%)

Total Size: 7.71 MB

📦 View Changed

Filename	Size	Change
build/scripts/block-editor/index.min.js	458 kB	+97 B (+0.02%)

_{compressed-size-action}

[ntsekouras]

Thanks for the PR Ella! Interesting stuff here and the API changes should be highlighted to the title/description of the PR.

I'll have to process it more, but I'm quite conflicted on this one..

I know when we remove all innerBlocks(ex. Group) is not in a great state right now, but I think most of the discussions around that were about improving that state/design. Of course this doesn't mean we shouldn't consider the new flag. I think what felt weird to me is when I removed the last block from the toolbar actions and removed everything..

Also in order for this to take effect in RichText blocks we need to have an onRemove property there too and that can make some experience not feel consistent? 🤔 .

In my video I added the flag in Group(I guess I could use Quote now too 😄 ) and doing what I'm describing above. For example Site Title doesn't have an onRemove.

Screen.Recording.2022-07-20.at.6.06.04.PM.mov

As I said I need to process this more to form a better opinion and would love to hear what others think.

[ellatrix]

I know when we remove all innerBlocks(ex. Group) is not in a great state right now, but I think most of the discussions around that were about improving that state/design.

Is this an argument against this change? I'm not sure I'm following. :)

I think what felt weird to me is when I removed the last block from the toolbar actions and removed everything..

Why does it feel weird? If you remove the last list item, you also remove the list. Is that unexpected?

Also in order for this to take effect in RichText blocks we need to have an onRemove property there too and that can make some experience not feel consistent? 🤔 .

That's just for removing the block with Backspace, which is independent from removing the wrapper block when all inner blocks have been removed.

In my video I added the flag in Group(I guess I could use Quote now too 😄 ) and doing what I'm describing above. For example Site Title doesn't have an onRemove.

Well, then it's also not possible to remove Site Title anywhere else like that? This is a problem with Site Title, not the group block. Just try Site Title outside of a group block and you have the same problem.

[ntsekouras]

Is this an argument against this change? I'm not sure I'm following. :)

I'm not against it's just this behavior was mostly discussed about Group block and I have the feeling that folks were leaning on improving the design when we have no blocks. This doesn't mean we can't have this new API.

Why does it feel weird? If you remove the last list item, you also remove the list. Is that unexpected?

Maybe a bit, but that's subjective and no strong opinion on this one 😄

I'll re-test the PR.

[ellatrix]

It's what we're doing now for the List v1. When you delete the only list item, it removes the block. Same with Quote v1.
Doing this for List and Quote doesn't mean all blocks should do this. Group can remain as it is. That's why it's an option. :)

[ntsekouras]

The only thing with this approach that we need to consider is the case where a block has other things besides innerBlocks. The example here would be the Quote block when we have set a citation. If we remove the last innerBlock it would still remove the whole block.

Would that be expected or okay? If yes, it should be documented somewhere what we consider empty in comments.

[ellatrix]

@ntsekouras Let's put this on hold. There may indeed be better ways to do this for keyboard. But I suspect this PR might still be necessary to handle #40979. In any case, this is lower priority then. I'm making an alternative PR for keyboard.

[t-hamano]

This PR may also resolve #45919.
Please see the comment here.

[github-actions]

Flaky tests detected in cba9be1.
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/6455956429
📝 Reported issues:

• [Flaky Test] dims background image down by 50% with the average image color when an image is uploaded #55086 in /test/e2e/specs/editor/blocks/cover.spec.js

[jorgefilipecosta]

I think we should give it a try to merge this PR. It solves so many issues e.g: button block (removing the last button should remove the buttons block itself, list, media & text). In the default blocks core support, this behavior seems to be positive in the user experience. I think for most third-party blocks this behavior is also positive, I can not think easily of a case where a block has inner blocks and if we don't change anything on the block and remove the inner blocks we want the block to still be there. But a case we are not considering may exist, I guess adding a dev note for this change could be useful to raise awareness on third-party block developers, I will leave the decision on adding the needs dev note label to @ellatrix.

The change itself looks good and worked well on my tests 👍

[fabiankaegy]

Hey all 👋

This ticket has been quiet for a while. We are coming up on the WordPress 6.5 Beta 1 (Feb 13th.) Currently this ticket is listed in the needs review column of the 6.5 task list. Are we still aiming for this? Whats blocking this currently?

[github-actions]

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: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>
Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org>
Co-authored-by: annezazu <annezazu@git.wordpress.org>

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

[colorful-tones]

Based on a recent Editor Triage async session - I'm moving this to the Punted to 6.6 column on the WordPress 6.5 Editor Tasks board.

[colorful-tones]

FWIW - I just pulled this down and rebased with trunk.

I then tested and obtained the same results. Perhaps this is already fixed and can be closed as it is no longer an issue?

WP 6.6 Beta-2 + Gutenberg 18.5

deletion-sequence-testing-latest.mp4

WP 6.6 Beta-2 + this PR rebased

deletion-sequence-testing-42503.mp4

[t-hamano]

Perhaps this is already fixed and can be closed as it is no longer an issue?

Regarding keyboard actions, they work fine in trunk, but the issue persists when removing blocks from the block toolbar. I think this PR will fix that remaining issue.

After rebasing this PR, I think it would be ok to backport it to WP6.6, but what do you think?

trunk

When the only block in a List block or Quote block is deleted from the block toolbar, an empty block with no inner blocks remains.

trunk.mp4

This PR

The issue in trunk will be resolved.

Note: This PR depends on rootClientId, but the rootClientId variable was deleted in #62547, so an undefined variable error occurs. Therefore, in order to properly test this PR, you need to rebase this PR with the latest trunk and then apply the following changes:

diff --git a/packages/block-editor/src/store/private-actions.js b/packages/block-editor/src/store/private-actions.js
index fc2a28a75f..27b276f1e3 100644
--- a/packages/block-editor/src/store/private-actions.js
+++ b/packages/block-editor/src/store/private-actions.js
@@ -154,6 +154,7 @@ export const privateRemoveBlocks =
                        }
                }
 
+               const rootClientId = select.getBlockRootClientId( clientIds[ 0 ] );
                const blockOrder = select.getBlockOrder( rootClientId );
                const removeRoot =
                        blockOrder.length === clientIds.length &&

pr.mp4

[ellatrix]

I think @ntsekouras had a good point about "what if there's other content besides inner blocks".
Maybe we should just disable the remove button, if it's required by the template to have at least 1?

[t-hamano]

if it's required by the template to have at least 1?

How can we determine that a block needs at least one inner block? Does it require any new block support 🤔

[ellatrix]

We can check the inner block template

[ellatrix]

Making a decision.

[jeryj]

To be consistent, should deleting the last image of a gallery block remove the entire gallery block? Right now it reverts back to the gallery placeholder.

[t-hamano]

Right now it reverts back to the gallery placeholder.

I believe this is because the linTo attribute is updated when the block is mounted.

gutenberg/packages/block-library/src/gallery/edit.js

Lines 593 to 603 in ec35342

	useEffect( () => {
	// linkTo attribute must be saved so blocks don't break when changing image_default_link_type in options.php.
	if ( ! linkTo ) {
	__unstableMarkNextChangeAsNotPersistent();
	setAttributes( {
	linkTo:
	window?.wp?.media?.view?.settings?.defaultProps?.link ||
	LINK_DESTINATION_NONE,
	} );
	}
	}, [ linkTo ] );

We should be able to resolve this by adding 'content' as the second argument to isUnmodifiedBlock.

gutenberg/packages/block-editor/src/store/private-actions.js

Lines 186 to 188 in ec35342

	isUnmodifiedBlock(
	select.__unstableGetBlockWithoutInnerBlocks( rootClientId )
	);

However, this alone does not solve all the problems. For example, even if you delete the last child block of a group block, the group block itself is not deleted. Perhaps we need to check not only for modified blocks but also for modified block variations.