Ensure keyboard navigation works with the drawer

The Keyboard navigation is working as expected. Attaching the screen capture for the reference

One detail in the context of keyboard navigation i wonder if it would make sense to add is the ability to close the drawer by pressing the esc key. at the moment you have to tab through the entire sub menu, the configuration sub menu requires quite a few tabs to get out.

On a side note not directly related to keyboard navigation but i've noticed while testing that several menu items miss focus outlines. without any patches applied to the 1.x-dev branch of the navigation module the logo, sub sub menus (for example the menu items underneath configuration -> system), and the sub menu items (view profile, edit profile and log out) for the logged in user are not visible. the username isnt tab-able at all, only clickable by mouse.

the navigation itself, except the aforementioned menu item for the user name is correctly tab-able and all sub menus and sub-sub menus are expandable and collapsible by space and return as well as VO-space with voiceover active (with collapsed as well as expanded sidebar).

@rkoller Agreed on Esc. I'm following the W3C guidelines, which also includes patterns for arrow key navigation, Home and End.

Regarding the items that are not keyboard navigable, thanks for spotting! I'll look into that.

I added ESC to close the sub-menu.

Thanks for the suggestion @silviu-s. As mentioned previously in the comment thread, I am working on this, and will open an MR to review with everything listed. As a point of process, an issue should not transition to "Needs Review" until all aspects of the scope are addressed.

I think we need some clear guide for when Arrow Keys Vs Tabbing & Escape should be working as the collection of actions that need to be this ticket.

Thanks for adding that @skaught! That table is what I'm going on.

One change from the table above I think should be made is reversing the Up/Down and Left/Right arrow functions. The W3C example is a horizontal menu, and this is a vertical menu.

I added code to handle keyboard navigation, patterned off the W3C guidelines outlined in the ticket description.

based on #23 i've updated the issue summary

Menu bar:

space/enter :
works

down arrow:
fails - the down arrow jumps not to the next top level menu item instead it jumps to the next top level menu item that has a submenu and is expandable. if you tab to a top level menu item that has no sub menu item the down button has no effect and is unresponsive
works - if i reach the username at the bottom and press the down key another time the shortcuts top level menu item is in focus next

up arrow:
fails - the up arrow jumps not to the next top level menu items instead it jumps to the next top level menu item that has a submenu and is expandable. if you tab to a top level menu item that has no sub menu item the up button has no effect and is unresponsive
works - if i reach shortcuts at the top and press the up key another time the username top level menu item is in focus next

right arrow:
fail - the drawer expands but the first sub menu item is not getting into focus

fail - in addition try the following. get the reports top level menu item into focus, press space, the drawer for reportsexpands, press now the up key and configuration gets into focus, press now the right arrow, the configuration drawer opens underneath the still open reports drawer

left arrow
fail(?) - if the top level menu item is collapsed and you press the left arrow nothing happens, while if the drawer is expanded and you press left the drawer closes. but the desired "if you press the left arrow and the drawer opens and you get the last sub menu item into focus" doesnt happen here.

home
works - ctrl fn left jumps to shortscuts on the mac

end
works - ctrl fn right jumps to the username on the mac

character
fails - i have content in focus and press m as well as M, no reaction. ahhhh just noticed. it has the same problem like the up and down button the character applies only to top level menu items that have a submenu. therefor m had no effect. if i type s i first jump to shortcuts if i press is another time i jump to structure.

Submenu:

Space/Enter:
fails - if you press space or return on a parent the focus remains on the parent instead of moving to the first item in the submenu
works - pressing space or return on menu items without a submenu forwards to the linked page

esc
works - closes drawer and moves focus to the parent top level menu item that opened the collapsed drawer

Right arrow:
fails - within the configuration submenu for example when the focus is on system and you press the right button nothing happens
fails - if you are in the structure submenu and you have the media types menu item in focus and press the right button nothing happens

Left arrow:
works (?): if the block types menu item is in focus under structure and you press the left button structure gets into focus but the focus isnt moving to the previous top level menu item in the menu bar

fail (?) if a sub sub menu item is in focus and you press the left key not the parent sub menu item gets into focus but the parent top level menu item

down arrow:
fail - it only works for the configuration submenu, which is the only that consists of menu items with sub menus. that way i am able to move down and after workflow i get back up to people

up arrow
fail - it only work for the configuration submenu, which is the only that consists of menu items with sub menus. that way i am able to move up and after people i get back down to workflow

