From b365431f382ed2487d7867151740e53a035981de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Wed, 1 Feb 2023 22:45:27 +0100 Subject: [PATCH 1/6] Revert #27299 (but leave its tests) --- .../mui-material/src/Select/SelectInput.js | 27 ++----------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/packages/mui-material/src/Select/SelectInput.js b/packages/mui-material/src/Select/SelectInput.js index 13391a0633d92a..8b3172fe663b39 100644 --- a/packages/mui-material/src/Select/SelectInput.js +++ b/packages/mui-material/src/Select/SelectInput.js @@ -350,7 +350,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) { } } - const items = childrenArray.map((child, index, arr) => { + const items = childrenArray.map((child) => { if (!React.isValidElement(child)) { return null; } @@ -391,26 +391,6 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) { foundMatch = true; } - if (child.props.value === undefined) { - return React.cloneElement(child, { - 'aria-readonly': true, - role: 'option', - }); - } - - const isFirstSelectableElement = () => { - if (value) { - return selected; - } - const firstSelectableElement = arr.find( - (item) => item?.props?.value !== undefined && item.props.disabled !== true, - ); - if (child === firstSelectableElement) { - return true; - } - return selected; - }; - return React.cloneElement(child, { 'aria-selected': selected ? 'true' : 'false', onClick: handleItemClick(child), @@ -427,10 +407,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) { } }, role: 'option', - selected: - arr[0]?.props?.value === undefined || arr[0]?.props?.disabled === true - ? isFirstSelectableElement() - : selected, + selected, value: undefined, // The value is most likely not a valid HTML attribute. 'data-value': child.props.value, // Instead, we provide it as a data attribute. }); From eb56017399a54d720d27ea436729bcdc46e90b1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Wed, 1 Feb 2023 22:46:24 +0100 Subject: [PATCH 2/6] Ignore non-focusable items when setting initial highlight --- .../src/ListSubheader/ListSubheader.js | 2 ++ packages/mui-material/src/MenuList/MenuList.js | 8 ++++++++ packages/mui-material/src/Select/Select.test.js | 17 ++++++++++++++++- 3 files changed, 26 insertions(+), 1 deletion(-) 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 8e8f42f36fac63..757da0be9122cb 100644 --- a/packages/mui-material/src/MenuList/MenuList.js +++ b/packages/mui-material/src/MenuList/MenuList.js @@ -232,6 +232,14 @@ const MenuList = React.forwardRef(function MenuList(props, ref) { activeItemIndex = index; } } + + if (activeItemIndex === index && (child.props.disabled || 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..2b0049a9c6a902 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( From 905bdda1303cf91ce140ee6af93bff6a42feace2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Wed, 8 Feb 2023 14:26:19 +0100 Subject: [PATCH 3/6] Support muiSkipOptionHighlight prop --- .../mui-material/src/MenuList/MenuList.js | 5 +- .../mui-material/src/Select/Select.test.js | 51 ++++++++++++++++++- 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/packages/mui-material/src/MenuList/MenuList.js b/packages/mui-material/src/MenuList/MenuList.js index 552bda3a9369f3..ad13b3e5033c63 100644 --- a/packages/mui-material/src/MenuList/MenuList.js +++ b/packages/mui-material/src/MenuList/MenuList.js @@ -233,7 +233,10 @@ const MenuList = React.forwardRef(function MenuList(props, ref) { } } - if (activeItemIndex === index && (child.props.disabled || child.type.muiSkipListHighlight)) { + 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. diff --git a/packages/mui-material/src/Select/Select.test.js b/packages/mui-material/src/Select/Select.test.js index 2b0049a9c6a902..1f9dd561466348 100644 --- a/packages/mui-material/src/Select/Select.test.js +++ b/packages/mui-material/src/Select/Select.test.js @@ -564,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( + Group 1 + Option 1 + Option 2 + Group 2 + Option 3 + Option 4 + {/* ... */} + +``` + +...or 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. From 7c6fa53036286fde67a2f7a6a1a83dd237c042be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Wed, 22 Feb 2023 09:19:20 +0100 Subject: [PATCH 5/6] Update docs/data/material/components/selects/selects.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Marija Najdova Signed-off-by: Michał Dudak --- docs/data/material/components/selects/selects.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/data/material/components/selects/selects.md b/docs/data/material/components/selects/selects.md index 9611168add386f..6c2af8e14dd6ef 100644 --- a/docs/data/material/components/selects/selects.md +++ b/docs/data/material/components/selects/selects.md @@ -130,7 +130,8 @@ Display categories with the `ListSubheader` component or the native `` :::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 can either define a static boolean field called `muiSkipListHighlight` on your component function, and set it to `true`: +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) { From 527709bf877de5dc464e92f1f06c1b65e62938da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Wed, 22 Feb 2023 09:22:19 +0100 Subject: [PATCH 6/6] Update docs/data/material/components/selects/selects.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Marija Najdova Signed-off-by: Michał Dudak --- docs/data/material/components/selects/selects.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/data/material/components/selects/selects.md b/docs/data/material/components/selects/selects.md index 6c2af8e14dd6ef..53acbc214ed801 100644 --- a/docs/data/material/components/selects/selects.md +++ b/docs/data/material/components/selects/selects.md @@ -155,7 +155,7 @@ return ( ``` -...or place a `muiSkipListHighlight` prop on each instance of your component. +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.