[Menu][Divider] Divider gets tabIndex 0 when all menu items are disabled

[JonneDeurloo]

Duplicates

• I have searched the existing issues

Latest version

• I have tested the latest version

Steps to reproduce 🕹

Link to live example:
https://codesandbox.io/s/vigorous-bas-2g9yfj?file=/demo.tsx

Steps:

1. Create a <Menu> with only disabled <MenuItem>'s,
2. Add <Divider/>,
3. Press Tab and see focus on divider.

Current behavior 😯

If a list of menu items also contains a divider, and all menu items are disabled, tabindex=0 will be set on the divider.

Expected behavior 🤔

A divider should never get tabindex=0, unless explicitly programmed to do so.

Context 🔦

Depending on user rights (user, admin, dev) or state (data isn't loaded yet, a save is in progress, or an underlying error), a menu item can either be enabled or disabled. Whenever there is no option available, all menu items could be disabled. If a menu contains a divider, keyboard navigation will focus on this divider, even though this should not be an element that gets focus.

[Dchole]

Is there any reason aside maybe the spacing, why you used the <Divider /> component inside the <Menu> instead of using the divider prop on the <MenuItem />?

[ZeeshanTamboli]

The tabIndex is calculated programmatically, as shown here. Since the Divider is a child of the Menu, and with all the menu items disabled, it is assigned a tabIndex value of 0.

I agree that this is a bug. In my opinion, the optimal solution would be to identify the Divider in children and avoid assigning a tabIndex to it.

[divyammadhok]

@ZeeshanTamboli I have the bandwidth to solve this bug if this is ready to be picked up

[ZeeshanTamboli]

@divyammadhok Feel free to work on it. If it is marked as a bug and nobody is working on it, it is ready to be picked up. :)

[divyammadhok]

@ZeeshanTamboli I've raised a PR addressing the fix, please take a look whenever you get some time - #38102

Thanks!

[mnajdova]

Could the fix be as simple as adding tabIndex={-1} on the Divider?I I wouldn't depend on the type of the element, as it can break custom elements. In the past we used child.type.muiSkipListHighlight for a similar use-case.

[divyammadhok]

Could the fix be as simple as adding tabIndex={-1} on the Divider?I wouldn't depend on the type of the element, as it can break custom elements. In the past, we used child.type.muiSkipListHighlight for a similar use case.

@mnajdova Sure, I will look into this. Although, the current implementation was done thinking to make this reusable in scenarios where we may see more such elements that get focussed.

I did test the current implementation by using different elements and making custom component wrappers which worked. The only issue was the if we depend on type.name it gets mangled in prod builds.

Would definitely check out child.type.muiSkipListHighlight

[divyammadhok]

@mnajdova I've incorporated the changes as suggested, please have a look - #38102