home
works - ctrl fn left jumps to shortscuts on the mac

end
works - ctrl fn right jumps to the username on the mac

character
fails - in submenus characters dont work at all not even for sub menu items that contain another submenu

a few more details i've noticed:
the menu item for shorcuts in the drawer is not reachable by keyboard but only by the mouse cursor. same for the username menu item in the username drawer, plus view profile, edit profile, and logout are missing the focus outline there. same for the sub sub menu items in general, they are all also missing the focus outline.

addendum: just one detail to add, while testing i thought the focus outline for 3rd level menu items was already added in one of the previous commits in this issue, but then wondered why all of a sudden it was gone again in my last tests of the day (tested this issue on different instances over the day). but when reading #3437696: Claro, Olivero and Umami hover/focus styles leak into the navigation now i realized that that https://www.drupal.org/files/issues/2024-04-02/umami-active.png was what i saw, which is a hover style leaking into the navigation on focus in my case (the instance i ran into this is using demo_umami).

Just tagging it here.

Thanks for the review @rkoller. I pushed an update that should address the following.

1. Up and down arrow keys moving focus to both expandable menu items and links.
2. Up and down arrow keys moving focus as above inside popovers when opened.
3. Typing a character jumping to either expandable menus or links, as above.
4. Left and right arrows should open and close popovers consistently.

There's some issues left, below.

1. Focus immediately moving to the first popover item when opened. Currently the first down arrow press after the popover is opened will focus on the first popover menu item.
2. The shortcuts popover menu items are still inaccessible from the keyboard.
3. Mixing navigation with tab and arrow keys can result in unexpected jumps in focus.

Since this MR does improve the keyboard navigation, and fixing the issues above won't be insignificant, I propose we split the remaining items into a new ticket.

Here's the follow up ticket https://www.drupal.org/project/navigation/issues/3440047

Have manually tested this. Is certainly worthy proof of concept and introduces needed javascript work that would be great for continued work around in beta phase, and for other to be able to also continue related work around in the active dev line!

note: some oddness in 3rd level menus between 'what i think i should be doing' between switching between needing to tab into 3rd item. This maybe what you are describing in 'leftover issue point1'.

thank you! I've quickly tested against the four points that got fixed in #32. in "general" they work but i've found some odd behavior:

1&2) yes in it general it works but the problems are:

• in safari moving by the arrow keys through the top level items sometimes just stops working and i am unable to move up and down anymore (in edge and firefox i was unable to reproduce that)
• there is some sort of active trail for the top level menu items (see safari.mp4 - reproducible in edge and firefox)

3) for top level menu items moving by character works for sub menus it does now
4) works - the only odd thing pattern wise, similar to the mix of hover for top level menu items and click for sub menus is the fact that with left and right you are able to collapse and expand the drawer but within a drawer left and right have no effect and you have to press space on a sub menu.

update: oh @skaught already set it to rtbc while i was writing up my comment. due to the active trail shown in the video i would set it back to needs work. if you would move the work on that newly introduce behavior to the follow up as well then put the status back to rtbc.

no worry. pardon my excitement!

in safari moving by the arrow keys through the top level items sometimes just stops working and i am unable to move up and down anymore (in edge and firefox i was unable to reproduce that)

I pushed a fix for this. I think the issue is pressing right arrow on menu items that don't have popovers, which pushes focus to a hidden element, preventing other arrow movement until left arrow is pressed.

Gonna review now

thank you for the update @bronzehedwick. i've applied the latest changes but somehow i am still able to reproduce being stuck. somehow i am able to trigger the behavior faster when i hover and or click a top level menu so a drawer expands and collapses and after that i go on navigating up and down through the top level menu items. for the video i've activated the keyboard viewer so you are able to see my keyboard input. and in addition i see the following type error in the console:

[Error] TypeError: undefined is not an object (evaluating 'focusableElements[focusedIndex-1].focus')
	(anonymous function) (js_Ez2GMQcuGAO5zipRlKWSB0V0jT7lLqDrF3Fi0L4o6dY.js:1970:3557)

hope that helps.

forgot to add the video. apologies :/

Looks like the problem here is the hover. The hover effect is controlled in JavaScript, and has a delay on toggling (by design). The hover off is triggered when you hover over another menu item. That means that if you quickly hover over a few items, then move your mouse outside the menu drawer, the hovered items will stay hovered. This blocks arrow movement, unless you press arrow left to "dismiss" the (hidden) popover.

