New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Select menu does not fully implement keyboard controls #8191

Open
corytheboyd opened this Issue Sep 14, 2017 · 12 comments

Comments

9 participants
@corytheboyd

corytheboyd commented Sep 14, 2017

  • 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

This comment has been minimized.

Show comment
Hide comment
@corytheboyd

corytheboyd Sep 14, 2017

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?

corytheboyd commented Sep 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@oliviertassinari

oliviertassinari Sep 14, 2017

Member

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

Member

oliviertassinari commented Sep 14, 2017

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

@kgregory

This comment has been minimized.

Show comment
Hide comment
@kgregory

kgregory Sep 25, 2017

Member

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 👍

Member

kgregory commented Sep 25, 2017

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 👍

@oliviertassinari oliviertassinari removed the v1 label Oct 11, 2017

@corytheboyd

This comment has been minimized.

Show comment
Hide comment
@corytheboyd

corytheboyd Oct 14, 2017

@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.

corytheboyd commented Oct 14, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@jordangarside

jordangarside Nov 5, 2017

How do we use the native select component?

jordangarside commented Nov 5, 2017

How do we use the native select component?

@zsinryu

This comment has been minimized.

Show comment
Hide comment
@zsinryu

zsinryu Feb 8, 2018

Any update for this issue?

zsinryu commented Feb 8, 2018

Any update for this issue?

@RobertApelian

This comment has been minimized.

Show comment
Hide comment
@RobertApelian

RobertApelian Feb 20, 2018

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

RobertApelian commented Feb 20, 2018

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

This comment has been minimized.

Show comment
Hide comment
@goodbomb

goodbomb Feb 21, 2018

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

goodbomb commented Feb 21, 2018

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

@tzfrs

This comment has been minimized.

Show comment
Hide comment
@tzfrs

tzfrs Apr 13, 2018

Contributor

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

Contributor

tzfrs commented Apr 13, 2018

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

@oliviertassinari

This comment has been minimized.

Show comment
Hide comment
@oliviertassinari

oliviertassinari Apr 13, 2018

Member

@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.

Member

oliviertassinari commented Apr 13, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@mathieuforest

mathieuforest Jul 6, 2018

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

mathieuforest commented Jul 6, 2018

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

@mathieuforest

This comment has been minimized.

Show comment
Hide comment
@mathieuforest

mathieuforest Jul 13, 2018

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;

mathieuforest commented Jul 13, 2018

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;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment