[Menu] Simplify focus and tabIndex logic further

* [MenuItem] Prevent setting tabIndex for disabled menu items (part of #13464)
mui · Apr 18, 2019 · 8445fb1 · 8445fb1
1 parent aab5ce1
commit 8445fb1
Show file tree

Hide file tree

Showing 4 changed files with 63 additions and 42 deletions.
diff --git a/packages/material-ui/src/Menu/Menu.js b/packages/material-ui/src/Menu/Menu.js
@@ -86,7 +86,6 @@ const Menu = React.forwardRef(function Menu(props, ref) {
   const checkForSelectedItem = variant === 'selectedMenu';
 
   let firstValidElementIndex = null;
-  let firstFocusableIndex = null;
   let firstSelectedIndex = null;
 
   const items = React.Children.map(children, (child, index) => {
@@ -100,42 +99,39 @@ const Menu = React.forwardRef(function Menu(props, ref) {
         'Consider providing an array instead.',
       ].join('\n'),
     );
-    let needToCloneItem = false;
     if (firstValidElementIndex === null) {
       firstValidElementIndex = index;
-      needToCloneItem = true;
     }
-    if (firstFocusableIndex === null) {
-      // This condition will eventually guard against disabled items and dividers
-      firstFocusableIndex = index;
-      needToCloneItem = true;
-    }
-    if (checkForSelectedItem && firstSelectedIndex === null && child.props.selected) {
+    let newChildProps = null;
+    if (
+      checkForSelectedItem &&
+      firstSelectedIndex === null &&
+      child.props.selected &&
+      !child.props.disabled
+    ) {
       firstSelectedIndex = index;
-      needToCloneItem = true;
-    }
-
-    if (needToCloneItem) {
-      const newChildProps = {};
-      if (index === firstSelectedIndex) {
-        if (autoFocus) {
-          newChildProps.autoFocus = true;
-        }
-        newChildProps.ref = instance => {
-          // StrictMode ready
-          firstSelectedItemRef.current = ReactDOM.findDOMNode(instance);
-          setRef(child.ref, instance);
-        };
-      } else if (index === firstValidElementIndex) {
-        newChildProps.ref = instance => {
-          // StrictMode ready
-          firstValidItemRef.current = ReactDOM.findDOMNode(instance);
-          setRef(child.ref, instance);
-        };
+      newChildProps = {};
+      if (autoFocus) {
+        newChildProps.autoFocus = true;
       }
-      if (index === firstFocusableIndex) {
+      if (child.props.tabIndex === undefined) {
         newChildProps.tabIndex = 0;
       }
+      newChildProps.ref = instance => {
+        // StrictMode ready
+        firstSelectedItemRef.current = ReactDOM.findDOMNode(instance);
+        setRef(child.ref, instance);
+      };
+    } else if (index === firstValidElementIndex) {
+      newChildProps = {};
+      newChildProps.ref = instance => {
+        // StrictMode ready
+        firstValidItemRef.current = ReactDOM.findDOMNode(instance);
+        setRef(child.ref, instance);
+      };
+    }
+
+    if (newChildProps !== null) {
       return React.cloneElement(child, newChildProps);
     }
     return child;

diff --git a/packages/material-ui/src/MenuItem/MenuItem.js b/packages/material-ui/src/MenuItem/MenuItem.js
@@ -32,20 +32,26 @@ const MenuItem = React.forwardRef(function MenuItem(props, ref) {
     classes,
     className,
     component,
+    disabled,
     disableGutters,
     role,
     selected,
-    tabIndex,
+    tabIndex: tabIndexProp,
     ...other
   } = props;
 
+  let tabIndex;
+  if (!disabled) {
+    tabIndex = tabIndexProp !== undefined ? tabIndexProp : -1;
+  }
   return (
     <ListItem
       button
       role={role}
       tabIndex={tabIndex}
       component={component}
       selected={selected}
+      disabled={disabled}
       disableGutters={disableGutters}
       className={clsx(
         classes.root,
@@ -80,6 +86,10 @@ MenuItem.propTypes = {
    * Either a string to use a DOM element or a component.
    */
   component: PropTypes.elementType,
+  /**
+   * @ignore
+   */
+  disabled: PropTypes.bool,
   /**
    * If `true`, the left and right padding is removed.
    */
@@ -102,7 +112,6 @@ MenuItem.defaultProps = {
   component: 'li',
   disableGutters: false,
   role: 'menuitem',
-  tabIndex: -1,
 };
 
 export default withStyles(styles, { name: 'MuiMenuItem' })(MenuItem);
diff --git a/packages/material-ui/src/MenuList/MenuList.js b/packages/material-ui/src/MenuList/MenuList.js
@@ -87,7 +87,7 @@ const MenuList = React.forwardRef(function MenuList(props, ref) {
       ref={handleRef}
       className={className}
       onKeyDown={handleKeyDown}
-      tabIndex={-1}
+      tabIndex={autoFocus ? 0 : -1}
       {...other}
     >
       {children}

diff --git a/packages/material-ui/test/integration/Menu.test.js b/packages/material-ui/test/integration/Menu.test.js
@@ -203,7 +203,14 @@ describe('<Menu> integration', () => {
     };
     const mountTrackingMenu = (
       variant,
-      { selectedIndex, invalidIndex, autoFocusIndex, autoFocus, themeDirection } = {},
+      {
+        selectedIndex,
+        invalidIndex,
+        autoFocusIndex,
+        disabledIndex,
+        autoFocus,
+        themeDirection,
+      } = {},
     ) => {
       const theme =
         themeDirection !== undefined
@@ -236,6 +243,7 @@ describe('<Menu> integration', () => {
             return (
               <TrackingMenuItem
                 key={itemIndex}
+                disabled={itemIndex === disabledIndex ? true : undefined}
                 itemIndex={itemIndex}
                 selected={itemIndex === selectedIndex}
                 autoFocus={itemIndex === autoFocusIndex ? true : undefined}
@@ -256,7 +264,7 @@ describe('<Menu> integration', () => {
       assert.deepStrictEqual(contentAnchorTracker, [true, false, false]);
       assert.deepStrictEqual(focusTracker, [false, false, false]);
       assert.strictEqual(menuListFocusTracker, true);
-      assert.deepStrictEqual(tabIndexTracker, [true, false, false]);
+      assert.deepStrictEqual(tabIndexTracker, [false, false, false]);
 
       // Adds coverage for Tab with no onClose
       TestUtils.Simulate.keyDown(document.activeElement, {
@@ -268,55 +276,63 @@ describe('<Menu> integration', () => {
       assert.deepStrictEqual(contentAnchorTracker, [true, false, false]);
       assert.deepStrictEqual(focusTracker, [false, false, false]);
       assert.strictEqual(menuListFocusTracker, true);
-      assert.deepStrictEqual(tabIndexTracker, [true, false, false]);
+      assert.deepStrictEqual(tabIndexTracker, [false, false, false]);
     });
 
     it('[variant=menu] nothing selected, autoFocus on third, content anchor: first, focus: third, tabIndex: third', () => {
       mountTrackingMenu('menu', { autoFocusIndex: 2 });
       assert.deepStrictEqual(contentAnchorTracker, [true, false, false]);
       assert.deepStrictEqual(focusTracker, [false, false, true]);
       assert.strictEqual(menuListFocusTracker, true);
-      assert.deepStrictEqual(tabIndexTracker, [true, false, false]);
+      assert.deepStrictEqual(tabIndexTracker, [false, false, false]);
     });
 
     it('[variant=selectedMenu] nothing selected, content anchor: first, focus: MenuList, tabIndex: first', () => {
       mountTrackingMenu('selectedMenu');
       assert.deepStrictEqual(contentAnchorTracker, [true, false, false]);
       assert.deepStrictEqual(focusTracker, [false, false, false]);
       assert.strictEqual(menuListFocusTracker, true);
-      assert.deepStrictEqual(tabIndexTracker, [true, false, false]);
+      assert.deepStrictEqual(tabIndexTracker, [false, false, false]);
     });
 
     it('[variant=selectedMenu] nothing selected, content anchor: first valid, focus: first valid, tabIndex: first valid', () => {
       mountTrackingMenu('selectedMenu', { invalidIndex: 0 });
       assert.deepStrictEqual(contentAnchorTracker, [false, true, false]);
       assert.deepStrictEqual(focusTracker, [false, false, false]);
       assert.strictEqual(menuListFocusTracker, true);
-      assert.deepStrictEqual(tabIndexTracker, [false, true, false]);
+      assert.deepStrictEqual(tabIndexTracker, [false, false, false]);
     });
 
     it('[variant=menu] second item selected, content anchor: first, focus: MenuList, tabIndex: first', () => {
       mountTrackingMenu('menu', { selectedIndex: 1 });
       assert.deepStrictEqual(contentAnchorTracker, [true, false, false]);
       assert.deepStrictEqual(focusTracker, [false, false, false]);
       assert.strictEqual(menuListFocusTracker, true);
-      assert.deepStrictEqual(tabIndexTracker, [true, false, false]);
+      assert.deepStrictEqual(tabIndexTracker, [false, false, false]);
     });
 
     it('[variant=selectedMenu] second item selected, content anchor: second, focus: second, tabIndex: second', () => {
       mountTrackingMenu('selectedMenu', { selectedIndex: 1 });
       assert.deepStrictEqual(contentAnchorTracker, [false, true, false]);
       assert.deepStrictEqual(focusTracker, [false, true, false]);
       assert.strictEqual(menuListFocusTracker, true);
-      assert.deepStrictEqual(tabIndexTracker, [true, false, false]);
+      assert.deepStrictEqual(tabIndexTracker, [false, true, false]);
+    });
+
+    it('[variant=selectedMenu] second item selected and disabled, content anchor: second, focus: second, tabIndex: second', () => {
+      mountTrackingMenu('selectedMenu', { selectedIndex: 1, disabledIndex: 1 });
+      assert.deepStrictEqual(contentAnchorTracker, [true, false, false]);
+      assert.deepStrictEqual(focusTracker, [false, false, false]);
+      assert.strictEqual(menuListFocusTracker, true);
+      assert.deepStrictEqual(tabIndexTracker, [false, false, false]);
     });
 
     it('[variant=selectedMenu] second item selected, no autoFocus, content anchor: second, focus: none, tabIndex: second', () => {
       mountTrackingMenu('selectedMenu', { selectedIndex: 1, autoFocus: false });
       assert.deepStrictEqual(contentAnchorTracker, [false, true, false]);
       assert.deepStrictEqual(focusTracker, [false, false, false]);
       assert.strictEqual(menuListFocusTracker, false);
-      assert.deepStrictEqual(tabIndexTracker, [true, false, false]);
+      assert.deepStrictEqual(tabIndexTracker, [false, true, false]);
     });
   });