Close block settings menu after removing block

[swissspidy]

Description

Fixes #15458 by closing the block settings menu after removing a block through that menu.
Also fixes #15313.

This prevents editor crashes when there are no more blocks left but the block settings menu is still rendered as it's open.

How has this been tested?

Following the steps outlined in #15458 I did this:

1. Enabled fixed toolbar
2. Create block
3. Click on "More options" to open block settings menu
4. Remove block
5. Verify that the menu is closed and no errors are thrown in the console and the editor is also still functioning.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[talldan]

Thanks for the work on this ❤️.

I thought I'd review as I'd seen this bug mentioned in a design triage, but then I couldn't find the comments related to the triage on the original issue. It turns out there's a separate issue from the accessibility audit - #15313

I know it broadens the scope a bit, but it'd be great to address this for each of the menu items, as mentioned on that issue.

[gziolo]

I know it broadens the scope a bit, but it'd be great to address this for each of the menu items, as mentioned on that issue.

I don't think it should block this PR.

[swissspidy]

I know it broadens the scope a bit, but it'd be great to address this for each of the menu items, as mentioned on that issue.

Since it was a rather easy fix, I've made that change now as well.

The only case where the settings menu is not closed is when the action (i.e. "Insert After") is performed via a keyboard shortcut.

[afercia]

Quickly tested and seems to me there's an issue with the Undo/Redo history. To reproduce:

• add a paragraph block with some text
• remove it via the More menu in the top bar
• click Undo
• at this point, the default block appears: expected the paragraph should be restored
• regardless, the "More" button is rendered in the top bar

• click the "More" button: the editor crashes displaying the "Attempt Recovery" dialog

Repeat the steps above, but this time click Undo twice:

• the deleted paragraph is restored
• the "More" button is rendered in the top bar
• same result: click the "More" button and the editor crashes

[swissspidy]

Thanks for testing, @afercia! I can reproduce this issue.

click the "More" button and the editor crashes

I think the issue here is that wp.data.select('core/block-editor').getSelectedBlockClientIds() still returns the previously selected block, even though it has been deleted.

So getSelectedBlockClientIds() returns a block, getBlockOrder() returns an empty array, and getBlock( getSelectedBlockClientIds() ) returns null.

[swissspidy]

ea26477 should fix the issue of the More menu being shown in that case, while not re-introducing the side effect originally mentioned in #15543 (comment).

As for the undo/redo history, the issue seems kinda logical.

For comparison, here's how it works on a fresh post:

1. Click on "Add New" to create a new post. You should see the "Start typing..." placeholder of the default block.
2. Click on placeholder.
3. Notice that placeholder text is removed and that the "undo" button is enabled, meaning that you can undo that action.

With this PR here, the following happens:

1. You write something, e.g. "Hello World"
2. You remove the block via the More menu
3. The block gets removed and he default block gets added [one undo action added]
4. The default block gets focused, i.e. the placeholder is removed [second undo action added]

It works correctly when there are multiple paragraph blocks:

1. Write multiple paragraph blocks
2. Remove the last block via the More menu
3. The previous block gets focused
4. Click undo once to restore the deleted block

---

tl;dr the undo/history works as expected, at least from a technical perspective.

However, as a user it indeed seems odd that it would require two undo actions to get back the previous block. I cannot think of a good solution for that off the top of my head. But it should probably be looked at in a separate PR.

[afercia]

However, as a user it indeed seems odd that it would require two undo actions to get back the previous block. I cannot think of a good solution for that off the top of my head. But it should probably be looked at in a separate PR.

Yep this was also previously noted for a similar issue when pressing Enter at the beginning of a block. Will create a separate issue with details.

Edit: created #15610

[swissspidy]

Awesome, thanks!

@talldan Would you mind reviewing this again?

[aduth]

I know it broadens the scope a bit, but it'd be great to address this for each of the menu items, as mentioned on that issue.

This was also discussed in #14754 . In fact, perhaps not here, but I suggest we'd go one step further and handle onClose within the implementation of MenuItem, so as not to make it the responsibility of the implementer to manage (error-prone). The effort in #14843 was partly intended as a step in this direction.

From #14754 (comment) :

Part of the problem is also that selecting the menu items does not close the menu. I'm unsure whether it was done so intentionally, but my reading of the recommendations is that the menu should be closed.

I wonder also if it's something we can improve in the abstractions and relationships between Dropdown and MenuItem components. There's also DropdownMenu which... is neither used by the block settings menu, nor itself uses the MenuItem component. At least in how it is implemented, it may serve as a suitable replacement for the block settings menu's use of the more generic Dropdown component, with the auto-close behavior built in.

[nerrad]

Looks okay, I haven't tested and I have a few comments that could be addressed.

[westonruter]

Also fixes the problem for me. Before I would get an error thrown when removing the initial block. With this PR, no error is raised.

[kienstra]

Works As Expected

This works here, and the menu doesn't appear anymore when deleting the only block.

Before

After

[aduth]

click the "More" button and the editor crashes

I think the issue here is that wp.data.select('core/block-editor').getSelectedBlockClientIds() still returns the previously selected block, even though it has been deleted.

Related: #12327, #12002

It's curious to me that an isValid condition makes any meaningful difference. Seems like an incidental workaround.

