Move the block settings menu to the block toolbar

[youknowriad]

Extracted from #9282 and related to #9074 #9075

In an effort to split and streamline the efforts to refactor the toolbar, this PR moves the block settings menu to the block's toolbar.

Follow-up PRs will implement collapsing toolbars for the block toolbar.

Testing instructions

• Check the block settings menu is showing up properly in all circumstances.

[mtias]

Looking good, but it seems it's missing from multi-selection.

[youknowriad]

Good catch @mtias it should be fixed now.

[mtias]

Great. One more thing is that before we were focusing the ellipsis menu after a multi-selection. We should probably focus on the toolbar now.

[jorgefilipecosta]

The code changes are looking good 👍 From the functionality point of view I did a set of tests and I think may have found a bug.
I think we are not showing the ellipsis menu for invalid blocks.
Before:

After:

[ZebulanStanphill]

I am seeing some styling issues in Firefox which do not occur in Chromium:

EDIT: It seems to have something to do with some CSS setting line-height to 1.8.

[youknowriad]

In my testing, this div was not necessary anymore. Can you confirm @aduth?

I removed it from here and from the slots because it messes with the styling of the block toolbars.

[youknowriad]

@ZebulanStanphill I fixed the styling issue

Great. One more thing is that before we were focusing the ellipsis menu after a multi-selection. We should probably focus on the toolbar now.

@mtias I'm not certain where this behavior was triggered? Do you know where it was introduced?

[jasmussen]

This is too cool.

Not critical, but can we control the default direction the popout opens in?

Right now it seems to default to opening down and left:

It would be nice if it could open down and right instead — until there's no space to the right of course.

[youknowriad]

Ok I restored the "Focus toolbar on Multiselection" but I'm focusing the first tabbable in the toolbar instead of the toolbar container for now. I can make the toolbar container a tabbable element but I wasn't sure about the a11y impacts.

I also updated the dropdown to open to the right by default.

[youknowriad]

@jorgefilipecosta I wonder if it's better to keep this way. It suggests the user has to make a decision before going forward with the block.

[ZebulanStanphill]

I am noticing a styling issue in the block inspector:

Not sure if it is related or if it was introduced by some other PR, though.

[youknowriad]

Also, the e2e tests caught another issue here where the block keyboard shortcuts were not working because the block settings menu is not always rendered.

To fix it, I refactored how those keywords work consolidating all the keyboard shortcuts in the EditorGlobalKeyboardShortcuts (I think this component needs a small refactoring but I left it for another PR).

By doing so and to avoid duplication, I created a new component BlockActions using render props that take clientIds as a prop and provide onDuplicate, onRemove... for these ids. This component is used both for the global editor shortcuts and for the block settings menu.

I think it's an improvement over the current code but would be great if @talldan can do a sanity check as I believe you worked on the initial implementation.

[youknowriad]

@ZebulanStanphill Good catch, it's fixed.

[ZebulanStanphill]

Summary of failing tests:

Summary of all failing tests
FAIL test/e2e/specs/links.test.js (18.239s)
  ● Links › can be edited
    No node found for selector: button[aria-label="Edit"]
      at assert (node_modules/puppeteer/lib/helper.js:282:11)
      at Frame.click (node_modules/puppeteer/lib/FrameManager.js:590:5)
  ● Links › can be removed
    No node found for selector: button[aria-label="Unlink"]
      at assert (node_modules/puppeteer/lib/helper.js:282:11)
      at Frame.click (node_modules/puppeteer/lib/FrameManager.js:590:5)
Test Suites: 1 failed, 29 passed, 30 total
Tests:       2 failed, 3 skipped, 105 passed, 110 total
Snapshots:   48 passed, 48 total
Time:        206.473s
Ran all test suites.

https://github.com/WordPress/gutenberg/blob/master/test/e2e/specs/links.test.js

[jorgefilipecosta]

@jorgefilipecosta I wonder if it's better to keep this way. It suggests the user has to make a decision before going forward with the block.

The most useful features of the settings menu for invalid blocks is the ability to remove the block and to go to the HTML edit mode so we can apply a simple fix to the block. But I guess that if we have another settings menu for this blocks maybe this actions should be available there and it is ok to remove the other settings menu from the toolbar. We can add this actions there after the merge of this PR.

[karmatosed]

This could be my install but running this I do see some funky stuff:

Beyond that I had a super weird moment of cognitive breaking as I have learnt 'ellipsis to the right'. I suspect for those existing users this could be a friction, but I do think it's worth taking that hit to improve for everyone going forwards.

[aduth]

This could be my install but running this I do see some funky stuff:

Common pitfall with falsy (not false) conditions:

https://codepen.io/aduth/pen/dWwzrL

[karmatosed]

Ah, thanks for context @aduth TIL!

[ZebulanStanphill]

@jorgefilipecosta I think that showing the Remove Block and Edit as HTML buttons in the invalid block UI (probably within its own existing ellipsis menu) is a good idea. No need to make the ellipsis visible for invalid blocks if you do that.

@karmatosed

Beyond that I had a super weird moment of cognitive breaking as I have learnt 'ellipsis to the right'. I suspect for those existing users this could be a friction, but I do think it's worth taking that hit to improve for everyone going forwards.

Yeah, that keeps happening to me, too. I keep hovering over the right side of blocks waiting for the ellipsis to appear and then remembering that it is in the toolbar. 😛

One thing I just noticed is that there is currently no way to access the ellipsis menu for a Classic block:

(The extra buttons in the toolbar are from Divi; they are not relevant to this issue.)

[youknowriad]

@jorgefilipecosta @ZebulanStanphill I restored the menu for invalid blocks. We can change later if needed.

[youknowriad]

@karmatosed The error should be fixed. Thanks for catching it.

[youknowriad]

I restored the button for freeform block. It might not be the best design-wise but I think it's important to have it and at least it's consistent with the other blocks design wise.

[jasmussen]

I pushed a fix for the ellipsis button on the classic toolbar. It's not perfect, but it's a great deal better. Because if it sits above the classic blocks toolbar, then it will overlap the toolbar when the sticky positioning kicks in.

[jasmussen]

I will make a note to revisit the classic block separately to polish the responsiveness a bit. But I don't think that should stop this change.

[youknowriad]

🎉

[talldan]

@youknowriad Sorry for the delay, looks like a good change. I had something very similar at one point during the initial development of those shortcuts. It's good they're no longer dependent on rendering the menu.

[ktmn]

this PR moves the block settings menu to the block's toolbar.

I like it better than the previous button, but I don't like that the button can be positioned on the far left of the block (when it doesn't have other toolbar items), in the middle (when it has some toolbar items) or on the far right (when it has lots of toolbar items). Would it be possible to make it fixed on either end so you know intuitively where to expect the button?

Small wishlist thing would be to have the Remove Block option right next to the block settings button for easy access. Are there any filters to facilitate doing that?

Small regression: you now have to focus the block to access the settings menu, previously all you needed to do was hover the right side of the block. Adds an extra click.

[jasmussen]

I like it better than the previous button, but I don't like that the button can be positioned on the far left of the block (when it doesn't have other toolbar items), in the middle (when it has some toolbar items) or on the far right (when it has lots of toolbar items). Would it be possible to make it fixed on either end so you know intuitively where to expect the button?

I feel you on this, but I would ask you give this a few weeks to get a feel for it.

The reason is that the ellipsis is nearly always used as an "overflow" menu that sits at the end of a list of items. Given left to right reading direction, that means it sits at the end of the block toolbar, which also means in situations where there's not a lot there, like the separator which has a switcher and nothing more, it will be far to the left. But it's still the logically right place for it, and after using it for a bit I felt it was the better place to put it.

If we were to put it on the right side, we'd have to either slit the toolbar causing a more complex silhuette, or extend the toolbar with a lot of whitespace in between formatting and ellipsis, which would cover additional content before the selected block.

[mcsf]

Reported a related issue: #9797