-
-
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 all 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,17 @@ const MenuList = React.forwardRef(function MenuList(props, ref) { | |
activeItemIndex = index; | ||
} | ||
} | ||
|
||
if ( | ||
activeItemIndex === index && | ||
(child.props.disabled || child.props.muiSkipListHighlight || child.type.muiSkipListHighlight) | ||
) { | ||
activeItemIndex += 1; | ||
if (activeItemIndex >= children.length) { | ||
// there are no focusable items within the list. | ||
activeItemIndex = -1; | ||
} | ||
} | ||
Comment on lines
+236
to
+245
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. Hi @michaldudak I have been looking for solutions to skip items in the menu list. And it looks like this only skips the initial focus when the menu is opened via keyboard. The traversal logic doesn't respect the In other words, Let me know if I should file an issue or how I can help further on fixing this. I might be missing the point of this prop too. So I'd be grateful if you can point out. Thanks in advance! 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. @ycmjason Answering on behalf of @michaldudak , the As for skipping the item in a menu list, I guess you need to disable the item, then it should be able to skip the highlight. |
||
}); | ||
|
||
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( | ||
|
@@ -549,8 +564,57 @@ describe('<Select />', () => { | |
}); | ||
}); | ||
|
||
describe('when the first child is a ListSubheader wrapped in a custom component', () => { | ||
describe('with the `muiSkipListHighlight` static field', () => { | ||
function WrappedListSubheader(props) { | ||
return <ListSubheader {...props} />; | ||
} | ||
|
||
WrappedListSubheader.muiSkipListHighlight = true; | ||
|
||
it('highlights the first selectable option below the header', () => { | ||
const { getByText } = render( | ||
<Select defaultValue="" open> | ||
<WrappedListSubheader>Category 1</WrappedListSubheader> | ||
<MenuItem value={1}>Option 1</MenuItem> | ||
<MenuItem value={2}>Option 2</MenuItem> | ||
<WrappedListSubheader>Category 2</WrappedListSubheader> | ||
<MenuItem value={3}>Option 3</MenuItem> | ||
<MenuItem value={4}>Option 4</MenuItem> | ||
</Select>, | ||
); | ||
|
||
const expectedHighlightedOption = getByText('Option 1'); | ||
expect(expectedHighlightedOption).to.have.attribute('tabindex', '0'); | ||
}); | ||
}); | ||
|
||
describe('with the `muiSkipListHighlight` prop', () => { | ||
function WrappedListSubheader(props) { | ||
const { muiSkipListHighlight, ...other } = props; | ||
return <ListSubheader {...other} />; | ||
} | ||
|
||
it('highlights the first selectable option below the header', () => { | ||
const { getByText } = render( | ||
<Select defaultValue="" open> | ||
<WrappedListSubheader muiSkipListHighlight>Category 1</WrappedListSubheader> | ||
<MenuItem value={1}>Option 1</MenuItem> | ||
<MenuItem value={2}>Option 2</MenuItem> | ||
<WrappedListSubheader muiSkipListHighlight>Category 2</WrappedListSubheader> | ||
<MenuItem value={3}>Option 3</MenuItem> | ||
<MenuItem value={4}>Option 4</MenuItem> | ||
</Select>, | ||
); | ||
|
||
const expectedHighlightedOption = getByText('Option 1'); | ||
expect(expectedHighlightedOption).to.have.attribute('tabindex', '0'); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('when the first child is a MenuItem disabled', () => { | ||
it('first selectable option is focused to use the arrow', () => { | ||
it('highlights the first selectable option below the header', () => { | ||
const { getAllByRole } = render( | ||
<Select defaultValue="" open> | ||
<MenuItem value="" disabled> | ||
|
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.