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.

Sign up for GitHub

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]Bug when the first child is a ListSubheader #27299

Merged
merged 13 commits into from
Apr 11, 2022
Merged

[Select]Bug when the first child is a ListSubheader #27299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8f97e85
Now check if the child is an option with value, and set the second op…
DouglasPds Jul 15, 2021
247e7fe
Merge branch 'next' into bug/listSubheader-not-navigate-with-arrow
DouglasPds Jul 15, 2021
3e2d580
Adding test to check the usability of the arrow when the ListSubheade…
DouglasPds Jul 20, 2021
5a970c5
Correcting test_types to circleci
DouglasPds Jul 20, 2021
52130ee
Merge with master
DouglasPds Mar 2, 2022
d028882
Deleting the file Select.test.js of material-ui, and passing my test …
DouglasPds Mar 2, 2022
5f197e1
Returning the GroupedSelect docs to the previous value
DouglasPds Mar 3, 2022
f966267
Improving the readability of the test name and checking when the firs…
DouglasPds Mar 3, 2022
408d3a0
Created a function to check which is the first selectable element, to…
DouglasPds Mar 3, 2022
9a45a10
Changing also the GroupedSelected typescript file
DouglasPds Mar 3, 2022
26a47e9
Merge branch 'master' into bug/listSubheader-not-navigate-with-arrow
DouglasPds Mar 31, 2022
efffa1f
Preventing an unnecessary arr.find
DouglasPds Mar 31, 2022
36402c8
Adding a test to check two headers at the top of the Select
DouglasPds Mar 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 83 additions & 0 deletions packages/mui-material/src/Select/Select.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from 'test/utils';
import { createTheme, ThemeProvider } from '@mui/material/styles';
import MenuItem from '@mui/material/MenuItem';
import ListSubheader from '@mui/material/ListSubheader';
import InputBase from '@mui/material/InputBase';
import OutlinedInput from '@mui/material/OutlinedInput';
import InputLabel from '@mui/material/InputLabel';
Expand Down Expand Up @@ -484,6 +485,88 @@ describe('<Select />', () => {
expect(getAllByRole('option')[1]).to.have.attribute('aria-selected', 'true');
});

describe('when the first child is a ListSubheader', () => {
michaldudak marked this conversation as resolved.
Show resolved Hide resolved
it('first selectable option is focused to use the arrow', () => {
const { getAllByRole } = render(
<Select defaultValue="" open>
<ListSubheader>Category 1</ListSubheader>
<MenuItem value={1}>Option 1</MenuItem>
<MenuItem value={2}>Option 2</MenuItem>
<ListSubheader>Category 2</ListSubheader>
<MenuItem value={3}>Option 3</MenuItem>
<MenuItem value={4}>Option 4</MenuItem>
</Select>,
);

const options = getAllByRole('option');
expect(options[1]).to.have.attribute('tabindex', '0');

act(() => {
fireEvent.keyDown(options[1], { key: 'ArrowDown' });
fireEvent.keyDown(options[2], { key: 'ArrowDown' });
fireEvent.keyDown(options[4], { key: 'Enter' });
});

expect(options[4]).to.have.attribute('aria-selected', 'true');
});

describe('when also the second child is a ListSubheader', () => {
it('first selectable option is focused to use the arrow', () => {
const { getAllByRole } = render(
<Select defaultValue="" open>
<ListSubheader>Empty category</ListSubheader>
<ListSubheader>Category 1</ListSubheader>
<MenuItem value={1}>Option 1</MenuItem>
<MenuItem value={2}>Option 2</MenuItem>
<ListSubheader>Category 2</ListSubheader>
<MenuItem value={3}>Option 3</MenuItem>
<MenuItem value={4}>Option 4</MenuItem>
</Select>,
);

const options = getAllByRole('option');
expect(options[2]).to.have.attribute('tabindex', '0');

act(() => {
fireEvent.keyDown(options[2], { key: 'ArrowDown' });
fireEvent.keyDown(options[3], { key: 'ArrowDown' });
fireEvent.keyDown(options[5], { key: 'Enter' });
});

expect(options[5]).to.have.attribute('aria-selected', 'true');
});
});
});

describe('when the first child is a MenuItem disabled', () => {
it('first selectable option is focused to use the arrow', () => {
const { getAllByRole } = render(
<Select defaultValue="" open>
<MenuItem value="" disabled>
<em>None</em>
</MenuItem>
<ListSubheader>Category 1</ListSubheader>
<MenuItem value={1}>Option 1</MenuItem>
<MenuItem value={2}>Option 2</MenuItem>
<ListSubheader>Category 2</ListSubheader>
<MenuItem value={3}>Option 3</MenuItem>
<MenuItem value={4}>Option 4</MenuItem>
</Select>,
);

const options = getAllByRole('option');
expect(options[2]).to.have.attribute('tabindex', '0');

act(() => {
fireEvent.keyDown(options[2], { key: 'ArrowDown' });
fireEvent.keyDown(options[3], { key: 'ArrowDown' });
fireEvent.keyDown(options[5], { key: 'Enter' });
});

expect(options[5]).to.have.attribute('aria-selected', 'true');
});
});

it('it will fallback to its content for the accessible name when it has no name', () => {
const { getByRole } = render(<Select value="" />);

Expand Down
27 changes: 25 additions & 2 deletions packages/mui-material/src/Select/SelectInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
}
}

const items = childrenArray.map((child) => {
const items = childrenArray.map((child, index, arr) => {
if (!React.isValidElement(child)) {
return null;
}
Expand Down Expand Up @@ -389,6 +389,26 @@ 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,
Copy link
Member

Choose a reason for hiding this comment

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

@DouglasPds Let's ensure that item is a valid React component here as well to prevent potential crash.

);
if (child === firstSelectableElement) {
return true;
}
return selected;
};

return React.cloneElement(child, {
'aria-selected': selected ? 'true' : 'false',
onClick: handleItemClick(child),
Expand All @@ -405,7 +425,10 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
}
},
role: 'option',
selected,
selected:
arr[0].props.value === undefined || arr[0].props.disabled === true
Copy link
Member

Choose a reason for hiding this comment

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

@michaldudak @DouglasPds

This assumes that the first child is always a valid React component.
This leads to crash if the first child is of different type. Here is a first report

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried here and when the first child is a number, string, NaN, it crashes. The first solution that works was to put optional chaining to check the values after the props. Here where the value is passed to selected and inside the function isFirstSelectableElement.

Copy link
Member

Choose a reason for hiding this comment

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

arr[0].props?.value === undefined || arr[0].props?.disabled === true

You are suggesting this, right? As long as this doesn't break the logic of the fix this PR originally contained, I am fine with that. Will you be willing to ensure that and open a PR with the change?

? isFirstSelectableElement()
: 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