I haven't been able to reproduce the JS error @rkoller and @finnsky are seeing yet.

This is an architectural issue, and will probably require some larger refactoring to resolve.

I think this and the other remaining issues should be moved into the follow up ticket, to reduce churn while the core MR is being made. Does that make sense with what you are thinking @ckrina?

Pushed a fix for the inaccessible Shortcuts menu via arrow keys. Simple oversight, just needed to add tabindex.

Moving to NR, given @ckrina's guidance, above. The follow up issues are below.

1. Improv keyboard navigation focus inside drawer
2. Hover effect can cause problems with keyboard focus
3. Third level menu accordions are not navigable via the arrow keys

I added an approach that, in my opinion, fits better here. Some problems that I tried to solve:

1. All keyboard events are controlled in one place. They are bubble so we can control them at the sidebar level using the current target.
2. I used https://www.npmjs.com/package/tabbable which is part of the Drupal library
3. I used the https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/inert attribute to hide the submenu from focus
4. I used data-index-text in twig to reduce client calculation and search time
5. All menus at all levels use the same logic. Branching occurs only where it is necessary.
6. The logo and bottom button are also part of keyboard navigation, even if they are not part of the menu.

Currently we need to add a description of how the arrows behave when the third menu level has focus. Now the focus falls into a trap from which you can escape only to the first level

Hey @finnsky, thanks for this! Some things I noticed.

• When closing a popover with left arrow, focus moves up to the previous menu item, instead of moving back to the popover's button.
• Pressing right arrow inside an open popover, the the popover is dismissed and focus is moved to the next main menu item below the current item. I would expect nothing to happen.
• When inside a third level menu (accordion), there's no way to move out of it.
  • Pressing either arrow left or right dismiss the entire popover.
  • Pressing Space scrolls the page.

  I would expect pressing arrow left (and maybe arrow right?) or Space to collapse the accordion, moving focus back to the top level of the accordion.

• After initially opening a popover, pressing up arrow dismisses the popover. I would expect the focus to move to the last menu item in the popover.
• When focus is on a collapsed third level accordion, pressing left arrow does nothing. I would expect the popover to be dismissed.

@bronzehedwick
Thank you for review! You are right!

I've managed initially how it requested in ticket. But seems logic which is fair for horizontal menu not really obvious here.

Now i added more realistic and obvious logic.
Also fixed focus trap on lower levels.

Please review.

Thanks @finnsky! Your updates addressed all my feedback.

There is one small, new regression, I think from the 1.x rebase. There's extra padding and grey bar below each third level accordion.

This seems to be caused by the recently merged additional padding MR running up against the new styles.

One solution would be to change line 38 of toolbar-menu.pcss.css from:

.toolbar-menu--level-2 {

to:

.toolbar-menu--level-2:not([inert]) {

reverted height. it was experiment anyway.

Added WCAG 2.2 success criteria.

@finnsky that last commit fixed the issue. Thanks!

This approach also resolves the follow up issues, listed below.

• Hover effect can cause problems with keyboard focus
• Improv keyboard navigation focus inside drawer
• Third level menu accordions are not navigable via the arrow keys

Moving to RTBC.

One minor detail I've noticed, you are able to bypass the constraint to expand only a single submenu with a combination of arrow and tab key:

- expand the configuration submenu
- move to the people menu item and press space
- tab until you reach user interface
- press the right arrow
- tab until you reach media
- press the right arrow

That way you are able to have three sub menus expanded at the same time. if you try the same with only tab and space, only a single sub menu remains open. while when just use the four arrow key you are only able to move inbetween the sub sub menu items with arrow up and down of for example the expanded media submenu.

@rkoller i agree that this should be covered somehow.

But this one is good candidate for follow up.

@ckrina removed that not used code.

I also think we need to rework current events system.

They should be better documented and all iterations click/hovers/keys should go in same manner.
Since only events cares globally about multiple things. EG

- Switch expand/collapse text
- Switch classes
- Controls sibling elements open/close

I also think we need to rework current events system.

They should be better documented and all iterations click/hovers/keys should go in same manner.
Since only events cares globally about multiple things. EG

- Switch expand/collapse text
- Switch classes
- Controls sibling elements open/close

@finnsky would this be appropriate for a follow up ticket?

RE #65

Thank you!

for a follow up ticket?

Yes sure. Now all works.
But i guess all system needs some refactoring.
We have all features onboard. And globally nothing should changed. But we can improve code readability and documentation.