[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:

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

Lines 904 to 909 in b294de8

	function isValidInputKey(target: HTMLInputElement, key: string) {
	// Only space should toggle checkboxes and radios, not enter.
	return target.type === 'checkbox' || target.type === 'radio'
	? key === ' '
	: nonTextInputTypes.has(target.type);
	}

But actually it does:

Screen.Recording.2024-05-17.at.12.30.39.AM.mov

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:

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

Lines 767 to 781 in d80999e

	function isValidKeyboardEvent(event: KeyboardEvent, currentTarget: Element): boolean {
	const {key, code} = event;
	const element = currentTarget as HTMLElement;
	const role = element.getAttribute('role');
	// Accessibility for keyboards. Space and Enter only.
	// "Spacebar" is for IE 11
	return (
	(key === 'Enter' || key === ' ' || key === 'Spacebar' || code === 'Space') &&
	!((element instanceof getOwnerWindow(element).HTMLInputElement && !isValidInputKey(element, key)) ||
	element instanceof getOwnerWindow(element).HTMLTextAreaElement ||
	element.isContentEditable) &&
	// Links should only trigger with Enter key
	!((role === 'link' || (!role && isHTMLAnchorLink(element))) && key !== 'Enter')
	);
	}

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:

react-spectrum/packages/@react-aria/toggle/src/useToggle.ts

Lines 66 to 77 in d80999e

	// This handles focusing the input on pointer down, which Safari does not do by default.
	let {pressProps, isPressed} = usePress({
	isDisabled
	});
	// iOS does not toggle checkboxes if you drag off and back onto the label, so handle it ourselves.
	let {pressProps: labelProps, isPressed: isLabelPressed} = usePress({
	isDisabled: isDisabled || isReadOnly,
	onPress() {
	state.toggle();
	}
	});

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:

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

Lines 767 to 781 in d80999e

	function isValidKeyboardEvent(event: KeyboardEvent, currentTarget: Element): boolean {
	const {key, code} = event;
	const element = currentTarget as HTMLElement;
	const role = element.getAttribute('role');
	// Accessibility for keyboards. Space and Enter only.
	// "Spacebar" is for IE 11
	return (
	(key === 'Enter' || key === ' ' || key === 'Spacebar' || code === 'Space') &&
	!((element instanceof getOwnerWindow(element).HTMLInputElement && !isValidInputKey(element, key)) ||
	element instanceof getOwnerWindow(element).HTMLTextAreaElement ||
	element.isContentEditable) &&
	// Links should only trigger with Enter key
	!((role === 'link' || (!role && isHTMLAnchorLink(element))) && key !== 'Enter')
	);
	}

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

Lines 876 to 890 in d80999e

	function shouldPreventDefaultKeyboard(target: Element, key: string) {
	if (target instanceof HTMLInputElement) {
	return !isValidInputKey(target, key);
	}
	if (target instanceof HTMLButtonElement) {
	return target.type !== 'submit' && target.type !== 'reset';
	}
	if (isHTMLAnchorLink(target)) {
	return false;
	}
	return true;
	}

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

Lines 904 to 909 in d80999e

	function isValidInputKey(target: HTMLInputElement, key: string) {
	// Only space should toggle checkboxes and radios, not enter.
	return target.type === 'checkbox' || target.type === 'radio'
	? key === ' '
	: nonTextInputTypes.has(target.type);
	}

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..)