[ButtonBase] Improve enter & space handling (#11585)

* Fix #11584 * add tests
mui · May 25, 2018 · 90c0ea2 · 90c0ea2
1 parent 2b7cb47
commit 90c0ea2
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 20 deletions.
diff --git a/docs/src/pages/guides/composition/ComponentProperty.js b/docs/src/pages/guides/composition/ComponentProperty.js
@@ -9,7 +9,8 @@ import Divider from '@material-ui/core/Divider';
 import InboxIcon from '@material-ui/icons/Inbox';
 import DraftsIcon from '@material-ui/icons/Drafts';
 import Typography from '@material-ui/core/Typography';
-import { MemoryRouter, Route } from 'react-router';
+import MemoryRouter from 'react-router/MemoryRouter';
+import Route from 'react-router/Route';
 import { Link } from 'react-router-dom';
 
 const styles = theme => ({
@@ -61,7 +62,7 @@ ListItemLink2.propTypes = {
   to: PropTypes.string.isRequired,
 };
 
-function SimpleList(props) {
+function ComponentProperty(props) {
   const { classes } = props;
   return (
     <MemoryRouter initialEntries={['/drafts']} initialIndex={0}>
@@ -89,8 +90,8 @@ function SimpleList(props) {
   );
 }
 
-SimpleList.propTypes = {
+ComponentProperty.propTypes = {
   classes: PropTypes.object.isRequired,
 };
 
-export default withStyles(styles)(SimpleList);
+export default withStyles(styles)(ComponentProperty);
diff --git a/packages/material-ui/src/ButtonBase/ButtonBase.js b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -146,7 +146,8 @@ class ButtonBase extends React.Component {
       event.target === event.currentTarget &&
       component &&
       component !== 'button' &&
-      (key === 'space' || key === 'enter')
+      (key === 'space' || key === 'enter') &&
+      !(this.button.tagName === 'A' && this.button.href)
     ) {
       event.preventDefault();
       if (onClick) {
@@ -164,7 +165,9 @@ class ButtonBase extends React.Component {
     ) {
       this.keyDown = false;
       event.persist();
-      this.ripple.stop(event, () => this.ripple.pulsate(event));
+      this.ripple.stop(event, () => {
+        this.ripple.pulsate(event);
+      });
     }
     if (this.props.onKeyUp) {
       this.props.onKeyUp(event);

diff --git a/packages/material-ui/src/ButtonBase/ButtonBase.test.js b/packages/material-ui/src/ButtonBase/ButtonBase.test.js
@@ -527,13 +527,12 @@ describe('<ButtonBase />', () => {
 
     describe('prop: onKeyDown', () => {
       it('should work', () => {
+        const onKeyDownSpy = spy();
         wrapper = mount(
-          <ButtonBaseNaked theme={{}} classes={{}}>
+          <ButtonBaseNaked theme={{}} classes={{}} onKeyDown={onKeyDownSpy}>
             Hello
           </ButtonBaseNaked>,
         );
-        const onKeyDownSpy = spy();
-        wrapper.setProps({ onKeyDown: onKeyDownSpy });
 
         const eventPersistSpy = spy();
         event = { persist: eventPersistSpy, keyCode: undefined };
@@ -555,33 +554,67 @@ describe('<ButtonBase />', () => {
 
     describe('Keyboard accessibility for non interactive elements', () => {
       it('should work', () => {
+        const onClickSpy = spy();
         wrapper = mount(
-          <ButtonBaseNaked theme={{}} classes={{}}>
+          <ButtonBaseNaked theme={{}} classes={{}} onClick={onClickSpy} component="div">
             Hello
           </ButtonBaseNaked>,
         );
-        const onClickSpy = spy();
-        wrapper.setProps({ onClick: onClickSpy, component: 'div' });
 
-        const eventTargetMock = 'woofButtonBase';
         event = {
-          persist: spy(),
           preventDefault: spy(),
           keyCode: keycode('space'),
-          target: eventTargetMock,
-          currentTarget: eventTargetMock,
+          target: 'target',
+          currentTarget: 'target',
         };
 
         instance = wrapper.instance();
         instance.keyDown = false;
         instance.handleKeyDown(event);
 
         assert.strictEqual(instance.keyDown, false, 'should not change keydown');
-        assert.strictEqual(event.persist.callCount, 0, 'should not call event.persist');
         assert.strictEqual(event.preventDefault.callCount, 1, 'should call event.preventDefault');
         assert.strictEqual(onClickSpy.callCount, 1, 'should call onClick');
         assert.strictEqual(onClickSpy.calledWith(event), true, 'should call onClick with event');
       });
+
+      it('should hanlde the link with no href', () => {
+        const onClickSpy = spy();
+        wrapper = mount(
+          <ButtonBaseNaked theme={{}} classes={{}} component="a" onClick={onClickSpy}>
+            Hello
+          </ButtonBaseNaked>,
+        );
+        event = {
+          preventDefault: spy(),
+          keyCode: keycode('enter'),
+          target: 'target',
+          currentTarget: 'target',
+        };
+        instance = wrapper.instance();
+        instance.handleKeyDown(event);
+        assert.strictEqual(event.preventDefault.callCount, 1);
+        assert.strictEqual(onClickSpy.callCount, 1);
+      });
+
+      it('should ignore the link with href', () => {
+        const onClickSpy = spy();
+        wrapper = mount(
+          <ButtonBaseNaked theme={{}} classes={{}} component="a" href="href" onClick={onClickSpy}>
+            Hello
+          </ButtonBaseNaked>,
+        );
+        event = {
+          preventDefault: spy(),
+          keyCode: keycode('enter'),
+          target: 'target',
+          currentTarget: 'target',
+        };
+        instance = wrapper.instance();
+        instance.handleKeyDown(event);
+        assert.strictEqual(event.preventDefault.callCount, 0);
+        assert.strictEqual(onClickSpy.callCount, 0);
+      });
     });
 
     describe('prop: disableRipple', () => {

diff --git a/packages/material-ui/src/ButtonBase/focusVisible.js b/packages/material-ui/src/ButtonBase/focusVisible.js
@@ -11,10 +11,10 @@ const internal = {
 };
 
 export function detectFocusVisible(instance, element, callback, attempt = 1) {
-  warning(instance.focusVisibleCheckTime, 'Material-UI: missing instance.focusVisibleCheckTime');
+  warning(instance.focusVisibleCheckTime, 'Material-UI: missing instance.focusVisibleCheckTime.');
   warning(
     instance.focusVisibleMaxCheckTimes,
-    'Material-UI: missing instance.focusVisibleMaxCheckTimes',
+    'Material-UI: missing instance.focusVisibleMaxCheckTimes.',
   );
 
   instance.focusVisibleTimeout = setTimeout(() => {
@@ -34,7 +34,7 @@ export function detectFocusVisible(instance, element, callback, attempt = 1) {
 const FOCUS_KEYS = ['tab', 'enter', 'space', 'esc', 'up', 'down', 'left', 'right'];
 
 function isFocusKey(event) {
-  return FOCUS_KEYS.indexOf(keycode(event)) !== -1;
+  return FOCUS_KEYS.indexOf(keycode(event)) > -1;
 }
 
 const handleKeyUpEvent = event => {