[Select] Fix listbox closing on Space keyUp (#18754)

mui · Dec 10, 2019 · e501325 · e501325
1 parent 1bb4ba0
commit e501325
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 3 deletions.
diff --git a/packages/material-ui/src/ButtonBase/ButtonBase.js b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -219,7 +219,15 @@ const ButtonBase = React.forwardRef(function ButtonBase(props, ref) {
     }
   });
   const handleKeyUp = useEventCallback(event => {
-    if (focusRipple && event.key === ' ' && rippleRef.current && focusVisible) {
+    // calling preventDefault in keyUp on a <button> will not dispatch a click event if Space is pressed
+    // https://codesandbox.io/s/button-keyup-preventdefault-dn7f0
+    if (
+      focusRipple &&
+      event.key === ' ' &&
+      rippleRef.current &&
+      focusVisible &&
+      !event.defaultPrevented
+    ) {
       keydownRef.current = false;
       event.persist();
       rippleRef.current.stop(event, () => {
@@ -231,7 +239,12 @@ const ButtonBase = React.forwardRef(function ButtonBase(props, ref) {
     }
 
     // Keyboard accessibility for non interactive elements
-    if (event.target === event.currentTarget && isNonNativeButton() && event.key === ' ') {
+    if (
+      event.target === event.currentTarget &&
+      isNonNativeButton() &&
+      event.key === ' ' &&
+      !event.defaultPrevented
+    ) {
       event.preventDefault();
       if (onClick) {
         onClick(event);

diff --git a/packages/material-ui/src/ButtonBase/ButtonBase.test.js b/packages/material-ui/src/ButtonBase/ButtonBase.test.js
@@ -715,6 +715,32 @@ describe('<ButtonBase />', () => {
         expect(onClickSpy.returnValues[0]).to.equal(true);
       });
 
+      it('does not call onClick when a spacebar is released and the default is prevented', () => {
+        const onClickSpy = spy(event => event.defaultPrevented);
+        const { getByRole } = render(
+          <ButtonBase
+            onClick={onClickSpy}
+            onKeyUp={
+              /**
+               * @param {React.SyntheticEvent} event
+               */
+              event => event.preventDefault()
+            }
+            component="div"
+          >
+            Hello
+          </ButtonBase>,
+        );
+        const button = getByRole('button');
+        button.focus();
+
+        fireEvent.keyUp(document.activeElement || document.body, {
+          key: ' ',
+        });
+
+        expect(onClickSpy.callCount).to.equal(0);
+      });
+
       it('calls onClick when Enter is pressed on the element', () => {
         const onClickSpy = spy(event => event.defaultPrevented);
         const { getByRole } = render(

diff --git a/packages/material-ui/src/Select/Select.test.js b/packages/material-ui/src/Select/Select.test.js
@@ -148,8 +148,10 @@ describe('<Select />', () => {
       getByRole('button').focus();
 
       fireEvent.keyDown(document.activeElement, { key });
+      expect(getByRole('listbox', { hidden: false })).to.be.ok;
 
-      expect(getByRole('listbox')).to.be.ok;
+      fireEvent.keyUp(document.activeElement, { key });
+      expect(getByRole('listbox', { hidden: false })).to.be.ok;
     });
   });
 
@@ -485,7 +487,9 @@ describe('<Select />', () => {
       getByRole('button').focus();
 
       fireEvent.keyDown(document.activeElement, { key: 'ArrowDown' });
+      expect(queryByRole('listbox')).not.to.be.ok;
 
+      fireEvent.keyUp(document.activeElement, { key: 'ArrowDown' });
       expect(queryByRole('listbox')).not.to.be.ok;
     });
   });
@@ -871,4 +875,20 @@ describe('<Select />', () => {
       expect(getByLabelText('A select')).to.have.property('tagName', 'SELECT');
     });
   });
+
+  it('prevents the default when releasing Space on the children', () => {
+    const keyUpSpy = spy(event => event.defaultPrevented);
+    render(
+      <Select value="one" open>
+        <MenuItem onKeyUp={keyUpSpy} value="one">
+          One
+        </MenuItem>
+      </Select>,
+    );
+
+    fireEvent.keyUp(document.activeElement, { key: ' ' });
+
+    expect(keyUpSpy.callCount).to.equal(1);
+    expect(keyUpSpy.returnValues[0]).to.equal(true);
+  });
 });
diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js
@@ -257,6 +257,18 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
     return React.cloneElement(child, {
       'aria-selected': selected ? 'true' : undefined,
       onClick: handleItemClick(child),
+      onKeyUp: event => {
+        if (event.key === ' ') {
+          // otherwise our MenuItems dispatches a click event
+          // it's not behavior of the native <option> and causes
+          // the select to close immediately since we open on space keydown
+          event.preventDefault();
+        }
+        const { onKeyUp } = child.props;
+        if (typeof onKeyUp === 'function') {
+          onKeyUp(event);
+        }
+      },
       role: 'option',
       selected,
       value: undefined, // The value is most likely not a valid HTML attribute.