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
2 changes: 2 additions & 0 deletions packages/mui-material/src/ListSubheader/ListSubheader.js
Original file line number Diff line number Diff line change
@@ -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 |
8 changes: 8 additions & 0 deletions packages/mui-material/src/MenuList/MenuList.js
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)) {
Copy link
Member

@mnajdova mnajdova Feb 3, 2023

Choose a reason for hiding this comment

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

Suggested change
if (activeItemIndex === index && (child.props.disabled || child.type.muiSkipListHighlight)) {
if (activeItemIndex === index && (child.props.disabled || child.props.tabindex < 0)) {

Instead of coming up with new static prop, should we rather teach developers to use the tabindex correctly?

Copy link

@litera litera Feb 3, 2023

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

The 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).

Copy link
Member

Choose a reason for hiding this comment

The 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 data- attribute instead of static field? Something like data-mui-select-skip-tab or something shorter :))

Copy link
Member Author

@michaldudak michaldudak Feb 8, 2023

Choose a reason for hiding this comment

The 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: muiSkipOptionHighlight. See the added tests for usage.

activeItemIndex += 1;
if (activeItemIndex >= children.length) {
// there are no focusable items within the list.
activeItemIndex = -1;
}
}
});

const items = React.Children.map(children, (child, index) => {
17 changes: 16 additions & 1 deletion packages/mui-material/src/Select/Select.test.js
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', () => {
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(
27 changes: 2 additions & 25 deletions packages/mui-material/src/Select/SelectInput.js
Original file line number Diff line number Diff line change
@@ -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.
});