From 572449d7518ef9b0c65ed5388519d52d81ef816b Mon Sep 17 00:00:00 2001
From: Lingzhi Xu <81196241+xulingzhihou@users.noreply.github.com>
Date: Wed, 27 Sep 2023 11:56:48 +0100
Subject: [PATCH] [material-ui][Select] Improve a11y by adding combobox role
and aria-controls attribute (#38785)
---
.../TablePagination/TablePagination.test.js | 31 +++----
.../mui-material/src/Select/Select.test.js | 93 ++++++++++---------
.../mui-material/src/Select/SelectInput.js | 8 +-
.../TablePagination/TablePagination.test.js | 35 ++++---
.../src/TextField/TextField.test.js | 4 +-
.../test/integration/Select.test.js | 12 +--
6 files changed, 96 insertions(+), 87 deletions(-)
diff --git a/packages/mui-material-next/src/TablePagination/TablePagination.test.js b/packages/mui-material-next/src/TablePagination/TablePagination.test.js
index aaf143d62a87b5..265b13954f51a6 100644
--- a/packages/mui-material-next/src/TablePagination/TablePagination.test.js
+++ b/packages/mui-material-next/src/TablePagination/TablePagination.test.js
@@ -83,7 +83,7 @@ describe('', () => {
describe('prop: labelRowsPerPage', () => {
it('labels the select for the current page', () => {
- const { getAllByRole } = render(
+ const { getByRole } = render(
@@ -100,13 +100,12 @@ describe('', () => {
,
);
- // will be `getByRole('combobox')` in aria 1.2
- const [combobox] = getAllByRole('button');
- expect(combobox).toHaveAccessibleName('lines per page: 10');
+ const combobox = getByRole('combobox');
+ expect(combobox).toHaveAccessibleName('lines per page:');
});
it('accepts React nodes', () => {
- const { getAllByRole } = render(
+ const { getByRole } = render(
@@ -127,15 +126,14 @@ describe('', () => {
,
);
- // will be `getByRole('combobox')` in aria 1.2
- const [combobox] = getAllByRole('button');
- expect(combobox).toHaveAccessibleName('lines per page: 10');
+ const combobox = getByRole('combobox');
+ expect(combobox).toHaveAccessibleName('lines per page:');
});
});
describe('prop: page', () => {
it('should disable the back button on the first page', () => {
- const { getAllByRole } = render(
+ const { getByRole } = render(
@@ -151,13 +149,14 @@ describe('', () => {
,
);
- const [, backButton, nextButton] = getAllByRole('button');
+ const backButton = getByRole('button', { name: 'Go to previous page' });
+ const nextButton = getByRole('button', { name: 'Go to next page' });
expect(backButton).to.have.property('disabled', true);
expect(nextButton).to.have.property('disabled', false);
});
it('should disable the next button on the last page', () => {
- const { getAllByRole } = render(
+ const { getByRole } = render(
@@ -173,7 +172,8 @@ describe('', () => {
,
);
- const [, backButton, nextButton] = getAllByRole('button');
+ const backButton = getByRole('button', { name: 'Go to previous page' });
+ const nextButton = getByRole('button', { name: 'Go to next page' });
expect(backButton).to.have.property('disabled', false);
expect(nextButton).to.have.property('disabled', true);
});
@@ -380,7 +380,7 @@ describe('', () => {
describe('prop: SelectProps', () => {
it('does allow manual label ids', () => {
- const { getAllByRole } = render(
+ const { getByRole } = render(
@@ -397,9 +397,8 @@ describe('', () => {
,
);
- // will be `getByRole('combobox')` in aria 1.2
- const [combobox] = getAllByRole('button');
- expect(combobox).toHaveAccessibleName('Rows per page: 10');
+ const combobox = getByRole('combobox');
+ expect(combobox).toHaveAccessibleName('Rows per page:');
});
});
diff --git a/packages/mui-material/src/Select/Select.test.js b/packages/mui-material/src/Select/Select.test.js
index 90940c073741db..eb863de4687d1f 100644
--- a/packages/mui-material/src/Select/Select.test.js
+++ b/packages/mui-material/src/Select/Select.test.js
@@ -68,7 +68,7 @@ describe('', () => {
,
);
- expect(getByRole('button')).to.have.property('tabIndex', 0);
+ expect(getByRole('combobox')).to.have.property('tabIndex', 0);
});
it('should accept null child', () => {
@@ -120,7 +120,7 @@ describe('', () => {
,
);
- const trigger = getByRole('button');
+ const trigger = getByRole('combobox');
fireEvent.mouseDown(trigger);
@@ -157,7 +157,7 @@ describe('', () => {
,
);
- const trigger = screen.getByRole('button');
+ const trigger = screen.getByRole('combobox');
act(() => {
trigger.focus();
});
@@ -177,7 +177,7 @@ describe('', () => {
,
);
- const button = getByRole('button');
+ const button = getByRole('combobox');
act(() => {
button.focus();
});
@@ -236,13 +236,13 @@ describe('', () => {
fireEvent.click(getByTestId('label'));
- expect(getByRole('button')).toHaveFocus();
+ expect(getByRole('combobox')).toHaveFocus();
});
it('should focus list if no selection', () => {
const { getByRole } = render();
- fireEvent.mouseDown(getByRole('button'));
+ fireEvent.mouseDown(getByRole('combobox'));
// TODO not matching WAI-ARIA authoring practices. It should focus the first (or selected) item.
expect(getByRole('listbox')).toHaveFocus();
@@ -258,7 +258,7 @@ describe('', () => {
,
);
- fireEvent.mouseDown(getByRole('button'));
+ fireEvent.mouseDown(getByRole('combobox'));
act(() => {
getAllByRole('option')[1].click();
});
@@ -279,7 +279,7 @@ describe('', () => {
,
);
- fireEvent.mouseDown(getByRole('button'));
+ fireEvent.mouseDown(getByRole('combobox'));
act(() => {
getAllByRole('option')[1].click();
});
@@ -296,7 +296,7 @@ describe('', () => {
,
);
- fireEvent.mouseDown(getByRole('button'));
+ fireEvent.mouseDown(getByRole('combobox'));
act(() => {
getAllByRole('option')[1].click();
});
@@ -308,7 +308,7 @@ describe('', () => {
describe('prop: defaultOpen', () => {
it('should be open on mount', () => {
const { getByRole } = render();
- expect(getByRole('button', { hidden: true })).to.have.attribute('aria-expanded', 'true');
+ expect(getByRole('combobox', { hidden: true })).to.have.attribute('aria-expanded', 'true');
});
});
@@ -371,7 +371,7 @@ describe('', () => {
,
);
- expect(getByRole('button')).to.have.text('Twenty');
+ expect(getByRole('combobox')).to.have.text('Twenty');
});
describe('warnings', () => {
@@ -439,19 +439,19 @@ describe('', () => {
// technically matter. This is only here in case we keep the rest accessible
const { getByRole } = render();
- expect(getByRole('button', { hidden: true })).to.have.attribute('aria-expanded', 'true');
+ expect(getByRole('combobox', { hidden: true })).to.have.attribute('aria-expanded', 'true');
});
specify('ARIA 1.2: aria-expanded="false" if the listbox isn\'t displayed', () => {
const { getByRole } = render();
- expect(getByRole('button')).to.have.attribute('aria-expanded', 'false');
+ expect(getByRole('combobox')).to.have.attribute('aria-expanded', 'false');
});
it('sets aria-disabled="true" when component is disabled', () => {
const { getByRole } = render();
- expect(getByRole('button')).to.have.attribute('aria-disabled', 'true');
+ expect(getByRole('combobox')).to.have.attribute('aria-disabled', 'true');
});
it('sets disabled attribute in input when component is disabled', () => {
@@ -463,13 +463,13 @@ describe('', () => {
specify('aria-disabled is not present if component is not disabled', () => {
const { getByRole } = render();
- expect(getByRole('button')).not.to.have.attribute('aria-disabled');
+ expect(getByRole('combobox')).not.to.have.attribute('aria-disabled');
});
it('indicates that activating the button displays a listbox', () => {
const { getByRole } = render();
- expect(getByRole('button')).to.have.attribute('aria-haspopup', 'listbox');
+ expect(getByRole('combobox')).to.have.attribute('aria-haspopup', 'listbox');
});
it('renders an element with listbox behavior', () => {
@@ -478,6 +478,13 @@ describe('', () => {
expect(getByRole('listbox')).toBeVisible();
});
+ it('indicates that input element has combobox role and aria-controls set to id of listbox', () => {
+ const { getByRole } = render();
+ const listboxId = getByRole('listbox').id;
+
+ expect(getByRole('combobox', { hidden: true })).to.have.attribute('aria-controls', listboxId);
+ });
+
specify('the listbox is focusable', () => {
const { getByRole } = render();
@@ -703,15 +710,15 @@ describe('', () => {
const { getByRole } = render();
// TODO what is the accessible name actually?
- expect(getByRole('button')).not.to.have.attribute('aria-labelledby');
+ expect(getByRole('combobox')).not.to.have.attribute('aria-labelledby');
});
it('is labelled by itself when it has a name', () => {
const { getByRole } = render();
- expect(getByRole('button')).to.have.attribute(
+ expect(getByRole('combobox')).to.have.attribute(
'aria-labelledby',
- getByRole('button').getAttribute('id'),
+ getByRole('combobox').getAttribute('id'),
);
});
@@ -725,7 +732,7 @@ describe('', () => {
,
);
- const triggers = getAllByRole('button');
+ const triggers = getAllByRole('combobox');
expect(triggers[0]).to.have.attribute(
'aria-labelledby',
@@ -745,9 +752,9 @@ describe('', () => {
,
);
- expect(getByRole('button')).to.have.attribute(
+ expect(getByRole('combobox')).to.have.attribute(
'aria-labelledby',
- `select-label ${getByRole('button').getAttribute('id')}`,
+ `select-label ${getByRole('combobox').getAttribute('id')}`,
);
});
@@ -776,7 +783,7 @@ describe('', () => {
,
);
- const target = getByRole('button');
+ const target = getByRole('combobox');
expect(target).to.have.attribute('aria-describedby', 'select-helper-text');
expect(target).toHaveAccessibleDescription('Helper text content');
});
@@ -790,7 +797,7 @@ describe('', () => {
,
);
- const trigger = screen.getByRole('button');
+ const trigger = screen.getByRole('combobox');
act(() => {
trigger.focus();
});
@@ -812,7 +819,7 @@ describe('', () => {
,
);
- fireEvent.mouseDown(getByRole('button'));
+ fireEvent.mouseDown(getByRole('combobox'));
clock.tick(99);
expect(onEntered.callCount).to.equal(0);
@@ -849,7 +856,7 @@ describe('', () => {
);
const paper = getByTestId('paper');
- const selectButton = getByRole('button', { hidden: true });
+ const selectButton = getByRole('combobox', { hidden: true });
expect(paper.style).to.have.property('minWidth', `${selectButton.clientWidth}px`);
});
@@ -888,7 +895,7 @@ describe('', () => {
,
);
- expect(getByRole('button')).to.have.attribute('data-test', 'SelectDisplay');
+ expect(getByRole('combobox')).to.have.attribute('data-test', 'SelectDisplay');
});
});
@@ -902,7 +909,7 @@ describe('', () => {
,
);
- expect(getByRole('button')).to.have.text('Ten');
+ expect(getByRole('combobox')).to.have.text('Ten');
});
});
@@ -916,7 +923,7 @@ describe('', () => {
,
);
- expect(getByRole('button')).to.have.text('0b100');
+ expect(getByRole('combobox')).to.have.text('0b100');
});
});
@@ -974,7 +981,7 @@ describe('', () => {
}
const { getByRole, queryByRole } = render();
- fireEvent.mouseDown(getByRole('button'));
+ fireEvent.mouseDown(getByRole('combobox'));
expect(getByRole('listbox')).not.to.equal(null);
act(() => {
@@ -1014,7 +1021,7 @@ describe('', () => {
,
);
- const trigger = getByRole('button');
+ const trigger = getByRole('combobox');
// If clicked by the right/middle mouse button, no options list should be opened
fireEvent.mouseDown(trigger, { button: 1 });
@@ -1033,7 +1040,7 @@ describe('', () => {
,
);
const parentEl = container.querySelector('.MuiInputBase-root');
- const button = getByRole('button');
+ const button = getByRole('combobox');
stub(parentEl, 'clientWidth').get(() => 14);
fireEvent.mouseDown(button);
@@ -1047,7 +1054,7 @@ describe('', () => {
,
);
const parentEl = container.querySelector('.MuiInputBase-root');
- const button = getByRole('button');
+ const button = getByRole('combobox');
stub(parentEl, 'clientWidth').get(() => 14);
fireEvent.mouseDown(button);
@@ -1081,7 +1088,7 @@ describe('', () => {
,
);
- fireEvent.mouseDown(getByRole('button'));
+ fireEvent.mouseDown(getByRole('combobox'));
expect(getByRole('listbox')).to.have.attribute('aria-multiselectable', 'true');
});
@@ -1097,7 +1104,7 @@ describe('', () => {
,
);
- expect(getByRole('button')).to.have.text('Ten, Twenty, Thirty');
+ expect(getByRole('combobox')).to.have.text('Ten, Twenty, Thirty');
});
it('should not throw an error if `value` is an empty array', () => {
@@ -1213,7 +1220,7 @@ describe('', () => {
});
const { getByRole, getAllByRole } = render();
- fireEvent.mouseDown(getByRole('button'));
+ fireEvent.mouseDown(getByRole('combobox'));
const options = getAllByRole('option');
fireEvent.click(options[2]);
@@ -1287,7 +1294,7 @@ describe('', () => {
it('should focus select after Select did mount', () => {
const { getByRole } = render();
- expect(getByRole('button')).toHaveFocus();
+ expect(getByRole('combobox')).toHaveFocus();
});
});
@@ -1321,7 +1328,7 @@ describe('', () => {
ref.current.focus();
});
- expect(getByRole('button')).toHaveFocus();
+ expect(getByRole('combobox')).toHaveFocus();
});
});
@@ -1329,13 +1336,13 @@ describe('', () => {
it('should have no id when name is not provided', () => {
const { getByRole } = render();
- expect(getByRole('button')).not.to.have.attribute('id');
+ expect(getByRole('combobox')).not.to.have.attribute('id');
});
it('should have select-`name` id when name is provided', () => {
const { getByRole } = render();
- expect(getByRole('button')).to.have.attribute('id', 'mui-component-select-foo');
+ expect(getByRole('combobox')).to.have.attribute('id', 'mui-component-select-foo');
});
});
@@ -1408,7 +1415,7 @@ describe('', () => {
});
expect(onChangeHandler.calledOnce).to.equal(true);
- expect(getByRole('button')).to.have.text('France');
+ expect(getByRole('combobox')).to.have.text('France');
});
it('should support native form validation', function test() {
@@ -1458,7 +1465,7 @@ describe('', () => {
,
);
- expect(document.activeElement).to.equal(getByRole('button'));
+ expect(document.activeElement).to.equal(getByRole('combobox'));
});
it('should not override the event.target on mouse events', () => {
@@ -1697,6 +1704,6 @@ describe('', () => {
fireEvent.click(getByTestId('test-element'));
- expect(getByRole('button')).not.toHaveFocus();
+ expect(getByRole('combobox')).not.toHaveFocus();
});
});
diff --git a/packages/mui-material/src/Select/SelectInput.js b/packages/mui-material/src/Select/SelectInput.js
index ec020a5a9e099c..9c36c98f142ef0 100644
--- a/packages/mui-material/src/Select/SelectInput.js
+++ b/packages/mui-material/src/Select/SelectInput.js
@@ -5,7 +5,7 @@ import PropTypes from 'prop-types';
import clsx from 'clsx';
import MuiError from '@mui/utils/macros/MuiError.macro';
import { unstable_composeClasses as composeClasses } from '@mui/base/composeClasses';
-import { refType } from '@mui/utils';
+import { refType, unstable_useId as useId } from '@mui/utils';
import ownerDocument from '../utils/ownerDocument';
import capitalize from '../utils/capitalize';
import Menu from '../Menu/Menu';
@@ -487,12 +487,15 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
...MenuProps.slotProps?.paper,
};
+ const listboxId = useId();
+
return (
', () => {
describe('prop: labelRowsPerPage', () => {
it('labels the select for the current page', () => {
- const { getAllByRole } = render(
+ const { getByRole } = render(
@@ -101,13 +101,12 @@ describe('', () => {
,
);
- // will be `getByRole('combobox')` in aria 1.2
- const [combobox] = getAllByRole('button');
- expect(combobox).toHaveAccessibleName('lines per page: 10');
+ const combobox = getByRole('combobox');
+ expect(combobox).toHaveAccessibleName('lines per page:');
});
it('accepts React nodes', () => {
- const { getAllByRole } = render(
+ const { getByRole } = render(
@@ -128,15 +127,14 @@ describe('', () => {
,
);
- // will be `getByRole('combobox')` in aria 1.2
- const [combobox] = getAllByRole('button');
- expect(combobox).toHaveAccessibleName('lines per page: 10');
+ const combobox = getByRole('combobox');
+ expect(combobox).toHaveAccessibleName('lines per page:');
});
});
describe('prop: page', () => {
it('should disable the back button on the first page', () => {
- const { getAllByRole } = render(
+ const { getByRole } = render(
@@ -152,13 +150,14 @@ describe('', () => {
,
);
- const [, backButton, nextButton] = getAllByRole('button');
+ const backButton = getByRole('button', { name: 'Go to previous page' });
+ const nextButton = getByRole('button', { name: 'Go to next page' });
expect(backButton).to.have.property('disabled', true);
expect(nextButton).to.have.property('disabled', false);
});
it('should disable the next button on the last page', () => {
- const { getAllByRole } = render(
+ const { getByRole } = render(
@@ -174,7 +173,8 @@ describe('', () => {
,
);
- const [, backButton, nextButton] = getAllByRole('button');
+ const backButton = getByRole('button', { name: 'Go to previous page' });
+ const nextButton = getByRole('button', { name: 'Go to next page' });
expect(backButton).to.have.property('disabled', false);
expect(nextButton).to.have.property('disabled', true);
});
@@ -381,7 +381,7 @@ describe('', () => {
describe('prop: SelectProps', () => {
it('does allow manual label ids', () => {
- const { getAllByRole } = render(
+ const { getByRole } = render(
@@ -398,14 +398,13 @@ describe('', () => {
,
);
- // will be `getByRole('combobox')` in aria 1.2
- const [combobox] = getAllByRole('button');
- expect(combobox).toHaveAccessibleName('Rows per page: 10');
+ const combobox = getByRole('combobox');
+ expect(combobox).toHaveAccessibleName('Rows per page:');
});
['standard', 'outlined', 'filled'].forEach((variant) => {
it(`should be able to apply the ${variant} variant to select`, () => {
- const { getAllByRole } = render(
+ const { getByRole } = render(
@@ -422,7 +421,7 @@ describe('', () => {
,
);
- const [combobox] = getAllByRole('button');
+ const combobox = getByRole('combobox');
const comboboxContainer = combobox.parentElement;
if (variant === 'standard') {
diff --git a/packages/mui-material/src/TextField/TextField.test.js b/packages/mui-material/src/TextField/TextField.test.js
index a0cb54184e6d46..4c28246272c122 100644
--- a/packages/mui-material/src/TextField/TextField.test.js
+++ b/packages/mui-material/src/TextField/TextField.test.js
@@ -202,7 +202,7 @@ describe('', () => {
,
);
- expect(getByRole('button')).toHaveAccessibleName('Release: Stable');
+ expect(getByRole('combobox')).toHaveAccessibleName('Release:');
});
it('creates an input[hidden] that has no accessible properties', () => {
@@ -224,7 +224,7 @@ describe('', () => {
,
);
- expect(getByRole('button')).toHaveAccessibleDescription('Foo bar');
+ expect(getByRole('combobox')).toHaveAccessibleDescription('Foo bar');
});
});
diff --git a/packages/mui-material/test/integration/Select.test.js b/packages/mui-material/test/integration/Select.test.js
index 446fbb4671b2fc..e9f31d3ef5981a 100644
--- a/packages/mui-material/test/integration/Select.test.js
+++ b/packages/mui-material/test/integration/Select.test.js
@@ -41,7 +41,7 @@ describe('