`useButton/usePress` should not propagate press events when `isDisabled`

[cevr]

Provide a general summary of the issue here

When a button is disabled, it still allows click events to propagate.

This is a problem when you have a pressable element nested within another pressable element (eg a card with a button in it).

This manifests for example if you disable a button when an action is loading.

🤔 Expected Behavior?

Even if the button is disabled, it still disallows click events to propagate.

😯 Current Behavior

When the button is disabled, click events are propagated.

https://stackblitz.com/edit/vitejs-vite-nyw2s2?file=src%2FApp.tsx

💁 Possible Solution

Disallow press event propagation when disabled

🔦 Context

This makes it so users accidentally fire the container element's onPress event if they try to press a disabled button - causing for a frustrating user experience.

🖥️ Steps to Reproduce

1. Create a container element that uses useButton
2. Create an inner element that uses useButton
3. Disable the inner element.
4. Watch events propagating through to the container element.

https://stackblitz.com/edit/vitejs-vite-nyw2s2?file=src%2FApp.tsx

Version

react-aria@3.32.1

What browsers are you seeing the problem on?

Chrome

If other, please specify.

No response

What operating system are you using?

MacOS

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

[LFDanLu]

Interesting, I'm not entirely sure the direction we'll want to go here since changing this behavior could be considered a breaking change if people were relying on the current behavior. Looks like native clicks aren't propagated to the parent but pointerdown is, so there is some precedence for both propagating/stopping propagation when a disabled element is interacted with. We could potentially make this non-breaking if we make this behavior opt in and keep the current behavior as the default. I'll bring this up with the team to see if there are any additional opinions.

[cevr]

@LFDanLu that makes sense. would you happen to know of any workarounds? i imagine not using isDisabled and adding the condition in the onPress would work

[LFDanLu]

That would probably be your best bet, but you'd need to make sure to make the button non-tabbable and have aria-disabled I imagine to mimic what a button with disabled does, otherwise it would be quite confusing to a screenreader user what that button is for.

[snowystinger]

A quick aside, but in your repro steps you have:

Create a container element that uses useButton
Create an inner element that uses useButton

this would be invalid, you can't have a button inside another button. https://w3c.github.io/aria/#button

Children Presentational: True

[cevr]

@snowystinger ah I see! does this also apply if i use usePress for the container? apologies for my ignorance

[snowystinger]

No worries!
usePress isn't a problem, it doesn't apply any roles.
useButton is because it applies the role="button"

That said, still be careful nesting interactive components, make sure all your users can access it and check the spec to see if the roles are applying are allowed.
Also if you make the container interactive and able to receive focus, then include a label/description and a role yourself.

[sookmax]

Not really related to the issue, but I've found that clicking the disabled button with the MacBook built-in trackpad ('Tap to click' enabled) in the example above doesn't trigger the propagation.

Perhaps (I'm superficially speculating without a deeper dive) it's because using trackpad with 'tap to click' enabled results in state.pointerType: 'virtual' in usePress() and the following if-statement (inside onClick()), which looks like it might set shouldStopPropagation to true while executing?

react-spectrum/packages/@react-aria/interactions/src/usePress.ts

Lines 314 to 332 in 3ebcc66

	// If triggered from a screen reader or by using element.click(),
	// trigger as if it were a keyboard click.
	if (!state.ignoreClickAfterPress && !state.ignoreEmulatedMouseEvents && !state.isPressed && (state.pointerType === 'virtual' || isVirtualClick(e.nativeEvent))) {
	// Ensure the element receives focus (VoiceOver on iOS does not do this)
	if (!isDisabled && !preventFocusOnPress) {
	focusWithoutScrolling(e.currentTarget);
	}
	let stopPressStart = triggerPressStart(e, 'virtual');
	let stopPressUp = triggerPressUp(e, 'virtual');
	let stopPressEnd = triggerPressEnd(e, 'virtual');
	shouldStopPropagation = stopPressStart && stopPressUp && stopPressEnd;
	}
	state.ignoreEmulatedMouseEvents = false;
	state.ignoreClickAfterPress = false;
	if (shouldStopPropagation) {
	e.stopPropagation();
	}

[cevr]

would there be a quick patch I could put out that would solve this issue in the meantime?

@LFDanLu would happen to know how I could approximate the effect of the isDisabled prop?

[LFDanLu]

If you are looking to patch usePress locally you could perhaps experiment with changing

react-spectrum/packages/@react-aria/interactions/src/usePress.ts

Lines 183 to 185 in cfd890e

	if (isDisabled || state.didFirePressStart) {
	return false;
	}

so that it returns true if isDisabled=true. As mentioned before this could breaking.

[snowystinger]

Closing as not changing.
The disabled button isn't actually propagating a press/click event. It's inert. This can be demonstrated here in Chrome https://stackblitz.com/edit/vitejs-vite-iwcthtdj?file=package.json,src%2FApp.tsx
You'll notice the onClick handler is never called on the disabled Button, and it is called on the parent with the target being the parent, meaning that the click never propagated down to the disabled child.