Fix keyboard form submission (#904)

snowystinger · web-flow · commit 320c0b4d3e57 · 2021-12-23T13:07:00.000-08:00
diff --git a/packages/@react-aria/button/src/useButton.ts b/packages/@react-aria/button/src/useButton.ts
@@ -96,8 +96,7 @@ export function useButton(props: AriaButtonProps<ElementType>, ref: RefObject<an
   if (allowFocusWhenDisabled) {
     focusableProps.tabIndex = isDisabled ? -1 : focusableProps.tabIndex;
   }
-  let buttonProps = mergeProps(focusableProps, pressProps);
-  buttonProps = mergeProps(buttonProps, filterDOMProps(props, {labelable: true}));
+  let buttonProps = mergeProps(focusableProps, pressProps, filterDOMProps(props, {labelable: true}));
 
   return {
     isPressed, // Used to indicate press state for visual
diff --git a/packages/@react-aria/checkbox/test/useCheckboxGroup.test.tsx b/packages/@react-aria/checkbox/test/useCheckboxGroup.test.tsx
@@ -10,18 +10,18 @@
  * governing permissions and limitations under the License.
  */
 
+import {act, render} from '@testing-library/react';
 import {AriaCheckboxGroupItemProps, AriaCheckboxGroupProps} from '@react-types/checkbox';
 import {CheckboxGroupState, useCheckboxGroupState} from '@react-stately/checkbox';
 import React, {useRef} from 'react';
-import {render} from '@testing-library/react';
 import {useCheckboxGroup, useCheckboxGroupItem} from '../';
 import userEvent from '@testing-library/user-event';
 
 function Checkbox({checkboxGroupState, ...props}: AriaCheckboxGroupItemProps & { checkboxGroupState: CheckboxGroupState }) {
   const ref = useRef<HTMLInputElement>();
   const {children} = props;
   const {inputProps} = useCheckboxGroupItem(props, checkboxGroupState, ref);
-  return <label>{children}<input ref={ref} {...inputProps} /></label>;
+  return <label><input ref={ref} {...inputProps} />{children}</label>;
 }
 
 function CheckboxGroup({groupProps, checkboxProps}: {groupProps: AriaCheckboxGroupProps, checkboxProps: AriaCheckboxGroupItemProps[]}) {
@@ -63,18 +63,18 @@ describe('useCheckboxGroup', () => {
     expect(checkboxes[1].value).toBe('cats');
     expect(checkboxes[2].value).toBe('dragons');
 
-    expect(checkboxes[0].checked).toBe(false);
-    expect(checkboxes[1].checked).toBe(false);
-    expect(checkboxes[2].checked).toBe(false);
+    expect(checkboxes[0].checked).toBeFalsy();
+    expect(checkboxes[1].checked).toBeFalsy();
+    expect(checkboxes[2].checked).toBeFalsy();
 
     let dragons = getByLabelText('Dragons');
     userEvent.click(dragons);
     expect(onChangeSpy).toHaveBeenCalledTimes(1);
     expect(onChangeSpy).toHaveBeenCalledWith(['dragons']);
 
-    expect(checkboxes[0].checked).toBe(false);
-    expect(checkboxes[1].checked).toBe(false);
-    expect(checkboxes[2].checked).toBe(true);
+    expect(checkboxes[0].checked).toBeFalsy();
+    expect(checkboxes[1].checked).toBeFalsy();
+    expect(checkboxes[2].checked).toBeTruthy();
   });
 
   it('can have a default value', () => {
@@ -159,13 +159,15 @@ describe('useCheckboxGroup', () => {
   });
 
   it('sets aria-disabled and makes checkboxes disabled when isDisabled is true', () => {
-    let {getAllByRole, getByRole} = render(
+    let groupOnChangeSpy = jest.fn();
+    let checkboxOnChangeSpy = jest.fn();
+    let {getAllByRole, getByRole, getByLabelText} = render(
       <CheckboxGroup
-        groupProps={{label: 'Favorite Pet', isDisabled: true}}
+        groupProps={{label: 'Favorite Pet', isDisabled: true, onChange: groupOnChangeSpy}}
         checkboxProps={[
           {value: 'dogs', children: 'Dogs'},
           {value: 'cats', children: 'Cats'},
-          {value: 'dragons', children: 'Dragons'}
+          {value: 'dragons', children: 'Dragons', onChange: checkboxOnChangeSpy}
         ]} />
     );
 
@@ -174,8 +176,15 @@ describe('useCheckboxGroup', () => {
 
     let checkboxes = getAllByRole('checkbox') as HTMLInputElement[];
     expect(checkboxes[0]).toHaveAttribute('disabled');
-    expect(checkboxes[0]).toHaveAttribute('disabled');
-    expect(checkboxes[0]).toHaveAttribute('disabled');
+    expect(checkboxes[1]).toHaveAttribute('disabled');
+    expect(checkboxes[2]).toHaveAttribute('disabled');
+    let dragons = getByLabelText('Dragons');
+
+    act(() => {userEvent.click(dragons);});
+
+    expect(groupOnChangeSpy).toHaveBeenCalledTimes(0);
+    expect(checkboxOnChangeSpy).toHaveBeenCalledTimes(0);
+    expect(checkboxes[2].checked).toBeFalsy();
   });
 
   it('doesn\'t set aria-disabled or make checkboxes disabled by default', () => {
@@ -194,8 +203,8 @@ describe('useCheckboxGroup', () => {
 
     let checkboxes = getAllByRole('checkbox') as HTMLInputElement[];
     expect(checkboxes[0]).not.toHaveAttribute('disabled');
-    expect(checkboxes[0]).not.toHaveAttribute('disabled');
-    expect(checkboxes[0]).not.toHaveAttribute('disabled');
+    expect(checkboxes[1]).not.toHaveAttribute('disabled');
+    expect(checkboxes[2]).not.toHaveAttribute('disabled');
   });
 
   it('doesn\'t set aria-disabled or make checkboxes disabled when isDisabled is false', () => {
@@ -214,25 +223,35 @@ describe('useCheckboxGroup', () => {
 
     let checkboxes = getAllByRole('checkbox') as HTMLInputElement[];
     expect(checkboxes[0]).not.toHaveAttribute('disabled');
-    expect(checkboxes[0]).not.toHaveAttribute('disabled');
-    expect(checkboxes[0]).not.toHaveAttribute('disabled');
+    expect(checkboxes[1]).not.toHaveAttribute('disabled');
+    expect(checkboxes[2]).not.toHaveAttribute('disabled');
   });
 
   it('sets aria-readonly="true" on each checkbox', () => {
-    let {getAllByRole} = render(
+    let groupOnChangeSpy = jest.fn();
+    let checkboxOnChangeSpy = jest.fn();
+    let {getAllByRole, getByLabelText} = render(
       <CheckboxGroup
-        groupProps={{label: 'Favorite Pet', isReadOnly: true}}
+        groupProps={{label: 'Favorite Pet', isReadOnly: true, onChange: groupOnChangeSpy}}
         checkboxProps={[
           {value: 'dogs', children: 'Dogs'},
           {value: 'cats', children: 'Cats'},
-          {value: 'dragons', children: 'Dragons'}
+          {value: 'dragons', children: 'Dragons', onChange: checkboxOnChangeSpy}
         ]} />
     );
 
     let checkboxes = getAllByRole('checkbox') as HTMLInputElement[];
     expect(checkboxes[0]).toHaveAttribute('aria-readonly', 'true');
     expect(checkboxes[1]).toHaveAttribute('aria-readonly', 'true');
     expect(checkboxes[2]).toHaveAttribute('aria-readonly', 'true');
+    expect(checkboxes[2].checked).toBeFalsy();
+    let dragons = getByLabelText('Dragons');
+
+    act(() => {userEvent.click(dragons);});
+
+    expect(groupOnChangeSpy).toHaveBeenCalledTimes(0);
+    expect(checkboxOnChangeSpy).toHaveBeenCalledTimes(0);
+    expect(checkboxes[2].checked).toBeFalsy();
   });
 
   it('should not update state for readonly checkbox', () => {
@@ -255,6 +274,6 @@ describe('useCheckboxGroup', () => {
 
     expect(groupOnChangeSpy).toHaveBeenCalledTimes(0);
     expect(checkboxOnChangeSpy).toHaveBeenCalledTimes(0);
-    expect(checkboxes[2].checked).toBe(false);
+    expect(checkboxes[2].checked).toBeFalsy();
   });
 });
diff --git a/packages/@react-aria/interactions/src/usePress.ts b/packages/@react-aria/interactions/src/usePress.ts
@@ -103,7 +103,7 @@ export function usePress(props: PressHookProps): PressResult {
     shouldCancelOnPointerExit,
     allowTextSelectionOnPress,
     // eslint-disable-next-line @typescript-eslint/no-unused-vars
-    ref: _, // Removing `ref` from `domProps` because TypeScript is dumb,
+    ref: _, // Removing `ref` from `domProps` because TypeScript is dumb
     ...domProps
   } = usePressResponderContext(props);
   let propsRef = useRef<PressHookProps>(null);
@@ -229,7 +229,9 @@ export function usePress(props: PressHookProps): PressResult {
     let pressProps: HTMLAttributes<HTMLElement> = {
       onKeyDown(e) {
         if (isValidKeyboardEvent(e.nativeEvent) && e.currentTarget.contains(e.target as HTMLElement)) {
-          e.preventDefault();
+          if (shouldPreventDefaultKeyboard(e.target as Element)) {
+            e.preventDefault();
+          }
           e.stopPropagation();
 
           // If the event is repeating, it may have started on a different element
@@ -283,7 +285,9 @@ export function usePress(props: PressHookProps): PressResult {
 
     let onKeyUp = (e: KeyboardEvent) => {
       if (state.isPressed && isValidKeyboardEvent(e)) {
-        e.preventDefault();
+        if (shouldPreventDefaultKeyboard(e.target as Element)) {
+          e.preventDefault();
+        }
         e.stopPropagation();
 
         state.isPressed = false;
@@ -765,6 +769,10 @@ function shouldPreventDefault(target: Element) {
   return !target.closest('[draggable="true"]');
 }
 
+function shouldPreventDefaultKeyboard(target: Element) {
+  return !((target.tagName === 'INPUT' || target.tagName === 'BUTTON') && (target as HTMLButtonElement | HTMLInputElement).type === 'submit');
+}
+
 function isVirtualPointerEvent(event: PointerEvent) {
   // If the pointer size is zero, then we assume it's from a screen reader.
   // Android TalkBack double tap will sometimes return a event with width and height of 1
diff --git a/packages/@react-spectrum/button/test/Button.test.js b/packages/@react-spectrum/button/test/Button.test.js
@@ -11,7 +11,10 @@
  */
 
 import {ActionButton, Button, ClearButton, LogicButton} from '../';
+import {Checkbox, defaultTheme} from '@adobe/react-spectrum';
 import {fireEvent, render} from '@testing-library/react';
+import {Form} from '@react-spectrum/form';
+import {Provider} from '@react-spectrum/provider';
 import React from 'react';
 import {triggerPress} from '@react-spectrum/test-utils';
 
@@ -188,13 +191,6 @@ describe('Button', function () {
     expect(onPressSpy).not.toHaveBeenCalled();
   });
 
-  // when a user uses the keyboard and keyDowns 'enter' or 'space' on a button, it fires an onclick.
-  // when code dispatches a keyDown for 'enter' or 'space', it does not fire onclick
-  // this means that it's impossible for us to write a test for the 'button' elementType for keyDown 'enter' or 'space'
-  // see https://jsfiddle.net/snowystinger/z6vmrw4d/1/
-  // it's also extraneous to test with 'enter' or 'space' on a button because it'd just be testing
-  // the spec https://www.w3.org/TR/WCAG20-TECHS/SCR35.html
-
   it.each`
     Name                | Component
     ${'ActionButton'}   | ${ActionButton}
@@ -206,4 +202,78 @@ describe('Button', function () {
     let button = getByRole('button');
     expect(document.activeElement).toBe(button);
   });
+
+
+  it('prevents default for non-submit types', function () {
+    let eventDown;
+    let eventUp;
+    let btn = React.createRef();
+    let {getByRole} = render(
+      <Provider theme={defaultTheme}>
+        <Form>
+          <Checkbox>An Input</Checkbox>
+          <Button variant="primary" ref={btn}>Click Me</Button>
+        </Form>
+      </Provider>
+    );
+    // need to attach event listeners after instead of directly on Button because the ones directly on Button
+    // will fire before the usePress ones
+    btn.current.UNSAFE_getDOMNode().addEventListener('keydown', e => eventDown = e);
+    btn.current.UNSAFE_getDOMNode().addEventListener('keyup', e => eventUp = e);
+
+    let button = getByRole('button');
+    fireEvent.keyDown(button, {key: 'Enter'});
+    fireEvent.keyUp(button, {key: 'Enter'});
+    expect(eventDown.defaultPrevented).toBeTruthy();
+    expect(eventUp.defaultPrevented).toBeTruthy();
+
+    fireEvent.keyDown(button, {key: ' '});
+    fireEvent.keyUp(button, {key: ' '});
+    expect(eventDown.defaultPrevented).toBeTruthy();
+    expect(eventUp.defaultPrevented).toBeTruthy();
+  });
+
+  // we only need to test that we allow the browser to do the default thing when space or enter is pressed on a submit button
+  // space submits on key up and is actually a click
+  it('submit in form using space', function () {
+    let eventUp;
+    let btn = React.createRef();
+    let {getByRole} = render(
+      <Provider theme={defaultTheme}>
+        <Form>
+          <Checkbox>An Input</Checkbox>
+          <Button variant="primary" type="submit" ref={btn}>Click Me</Button>
+        </Form>
+      </Provider>
+    );
+    btn.current.UNSAFE_getDOMNode().addEventListener('keyup', e => eventUp = e);
+
+    let button = getByRole('button');
+    fireEvent.keyDown(button, {key: ' '});
+    fireEvent.keyUp(button, {key: ' '});
+    expect(eventUp.defaultPrevented).toBeFalsy();
+  });
+
+  // enter submits on keydown
+  it('submit in form using enter', function () {
+    let eventDown;
+    let btn = React.createRef();
+    let {getByRole} = render(
+      <Provider theme={defaultTheme}>
+        <Form>
+          <Checkbox>An Input</Checkbox>
+          <Button variant="primary" type="submit" ref={btn}>Click Me</Button>
+        </Form>
+      </Provider>
+    );
+    btn.current.UNSAFE_getDOMNode().addEventListener('keydown', e => eventDown = e);
+
+    let button = getByRole('button');
+    fireEvent.keyDown(button, {key: 'Enter'});
+    fireEvent.keyUp(button, {key: 'Enter'});
+    expect(eventDown.defaultPrevented).toBeFalsy();
+  });
+
+
+  // 'implicit submission' can't be tested https://github.com/testing-library/react-testing-library/issues/487
 });
diff --git a/packages/@react-spectrum/checkbox/test/Checkbox.test.js b/packages/@react-spectrum/checkbox/test/Checkbox.test.js
@@ -10,9 +10,9 @@
  * governing permissions and limitations under the License.
  */
 
+import {act, render} from '@testing-library/react';
 import {Checkbox} from '../';
 import React from 'react';
-import {render} from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 
 
@@ -233,4 +233,18 @@ describe('Checkbox', function () {
     expect(checkbox.checked).toBeTruthy();
     expect(onChangeSpy).not.toHaveBeenCalled();
   });
+
+  it.each`
+    Name                       | Component      | props
+    ${'Checkbox'}              | ${Checkbox}    | ${{onChange: onChangeSpy, isReadOnly: true}}
+  `('$Name supports uncontrolled readOnly', function ({Component, props}) {
+    let {getByLabelText} = render(<Component {...props}>Click Me</Component>);
+
+    let checkbox = getByLabelText('Click Me');
+    expect(checkbox.checked).toBeFalsy();
+
+    act(() => {userEvent.click(checkbox);});
+    expect(checkbox.checked).toBeFalsy();
+    expect(onChangeSpy).not.toHaveBeenCalled();
+  });
 });
diff --git a/packages/@react-spectrum/form/stories/Form.stories.tsx b/packages/@react-spectrum/form/stories/Form.stories.tsx
@@ -273,7 +273,7 @@ function FormWithControls(props: any = {}) {
 
   return (
     <Flex>
-      <Checkbox isSelected={preventDefault} onChange={setPreventDefault}>Prevent Default onSubmit</Checkbox>
+      <Checkbox alignSelf="start" isSelected={preventDefault} onChange={setPreventDefault}>Prevent Default onSubmit</Checkbox>
       <Form
         onSubmit={e => {
           action('onSubmit')(e);

Original file line number	Diff line number	Diff line change
`@@ -96,8 +96,7 @@ export function useButton(props: AriaButtonProps<ElementType>, ref: RefObject<an`
`96`	`96`	`if (allowFocusWhenDisabled) {`
`97`	`97`	`focusableProps.tabIndex = isDisabled ? -1 : focusableProps.tabIndex;`
`98`	`98`	`}`
`99`		`- let buttonProps = mergeProps(focusableProps, pressProps);`
`100`		`- buttonProps = mergeProps(buttonProps, filterDOMProps(props, {labelable: true}));`
	`99`	`+ let buttonProps = mergeProps(focusableProps, pressProps, filterDOMProps(props, {labelable: true}));`
`101`	`100`
`102`	`101`	`return {`
`103`	`102`	`isPressed, // Used to indicate press state for visual`