[Menu] Fix prop merging issues

[michaldudak]

1. Made it possible to prevent default Base UI logic by calling preventBaseUIHandler in event handlers (fixes [Menu] Trigger's default behavior cannot be prevented #1441) (before / after).
2. Fixed the behavior of disabled non-interactive triggers (i.e., <Menu.Trigger disabled render={<span />} />). Clicking on such a trigger used to open the menu anyway. (before / after)
3. Removed mergeReactProps calls where they were not necessary.
4. Removed the disabled attribute from SubmenuTrigger. The submenus should be disabled by setting disabled on the Root part.
5. Updated the fully featured menu experiment to cover more cases.

The main problem in 1. was that event handlers returned from Floating UI's useInteractions didn't respect preventBaseUIHandler as they have their own prop merging logic.
Instead of using these prop getters directly, I now call them without arguments and pass the result to our mergeReactProps function.

Another issue I found was that we had to nest mergeReactProps calls and props getters, such as getButtonProps(mergeReactProps(external, internal)). In some cases this was confusing and error-prone (point 2.), so I updated mergeReactProps to accept either props directly or a props getter function.

When a function is provided to mergeReactProps, it will be called with all the props defined to the right of it, so mergeReactProps(obj1, obj2, fn, obj3, obj4), will be equivalent to mergeReactProps(obj, obj2, fn(mergeReactProps(obj3, obj4))).
I assume that in such cases it is the props getter functionality to merge incoming and internal props and prevent default logic if needed.

[netlify]

✅ Deploy Preview for base-ui ready!

Name	Link
🔨 Latest commit	3f2d30a
🔍 Latest deploy log	https://app.netlify.com/sites/base-ui/deploys/67b6f6d2f7f886000801a0cc
😎 Deploy Preview	https://deploy-preview-1445--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[mj12albert]

When a function is provided to mergeReactProps, it will be called with all the props defined to the right of it

If I understand correctly, this: getButtonProps(mergeReactProps(external, internal))
now becomes this? mergeReactProps(getButtonProps, externalProps, internalProps)

[michaldudak]

If I understand correctly, this: getButtonProps(mergeReactProps(external, internal))
now becomes this? mergeReactProps(getButtonProps, externalProps, internalProps)

Yup, that's right.

[mj12albert]

Fixed the behavior of disabled non-interactive triggers

I also encountered this when these are still <button>s but "disabled" using aria-disabled+data-disabled and not the disabled attr to keep them focusable-when-disabled:

base-ui/packages/react/src/use-button/useButton.ts

Lines 92 to 129 in 6a00daf

	onKeyDown(event: React.KeyboardEvent) {
	if (
	disabled ||
	(event.target === event.currentTarget && !isNativeButton() && event.key === ' ')
	) {
	event.preventDefault();
	}
	// Keyboard accessibility for non interactive elements
	if (
	event.target === event.currentTarget &&
	!isNativeButton() &&
	!isValidLink() &&
	event.key === 'Enter' &&
	!disabled
	) {
	onClickProp(event);
	event.preventDefault();
	}
	},
	onKeyUp(event: React.KeyboardEvent) {
	// calling preventDefault in keyUp on a <button> will not dispatch a click event if Space is pressed
	// https://codesandbox.io/p/sandbox/button-keyup-preventdefault-dn7f0
	// Keyboard accessibility for non interactive elements
	if (
	event.target === event.currentTarget &&
	!isNativeButton() &&
	!disabled &&
	event.key === ' '
	) {
	onClickProp(event);
	}
	},
	onPointerDown(event: React.PointerEvent) {
	if (disabled) {
	event.preventDefault();
	}
	},

This fix looks more robust 😄
In mine, when the disabled menu trigger is focused, Up/Down arrows are still opening the menu (Enter/Space doesn't)

[michaldudak]

I figured that the disabled button shouldn't respond to key presses the same way it doesn't respond to clicks.

[atomiks]

Can you elaborate more on this? Will all event handlers specified in our internal prop getters for components work by default without needing a check?

[michaldudak]

Whenever a function is passed to mergeReactProps, it will be called with the merged props and the preventBaseUIHandler is not taken into consideration. Most of our prop getter functions actually call mergeReactProps internally, with one notable exception: useButton. In its case, the logic for prop merging is more complex as it depends on the button's disabled state.

In the case of functions returned from Floating UI's useInteractions, they don't know anything about preventBaseUIHandler, so I'm calling them without parameters and provide the result to mergeReactProps so our merging logic is run on them. This alone should be sufficient to fix 1., but I refactored mergeReactProps to avoid having to nest prop getters and help prevent bugs like 2.

[atomiks]

The ordering logic makes sense to me

[mj12albert]

2. Fixed the behavior of disabled non-interactive triggers

Was just testing this and found that dialog also has this issue:
https://codesandbox.io/p/sandbox/clever-alex-6472d7

I think triggers that currently don't use useButton (Dialog, AlertDialog, Popover) need to start using it with the fixes in this PR

[michaldudak]

Yes, there's possibly more components with these problems. I started with the Menu to keep the PR size managable.