Select menu does not fully implement keyboard controls

[corytheboyd]

• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

The native browser select element allows you to move to options by typing arbitrary characters when the select menu is open. The newly added Select component (which is greatly appreciated by the way!) does not support this functionality.

Current Behavior

When select menu is open, and I type the label of an option that is not selected, nothing happens.

Steps to Reproduce (for bugs)

1. Open a Select component
2. Start typing to select an option

Context

Material UI is an amazing asset, but in pursuit of the new and shiny, let's not abandon something as fundamental as basic select functionality :)

Your Environment

Tech	Version
Material-UI	1.0.0-beta-9
React	15.5.4
browser	Chrome 61.0.3163.79 (Official Build) (64-bit)

[corytheboyd]

Given that this would require the component to hold internal state, rolling it in may not be the best move. Perhaps add a demo for this that hooks into the Input's onKeyUp handler though. Thoughts?

[oliviertassinari]

I'm glad we expose a native select version too that don't suffer from this issue.

[kgregory]

Material UI is an amazing asset, but in pursuit of the new and shiny, let's not abandon something as fundamental as basic select functionality :)

@oliviertassinari did not abandon this, he provided a component with the familiar look and feel and a mode that supports native select.

Given that this would require the component to hold internal state, rolling it in may not be the best move. Perhaps add a demo for this that hooks into the Input's onKeyUp handler though. Thoughts?

This was implemented in the Menu component in 0.x by handling key presses, accumulating keys and looking for an item that starts with the resulting string. The accumulation would be reset after a short period of time (500ms). It hasn't been implemented yet, but maybe it will be. You could submit a PR 👍

[corytheboyd]

@kgregory Apologies if I came off as rude bringing this up, I did indeed notice the native select implementation before reporting this issue. Just consider this an enhancement request for the existing Material Select component.

This was implemented in the Menu component in 0.x by handling key presses, accumulating keys and looking for an item that starts with the resulting string. The accumulation would be reset after a short period of time (500ms)

That sounds doable for the new Select component, I may take a stab at it when I have some free time in the next week or so.

[jordangarside]

How do we use the native select component?

[zsinryu]

Any update for this issue?

[RobertApelian]

Ran into the same issue... seems like the intended course of action here is to instead use something like React Select with an Autocomplete field

[goodbomb]

Just as a heads up, this issue is still occurring as of 1.0.0-beta.34

[kobelobster]

@oliviertassinari Is this a feature you would accept a PR for or is it intentional left out of the non-native selects?

[oliviertassinari]

@tzfrs It was left out by lack of time. Personally, I'm always using the native select as the non native version doesn't provide a good enough UX for our taste. We will definitely accept a pull request for it, the sooner we can close the UX gap between native and non-native, the better.

[mathieuforest]

@oliviertassinari the problem with native is that you cannot use the multiple prop.

[mathieuforest]

I took the logic from v0.x and created a custom component using v1 components that support the type on select. Maybe that could help you.

import React, { Component } from 'react';
import ReactDOM from 'react-dom';
import Select from '@material-ui/core/Select';
import MenuItem from '@material-ui/core/MenuItem';
import keycode from 'keycode';

class EnhancedSelect extends Component {

    constructor(props) {
        super(props);

        const selectedIndex = 0;
        const newFocusIndex = selectedIndex >= 0 ? selectedIndex : 0;
        if (newFocusIndex !== -1 && props.onMenuItemFocusChange) {
            props.onMenuItemFocusChange(null, newFocusIndex);
        }

        this.state = {
            focusIndex: 0
        }
        this.focusedMenuItem = React.createRef();
        this.selectContainer = React.createRef();
    }

    componentDidMount = () => {
        this.setScrollPosition();
    }

    clearHotKeyTimer = () => {
        this.timerId = null;
        this.lastKeys = null;
    }

    hotKeyHolder = (key) => {
        clearTimeout(this.timerId);
        this.timerId = setTimeout(this.clearHotKeyTimer, 500);
        return this.lastKeys = (this.lastKeys || '') + key;
    }

    handleKeyPress = (event) => {
        const filteredChildren = this.getFilteredChildren(this.props.children);
        if (event.key.length === 1) {
            const hotKeys = this.hotKeyHolder(event.key);
            if (this.setFocusIndexStartsWith(event, hotKeys, filteredChildren)) {
                event.preventDefault();
            }
        }
        this.setScrollPosition();
    };

