Fix block edit toolbar appearing twice

[johngodley]

If you duplicate a block with the keyboard shortcut shift+cmd+d it duplicates the block toolbar:

This PR stops this happening, but doesn’t fix the underlying problem. I'm putting it here as an open question (I'm not really sure what is supposed to happen), along with some fixes.

As far as I can tell the problem is caused by the Slot/Fill system, where it’s possible for two same-named slots to be added, with the second overwriting the first. When either slot is removed it then removes both. This is only made apparent when using the keyboard shortcut as it changes the order in which slots are created and removed.

To make it easier to understand I'll use the duplicate block toolbar as an example. Note in this context, BlockActions is the slot used to show the block edit toolbar. I've added some debug to Gutenberg so it outputs the slot/fill actions to make it clearer what is happening.

I may have misunderstood some/all parts of this!

Duplicating a block using the block menu

1. Block is selected and BlockActions slot appears

SLOT: register
FILL: register

2. Duplicate is chosen from block menu
3. Block menu disappears, and slot is removed

SLOT: unregister
FILL: unregister

4. Block is duplicated
5. New block is auto-selected and a new BlockActions slot is created for it:

SLOT: register
FILL: register

6. Duplicated block appears with edit toolbar, as expected:

Duplicating a block using the keyboard shortcut shift+cmd+d

1. Block is selected and BlockActions slot appears

SLOT: register
FILL: register

2. Shortcut is triggered
3. Block is duplicated, existing BlockActions slot still exists
4. New block is auto-selected and another BlockControls slot is created, overwriting the existing BlockControls slot. The old one is still visible as it's not been updated yet (I am stepping through the code here to show each part)

SLOT: register
FILL: register

5. The new slot is updated and we see the original slot with its fill, and the new slot with the original fill and the new block fill:

6. The original block is no longer selected so it tries to remove itself. However, it actually removes the new `BlockControls` slot

SLOT: unregister
FILL: unregister

We are left in this state:

7. At some point something happens (a heartbeat maybe?) that triggers an update, and the slot returns to normal:

What is happening?

The above example is trying to show that when using the menu, the slot is created, removed, and then created again, and everything works as expected. When using the keyboard shortcut, the slot is created for the first block, created for the second block, and then the first removed. Because of the way the slot/fill system works the second slot overwrites the first, and when the first is removed, it's actually removing the second.

The fix

Back to this PR!

I'm not really sure how the slot/fill system is supposed to behave when two slots of the same name are added. This is the real problem, which I haven't attempted to fix. I note in https://github.com/WordPress/gutenberg/blob/master/packages/components/src/slot-fill/slot.js#L37 that an instance is being passed, even though it's not used, and I wonder if this indicates it could work like the fills (where multiple same named fills can exist)?

I have included two fixes here. Both of them fix the problem in different ways, and I wanted to get some feedback about the preferred route (which may be neither):

• BlockActions has been modified to clear the block selection when the duplicate action is triggered. This forces it to behave the same regardless of if from the menu or keyboard. This only fixes the block edit toolbar and shouldnt affect anything else
• SlotFillProvider has been modified to forceUpdateSlot before deleting a slot. This ensures the slot is cleared first, and stops the duplicate. This fixes the display, but not what happens underneath. I don't know if it could affect other things.

There's also a bunch of unit tests, of which one tests the duplicate slot problem and will always fail. I've included it here to make it easier to test, but it would need to be fixed properly or removed.

Fixes #12355

[johngodley]

@aduth I think you may know about this - appreciate any insight

[aduth]

an instance is being passed, even though it's not used

It's indirect, but it is used by the Fill, in creating its portal to the slot instance's node:

gutenberg/packages/components/src/slot-fill/fill.js

Line 63 in a617aee

const slot = getSlot( name );

gutenberg/packages/components/src/slot-fill/fill.js

Line 74 in a617aee

return createPortal( children, slot.node );

[aduth]

I'm not really sure how the slot/fill system is supposed to behave when two slots of the same name are added.

Two opposing thoughts come to mind:

• Fully support multiple slots, where the same fills are shown / replicated across each instance.
  • This is made challenging by the implementation of the Fill to render into its own slot via createPortal, assuming a single slot node.
• Check whether another instance already exists and force it to destroy itself.

[aduth]

6. The original block is no longer selected so it tries to remove itself. However, it actually removes the new BlockControls slot

I think there's actually a quick fix for this. We pass the instance to unregisterSlot, so before it deletes the slot, check that the instance for the slot matches its current tracked value.

This actually seems to solve the overall problem for me, but needs more testing. I'll try to put together some proposal.

[aduth]

I think we shouldn't need to explicitly clear here. What's causing the new blocks to become selected? Is it insertBlocks ? Thinking the editor's reducer for tracking selection should be responding to the insertion, rather than needing to be explicit with clearing the block here.

[aduth]

I'll try to put together some proposal.

See #12882