Dividers in menus receive keyboard focus

[eric-parsons]

When navigating through items in a menu using arrow keys, dividers don't get skipped over. Instead, they will receive keyboard focus like any other item. Since dividers serve no purpose other than to visually group menu items, this is a hindrance for keyboard navigation.

• [x ] This is not a v0.x issue.
• [x ] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

Using arrow key navigation in a Menu component should skip over any contained Divider elements when changing focus.

Current Behavior 😯

Divider elements get keyboard focus.

Steps to Reproduce 🕹

Link: https://codesandbox.io/s/q8p57z1j4

1. Click the Open Menu button.
2. Press the down arrow key twice.

Your Environment 🌎

Tech	Version
Material-UI	v3.3.2
React	16.4.2
Browser	Chrome, Firefox
TypeScript	2.9.2

[oliviertassinari]

@eric-parsons Thank you for opening this issue. We are already aware of this issue, not proud of it. It comes down to a non-ideal focus implementation logic, that also causes: #10847. I have been discussing it with @RobertPurcea he was proposing an alternative implementation of the MenuList.

class MenuList extends React.Component {
  componentDidMount() {
    if (!this.props.disableAutoFocusItem) {
      this.defaultFocus();
    }
  }

  // focus selected or first child
  defaultFocus = () => {
    const { children } = this.props;

    let indexOfChildToFocus = 0;

    React.Children.forEach(children, (child, index) => {
      if (child.props.selected) {
        indexOfChildToFocus = index;
      }
    });

    this.listRef.children[indexOfChildToFocus].focus();
  }

  handleBlur = event => {
    if (this.props.onBlur) {
      this.props.onBlur(event);
    }
  };

  // TODO: focus a certain kind of elements (up to us, we could focus all elements with a tabIndex, or all focusable elements like buttons, inputs etc.)
  handleKeyDown = event => {
    const key = keycode(event);
    const list = this.listRef;

    const currentFocus = ownerDocument(list).activeElement;

    if (
      (key === 'up' || key === 'down') &&
      (!currentFocus || (currentFocus && !list.contains(currentFocus)))
    ) {
      this.defaultFocus();
    } else if (key === 'down') {
      event.preventDefault();

      if (currentFocus.nextElementSibling) {
        currentFocus.nextElementSibling.focus();
      }
    } else if (key === 'up') {
      event.preventDefault();
      if (currentFocus.previousElementSibling) {
        currentFocus.previousElementSibling.focus();
      }
    }

    if (this.props.onKeyDown) {
      this.props.onKeyDown(event, key);
    }
  };

  render() {
    const { children, className, onBlur, onKeyDown, ...other } = this.props;

    return (
      <List
        role="menu"
        ref={ref => {
          this.listRef = ReactDOM.findDOMNode(ref);
        }}
        className={className}
        onKeyDown={this.handleKeyDown}
        onBlur={this.handleBlur}
        {...other}
      >
        {children}
      </List>
    );
  }
}

It's going in the right direction.

[Billy-]

This bug appears to still be occurring, can we re-open this issue?

[oliviertassinari]

@Billy- If you can reproduce on v5, please open a new issue with a codesandbox to showcase the problem.