    setFocusIndexStartsWith(event, keys, filteredChildren) {
        let foundIndex = -1;
        React.Children.forEach(filteredChildren, (child, index) => {
            if (foundIndex >= 0) {
                return;
            }
            const primaryText = child.props.children;
            if (typeof primaryText === 'string' && primaryText.substr(0, keys.length).toLowerCase() === keys.toLowerCase()) {
                foundIndex = index;
            }
        });
        if (foundIndex >= 0) {
            this.setState({
                focusIndex: foundIndex
            });
            return true;
        }
        return false;
    }

    setScrollPosition() {
        const desktop = this.props.desktop;
        const focusedMenuItem = this.focusedMenuItem;
        const menuItemHeight = desktop ? 32 : 48;
        if (this.focusedMenuItem!== null && this.focusedMenuItem.current!== null) {
          const selectedOffSet = ReactDOM.findDOMNode(this.focusedMenuItem.current).offsetTop;
          // Make the focused item be the 2nd item in the list the user sees
          let scrollTop = selectedOffSet - menuItemHeight;
          if (scrollTop < menuItemHeight) scrollTop = 0;
          ReactDOM.findDOMNode(this.selectContainer).scrollTop = scrollTop;
        }
    }

    cloneMenuItem(child, childIndex, index) {
        const childIsDisabled = child.props.disabled;
        const extraProps = {};
        if (!childIsDisabled) {
            const isFocused = childIndex === this.state.focusIndex;
            Object.assign(extraProps, {
                ref: isFocused ? this.focusedMenuItem : null,
            });
        }
        return React.cloneElement(child, extraProps);
    }

    getFilteredChildren(children) {
        const filteredChildren = [];
        React.Children.forEach(children, (child) => {
            if (child) {
                filteredChildren.push(child);
            }
        });
        return filteredChildren;
    }

    render() {

        const {
            children,
        } = this.props

        const filteredChildren = this.getFilteredChildren(children);

        let menuItemIndex = 0;
        const newChildren = React.Children.map(filteredChildren, (child, index) => {
            const childIsDisabled = child.props.disabled;
            let newChild = child;
            newChild = this.cloneMenuItem(child, menuItemIndex, index);
            if (!childIsDisabled) {
                menuItemIndex++;
            }
            return newChild;
        });

        return (
            <Select
                {...this.props}
                MenuProps={{
                    PaperProps:{
                        style:{
                            overflowY: 'auto',
                            maxHeight: '450px'
                        },
                        onKeyPress:(e) => {
                            this.handleKeyPress(e)
                        },
                        ref:(node) => {
                            this.selectContainer = node;
                        }
                    }
                }}
            >
                {newChildren}
            </Select>
        );
    }
}

export default EnhancedSelect;

[inerc]

I took the logic from v0.x and created a custom component using v1 components that support the type on select. Maybe that could help you.

import React, { Component } from 'react';
import ReactDOM from 'react-dom';
import Select from '@material-ui/core/Select';
import MenuItem from '@material-ui/core/MenuItem';
import keycode from 'keycode';

class EnhancedSelect extends Component {

    constructor(props) {
        super(props);

        const selectedIndex = 0;
        const newFocusIndex = selectedIndex >= 0 ? selectedIndex : 0;
        if (newFocusIndex !== -1 && props.onMenuItemFocusChange) {
            props.onMenuItemFocusChange(null, newFocusIndex);
        }

        this.state = {
            focusIndex: 0
        }
        this.focusedMenuItem = React.createRef();
        this.selectContainer = React.createRef();
    }

    componentDidMount = () => {
        this.setScrollPosition();
    }

    clearHotKeyTimer = () => {
        this.timerId = null;
        this.lastKeys = null;
    }

    hotKeyHolder = (key) => {
        clearTimeout(this.timerId);
        this.timerId = setTimeout(this.clearHotKeyTimer, 500);
        return this.lastKeys = (this.lastKeys || '') + key;
    }

