[Menu] Improve performance and add support for variants

docs/src/pages/demos/menus/menus.md

@@ -21,8 +21,10 @@ Choosing an option should immediately ideally commit the option and close the me
 
 ## Selected menus
 
-If used for item selection, when opened, simple menus attempt to vertically align the currently selected menu item with the anchor element.
+If used for item selection, when opened, simple menus attempt to vertically align the currently selected menu item with the anchor element,
+and the initial focus will be placed on the selected menu item.
 The currently selected menu item is set using the `selected` property (from [ListItem](/api/list-item/)).
+To use a selected menu item without impacting the initial focus or the vertical positioning of the menu, set the `variant` property to `menu`.
 
 {{"demo": "pages/demos/menus/SimpleListMenu.js"}}
 

packages/material-ui-styles/src/withStyles/withStyles.js

@@ -76,8 +76,8 @@ const withStyles = (stylesOrCreator, options = {}) => Component => {
      */
     classes: PropTypes.object,
     /**
-     * @deprecated
      * Use that property to pass a ref callback to the decorated component.
+     * @deprecated
      */
     innerRef: chainPropTypes(PropTypes.oneOfType([PropTypes.func, PropTypes.object]), props => {
       if (props.innerRef == null) {

packages/material-ui-styles/src/withTheme/withTheme.js

@@ -25,8 +25,8 @@ export function withThemeCreator(options = {}) {
 
     WithTheme.propTypes = {
       /**
-       * @deprecated
        * Use that property to pass a ref callback to the decorated component.
+       * @deprecated
        */
       innerRef: chainPropTypes(PropTypes.oneOfType([PropTypes.func, PropTypes.object]), props => {
         if (props.innerRef == null) {

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

@@ -4,8 +4,10 @@ import clsx from 'clsx';
 import { chainPropTypes } from '@material-ui/utils';
 import withStyles from '../styles/withStyles';
 import ButtonBase from '../ButtonBase';
-import { isMuiElement } from '../utils/reactHelpers';
+import { isMuiElement, useForkRef } from '../utils/reactHelpers';
 import ListContext from '../List/ListContext';
+import ReactDOM from 'react-dom';
+import warning from 'warning';
 
 export const styles = theme => ({
   /* Styles applied to the (normally root) `component` element. May be wrapped by a `container`. */
@@ -86,6 +88,7 @@ export const styles = theme => ({
 const ListItem = React.forwardRef(function ListItem(props, ref) {
   const {
     alignItems,
+    autoFocus,
     button,
     children: childrenProp,
     classes,
@@ -107,11 +110,30 @@ const ListItem = React.forwardRef(function ListItem(props, ref) {
     dense: dense || context.dense || false,
     alignItems,
   };
+  const listItemRef = React.useRef();
+  React.useLayoutEffect(() => {
+    if (autoFocus) {
+      if (listItemRef.current) {
+        listItemRef.current.focus();
+      } else {
+        warning(
+          false,
+          'Material-UI: unable to set focus to a ListItem whose component has not been rendered.',
+        );
+      }
+    }
+  }, [autoFocus]);
 
   const children = React.Children.toArray(childrenProp);
   const hasSecondaryAction =
     children.length && isMuiElement(children[children.length - 1], ['ListItemSecondaryAction']);
 
+  const handleOwnRef = React.useCallback(instance => {
+    // #StrictMode ready
+    listItemRef.current = ReactDOM.findDOMNode(instance);
+  }, []);
+  const handleRef = useForkRef(handleOwnRef, ref);
+
   const componentProps = {
     className: clsx(
       classes.root,
@@ -155,7 +177,7 @@ const ListItem = React.forwardRef(function ListItem(props, ref) {
       <ListContext.Provider value={childContext}>
         <ContainerComponent
           className={clsx(classes.container, ContainerClassName)}
-          ref={ref}
+          ref={handleRef}
           {...ContainerProps}
         >
           <Component {...componentProps}>{children}</Component>
@@ -167,7 +189,7 @@ const ListItem = React.forwardRef(function ListItem(props, ref) {
 
   return (
     <ListContext.Provider value={childContext}>
-      <Component ref={ref} {...componentProps}>
+      <Component ref={handleRef} {...componentProps}>
         {children}
       </Component>
     </ListContext.Provider>
@@ -179,6 +201,11 @@ ListItem.propTypes = {
    * Defines the `align-items` style property.
    */
   alignItems: PropTypes.oneOf(['flex-start', 'center']),
+  /**
+   * If `true`, the list item will be focused during the first mount.
+   * Focus will also be triggered if the value changes from false to true.
+   */
+  autoFocus: PropTypes.bool,
   /**
    * If `true`, the list item will be a button (using `ButtonBase`).
    */

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

@@ -14,6 +14,10 @@ import ListItem from './ListItem';
 import ButtonBase from '../ButtonBase';
 import ListContext from '../List/ListContext';
 
+const NoContent = React.forwardRef(() => {
+  return null;
+});
+
 describe('<ListItem />', () => {
   let mount;
   let classes;
@@ -163,6 +167,15 @@ describe('<ListItem />', () => {
           'Warning: Failed prop type: Material-UI: you used an element',
         );
       });
+
+      it('should warn (but not error) with autoFocus with a function component with no content', () => {
+        mount(<ListItem component={NoContent} autoFocus />);
+        assert.strictEqual(consoleErrorMock.callCount(), 1);
+        assert.include(
+          consoleErrorMock.args()[0][0],
+          'Warning: Material-UI: unable to set focus to a ListItem whose component has not been rendered.',
+        );
+      });
     });
   });
 

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

@@ -2,9 +2,13 @@
 
 import React from 'react';
 import PropTypes from 'prop-types';
+import clsx from 'clsx';
 import withStyles from '../styles/withStyles';
 import Popover from '../Popover';
 import MenuList from '../MenuList';
+import warning from 'warning';
+import ReactDOM from 'react-dom';
+import { setRef } from '@material-ui/core/utils/reactHelpers';
 
 const RTL_ORIGIN = {
   vertical: 'top',
@@ -26,34 +30,40 @@ export const styles = {
     // Add iOS momentum scrolling.
     WebkitOverflowScrolling: 'touch',
   },
+  /* Styles applied to the `List` component via `MenuList`. */
+  list: {
+    // We disable the focus ring for mouse, touch and keyboard users.
+    outline: 'none',
+  },
 };
 
 const Menu = React.forwardRef(function Menu(props, ref) {
   const {
+    autoFocus: autoFocusProp,
     children,
     classes,
     disableAutoFocusItem,
-    MenuListProps,
+    MenuListProps = {},
     onClose,
     onEntering,
     open,
     PaperProps = {},
     PopoverClasses,
     theme,
+    variant,
     ...other
   } = props;
+
+  const autoFocus = autoFocusProp !== undefined ? autoFocusProp : !disableAutoFocusItem;
+
   const menuListActionsRef = React.useRef();
+  const firstValidItemRef = React.useRef();
+  const firstSelectedItemRef = React.useRef();
 
-  const getContentAnchorEl = () => {
-    return menuListActionsRef.current.getContentAnchorEl();
-  };
+  const getContentAnchorEl = () => firstSelectedItemRef.current || firstValidItemRef.current;
 
   const handleEntering = element => {
     if (menuListActionsRef.current) {
-      // Focus so the scroll computation of the Popover works as expected.
-      if (disableAutoFocusItem !== true) {
-        menuListActionsRef.current.focus();
-      }
       menuListActionsRef.current.adjustStyleForScrollbar(element, theme);
     }
 
@@ -72,6 +82,59 @@ const Menu = React.forwardRef(function Menu(props, ref) {
     }
   };
 
+  let firstValidElementIndex = null;
+  let firstSelectedIndex = null;
+
+  const items = React.Children.map(children, (child, index) => {
+    if (!React.isValidElement(child)) {
+      return null;
+    }
+    warning(
+      child.type !== React.Fragment,
+      [
+        "Material-UI: the Menu component doesn't accept a Fragment as a child.",
+        'Consider providing an array instead.',
+      ].join('\n'),
+    );
+    if (firstValidElementIndex === null) {
+      firstValidElementIndex = index;
+    }
+    let newChildProps = null;
+    if (
+      variant === 'selectedMenu' &&
+      firstSelectedIndex === null &&
+      child.props.selected &&
+      !child.props.disabled
+    ) {
+      firstSelectedIndex = index;
+      newChildProps = {};
+      if (autoFocus) {
+        newChildProps.autoFocus = true;
+      }
+      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 = {
+        ref: instance => {
+          // #StrictMode ready
+          firstValidItemRef.current = ReactDOM.findDOMNode(instance);
+          setRef(child.ref, instance);
+        },
+      };
+    }
+
+    if (newChildProps !== null) {
+      return React.cloneElement(child, newChildProps);
+    }
+    return child;
+  });
+
   return (
     <Popover
       getContentAnchorEl={getContentAnchorEl}
@@ -94,10 +157,12 @@ const Menu = React.forwardRef(function Menu(props, ref) {
       <MenuList
         data-mui-test="Menu"
         onKeyDown={handleListKeyDown}
-        {...MenuListProps}
         actions={menuListActionsRef}
+        autoFocus={autoFocus && firstSelectedIndex === null}
+        {...MenuListProps}
+        className={clsx(classes.list, MenuListProps.className)}
       >
-        {children}
+        {items}
       </MenuList>
     </Popover>
   );
@@ -108,6 +173,10 @@ Menu.propTypes = {
    * The DOM element used to set the position of the menu.
    */
   anchorEl: PropTypes.oneOfType([PropTypes.object, PropTypes.func]),
+  /**
+   * If `true` (default), the menu list (possibly a particular item depending on the menu variant) will receive focus on open.
+   */
+  autoFocus: PropTypes.bool,
   /**
    * Menu contents, normally `MenuItem`s.
    */
@@ -118,7 +187,8 @@ Menu.propTypes = {
    */
   classes: PropTypes.object.isRequired,
   /**
-   * If `true`, the selected / first menu item will not be auto focused.
+   * Same as `autoFocus=false`.
+   * @deprecated Use `autoFocus` instead
    */
   disableAutoFocusItem: PropTypes.bool,
   /**
@@ -180,11 +250,17 @@ Menu.propTypes = {
     PropTypes.shape({ enter: PropTypes.number, exit: PropTypes.number }),
     PropTypes.oneOf(['auto']),
   ]),
+  /**
+   * The variant to use. Use `menu` to prevent selected items from impacting the initial focus
+   * and the vertical alignment relative to the anchor element.
+   */
+  variant: PropTypes.oneOf(['menu', 'selectedMenu']),
 };
 
 Menu.defaultProps = {
   disableAutoFocusItem: false,
   transitionDuration: 'auto',
+  variant: 'selectedMenu',
 };
 
 export default withStyles(styles, { name: 'MuiMenu', withTheme: true })(Menu);

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

@@ -147,13 +147,26 @@ describe('<Menu />', () => {
   it('should open during the initial mount', () => {
     const wrapper = mount(
       <Menu {...defaultProps} open>
-        <div />
+        <div tabIndex={-1} />
       </Menu>,
     );
     const popover = wrapper.find(Popover);
     assert.strictEqual(popover.props().open, true);
     const menuEl = document.querySelector('[data-mui-test="Menu"]');
-    assert.strictEqual(document.activeElement, menuEl && menuEl.firstChild);
+    assert.strictEqual(document.activeElement, menuEl);
+  });
+
+  it('should not focus list if autoFocus=false', () => {
+    const wrapper = mount(
+      <Menu {...defaultProps} autoFocus={false} open>
+        <div tabIndex={-1} />
+      </Menu>,
+    );
+    const popover = wrapper.find(Popover);
+    assert.strictEqual(popover.props().open, true);
+    const menuEl = document.querySelector('[data-mui-test="Menu"]');
+    assert.notStrictEqual(document.activeElement, menuEl);
+    assert.strictEqual(false, menuEl.contains(document.activeElement));
   });
 
   it('should call props.onEntering with element if exists', () => {

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

@@ -28,13 +28,26 @@ export const styles = theme => ({
 });
 
 const MenuItem = React.forwardRef(function MenuItem(props, ref) {
-  const { classes, className, component, disableGutters, role, selected, ...other } = props;
+  const {
+    classes,
+    className,
+    component,
+    disableGutters,
+    role,
+    selected,
+    tabIndex: tabIndexProp,
+    ...other
+  } = props;
 
+  let tabIndex;
+  if (!props.disabled) {
+    tabIndex = tabIndexProp !== undefined ? tabIndexProp : -1;
+  }
   return (
     <ListItem
       button
       role={role}
-      tabIndex={-1}
+      tabIndex={tabIndex}
       component={component}
       selected={selected}
       disableGutters={disableGutters}
@@ -71,6 +84,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.
    */
@@ -83,6 +100,10 @@ MenuItem.propTypes = {
    * @ignore
    */
   selected: PropTypes.bool,
+  /**
+   * @ignore
+   */
+  tabIndex: PropTypes.number,
 };
 
 MenuItem.defaultProps = {