-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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] Fix incorrect selecting of first element #36024
Changes from 2 commits
b365431
eb56017
c07f03e
905bdda
7319f17
d94cf02
7c6fa53
527709b
f0d7800
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -232,6 +232,14 @@ const MenuList = React.forwardRef(function MenuList(props, ref) { | |||||
activeItemIndex = index; | ||||||
} | ||||||
} | ||||||
|
||||||
if (activeItemIndex === index && (child.props.disabled || child.type.muiSkipListHighlight)) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Instead of coming up with new static prop, should we rather teach developers to use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is a very good suggestion that is also completely independent of the list item components being used. I like it lots more than the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The MenuList (and dependent compoenents) uses the roving tabindex pattern. Using this pattern, only one of the options (the highlighted one) should have tabindex=0. The rest should have tabindex=-1, so they are not tabbable (navigation between options is done using the arrow keys, not the tab key). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, using the tabindex conflicts with the roving tabindex pattern. @michaldudak have you consider using a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the record, we've discussed it outside GitHub and agreed to provide both options - the static field and a prop. I'll document both options. Since we're looking for a prop, not an attribute (as the DOM is not constructed at this point), I chose the same name as the static field: |
||||||
activeItemIndex += 1; | ||||||
if (activeItemIndex >= children.length) { | ||||||
// there are no focusable items within the list. | ||||||
activeItemIndex = -1; | ||||||
} | ||||||
} | ||||||
}); | ||||||
|
||||||
const items = React.Children.map(children, (child, index) => { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ import { | |
screen, | ||
} from 'test/utils'; | ||
import { createTheme, ThemeProvider } from '@mui/material/styles'; | ||
import MenuItem from '@mui/material/MenuItem'; | ||
import MenuItem, { menuItemClasses } from '@mui/material/MenuItem'; | ||
import ListSubheader from '@mui/material/ListSubheader'; | ||
import InputBase from '@mui/material/InputBase'; | ||
import OutlinedInput from '@mui/material/OutlinedInput'; | ||
|
@@ -393,6 +393,21 @@ describe('<Select />', () => { | |
}); | ||
}); | ||
|
||
it('should not have the selectable option selected when inital value provided is empty string on Select with ListSubHeader item', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An additional test should be added with just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There already is a test with the first item disabled around line 567. |
||
render( | ||
<Select open value=""> | ||
<ListSubheader>Category 1</ListSubheader> | ||
<MenuItem value={10}>Ten</MenuItem> | ||
<ListSubheader>Category 2</ListSubheader> | ||
<MenuItem value={20}>Twenty</MenuItem> | ||
<MenuItem value={30}>Thirty</MenuItem> | ||
</Select>, | ||
); | ||
|
||
const options = screen.getAllByRole('option'); | ||
expect(options[1]).not.to.have.class(menuItemClasses.selected); | ||
}); | ||
|
||
describe('SVG icon', () => { | ||
it('should not present an SVG icon when native and multiple are specified', () => { | ||
const { container } = render( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a detection mechanism implemented that skips non focusable items in the list that seems to work reliably otherwise keyboard interaction wouldn't work correctly (see grouping in the docs demo). I suggest to use the same detection for this PR instead of introducing a new static prop that adds a fairly important implementation detail to all the implementers? The other one works out of the box without anyone thinking or doing anything about it. It just skips non focusable items. It also works if you disable some otherwise focusable
MenuItem
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't affect all implementers, just those who wrap the ListSubheader in a custom component.
The existing keyboard navigation behavior relies on the DOM nodes already being constructed, as it checks the existence of
aria-disabled
andtabindex
attributes and focuses the right item.The initial highlight works differently as it has to overwrite the tabIndex prop of the first focusable item. Overwriting the
tabindex
attribute won't work here, as it would conflict with the prop.There certainly could be a way to redesign how this logic works, but I'm not eager to rewrite the Select. We've already implemented the SelectUnstyled, which will be used to power the Material UI's Select in the upcoming version. It is free from this bug (and many others).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
tabIndex
solution seems much better and universal than this prop.