    handleKeyPress = (event) => {
        const filteredChildren = this.getFilteredChildren(this.props.children);
        if (event.key.length === 1) {
            const hotKeys = this.hotKeyHolder(event.key);
            if (this.setFocusIndexStartsWith(event, hotKeys, filteredChildren)) {
                event.preventDefault();
            }
        }
        this.setScrollPosition();
    };

    setFocusIndexStartsWith(event, keys, filteredChildren) {
        let foundIndex = -1;
        React.Children.forEach(filteredChildren, (child, index) => {
            if (foundIndex >= 0) {
                return;
            }
            const primaryText = child.props.children;
            if (typeof primaryText === 'string' && primaryText.substr(0, keys.length).toLowerCase() === keys.toLowerCase()) {
                foundIndex = index;
            }
        });
        if (foundIndex >= 0) {
            this.setState({
                focusIndex: foundIndex
            });
            return true;
        }
        return false;
    }

    setScrollPosition() {
        const desktop = this.props.desktop;
        const focusedMenuItem = this.focusedMenuItem;
        const menuItemHeight = desktop ? 32 : 48;
        if (this.focusedMenuItem!== null && this.focusedMenuItem.current!== null) {
          const selectedOffSet = ReactDOM.findDOMNode(this.focusedMenuItem.current).offsetTop;
          // Make the focused item be the 2nd item in the list the user sees
          let scrollTop = selectedOffSet - menuItemHeight;
          if (scrollTop < menuItemHeight) scrollTop = 0;
          ReactDOM.findDOMNode(this.selectContainer).scrollTop = scrollTop;
        }
    }

    cloneMenuItem(child, childIndex, index) {
        const childIsDisabled = child.props.disabled;
        const extraProps = {};
        if (!childIsDisabled) {
            const isFocused = childIndex === this.state.focusIndex;
            Object.assign(extraProps, {
                ref: isFocused ? this.focusedMenuItem : null,
            });
        }
        return React.cloneElement(child, extraProps);
    }

    getFilteredChildren(children) {
        const filteredChildren = [];
        React.Children.forEach(children, (child) => {
            if (child) {
                filteredChildren.push(child);
            }
        });
        return filteredChildren;
    }

    render() {

        const {
            children,
        } = this.props

        const filteredChildren = this.getFilteredChildren(children);

        let menuItemIndex = 0;
        const newChildren = React.Children.map(filteredChildren, (child, index) => {
            const childIsDisabled = child.props.disabled;
            let newChild = child;
            newChild = this.cloneMenuItem(child, menuItemIndex, index);
            if (!childIsDisabled) {
                menuItemIndex++;
            }
            return newChild;
        });

        return (
            <Select
                {...this.props}
                MenuProps={{
                    PaperProps:{
                        style:{
                            overflowY: 'auto',
                            maxHeight: '450px'
                        },
                        onKeyPress:(e) => {
                            this.handleKeyPress(e)
                        },
                        ref:(node) => {
                            this.selectContainer = node;
                        }
                    }
                }}
            >
                {newChildren}
            </Select>
        );
    }
}

export default EnhancedSelect;

Can you give me the format of the input values "children"

[ryancogswell]

@oliviertassinari This has come up at my company, so I'll eventually do a pull request for this. I'll start work on it right away, but I won't be able to dedicate much of my time to it, so I suspect it will take me several weeks to finish.

There are two aspects to this functionality:

• Change the focus based on keyDown events. MenuList already has functionality for changing the focus on children based on up/down arrow events. This would be enhanced to support changing the focus based on matching the remembered hot-keys.
• Within SelectInput, automatically select a child (i.e. trigger onChange appropriately) when the focus changes to better mimic the native select behavior.

The native behavior on focus change is actually to make it "selected", but to wait until you close the select to trigger the onChange. SelectInput requires going through onChange to allow the outer code to change the selected value, so I don't think it will be possible to mimic native exactly. The two main options are to trigger onChange with the focus changes, or to continue to require the user to cause a "click" (e.g. space, Enter). The first is closer to native behavior aside from having more onChange noise if the user is keying through lots of items. The second reduces the scope (would only need the MenuList changes) and is less of a change in behavior for existing applications. From a user standpoint, I think the first is what I would want and would be less surprising (due to matching the native behavior more closely); otherwise if I use the keyboard to change the focus to what I want selected and then tab away or click away, I just lose that selection.

