Undesired delay on highlighting MenuItem when navigating Menu with arrow keys

[semiadam]

When a user navigates on a menu (dropdown) using arrow keys, the "current" item is not right away visually highlighted. Therefore, user cannot quickly navigate a long menu, and has to wait after each keypress for the highlight to appear to show him where he is.

• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

Navigating on a Menu using arrow keys must immediately highlight the current MenuItem so user does not have to wait to see which items is highlighted.

Current Behavior

Currently there is a fade-in transition to highlight the background of MenuItem. When navigating quickly on the items using keyboard, this transition prevents user from quickly seeing which item is currently highlighted. Therefore, user has to wait after each keypress to see which item he is on.

Steps to Reproduce (for bugs)

This can be observed on the official documentation page:
https://material-ui.com/demos/menus/

Context

Filling forms with keyboard is common practice for heavy users of business apps. I noticed they have difficulties selecting from MUI dropdowns due to the misbehavior described above.

Your Environment

Tech	Version
Material-UI	beta 29
React	16.1
browser	Chrome
etc

[oliviertassinari]

@semiadam I have noticed this undesirable behavior too. The whole menu is rendered when the focus moves from one item to another. It's not good!

component updates are highlighted

[ryancogswell]

@oliviertassinari My current thoughts on how to deal with tabIndex are as follows:

