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.
When select menu is open, and I type the label of an option that is not selected, nothing happens.
Select
component
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 :)
| Tech | Version |
|--------------|---------|
| Material-UI | 1.0.0-beta-9 |
| React | 15.5.4 |
| browser | Chrome 61.0.3163.79 (Official Build) (64-bit) |
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?
I'm glad we expose a native select version too that don't suffer from this issue.
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 馃憤
@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.
How do we use the native select component?
Any update for this issue?
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
Just as a heads up, this issue is still occurring as of 1.0.0-beta.34
@oliviertassinari Is this a feature you would accept a PR for or is it intentional left out of the non-native selects?
@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.
@oliviertassinari the problem with native is that you cannot use the multiple prop.
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;
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"
@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:
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.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.
@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:
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?
@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.
@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.
@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?
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:
--- 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;
@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.
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.
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.
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.
- This new [a-z] key problem.
Also:
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.
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.
Is there anyway to disable this focus functionality? I've recently upgraded from V3.x and have run into an issue with one of my components.
The component is something like:
That is, a single text input at the top of my Select options that allows users to create additional items in the menu. Post upgrading to 4.X, this component no longer works, as the users focus is dragged around as they try to type, which I want to avoid!
@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.
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.
We have no plan for it.
Most helpful comment
Any update for this issue?