I might submit some pull requests with some tests on the existing behavior for keyDown events in MenuList and SelectInput before I move forward with the enhancements. Next I'll get the up/down arrow keys to change the selection on SelectInput rather than just changing focus. Finally I'll add the hot-key matching.

The 0.x Menu implementation that @kgregory referenced looks for a primaryText prop on the child. The code above from @mathieuforest uses child.props.children and only matches if it is a string. This seems like a good default, but I think a lot of the value in the non-native Select is to be able to render something more complex than a string within a MenuItem, so I think there should be an optional property on MenuItem to use for this matching (e.g. primaryText or hotKeyText).

Let me know if any of this approach sounds off-base and let me know your thoughts on the desired onChange timing.

[oliviertassinari]

@ryancogswell We would love to review your pull request!

Yes, the MenuList already has functionality for changing the focus on children based on up/down arrow events. However, this logic is far from optimal. We want to rewrite it with #13708. The current implementation is too slow. We want to avoid the usage of React for handling the focus.

I'm not sure to understand your point regarding triggering onChange. From what I understand, we should consider two cases differently:

• When the menu is open, we should move the focus and only the focus. The UI style will change based on the focus state, without relying on React. The ButtonBase listen for the enter key down. It will trigger a click event if he has the focus, the click event then triggers the change event. It should be enough. I have tried on MacOS, the change event is not called after each user keystroke. The user has to validate the focused item with a click or the enter key.
  I don't think that we have the performance budget necessary for re-rendering the open menu component for each user keystroke.
• When the menu is closed but has the focus, we should trigger the change event after each user keystroke that matches an item.

I think that we should try to use the menu item text when possible. However, we might not have access to this information. I agree with you. It's an excellent idea to provide a hotkey property string for people rendering a complex item. It's even more important as people rendering raw strings will be better off with the native version of the select.

Does it make sense?

[ryancogswell]

@oliviertassinari Yes, I think that all makes sense to me. As far as the onChange, the performance concerns around re-rendering when open make sense -- especially in light of #13708 and #10847. I was assuming that the menu would always be open when keying through items, so I was trying to decide between the same two behaviors you laid out for the open and close cases to use as the behavior for when it was open. Currently when you have focus on a SelectInput and start keying through items, the menu automatically opens, but this does not match the native behavior and my understanding of your comments is that we should change the behavior to more closely match the native behavior so that the menu stays closed throughout the keystroke focus changes.

The main thing I'm unclear on is whether someone is already doing further work on #13708 or if I should take that on as a first step in this work. I don't think it will make sense for me to add this hot-key matching enhancement to the existing MenuList implementation since I'm unlikely to achieve acceptable performance with large lists without the rewrite already being in place, and the hot-key matching is most useful for large lists (in our application, we noticed needing this on a Select of countries). It looks like the primary aim of the rewrite is to get rid of the currentTabIndex state so that focus changes only require re-rendering (at most) the two menu items involved in the focus change (and then further changes to deal with dividers more appropriately and remove unnecessary transitions).

I'll take a stab at the rewrite (taking the proposed implementation from @RobertPurcea into account), but my timeline may be fairly slow since I'll have only very limited time to work on this over the next month and it will take me some time to fully digest some of the subtleties involved (though I just found the MenuList integration tests which are helpful and also means that there is more test coverage for the existing functionality than I initially realized).

With this rewrite, is it fine to assume React >= 16.8.0 and not use classes? It looks like requiring hook support is in the plans for 4.0.

[oliviertassinari]

@ryancogswell Yes, I think that the closer we are to the native select, the better. It's what people are already used to. It's interesting to consider that the native select behavior change between MacOS and Windows. Looking at one platform isn't enough. It's better to check both.

I'm not aware of any people working on the menu list issue. It's probably not an easy issue, but it's not crazy hard either. Completing this "focus" effort would be incredibly valuable for the library. I know that Bootstrap has an interesting focus logic handling for its dropdown, it's a good source of benchmarking. So if you want to improve the country selection problem, improving the up and down key down performance is a perfect starting point.

Yes, it was going to be my next suggestion. You can fully take advantage of the hook API. It's a good opportunity to migrate from a class to a functional component. We understand that you have a limited development bandwidth, we will do our best to make it fun.

[ryancogswell]

@oliviertassinari I thought I had this all done -- all my tests were passing; and then I looked at it visually. When I changed focus based on text, the focus styling didn't appear. Eventually I traced this to focusVisible.js and how the focusVisible class is controlled. focusVisible.js watches for certain keys that it thinks can potentially change focus, but with my changes just about any key can cause a focus change.

I think this logic around the focusVisible class is also responsible for the perceived slowness in #10847. I addressed the actual rendering performance, but there is still a delay. I think part of the reason for the delay is that the focusVisible logic relies on keyup even though the focus changes are being applied in keydown.

If I change the following styles in ListItem:

  /* Styles applied to the (normally root) `component` element. May be wrapped by a `container`. */
  root: {
    '&$selected, &$selected:hover': {
      backgroundColor: theme.palette.action.selected,
    },
  },
  // To remove in v4
  /* Styles applied to the `component`'s `focusVisibleClassName` property if `button={true}`. */
  focusVisible: {
    backgroundColor: theme.palette.action.selected,
  },

to instead be (using :focus pseudo-class instead of focusVisible class):

  /* Styles applied to the (normally root) `component` element. May be wrapped by a `container`. */
  root: {
    '&$selected, &$selected:hover, &:focus': {
      backgroundColor: theme.palette.action.selected,
    },
  },

then the focus highlighting is instant and works fully for my text-based focus changes.

In that code there's a // To remove in v4 comment. Is this still something planned for v4? And if so, what is the extent of the intended change?

[oliviertassinari]

I thought I had this all done -- all my tests were passing; and then I looked at it visually.

@ryancogswell I guess it's were TDD fails with UI libraries 😂. This focusVisible.js topic is super interesting. The core idea behind it is to provide a ponyfill to this up-coming CSS feature. You can find a polyfill in https://github.com/WICG/focus-visible. However, we need the feature in the JavaScript land, not just the CSS land for the ripple pulsating logic. A potential area of simplification is to remove this JavaScript requirement, maybe the pulsating ripple it no longer in the specification 🙏. The current logic has 4 flaws:

1. On this demo: https://next.material-ui.com/utils/modal/#simple-modal. Use the keyboard to open the modal (enter) than to close it (escape), the pulsating ripple is missing. This problem root cause seems to be coming from .focus() not triggering the focus event. For a reason I can't explain, this diff makes the focus event to be triggered correctly on the button:

--- a/packages/material-ui/src/Modal/TrapFocus.js
+++ b/packages/material-ui/src/Modal/TrapFocus.js
@@ -106,7 +106,10 @@ function TrapFocus(props) {
         // Because IE 11 market share is low, we accept the restore focus being broken
         // and we silent the issue.
         if (lastFocus.current.focus) {
-          lastFocus.current.focus();
+          const node = lastFocus.current
+          setTimeout(() => {
+            node.focus();
+          })
         }

         lastFocus.current = null;

2. When using the keyboard keys to switch between two tabs. The focus is removed when leaving the tab, it's set back by the browser when returning to the tab. However, we don't have any key event. The focus visible is missing.
3. The delay issue you have just analyzed in Undesired delay on highlighting MenuItem when navigating Menu with arrow keys #10847.
4. This new [a-z] key problem.

@eps1lon has tried something in #15398.

In that code there's a // To remove in v4 comment. Is this still something planned for v4? And if so, what is the extent of the intended change?

This comment is outdated, we should remove it. We have tried to remove it in #14212. We had to revert in #14680.

[eps1lon]

The inofficial polyfill looks pretty good. I would prefer a fork of that that also let's us subscribe to the change from a component. Especially since our current implementation doesn't quite match :focus-visible in chrome.

@ryancogswell I guess it's were TDD fails with UI libraries

One day I'll look into our visual regression test to get diffable screenshots for component states.

[oliviertassinari]

I think this logic around the focusVisible class is also responsible for the perceived slowness in #10847. I addressed the actual rendering performance, but there is still a delay. I think part of the reason for the delay is that the focusVisible logic relies on keyup even though the focus changes are being applied in keydown.

@ryancogswell I would like to go back to this point n°3. I don't think that the root cause is correct. In #15398, I used useKeyboardFocus.js to implement the focus visible logic:

--- a/packages/material-ui/src/ListItem/ListItem.js
+++ b/packages/material-ui/src/ListItem/ListItem.js
@@ -8,6 +8,7 @@ import { isMuiElement, useForkRef } from '../utils/reactHelpers';
 import ListContext from '../List/ListContext';
 import ReactDOM from 'react-dom';
 import warning from 'warning';
+import useKeyboardFocus from '../Tooltip/useKeyboardFocus';

 export const styles = theme => ({
   /* Styles applied to the (normally root) `component` element. May be wrapped by a `container`. */
@@ -107,6 +108,7 @@ const ListItem = React.forwardRef(function ListItem(props, ref) {
     ...other
   } = props;

+  const [focusVisible, setFocusVisible] = React.useState(false);
   const context = React.useContext(ListContext);
   const childContext = {
     dense: dense || context.dense || false,
@@ -136,6 +138,8 @@ const ListItem = React.forwardRef(function ListItem(props, ref) {
   }, []);
   const handleRef = useForkRef(handleOwnRef, ref);

+  const isFocusVisible = useKeyboardFocus();
+
   const componentProps = {
     className: clsx(
       classes.root,
@@ -145,6 +149,7 @@ const ListItem = React.forwardRef(function ListItem(props, ref) {
         [classes.divider]: divider,
         [classes.disabled]: disabled,
         [classes.button]: button,
+        [classes.focusVisible]: focusVisible,
         [classes.alignItemsFlexStart]: alignItems === 'flex-start',
         [classes.secondaryAction]: hasSecondaryAction,
         [classes.selected]: selected,
@@ -158,7 +163,13 @@ const ListItem = React.forwardRef(function ListItem(props, ref) {

   if (button) {
     componentProps.component = componentProp || 'div';
-    componentProps.focusVisibleClassName = clsx(classes.focusVisible, focusVisibleClassName);
+    // componentProps.focusVisibleClassName = clsx(classes.focusVisible, focusVisibleClassName);
+    componentProps.onFocus = () => {
+      setFocusVisible(isFocusVisible());
+    };
+    componentProps.onBlur = () => {
+      setFocusVisible(false);
+    };
     Component = ButtonBase;
   }

You would notice a reduced delay. This made me realized that the current delay issue is in focusVisible.js where we wait for focusVisibleCheckTime before looking for the focus state. We kind of throttle the focus event. I think that it was done this way to solve a problem where the focus event was called but the blur event was not. A better strategy would probably to be optimistic about the future, check the focus right away, check a second time a few ms later.

[ryancogswell]

You can find a polyfill in https://github.com/WICG/focus-visible.

@oliviertassinari This was very helpful background. I didn't really understand what focusVisible.js was trying to achieve.

4. This new [a-z] key problem.

Also:
5. When using keyboard focus navigation in menus, if you hold the key down just a little longer than normal (I suspect it's when you are just over the 500ms threshold) you lose the focus styling. If you hold the key down really long (e.g. a second or more) its fine and a normal quick key press also works fine, but there is a range in the middle where you lose the focus styling.

Later today I'll go ahead and create a pull request with my MenuList changes. It won't make sense to merge until the focusVisible issue is addressed, but the code review can go ahead and occur.

I'll see if I can digest the existing focus visible logic well enough to attempt a replacement based on the polyfill, but it looks like something that will be difficult to fix without breaking it in new ways. If I make a go at this, I'll do it in a separate branch from my MenuList changes.

[ryancogswell]

I'll see if I can digest the existing focus visible logic well enough to attempt a replacement based on the polyfill

Since @eps1lon is already digging into reworking the focus visible logic in #15484, I'll leave this alone since it would be a hefty learning curve.

[ryancogswell]

@delewis13 This exact problem is already answered on StackOverflow here: https://stackoverflow.com/questions/58378786/how-to-disable-the-selection-of-an-item-when-the-first-letter-of-the-option-is-p/58379376#58379376.

[blitzbohne]

Not sure if this belongs in here but is there any plan to also support Keyboard while the Select's Dropdown is closed but the Select component has focus? As for native controls this works fine you can have the dropdown being close and still type values and choose from.

[oliviertassinari]

We have no plan for it.