Components: Add a generic navigable menu component

[youknowriad]

I found that in several places, we'll have to create menus navigable using arrow keys. (toolbar, switcher ). I'm trying to extract this logic to a generic Navigable Menu Component.

I also dropped the DropdownMenu component which was not very reusable. (Tried and failed reusing it several times). It can be replaced with a combo Dropdown and NavigableMenu

The name of the component is probably not great, but I'm having a hard time finding a good name.

I'm using the component in the table block toolbar and in the block switcher dropdown. Could be used later for #2960

closes #696

[codecov]

Codecov Report

Merging #2989 into master will increase coverage by 0.24%.
The diff coverage is 29.5%.

@@            Coverage Diff             @@
##           master    #2989      +/-   ##
==========================================
+ Coverage   34.38%   34.63%   +0.24%     
==========================================
  Files         196      197       +1     
  Lines        5810     6153     +343     
  Branches     1027     1112      +85     
==========================================
+ Hits         1998     2131     +133     
- Misses       3222     3384     +162     
- Partials      590      638      +48

Impacted Files	Coverage Δ
components/popover/index.js	87.5% <0%> (-0.55%)	⬇️
editor/block-switcher/index.js	0% <0%> (ø)	⬆️
components/navigable-menu/index.js	28% <28%> (ø)
components/dropdown-menu/index.js	75% <91.66%> (-19.8%)	⬇️
editor/header/index.js	0% <0%> (ø)	⬆️
editor/block-settings-menu/content.js	0% <0%> (ø)	⬆️
editor/sidebar/header.js	0% <0%> (ø)	⬆️
editor/block-mover/index.js	0% <0%> (ø)	⬆️
blocks/api/registration.js	100% <0%> (ø)	⬆️
... and 13 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e09410e...9de328b. Read the comment docs.

[afercia]

The name of the component is probably not great, but I'm having a hard time finding a good name.

Yep, for example when used for #2960 the word "Menu" wouldn't be so appropriate, since that's a toolbar.

More importantly, ARIA roles need to be set properly. The DropDownMenu used role="menu" and role="menuitem", in this PR I see only the second one.

Also, for a toolbar this component wouldn't be appropriate because a toolbar needs a role="toolbar" and the children shouldn't have a role="menuitem".

[youknowriad]

This component is unopinionated, it doesn't enforce a role, the component using it has to set the roles itself. So it can be used for both toolbars and menus.

I might consider adding a wrapper component DropdownMenu using Dropdown and NavigableMenu. This one could be more opinionated. I didn't add it at first because I wasn't sure we'd have a lot of use-cases for this wrapper.

[afercia]

See also the Format list used in the "Classic Text" block, #1879

Also for the block switcher: #696

[afercia]

This component is unopinionated, it doesn't enforce a role

Then why it uses role="menuitem" on the items? role="menuitem" can be used only if the parent uses role="menu". Instead, toolbars should use role="toolbar" and no role on the children. So, actually, this component is opinionated and can be used only for menus. Unless it gets changed and the roles on the wrapper and items are passed as props. But I'd advise against that, because it could be abused.

Ok ok I was wrong, sorry :)
Looking better it's the implementation that uses role="menuitem". However, the wrapper <NavigableMenu> should use role="menu".

Then in case of a toolbar <NavigableMenu> should use role="toolbar" and no role on the children.

[youknowriad]

Is this good to merge as is? Would help me move forward with #2960 and #2998

[afercia]

I'm trying to finish my review, but I think there are some fundamental differences with how the DropDownMenu works that should be addressed.

[youknowriad]

Ok, I made some updates here to bring back the DropdownMenu component, it's reusing the lower level components now. It should be better

[afercia]

Tested a bit. Not sure why the table block uses the DropDown (which in turn uses the PopOver) and the Block switcher doesn't. Why they should look different? Seems to me they are functionally similar and they should look the same.