• Change focus-handling to be more declarative by adding an autoFocus property to MenuItem
  • Change SelectInput/Menu to set this property appropriately on MenuItem rather than imperatively calling focus() on MenuList. For Menu, autoFocus would be set on the first MenuItem (unless Menu's disableAutoFocusItem is true). For SelectInput, autoFocus would be set on the first selected item if one is selected otherwise the first MenuItem.
• MenuItem would handle setting focus to itself on mount if autoFocus is true
• Change MenuItem to manage its own tabIndex rather than MenuList setting it
  • tabIndex would be 0 if autoFocus or selected otherwise -1 unless disabled in which case undefined
  • This means that non-MenuItem children of MenuList such as Divider will no longer be assigned a tabIndex by MenuList
• MenuList handleKeyDown would change to skip any items that do not have a tabIndex
• currentTabIndex state on MenuList can be removed along with handleItemFocus

I think these changes should improve performance due to removing the MenuList state changes on focus change. It will also address the dividers (#13708) and disabled MenuItems (#13464). It will be a few days before I'll have a chance to work on the code changes, but I wanted to get your thoughts on the overall approach.

[oliviertassinari]

1. Change focus-handling to be more declarative by adding an autoFocus property to MenuItem

@ryancogswell 👍 This sounds like an excellent idea!

2. MenuItem would handle setting focus to itself on mount if autoFocus is true

👍

3. Change MenuItem to manage its own tabIndex rather than MenuList setting it

What's the need for the distinction between 0 and -1? Let's continue the conversation of

Do we need to change the tab index? Could we keep -1 (menu), 0 (dropdown) or nothing (comboxbox) depending on the mode? If we might be able to can kill the currentTabIndex state.

#14865 (comment). So, I have benchmarked as many dropdown and menu implementations as possible:

• https://a11y-guidelines.orange.com/web_EN/exemples/simple-menu/simple-menu.html
• https://www.w3.org/TR/wai-aria-practices/examples/listbox/listbox-collapsible.html
• https://getbootstrap.com/docs/4.3/components/dropdowns/
• https://ant.design/components/dropdown/#header
• https://evergreen.segment.com/components/menu
• https://blueprintjs.com/docs/#core/components/menu
• https://vuetifyjs.com/en/components/menus#menu
• https://jamesmfriedman.github.io/rmwc/menus
• https://primer.style/components/docs/Dropdown
• https://www.carbondesignsystem.com/experimental/dropdown/code#skip-to-content
• https://element.eleme.io/#/en-US/component/dropdown#dropdown
• https://react-bootstrap.netlify.com/components/dropdowns/#dropdowns
• https://reactstrap.github.io/components/dropdowns/#app
• https://www.telerik.com/kendo-react-ui/components/dropdowns/
• https://developer.microsoft.com/en-us/fabric/#/components/dropdown
• https://ui.reach.tech/menu-button/

From what I could observe, the majority of the implementations make tabbing close the dropdown/menu to move to the next tabbable element in the body of the page. A few use the tab to move the focus within the dropdown/menu.

What's the motivation for the correlation between the tab index and the auto focus or selected property? What do you think of using the "mode" (dropdown / menu / combobox) for changing the tabbable behavior?

MenuList handleKeyDown would change to skip any items that do not have a tabIndex

This is an interesting strategy. There is a tabbable library that we might be able to use to get the list of items we can navigate within, but it comes with a nonnegligeable cost: https://bundlephobia.com/[email protected]. I'm eager to find a good heuristic for the problem. e.g. Bootstrap uses a CSS selector: https://github.com/twbs/bootstrap/blob/1f7f876810c51d31baa6096465bd8501b2e23d9a/js/src/dropdown.js#L67.

currentTabIndex state on MenuList can be removed along with handleItemFocus

💯🎉

[ryancogswell]

There is a tabbable library that we might be able to use to get the list of items we can navigate within, but it comes with a nonnegligeable cost

@oliviertassinari In the case of MenuList, I don't think we care whether the items are tabbable. Most MenuItems would have a tabIndex of -1 which would not be considered tabbable, but should still be included in the arrow key navigation. I think it would be sufficient to include any items where child.tabIndex !== undefined && child.tabIndex !== null or just verify that child.tabIndex is a number. If someone uses a child other than a MenuItem that is inherently tabbable (button and a[href] would be the main possibilities), they would just need to include tabIndex on those elements for them to be included. Even when someone is using a button or link as an item, I would expect that they are usually doing it by specifying the component property of MenuItem rather than bypassing MenuItem entirely -- in which case the tabIndex would still get set properly with no additional work.

I'll comment later regarding 0 vs. -1 tabIndex. There are some scenarios I want to experiment with and be able to demonstrate in sandboxes in order to communicate my concerns more clearly. My thoughts have evolved on some of the specifics of when/how tabIndex would be 0, but I still think we probably need the distinction.

[oliviertassinari]

@ryancogswell I have no objection to the tab index heuristic. I assume we would check the existence of the attribute on the DOM node and not the property on the React element?

[ryancogswell]

I assume we would check the existence of the attribute on the DOM node and not the property on the React element?

@oliviertassinari Correct. MenuList:handleKeyDown would be looking at the children of the list DOM node and checking for tab index on those children.

[ryancogswell]

@oliviertassinari This sandbox demonstrates one of my concerns regarding tab index. I've copied the master branch versions of Menu and MenuList into this sandbox and changed MenuList to put a tab index of -1 on all the items.

Here's a link for full page output for more easily testing some of the cases below: https://y335lzoqo9.codesandbox.io/

• the first button opens the modified Menu/MenuList with disableAutoFocusItem set to true
• the second button opens the current master branch version of Menu/MenuList with disableAutoFocusItem set to true
• the third button opens the modified Menu/MenuList with disableAutoFocusItem set to false

For the two menus without auto focus, if you click on the button, the Menu will open but the focus will remain on the button.

On the modified Menu without auto focus (the first one), you will notice that without a tab index of 0 on the first item, there is no way to get to the items via the keyboard.

On the second Menu (original master branch version without auto focus), after clicking the button you can use tab to get to the first MenuItem (since it has a tab index of 0) and then the arrow keys to get to the others.

On the third Menu (modified but using auto focus) you start out with focus on the first item, but if you remove the focus in a manner that doesn't close the menu (I did this by clicking within the address bar of the browser), you can't tab your way back to the menu items. If you do the same steps on the middle Menu, the tab index of 0 allows you to tab back to the first menu item.

I think for accessibility it is important that we keep a tab index of 0 on whichever MenuItem we would typically give initial focus to. So whether the Menu is using auto focus or not, it would then be possible to tab into the list of menu items; and if focus leaves the Menu without it closing, it would be possible to tab back to the menu items. I think I would not have MenuItem handle this aspect though. Rather than it automatically setting tab index to 0 based on its own autoFocus or selected properties as I first indicated, I think it should always default to -1 (or no tab index if disabled) unless it is given an explicit tab index prop. I think Menu or SelectInput would then be responsible for giving a tab index of 0 to the first MenuItem (in the case of Menu) or the first selected MenuItem (in the case of SelectInput). This would correspond to the same MenuItem that would receive the autoFocus property, but the tabIndex property would be specified even if disableAutoFocusItem is true. There isn't any need though for this tab index to change as focus changes (as occurs in the current implementation), so we still don't need the tab index state in MenuList. For SelectInput, which MenuItem has a tab index of 0 would change only when the selected item changes. For Menu it wouldn't change.

[oliviertassinari]

@ryancogswell Ok, here is how I see it:

1. On the modified Menu without auto focus (the first one), you will notice that without a tab index of 0 on the first item, there is no way to get to the items via the keyboard.

I have tried to open this menu with the ArrowUp and ArrowDown, it doesn't work. Shouldn't we be able to use those keys to open it? I think that the tab event should move the focus to the next tabbable element.
Once the menu is open, I think that we should be able to focus the first item with ArrowDown and the last item with ArrowUp. The tab even should probably be ignored by default, with an option to make it close the menu and move to the next tabbable elemet.

2. On the second Menu (original master branch version without auto focus), after clicking the button you can use tab to get to the first MenuItem (since it has a tab index of 0) and then the arrow keys to get to the others.

Same comment than for n°1.

3. On the third Menu (modified but using auto focus) you start out with focus on the first item, but if you remove the focus in a manner that doesn't close the menu (I did this by clicking within the address bar of the browser), you can't tab your way back to the menu items. If you do the same steps on the middle Menu, the tab index of 0 allows you to tab back to the first menu item.

Should clicking within the address bar of the browser close the menu? (It's something we can detect, we do it for the snackbar). If we don't close it, then maybe the ArrowUp and ArrowDown could solve the problem?

[ryancogswell]

I have tried to open this menu with the ArrowUp and ArrowDown, it doesn't work. Shouldn't we be able to use those keys to open it?

@oliviertassinari I'm a little confused by this comment. For Menu, we don't control what causes it to open. It is entirely in the control of the developer using the library via the open property.

Should clicking within the address bar of the browser close the menu?

Same as above. We don't control when the menu closes. We can suggest it via the onClose callback, but I wouldn't think it would be a good idea to do anything based on an action (e.g. clicking in the address bar) that isn't an interaction within the HTML document.

Once the menu is open, I think that we should be able to focus the first item with ArrowDown and the last item with ArrowUp.

If the focus isn't already somewhere in the Menu (e.g. if the focus is still on the button due to disableAutoFocusItem being true), then Menu/MenuList won't receive any of the keyboard events. It won't start receiving the events unless you tab to one of its items which isn't possible if none of them have a tab index of 0 or above.

[oliviertassinari]

I'm a little confused by this comment. For Menu, we don't control what causes it to open. It is entirely in the control of the developer using the library via the open property.

You are right. It's something the select component implements on its own. We would need an API like this one https://github.com/jcoreio/material-ui-popup-state to be able to do something about it.

Same as above. We don't control when the menu closes. We can suggest it via the onClose callback, but I wouldn't think it would be a good idea to do anything based on an action (e.g. clicking in the address bar) that isn't an interaction within the HTML document.

You can find different implementations that hide the menu as soon as the focus leaves the documentation. You can't know what triggers the document blur, it can be a tab switch, a window switch, etc.

If the focus isn't already somewhere in the Menu (e.g. if the focus is still on the button due to disableAutoFocusItem being true), then Menu/MenuList won't receive any of the keyboard events. It won't start receiving the events unless you tab to one of its items which isn't possible if none of them have a tab index of 0 or above.

Maybe we need to rethink the API, to give him more integration area surface?

[ryancogswell]

Maybe we need to rethink the API, to give him more integration area surface?

I would opt to do this later when we look at providing better support for some of the menu variants (exposed, cascading menus, etc.). Any objection to continuing with the approach I've outlined for now so that we can do this more incrementally and take care of some of the outstanding issues?

My own personal priority is to finish this change and then take care of #8191, but I intend to continue contributing regularly after that. Since I'm building up expertise around Select/Menu/MenuList and since there is a fair amount more that can be done in this area, my intent is to keep chipping away at the issues/enhancements in this area unless something else comes up in my company's use of Material-UI that causes me to prioritize working on some other aspect.

[oliviertassinari]

Any objection to continuing with the approach I've outlined for now so that we can do this more incrementally and take care of some of the outstanding issues?

No objection, I agree, we should solve one problem at the time. I was interested in the global picture.

My own personal priority is to finish this change and then take care of #8191

Solving this issue would make me very happy. It's a great objective 🧭 ! It's a pleasure and an honor to review your work. Your pull requests have always been relevant.