[RAC] Checkbox should not be toggled with 'Enter' key

[sookmax]

Provide a general summary of the issue here

Based on the code below and https://www.w3.org/WAI/ARIA/apg/patterns/checkbox/#keyboardinteraction, it should be the case that 'Enter' key shouldn't invoke Checkbox toggling: https://github.com/adobe/react-spectrum/blob/b294de874c16fad64bc4b930ccee1c9ffeb1c20f/packages/%40react-aria/interactions/src/usePress.ts#L904-L909

But actually it does:

https://github.com/adobe/react-spectrum/assets/71210554/93a38035-2f43-469d-980a-4a6772c6eaf1

React Spectrum Checkbox, on the other hand, doesn't suffer from this issue as shown in the video above.

I did some digging and found out the element below when the key is 'Enter' could actually be HTMLLabelElement in addition to HTMLInputElement, rendering the second condition in the return expression (three lines starting with !) true when it's HTMLLabelElement and hence recognized as a valid key: https://github.com/adobe/react-spectrum/blob/d80999e897b4d4db9fcfb4e9b8fcdc9fdd700882/packages/%40react-aria/interactions/src/usePress.ts#L767-L781

On the other hand, when 'Space' is pressed, it looks like the element above only evaluates to HTMLInputElement, which is desirable.

I think this is due to the double usage of usePress() in useToggle() below: https://github.com/adobe/react-spectrum/blob/d80999e897b4d4db9fcfb4e9b8fcdc9fdd700882/packages/%40react-aria/toggle/src/useToggle.ts#L66-L77

where one pressProps is fed into labelProps and the other into inputProps.

🤔 Expected Behavior?

Checkbox should not be toggled with 'Enter' key

😯 Current Behavior

Checkbox can be toggled with 'Enter' key

💁 Possible Solution

-- (element instanceof getOwnerWindow(element).HTMLInputElement && !isValidInputKey(element, key))
++ ((element instanceof getOwnerWindow(element).HTMLInputElement || element instanceof getOwnerWindow(element).HTMLLabelElement) && !isValidInputKey(element, key))

The diff above would fix it, but I'm not sure it's the best way to go about it.

🔦 Context

No response

🖥️ Steps to Reproduce

RAC Checkbox (can be toggled on/off with Enter key)

https://react-spectrum.adobe.com/react-aria/Checkbox.html#example

React Spectrum Checkbox (cannot be toggled on/off with Enter key)

https://react-spectrum.adobe.com/react-spectrum/Checkbox.html#example

Version

1.2.0

What browsers are you seeing the problem on?

Chrome

If other, please specify.

No response

What operating system are you using?

macOS Sonoma 14.4.1

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

[snowystinger]

Your analysis looks correct. I'm also not too sure about the fix for it, but this seems like a reasonable starting place. Thank you for the very detailed issue!

[sookmax]

I've looked into this a little more and realized the way RAC Checkbox's <label> and <input> are laid out is fundamentally different than that of React Spectrum's.

To be more specific, it looks like <label> handles all the Checkbox interactions while <input> is hidden away visually in a RAC Checkbox, whereas the situation for a React Spectrum Checkbox is quite the opposite—<input> is styled in a way that it sits on top of <label> and other elements that are part of a Checkbox and handles all the interaction itself.

So because of this duality of the roles of <label> and <input>, I now think adding checks for HTMLLabelElement wherever HTMLInputElement is checked in usePress() is a valid way to fix this issue.

Specifically in these functions: https://github.com/adobe/react-spectrum/blob/d80999e897b4d4db9fcfb4e9b8fcdc9fdd700882/packages/%40react-aria/interactions/src/usePress.ts#L767-L781

https://github.com/adobe/react-spectrum/blob/d80999e897b4d4db9fcfb4e9b8fcdc9fdd700882/packages/%40react-aria/interactions/src/usePress.ts#L876-L890

https://github.com/adobe/react-spectrum/blob/d80999e897b4d4db9fcfb4e9b8fcdc9fdd700882/packages/%40react-aria/interactions/src/usePress.ts#L904-L909

Lastly, I think we shouldn't pass pressProps, which is baked-in in inputProps returned from useToggle() to the visually hidden <input> element in a RAC Checkbox since pressProps contains interaction-related event handlers, and said <input> in the RAC Checkbox is not supposed to be interacted with (not sure how to efficiently filter out those props though..)

[zachbwh]

Bumping this issue as I'm coming against this problem now. Not sure what the fix is but thanks for the very detailed write up @sookmax!

[zachbwh]

Hmm it seems like this issue could be fixed in the latest main branch? Potentially from this usePress refactor? Maybe @snowystinger can confirm :)

[snowystinger]

Looks like it has been fixed. Thanks for the ping. Here's the latest on main https://reactspectrum.blob.core.windows.net/reactspectrum/cfcd697664730b4750f5acbfd81219754914e8e1/verdaccio/docs/react-aria/Checkbox.html

It'll be in our next release