Additionally, this introduces a difference in the behavior and focus management because the PopOver uses withFocusReturn. When opening the table block, focus is moved to the first item (that's correct!) while this doesn't happen on the block switcher (see screenshot above).

Also, this is what happens when I open the table block and then scroll the page with the trackpad: the PopOver stays fixed in its position, while the page scrolls:

Trying to list the main issues I've noticed (please consider many of these things work nicely on the current DropDownMenu):

• down arrow on the toggle button should open the menu (already works with Enter and Spacebar)
• when the menu opens, the first item should receive focus: this works only on the block table, not on the block switcher
• block switcher: the "Transform into:" span should be removed: elements with role="menu" should contain just menu items
• block switcher: about the "Transform into:" span, I'd also recommend to consider that if an UI needs to be explained, then it's very likely there's a problem with the UI design in the first place
• block switcher: if the span gets removed, the wrapper div should be removed: role, aria-label, and tabindex should be set on the <NavigableMenu>
• table block: missing role="menu", aria-label, and tabindex.
• arrowing should loop through the items, instead arrowing on the last or first items completely loses focus (I guess navigation through blocks kicks in)
• Escape should close the menu and move focus back to the toggle button
• Escape should close just the menu, instead it also makes the toolbar disappear and there's a focus loss. This was fixed in the DropDownMenu with some stopPropagation, I guess there's some event not prevented here
• seems to me the default action is not prevented when using the arrow keys: the page scrolls when using the block switcher. Not sure why, on the table block seems this doesn't happen and the page scrolls only when I keep my browser's console open.

I'd recommend to check carefully what the current DropDownMenu does. Interaction and accessibility there work pretty nicely and they should be replicated in this new component.

I understand the DropDownMenu is not easily reusable, but at the moment this PR introduces an usability and accessibility regression.

If there's the need to have some higher abstraction and more generic, reusable components, that's good but it shouldn't come at the cost of breaking what was already achieved. At the moment, I don't see why the current DropDownMenu menu should be dropped in favor of something that worsen the user experience. Happy to try to help in making it better, if possible 🙂

[afercia]

Please see comment. At the moment I see an usability and accessibility regression here and there are a few things that should be addressed.

[afercia]

Note: my review refers to the PR state before the latest updates.

[youknowriad]

Not sure why the table block uses the DropDown (which in turn uses the PopOver) and the Block switcher doesn't.

The dropdownMenu is too opinionated, we can't add extra children (needed in the block switcher), I can add a prop to enable this but seems like a hack compared to reusing the lower level components

Why they should look different?

They don't look different for me, I'm not sure what you mean (you probably tested before my refactoring)

Also, this is what happens when I open the table block and then scroll the page with the trackpad: the PopOver stays fixed in its position, while the page scrolls:

This is already tracked elsewhere #2999

down arrow on the toggle button should open the menu

Already implemented in DropdownMenu

block switcher: the "Transform into:" span should be removed: elements with role="menu" should contain just menu items

I'd leave this out of this PR because it was added explicitely with its own issue/PR. This should be discusseed in the issue suggesting this.

table block: missing role="menu", aria-label, and tabindex.

You tested before my refactoring :)

arrowing should loop through the items, instead arrowing on the last or first items completely loses focus (I guess navigation through blocks kicks in)

It doesn't loop but it stays at the last position if no more items

Escape should close the menu and move focus back to the toggle button

done in the dropdown menu

---

I won't focus on the changes to the BlockSwitcher in this PR, it's an improvment over master and the switcher won't be a similar dropdown menu with #2984

Looping and prevent default is fixed in df5ee5cc

[afercia]

@youknowriad

you probably tested before my refactoring

yes 😬 I was writing the review while you pushed the updates, with a few minutes difference. The PR was in a pending review state, I'd expect to test something that doesn't change while testing 😉 Next time please try to inform me if updates are coming on a PR in a pending review state, I could have saved a few hours of testing and debugging...

• down arrow to open the menu works just on the table block for me, doesn't work on the block switcher, this should be fixed
• pressing Escape still closes also the toolbar for me, this should be fixed. there are 2 onKeyDown events on the same div, I'm not even sure closeOnEscape actually runs, so I guess propagation is not stopped when pressing Escape. Instead, I see the PopOver maybeClose running and that's what actually closing the menu
• I see you've added looping, thanks

About

block switcher: the "Transform into:" span should be removed

this should be taken into consideration together with the fact that tabindex=0 should be on the element with role=menu and not on the popover. This needs to be tested a bit with different screen readers to check if they're able to announce the menu aria-label. VoiceOver doesn't announce it, I need a bit of time to test with other screen readers.

[youknowriad]

tabindex=0 should be on the element with role=menu and not on the popover.

Can you explain why the tabIndex=0 should be on the menu? This is causing an issue when opening the popover, because opening the popover moves the focus to the first tabbable element in the popover (whether the popover contains a menu or not) and if it contains a menu, it's moving the focus to the menu if we add tabIndex=0 instead of the first element of the menu. Is this correct?

[youknowriad]

yes 😬 I was writing the review while you pushed the updates,

Yes, sorry about this, I knew I had to do this refactoring after your first comment ;)

[afercia]

Can you explain why the tabIndex=0 should be on the menu

I've just said "needs to be tested a bit". I'm not sure screen readers are able to announce an aria-label on an element that is not focusable.

[afercia]

About the span with the "Transform into:" text, see #493 (comment)

[afercia]

There's one thing left that worked on the DropdownMenu and should work here too for feature parity: constraining tabbing within the menu.
This is even more important now, since the menu is actually rendered at the bottom of the source and tabbing away makes you start keyboard navigation again from the beginning of the document.

Note: in the future we could possibly switch to the W3C model for this kind of widgets, where keyboard navigation inside the widget should work with arrow keys only and not with the Tab key. This should be discussed sooner or later :)