[menu] Disabled menu items get highlighted and can even get clicked (sort of)

[benface]

Bug report

Current behavior

When Menu.Item (or Menu.CheckboxItem or Menu.RadioItem) is disabled, it still gets the data-highlighted attribute when hovered or navigated to with the keyboard. Already, I'm not sure if that's intentional, and if so, I believe it would make sense to make that behavior customizable. For instance, macOS menus don't have any kind of hover state on disabled items, and trying to navigate to one with the keyboard skips the item altogether.

I also found a strange inconsistency: if I use the render function of Menu.Item to render a <button>, and more specifically in the case of a disabled menu item, a <button disabled> (I know that's redundant because Base UI applies aria-disabled="true" but in my actual project I render a component that accepts a disabled prop both for styling and functionality, and it ultimately renders <button disabled>), then that Menu.Item doesn't get the data-highlighted attribute on hover... but it still does when navigating to it with the keyboard. 🤔

Expected behavior

At the very least, I wouldn't expect render={<button disabled /> to change the functionality of an already-disabled Menu.Item (or Menu.RadioItem).

But also, I would personally prefer if disabled menu items were not highlighted, which I can handle in CSS for the hover part, but not for the keyboard part (the disabled items will not be skipped, so I have to style them differently when they're highlighted).

Reproducible example

https://codesandbox.io/p/sandbox/zen-jerry-xst8gg

Base UI version

1.0.0-alpha.8

Which browser are you using?

Chrome, Safari, and Firefox

Which OS are you using?

macOS

Which assistive tech are you using (if applicable)?

None

[benface]

Not sure how it happened, but the CodeSandbox URL was wrong (it was the default one that you provide in the docs for the Menu component). I just updated it, and also confirmed that this is still an issue in version 1.0.0-alpha.8.

[benface]

I also found a strange inconsistency: if I use the render function of Menu.Item to render a <button>, and more specifically in the case of a disabled menu item, a <button disabled> (I know that's redundant because Base UI applies aria-disabled="true" but in my actual project I render a component that accepts a disabled prop both for styling and functionality, and it ultimately renders <button disabled>), then that Menu.Item doesn't get the data-highlighted attribute on hover... but it still does when navigating to it with the keyboard. 🤔

It's actually worse than this. The render={<button disabled />} thing makes the menu item "clickable" with the keyboard (i.e. you can press Enter or Space when it's highlighted) but it actually executes the previous menu item's onClick! I updated the CodeSandbox in my OP: try navigating to the 3rd item with the keyboard and then pressing Enter or Space, and you'll see "Clicked on Not Disabled" instead of "Clicked on Disabled Button".

[atomiks]

macOS briefly (in Sequoia) did allow disabled menu items to be focused, but it seems like a recent update reverted that.

Anyway, we went off the ARIA pattern here: https://www.w3.org/WAI/ARIA/apg/patterns/menubar/

Disabled menu items are focusable but cannot be activated.

Every menuitem in a menu is focusable, whether or not it is disabled. Indicate a menuitem is disabled by setting aria-disabled="true" on the element with the role.

[benface]

Huh, TIL. Thank you @atomiks! (I'm guessing you're keeping this issue open for the second half of it, the render={<button disabled /> making a disabled menu item clickable?)

[michaldudak]

At the very least, I wouldn't expect render={ to change the functionality of an already-disabled Menu.Item (or Menu.RadioItem).

That's not quite possible. Browsers can't focus truly disabled buttons, so by adding the disabled prop to the rendered button you're effectively preventing the highlighting logic and possibly introducing other issues (like calling the previous item's onClick). So our recommendation would be: don't pass disabled to the rendered button.

[benface]

Ok, got it @michaldudak. Closing this issue then, thank you for the explanation!