Skip to content
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

Merged
merged 9 commits into from
Feb 22, 2023
61 changes: 61 additions & 0 deletions docs/data/material/components/selects/selects.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,67 @@ Display categories with the `ListSubheader` component or the native `<optgroup>`

{{"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 <ListSubheader {...props} />;
}

MyListSubheader.muiSkipListHighlight = true;
export default MyListSubheader;

// elsewhere:

return (
<Select>
<MyListSubheader>Group 1</MyListSubheader>
<MenuItem value={1}>Option 1</MenuItem>
<MenuItem value={2}>Option 2</MenuItem>
<MyListSubheader>Group 2</MyListSubheader>
<MenuItem value={3}>Option 3</MenuItem>
<MenuItem value={4}>Option 4</MenuItem>
{/* ... */}
</Select>
```

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 <ListSubheader {...other} />;
}

// elsewhere:

return (
<Select>
<MyListSubheader muiSkipListHighlight>Group 1</MyListSubheader>
<MenuItem value={1}>Option 1</MenuItem>
<MenuItem value={2}>Option 2</MenuItem>
<MyListSubheader muiSkipListHighlight>Group 2</MyListSubheader>
<MenuItem value={3}>Option 3</MenuItem>
<MenuItem value={4}>Option 4</MenuItem>
{/* ... */}
</Select>
);
```

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.
Expand Down
2 changes: 2 additions & 0 deletions packages/mui-material/src/ListSubheader/ListSubheader.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ const ListSubheader = React.forwardRef(function ListSubheader(inProps, ref) {
);
});

ListSubheader.muiSkipListHighlight = true;
Copy link

@litera litera Feb 2, 2023

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.

Copy link
Member Author

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 and tabindex 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).

Copy link

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.


ListSubheader.propTypes /* remove-proptypes */ = {
// ----------------------------- Warning --------------------------------
// | These PropTypes are generated from the TypeScript type definitions |
Expand Down
11 changes: 11 additions & 0 deletions packages/mui-material/src/MenuList/MenuList.js
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

The 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 muiSkipListHighlight prop.

In other words, muiSkipListHighlight in MenuList will only skip the focusVisible state for the initial render, but it is ignored when the user press key up/down to traverse the list items.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ycmjason Answering on behalf of @michaldudak , the muiSkipListHighlight prop is an internal prop and should be used only if you wrap the ListSubheader in a custom component. We document it's use-case here.

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) => {
Expand Down
68 changes: 66 additions & 2 deletions packages/mui-material/src/Select/Select.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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', () => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An additional test should be added with just MenuItem components where the first item is disabled.

Copy link
Member Author

Choose a reason for hiding this comment

The 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(
Expand Down Expand Up @@ -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>
Expand Down
27 changes: 2 additions & 25 deletions packages/mui-material/src/Select/SelectInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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),
Expand All @@ -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.
});
Expand Down