Top link disclosure menu

[NickDJM]

Description

Adding the new subclass for Disclosure Navigation Menu with Top-Level Links.

Another good example (specifically for hover options): https://www.davidmacd.com/widgets/menu/tutorial-ar/wcag-tutorial-add-hover-invisible.html

Related Issues

See #164

Completion criteria

• [x] ~~Handle new topLevelSubmenuToggleSelector~~
• [ ] Update hover methods to properly handle the new split toggle/link
• [ ] Update keybindings to properly handle toggle vs link focus
• [ ] Write tests for new menu type
• [ ] Browser test new menu type where automated tests can't cover
• [ ] Add documentation to explain how new menu type works

[NickDJM]

I believe the initialization for the top level toggles is covered now. Keybindings are still super messed up, but tabbing/clicking through the menu seems to work.

[NickDJM]

The hover functionality doesn't work as expected either. Hovering over either the link or the dropdown should open the submenu- only the submenu triggers opening at the moment.

I have added another task item to the initial MR description to handle this.

[pyrello]

Edit: I'm realizing by looking at this example that the toggle elements are already supposed to be there: https://codepen.io/pyrello/pen/WNdEKRQ (Saved from the CodePen button on https://www.w3.org/TR/wai-aria-practices-1.2/examples/disclosure/disclosure-navigation-hybrid.html)

@NickDJM We are working on implementing it as a way of testing and contributing back. We are not seeing the toggles. Is there something we need to change in our implementation? https://github.com/uiowa/uiowa/blob/310b799e1fdb28b5e077920f40b76e0f4320b629/docroot/themes/custom/uids_base/assets/js/accessible-menu.init.js#L4-L9

[NickDJM]

@pyrello awesome, sounds good! Yeah the buttons need to be there already.

I'll be curious to see how you implement this into Drupal. I've been tempted to write a module that just adds an option to the menu interface that will just allow you to select the kind of menu you want and have it work automatically.

[pyrello]

@NickDJM We built a quick demo of our implementation: https://sandbox.dev.drupal.uiowa.edu/media. It actually works great already, although we have not fully tested it from an accessibility standpoint.

You indicated in previous comments that there appears to be problems with hover states and keybindings. What do we need to do to reproduce those issues so that we can help troubleshoot? We are looking for some direction as to how we can help wrap this thing up.

Thank you very much for your work on this!

[NickDJM]

@pyrello For keybinding tests you can set optionalKeySupport to true. Once that's set you'll be able to navigate the menu with your arrow keys once you tab in.

The behaviour I was seeing was the arrows would only target the links inside the menu items and skip over the buttons.

We'll need to make sure all of the enter/space key bindings work as well- which I don't think they do.

---

For the hover issues we'll need to test hoverType set to "on" and "dynamic" to make sure it behaves similarly to what is shown here: https://www.davidmacd.com/widgets/menu/tutorial-ar/wcag-tutorial-add-hover-invisible.html

[NickDJM]

@pyrello Taking a look at your sandbox thats up. I see you're using the button dropdown for child submenus as well- the initial implementation I have assumes the buttons would only be on the top level menu, however I think yours makes more sense.

It makes me wonder if we're going in the wrong direction for the subclass of menu... Part of me thinks it may be best to take a look at how the base menu functions and possibly just split out how it handles menu links vs menu toggles. If the base menu knows when to differentiate between links and toggles then it would remove the need for the new subclass (and all existing subclasses would gain the ability to have split links and toggles).

[NickDJM]

This being said, we should pursue the subclass for now. If it makes more sense to merge it into base menu, we will.

[bspeare]

@NickDJM I did some keybinding and hover tests on https://sandbox.dev.drupal.uiowa.edu/media yesterday and here's a few things that I noticed.

Hover

• “dynamic” only starts working after a dropdown has been clicked on once
• “on” works when hovering on the button but not on the link next to the button

Keybinding

Arrows

• Left to right arrows skip links and only go to buttons

Tabs

• Tabbing goes to each link and button
  • https://www.w3.org/TR/wai-aria-practices-1.2/examples/disclosure/disclosure-navigation-hybrid.html has the reverse behavior - tabbing goes to each button and skips the links

Enter/Space:

• Enter and spacebar on button opens menu once but doesn’t close it
  • https://www.w3.org/TR/wai-aria-practices-1.2/examples/disclosure/disclosure-navigation-hybrid.html toggles open and close with enter on button

[NickDJM]

@bspeare dynamic hovering is only supposed to activate once a menu is first opened, so that's fine.

Hover

We'll need to make sure hoverType: "on" works for both the link and the button, and hoverType: "dynamic" should wait for the first button toggle and then work for both the link and button from then on.

Keybindings

Arrows:

Arrows will need to be fixed so it properly cycles through links and buttons sequentially.

Tabs:

I believe tabbing is fine as-is, it should cycle through all the links and buttons sequentially- if the example one isn't doing that its definitely a bug since their documentation says tab/shift tab should:

Move keyboard focus among top-level links and buttons, and if a dropdown is open, through links in the dropdown.

Enter/Space

We'll need to make sure it closes the menu as well.

[bspeare]

@NickDJM Thanks for the clarification!

[pyrello]

@NickDJM Thanks for taking a look at our example and providing clarification. For our initial release of this feature in our platform:

• We are not concerned with left/right arrow keybinding working, since Superfish does not have that functionality and that is what we are currently using for our top level menu.
• We will set hoverType: off until that is working and then add it as an optional enhancement later.
• The enter/space toggle is a blocker, so if you have any thoughts on how we can help resolve that, we would like to help.

I am assuming we will be ready to adopt this branch prior to it getting merged (since we are getting some pressure to wrap up the first slice of this work). Our intention is to target a particular commit hash to ensure that we are pulling what we expect for our build process. As soon as this gets merged and a new release is tagged, we'll switch to using that.

Also, FYI, at this point we are doing a pretty bespoke implementation. Our early use case focuses on adding the option for this type of menu to blocks that are placed by users via Layout Builder. Eventually we are interested in adopting this as a replacement for our Superfish-based top-level navigation (which is placed via configuration). At that point in time, it might make sense to write a module to implement the functionality in a more generic way.

[NickDJM]

Ok I have removed topLevelSubmenuToggleSelector since it really just added a load of complexity that didn't need to be there. We'll go with assuming all toggles are buttons for this menu type and people can use a class selector if they want something different.

[NickDJM]

@pyrello I have corrected the enter/space command to toggle the submenus open/closed.

Now, this is still an issue since the event listener is on the menu item, not the toggle itself. This means if a user hits enter on the link instead of the button it will still toggle the submenu.

This is where the biggest issue lies... Its possible we might need to make a new class called MenuLink so we can track both links and toggles separately. This is going to be a pretty big change.

[NickDJM]

Regardless of the new class, we'll still need a list of all links + toggles in the menu, which will need to be hooked into currentChild somehow. I'll need to think about this one since it's going to be changing a whole whack of things.

[pyrello]

@NickDJM Thanks for your follow-up! We have updated our example at https://sandbox.dev.drupal.uiowa.edu/media to include your recent commits. What we are seeing is that the enter/space keys focused on a toggle button opens a menu, but does not close it. This is true at all levels of the menu. Not sure if this is a problem with our implementation or not: https://github.com/uiowa/uiowa/blob/1b0ec0076e8f29d47558cad493b00a69a7c4a26a/docroot/themes/custom/uids_base/assets/js/accessible-menu.init.js#L35-L43

If you know what file we should be tinkering with to try to fix this, that would be helpful.

[GaryRidgway]

As I have researched this I have found that the escape key is the intended way to exit the second level menus. Additionally, I have found multiple sources saying that this is the correct way to implement accessible menu navigation. Here are a few sources: https://www.evinced.com/blog/a11y-nav-menus/ https://www.smashingmagazine.com/2017/11/building-accessible-menu-systems/#keyboard-and-focus-behavior

So it actually seems like the menu is functioning correctly in accordance to accessibility Guidelines. My initial impression is that it is supposed to work this way to keep interaction consistent across menus, modals, etc.

[NickDJM]

@pyrello currentChild is all handled int he menu class itself, not in the toggle or item classes. You'll need to look there. There are API docs if you need more information on how it all works: https://accessible-menu.netlify.app/basemenu#currentChild

Mostly this is changed by using the focusChild methods which handle changing the current child/menu item and also they check to see if the menu should actually focus the new child. We'll need to change how currentChild works for selector links and buttons and possibly change how the focusChild mehtods work.

[NickDJM]

@GaryRidgway I would agree- I believe the official docs used to just say that it opens the menu. However, it looks like the wording changed to "toggles" the menu on W3C and their example opens and closes the menu, so it seems as though it should have this new functionality.

[NickDJM]

@pyrello I pushed up a compiled version of the changes. If you're using the dist/ then it should work now.

[GaryRidgway]

This is working as you described now @NickDJM , thanks!

[GaryRidgway]

@NickDJM, on our platform, we have the option to add a menu item that is not a link (called the <nolink> option). It gets rendered as a <span> instead of an <a>. In our current menu system, the essentially serves as a trigger to show a drop-down menu. Here is an example. In this example, the “Media - YouTube” menu item is a <nolink>. We are experiencing quite an odd phenomenon with our <nolinks>. If you tab forward to the “Media - YouTube” item and press enter or space, it activates the link that it most recently passed over, in our example, the Media - Vimeo link. Alternatively, if you tab past the “Media - YouTube” menu item and then back to it with shift+tab, it activates the dropdown button that it just passed over (just to the right of the “Media - YouTube” menu item). It seems that the menu is holding on to the last action and activating it on enter when focusing the span. This may be a consequence of how we have this set up: https://github.com/uiowa/uiowa/blob/79b56633f0f032061dc01456f8f43fe290020586/docroot/themes/custom/uids_base/assets/js/accessible-menu.init.js#L47 Do we need to configure our implementation differently? Or is this a bug?

[NickDJM]

@GaryRidgway without being able to debug the instantiated menu itself, I'm not sure why that would be happening-. Is there a reason you would want users to be able to focus the span at all? It's just adding a non-interactive item to the tab order.

[pyrello]

@NickDJM We figured out that it was some of our own custom code that was causing this issue by adding tabindex to the spans. We tested this and were able to move forward with it in its current state. Thanks for all your help!

[NickDJM]

@pyrello Cool.

When I have some more free time I'll look into getting more work done for this menu type. I can ping you when its feature-complete so you know when you can fully upgrade to the new version of accessible menu.

[NickDJM]

It's been a while, but I have a bit of time to work on this today. I have fixed the issue with arrow keys not being able to target the toggles, as well as escape keybindings not working to close open menus when you have the toggle focused.

[NickDJM]

I believe this is functionally ready to go.

Still need documentation and tests written, but through my (very limited) manual testing it looks like everything works. Obviously the documentation and tests are 100% required, so there won't be anything merged in until those are done.

[NickDJM]

Going to be finalizing all functionality and written tests over the next couple weeks.

To get the tests working in a cleaner way, this will be part of a break change so TopLinkDisclosureMenus will be part of 4.0.0.