However, the way it is solved should be further improved in a more global fashion (perhaps as part of #14843) as other components might also benefit from this. Is that accurate?

Yes. Would you mind to create an issue to track this?

[aduth]

It's curious to me that an isValid condition makes any meaningful difference. Seems like an incidental workaround.

Aside: If I understand correctly, this "fix" is not related to the original scope of the pull request. It would have simplified review if it had been proposed as its own pull request.

[aduth]

I think the issue here is that wp.data.select('core/block-editor').getSelectedBlockClientIds() still returns the previously selected block, even though it has been deleted.

So getSelectedBlockClientIds() returns a block, getBlockOrder() returns an empty array, and getBlock( getSelectedBlockClientIds() ) returns null.

In my testing, the error occurs here:

gutenberg/packages/edit-post/src/components/block-settings-menu/plugin-block-settings-menu-group.js

Line 15 in 8960509

selectedBlocks = map( selectedBlocks, ( block ) => block.name );

Because getBlocksByClientId generates an array of [ null ] since, as you mention, the selected blocks remain in state despite having since been deleted:

gutenberg/packages/edit-post/src/components/block-settings-menu/plugin-block-settings-menu-group.js

Line 29 in 8960509

selectedBlocks: select( 'core/block-editor' ).getBlocksByClientId( clientIds ),

Part of this comes down to: We don't prescribe an expected behavior of getBlocksByClientId when it's provided an array of client IDs which don't exist as blocks. Per #12327, the specific circumstances under which this invalid array of client IDs exists is not intended, but the underlying question of "What should getBlocksByClientId return?" is valid all the same. As an alternative approach, I think the error could be resolved if it only included members for valid client IDs. However, there are downsides of (a) no guarantee that getBlocksByClientId( clientIds ).length === clientIds.length and (b) we set expectations in similar singular selectors like getBlock to return a null value for an unknown block, so a similar expectation may exist here. If we were to say that the result of getBlocksByClientId can be expected to include null values, then that should be anticipated in any use of the array to guard against property access on a null value (i.e. at the error'ing line above).

The proposed condition of isValid works only because the isBlockValid selector considers whether a block exists (the !! block):

gutenberg/packages/block-editor/src/store/selectors.js

Lines 119 to 122 in 8960509

	export function isBlockValid( state, clientId ) {
	const block = state.blocks.byClientId[ clientId ];
	return !! block && block.isValid;
	}

This is a pretty loose guarantee, though. For example, hypothetically I imagine an error could still occur if a multi-selection of blocks included a client ID which did not exist, in the rendering of BlockSettingsMenu here:

gutenberg/packages/block-editor/src/components/block-toolbar/index.js

Lines 20 to 27 in 8960509

	if ( blockClientIds.length > 1 ) {
	return (
	<div className="editor-block-toolbar block-editor-block-toolbar">
	<MultiBlocksSwitcher />
	<BlockSettingsMenu clientIds={ blockClientIds } />
	</div>
	);
	}

[swissspidy]

Aside: If I understand correctly, this "fix" is not related to the original scope of the pull request. It would have simplified review if it had been proposed as its own pull request.

I disagree about the "not related to the original scope" part. This PR was intended to fix #15458 (editor crash) and coincidentally also fixes #15313. Without this fix the PR would be incomplete as the editor crashes still in the following scenario:

1. Turn on fixed block toolbar setting
2. Add Columns block.
3. Click on appender to create paragraph block within Columns block
4. In the block toolbar, click on three dots and then "Remove block"

You'll get this:

1. Now, click on three dots
2. Editor crashes
3. Attempt recovery
4. Click on three dots again
5. Editor crashes

---

Thanks for the detailed breakdown after that. Is this something we can address in a separate issue/PR or would you prefer it to be handled here? Since you said:

Yes. Would you mind to create an issue to track this?

I'm asking because I want to get the workflows right here in this project. At the same time, I'd also love to see this fixed rather sooner than later, as this error occurs frequently in our user testing 🙂

[aduth]

The changes here include a different behavior that I don't believe should be expected: In master, the block "More Options" button is still available in the block toolbar even if the block is invalid. That is no longer the case in the current state of this branch.

I'm not sure it helps with the issues for "Undo", but I've discovered the underlying cause to the original issues reported in #15458:

1. When a block is removed, we try to select the previous block, but we don't check that a previous block actually exists, so we end up calling selectBlock( null ).
2. The selectBlock reducer handler will happily accept an invalid clientId (null in this instance, resulting in getSelectedBlockClientIds() being [ null ] rather than []). We might not need to worry too much about this if the first point is addressed, but it wouldn't hurt to have some sanity checks here to at minimum accept only strings or at most accept only valid client IDs.

selectBlock( null ) could make sense as an expression of "clear the currently selected block", but that's not how the reducer is implemented to interpret it. It's the clearSelectedBlock action which serves this purpose.

In 2f1a82f, I add a condition to only select the previous block if one exists. Could you check to see if the isValid check is still necessary at this point?

I have some local changes to include an end-to-end test to verify against this regression, but I see another (unrelated) error which occurs here, which we may want to address separately instead. It is an error caused by ensureDefaultBlock when the default block type is unregistered.

To the question of process: I understand the urgency of having the user-facing issue resolved. My concern is that urgency is lost as soon as the symptom is treated. If the underlying issue is left to remain, it will inevitably manifest itself again in the future (or in the present by some yet-to-be-discovered circumstances).

[swissspidy]

Thanks a ton @aduth! I can confirm that with this change the isValid check is no longer necessary. That means the "More Options" button continues to be available in the block toolbar even if the block is invalid.

[aduth]

Thanks for bearing with me. Looks good to me.

Per my previous comment, I'll follow-up with an end-to-end test case which includes a fix for the unrelated issue with ensureDefaultBlock.

[aduth]

Per my previous comment, I'll follow-up with an end-to-end test case which includes a fix for the unrelated issue with ensureDefaultBlock.

Pull request at #15786 (currently cherry-picks the commit from this branch).