[menu] What is disabling `Menu.Root` supposed to do?

[benface]

Bug report

Current behavior

When setting a Menu.Root to disabled, the Menu.Trigger becomes disabled, but it can already be disabled on its own. If the menu happens to be open while disabled (for instance if forced with the open prop), it seems like the intention is to also disable all the Menu.Items it contains because they don't get data-highlighted when hovered, but accessibility-wise they are still enabled (no aria-disabled="true", and they can be clicked).

Expected behavior

Not sure:

• Most likely, the Menu.Items are meant to be disabled when the Menu.Root is disabled.
• ...or Menu.Root's disabled prop is only meant to prevent the menu from opening (but if that's the case, it seems redundant with Menu.Trigger's disabled prop)

Reproducible example

https://codesandbox.io/p/sandbox/cool-worker-58fxvw

Base UI version

v1.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]

I just noticed that Menu.Root's disabled prop was the only way to disable an item that opens a submenu. I feel like it would make sense for Menu.SubmenuTrigger to accept a disabled prop, but it doesn't.

[mj12albert]

I expected <Menu.Root disabled> to prevent the menu from being opened externally but it doesn't 🤔

<button onClick={() => setOpen(!open)}>ext trigger</button>
// still opens
<Menu.Root open={open} onOpenChange={setOpen} disabled />

[aarongarciah]

Potential related bug: it appears that Menu.Trigger only receives the data-disabled attribute when the disabled prop is applied to Menu.Trigger, but not when it is applied to Menu.Root.

[michaldudak]

We should remove the disabled props from Trigger and SubmenuTrigger.

I expected <Menu.Root disabled> to prevent the menu from being opened externally but it doesn't

Since you're controlling it, we assume you know better. We can't say no to you setting the state explicitly.

[benface]

We should remove the disabled props from Trigger and SubmenuTrigger.

I was hoping you would add it to SubmenuTrigger, haha.

But regardless of this, should a disabled Menu.Root add aria-disabled etc. to nested Menu.Items? I assume so?

[michaldudak]

But regardless of this, should a disabled Menu.Root add aria-disabled etc. to nested Menu.Items?

Yes, I'm going to make it work like that.

I was hoping you would add it to SubmenuTrigger, haha.

It makes more sense to have it on the root, as there's whole lot of logic that depends on this prop. We could hoist it from the Trigger to the nearest Root, but this would take one render more (~and wouldn't work during SSR~ EDIT: that's actually incorrect, see my next comment), so having it on the Root seems to be the only sensible option.

[mj12albert]

We should remove the disabled props from Trigger and SubmenuTrigger.

Would this apply to all popups with XXX.Trigger and similar parts/anatomy?

[michaldudak]

Actually, the more I work on it, the less I'm sure the prop should be on the root :/

Obviously, having it on the root will ensure that all interaction hooks work correctly on the first render. However, it's the Trigger that needs to know if it's disabled immediately (especially during SSR). What's more, having it on the Root doesn't play nicely with the Toolbar (to effectively disable a toolbar menu button, we'd have to place disabled on both Menu.Root and Toolbar.Button, which isn't great DX (see https://github.com/mui/base-ui/pull/1984).

I'll explore the opposite solution next - placing the prop on the Trigger (and SubmenuTrigger).

Would this apply to all popups with XXX.Trigger and similar parts/anatomy?

I suppose so. Let's choose a solution first and see if it makes sense on a case-by-case basis.