From e3a37cb45422b8b0d04668836620b7af667e6cda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Wed, 22 Feb 2023 11:05:28 +0100 Subject: [PATCH] [Select] Fix incorrect selecting of first element (#36024) Co-authored-by: Marija Najdova --- .../material/components/selects/selects.md | 61 +++++++++++++++++ .../src/ListSubheader/ListSubheader.js | 2 + .../mui-material/src/MenuList/MenuList.js | 11 +++ .../mui-material/src/Select/Select.test.js | 68 ++++++++++++++++++- .../mui-material/src/Select/SelectInput.js | 27 +------- 5 files changed, 142 insertions(+), 27 deletions(-) diff --git a/docs/data/material/components/selects/selects.md b/docs/data/material/components/selects/selects.md index f173272e057968..53acbc214ed801 100644 --- a/docs/data/material/components/selects/selects.md +++ b/docs/data/material/components/selects/selects.md @@ -127,6 +127,67 @@ Display categories with the `ListSubheader` component or the native `` {{"demo": "GroupedSelect.js"}} +:::warning +If you wish to wrap the ListSubheader in a custom component, you'll have to annotate it so Material UI can handle it properly when determining focusable elements. + +You have two options for solving this: +Option 1: Define a static boolean field called `muiSkipListHighlight` on your component function, and set it to `true`: + +```tsx +function MyListSubheader(props: ListSubheaderProps) { + return ; +} + +MyListSubheader.muiSkipListHighlight = true; +export default MyListSubheader; + +// elsewhere: + +return ( + +``` + +Option 2: Place a `muiSkipListHighlight` prop on each instance of your component. +The prop doesn't have to be forwarded to the ListSubheader, nor present in the underlying DOM element. +It just has to be placed on a component that's used as a subheader. + +```tsx +export default function MyListSubheader( + props: ListSubheaderProps & { muiSkipListHighlight: boolean }, +) { + const { muiSkipListHighlight, ...other } = props; + return ; +} + +// elsewhere: + +return ( + +); +``` + +We recommend the first option as it doesn't require updating all the usage sites of the component. + +Keep in mind this is **only necessary** if you wrap the ListSubheader in a custom component. +If you use the ListSubheader directly, **no additional code is required**. +::: + ## Accessibility To properly label your `Select` input you need an extra element with an `id` that contains a label. diff --git a/packages/mui-material/src/ListSubheader/ListSubheader.js b/packages/mui-material/src/ListSubheader/ListSubheader.js index 6fcb2db0d9d095..a0b566b6a866ce 100644 --- a/packages/mui-material/src/ListSubheader/ListSubheader.js +++ b/packages/mui-material/src/ListSubheader/ListSubheader.js @@ -100,6 +100,8 @@ const ListSubheader = React.forwardRef(function ListSubheader(inProps, ref) { ); }); +ListSubheader.muiSkipListHighlight = true; + ListSubheader.propTypes /* remove-proptypes */ = { // ----------------------------- Warning -------------------------------- // | These PropTypes are generated from the TypeScript type definitions | diff --git a/packages/mui-material/src/MenuList/MenuList.js b/packages/mui-material/src/MenuList/MenuList.js index a9f2337a528998..ad13b3e5033c63 100644 --- a/packages/mui-material/src/MenuList/MenuList.js +++ b/packages/mui-material/src/MenuList/MenuList.js @@ -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; + } + } }); const items = React.Children.map(children, (child, index) => { diff --git a/packages/mui-material/src/Select/Select.test.js b/packages/mui-material/src/Select/Select.test.js index ecf81decd27f7f..1f9dd561466348 100644 --- a/packages/mui-material/src/Select/Select.test.js +++ b/packages/mui-material/src/Select/Select.test.js @@ -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(' + Category 1 + Ten + Category 2 + Twenty + Thirty + , + ); + + 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(' + Category 1 + Option 1 + Option 2 + Category 2 + Option 3 + Option 4 + , + ); + + 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 ; + } + + it('highlights the first selectable option below the header', () => { + const { getByText } = render( + , + ); + + 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(