[ButtonBase] Fix space calling onClick on keyDown instead of keyUp (#18319)

Author: eps1lon

packages/material-ui/src/ButtonBase/ButtonBase.js

@@ -182,6 +182,11 @@ const ButtonBase = React.forwardRef(function ButtonBase(props, ref) {
     }
   });
 
+  const isNonNativeButton = () => {
+    const button = getButtonNode();
+    return component && component !== 'button' && !(button.tagName === 'A' && button.href);
+  };
+
   /**
    * IE 11 shim for https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/repeat
    */
@@ -206,15 +211,8 @@ const ButtonBase = React.forwardRef(function ButtonBase(props, ref) {
       onKeyDown(event);
     }
 
-    const button = getButtonNode();
     // Keyboard accessibility for non interactive elements
-    if (
-      event.target === event.currentTarget &&
-      component &&
-      component !== 'button' &&
-      (event.key === ' ' || event.key === 'Enter') &&
-      !(button.tagName === 'A' && button.href)
-    ) {
+    if (event.target === event.currentTarget && isNonNativeButton() && event.key === 'Enter') {
       event.preventDefault();
       if (onClick) {
         onClick(event);
@@ -232,6 +230,14 @@ const ButtonBase = React.forwardRef(function ButtonBase(props, ref) {
     if (onKeyUp) {
       onKeyUp(event);
     }
+
+    // Keyboard accessibility for non interactive elements
+    if (event.target === event.currentTarget && isNonNativeButton() && event.key === ' ') {
+      event.preventDefault();
+      if (onClick) {
+        onClick(event);
+      }
+    }
   });
 
   let ComponentProp = component;

packages/material-ui/src/ButtonBase/ButtonBase.test.js

@@ -680,7 +680,7 @@ describe('<ButtonBase />', () => {
     });
 
     describe('keyboard accessibility for non interactive elements', () => {
-      it('calls onClick when a spacebar is pressed on the element', () => {
+      it('does not call onClick when a spacebar is pressed on the element', () => {
         const onClickSpy = spy(event => event.defaultPrevented);
         const { getByRole } = render(
           <ButtonBase onClick={onClickSpy} component="div">
@@ -694,7 +694,24 @@ describe('<ButtonBase />', () => {
           key: ' ',
         });
 
-        expect(onClickSpy.calledOnce).to.equal(true);
+        expect(onClickSpy.callCount).to.equal(0);
+      });
+
+      it('does call onClick when a spacebar is released on the element', () => {
+        const onClickSpy = spy(event => event.defaultPrevented);
+        const { getByRole } = render(
+          <ButtonBase onClick={onClickSpy} component="div">
+            Hello
+          </ButtonBase>,
+        );
+        const button = getByRole('button');
+        button.focus();
+
+        fireEvent.keyUp(document.activeElement || document.body, {
+          key: ' ',
+        });
+
+        expect(onClickSpy.callCount).to.equal(1);
         // defaultPrevented?
         expect(onClickSpy.returnValues[0]).to.equal(true);
       });
@@ -735,20 +752,20 @@ describe('<ButtonBase />', () => {
         expect(onClickSpy.callCount).to.equal(0);
       });
 
-      it('does not call onClick if Space was pressed on a child', () => {
+      it('does not call onClick if Space was released on a child', () => {
         const onClickSpy = spy(event => event.defaultPrevented);
-        const onKeyDownSpy = spy();
+        const onKeyUpSpy = spy();
         render(
-          <ButtonBase onClick={onClickSpy} onKeyDown={onKeyDownSpy} component="div">
+          <ButtonBase onClick={onClickSpy} onKeyUp={onKeyUpSpy} component="div">
             <input autoFocus type="text" />
           </ButtonBase>,
         );
 
-        fireEvent.keyDown(document.activeElement, {
+        fireEvent.keyUp(document.activeElement, {
           key: ' ',
         });
 
-        expect(onKeyDownSpy.callCount).to.equal(1);
+        expect(onKeyUpSpy.callCount).to.equal(1);
         expect(onClickSpy.callCount).to.equal(0);
       });
 

packages/material-ui/src/Chip/Chip.js

@@ -277,7 +277,6 @@ const Chip = React.forwardRef(function Chip(props, ref) {
     label,
     onClick,
     onDelete,
-    onKeyDown,
     onKeyUp,
     size = 'medium',
     variant = 'default',
@@ -295,21 +294,6 @@ const Chip = React.forwardRef(function Chip(props, ref) {
     }
   };
 
-  const handleKeyDown = event => {
-    if (onKeyDown) {
-      onKeyDown(event);
-    }
-
-    // Ignore events from children of `Chip`.
-    if (event.currentTarget !== event.target) {
-      return;
-    }
-
-    if ([' ', 'Enter', 'Backspace', 'Delete', 'Escape'].indexOf(event.key) !== -1) {
-      event.preventDefault();
-    }
-  };
-
   const handleKeyUp = event => {
     if (onKeyUp) {
       onKeyUp(event);
@@ -409,7 +393,6 @@ const Chip = React.forwardRef(function Chip(props, ref) {
       aria-disabled={disabled ? true : undefined}
       tabIndex={clickable || onDelete ? 0 : undefined}
       onClick={onClick}
-      onKeyDown={handleKeyDown}
       onKeyUp={handleKeyUp}
       ref={handleRef}
       {...moreProps}

packages/material-ui/src/Chip/Chip.test.js

@@ -416,15 +416,15 @@ describe('<Chip />', () => {
         onClickSpy.resetHistory();
       });
 
-      it('should call onClick when `space` is pressed ', () => {
+      it('should call onClick when `space` is released ', () => {
         const preventDefaultSpy = spy();
         const spaceKeyDown = {
           preventDefault: preventDefaultSpy,
           key: ' ',
         };
         wrapper.find('div').simulate('keyDown', spaceKeyDown);
-        assert.strictEqual(preventDefaultSpy.callCount, 2);
-        assert.strictEqual(onClickSpy.callCount, 1);
+        assert.strictEqual(preventDefaultSpy.callCount, 0);
+        assert.strictEqual(onClickSpy.callCount, 0);
 
         const spaceKeyUp = {
           key: ' ',
@@ -441,7 +441,7 @@ describe('<Chip />', () => {
           key: 'Enter',
         };
         wrapper.find('div').simulate('keyDown', enterKeyDown);
-        assert.strictEqual(preventDefaultSpy.callCount, 2);
+        assert.strictEqual(preventDefaultSpy.callCount, 1);
         assert.strictEqual(onClickSpy.callCount, 1);
 
         const enterKeyUp = {
@@ -464,7 +464,7 @@ describe('<Chip />', () => {
           key: 'Backspace',
         };
         wrapper.find('div').simulate('keyDown', backspaceKeyDown);
-        assert.strictEqual(preventDefaultSpy.callCount, 1);
+        assert.strictEqual(preventDefaultSpy.callCount, 0);
         assert.strictEqual(onDeleteSpy.callCount, 0);
 
         const backspaceKeyUp = {
@@ -485,7 +485,7 @@ describe('<Chip />', () => {
           key: 'Delete',
         };
         wrapper.find('div').simulate('keyDown', backspaceKeyDown);
-        assert.strictEqual(preventDefaultSpy.callCount, 1);
+        assert.strictEqual(preventDefaultSpy.callCount, 0);
         assert.strictEqual(onDeleteSpy.callCount, 0);
 
         const backspaceKeyUp = {