[material-ui][Select] Improve a11y by adding combobox role and aria-controls attribute #38785

xulingzhihou · 2023-09-03T02:56:15Z

Resolves: #35586

I have followed (at least) the PR section of the contributing guide.

Changelog instructions

If your tests require selecting the triggering element by the "button" role, then you'll have to change it to use "combobox"

…ls attribute Resolves: mui#35586

mui-bot · 2023-09-03T02:59:04Z

Netlify deploy preview

https://deploy-preview-38785--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 1acf350

DiegoAndai · 2023-09-05T14:06:10Z

Hey @xulingzhihou, thanks for working on this!

The attributes are being added only to the Native Select component. That's why the test is failing
I suggest taking a look at Base UI's useSelect implementation for guidance on these attributes. For example, we should generate a unique id for the aria-controls attribute.

xulingzhihou · 2023-09-05T22:11:50Z

generate a unique id for the aria-controls attribute

@DiegoAndai thanks for your feedback. I am very new to this repo and I want to clarify something here:

what do you mean by "Native Select component"? which other components should I add the attributes to?
regarding the id, I noticed there is already an id assigned to Menu component, and aria-controls should "Identifies the element that serves as the popup" which in this case is the Menu

DiegoAndai · 2023-09-06T15:45:52Z

@xulingzhihou thank you for taking an interest in contributing to Material UI 😊

what do you mean by "Native Select component"? which other components should I add the attributes to?

Sorry, I got confused. Don't consider my Native Select component comment 😅 I confused the SelectNativeInput here with NativeSelectInput, another component we use to render a native browser select.

regarding the id, I noticed there is already an id assigned to Menu component

The aria-controls ID should be the ID of the listbox element, and here we're referencing the menu root:

Finally, I think the combobox role and aria-controls properties should go to the element that right now has an incorrect "button" role. For comparison, look at Base UI's Select.

I don't know why we have the hidden input 🤔

@mui/core, does anyone know the use of this element?

oliviertassinari · 2023-09-11T01:31:05Z

does anyone know the use of this element?

Form auto-fill support, Form post support

packages/mui-material/test/integration/Select.test.js

packages/mui-material/src/Select/SelectInput.js

packages/mui-material/test/integration/Select.test.js

DiegoAndai · 2023-09-15T15:41:45Z

@xulingzhihou let me know if you need help with the failing tests

packages/mui-material-next/src/TablePagination/TablePagination.test.js

DiegoAndai · 2023-09-21T20:00:03Z

Hey @xulingzhihou, is this ready for review? 😊

Could you provide an example with the before and after to make this review easier? To do so, you can provide the same example in these different environments:

Latest released version: https://codesandbox.io/s/material-ui-issue-latest-s2dsx
Build for this PR: https://codesandbox.io/s/mui-issue-latest-f4wfn7

This should allow us to see the correct role and aria-controls attribute being applied. It would be perfect if you could do so with all components being affected (Select, TextField, TablePagination).

xulingzhihou · 2023-09-21T22:23:20Z

Hey @xulingzhihou, is this ready for review? 😊

Could you provide an example with the before and after to make this review easier? To do so, you can provide the same example in these different environments:

Latest released version: https://codesandbox.io/s/material-ui-issue-latest-s2dsx

Build for this PR: https://codesandbox.io/s/mui-issue-latest-f4wfn7

This should allow us to see the correct role and aria-controls attribute being applied. It would be perfect if you could do so with all components being affected (Select, TextField, TablePagination).

@DiegoAndai yes it is ready for review!

Here are the examples requested:

Latest released version: https://codesandbox.io/s/jovial-wildflower-xttyx6?file=/src/App.tsx:0-1384
Build for this PR: https://codesandbox.io/s/mui-issue-latest-forked-jq9yg9

packages/mui-material-next/src/TablePagination/TablePagination.test.js

DiegoAndai

This is ready to merge. There is just one last small change to clean up some comments fixed with this PR 🚀

To fix the Argos failures, we'll have to merge the updated master branch into this branch before merging the PR. Feel free to let me know if you need help with this.

packages/mui-material/src/Select/Select.test.js

This reverts commit 7e3539c.

DiegoAndai

Ready to merge 🎉
Great work 🙌🏼
I'll merge as soon as the CI passes

xulingzhihou · 2023-09-22T18:20:47Z

Ready to merge 🎉 Great work 🙌🏼 I'll merge as soon as the CI passes

Thank you for helping me with my first mui pr @DiegoAndai !!

plee-fn · 2023-10-06T18:25:05Z

packages/mui-material/src/Select/SelectInput.js

  return (
    <React.Fragment>
      <SelectSelect
        ref={handleDisplayRef}
        tabIndex={tabIndex}
-        role="button"
+        role="combobox"


Shouldn't this be considered a breaking change, so it should be a minor version change instead of a patch change?
v5.14.11 -> v5.15.0

Hi! It's an a11y bug fix; the "button" role was incorrect, so I think it falls under a patch version. Also, this should keep functionality intact; tests should only be impacted by how the component is selected. Please let me know if this is not the case for you 😊

The unit tests on the app that I'm working on broke because of this change when bumping MUI patch versions. Please do consider bumping the minor next time instead when making such a change

This was an absolutely awful decision. The notes on 5.14.12 did not detail that this would be a breaking change. It should have been called out immediately. I am working on an enterprise application with thousands of tests. Now they are all broken because of this one change that was supposedly "minor." How can we rely on a library that may break functionality on patch updates?

Hey @Lori-Becker and @mrharispe, I'm sorry for the inconvenience this might've caused. We will learn from this and be more careful moving forward. Thank you for using the library and providing feedback.

Improve Select Component a11y by adding combobox role and aria-contro…

942de8f

…ls attribute Resolves: mui#35586

zannager added the component: select This is the name of the generic UI component, not the React module! label Sep 4, 2023

zannager requested a review from DiegoAndai September 4, 2023 10:43

danilo-leal changed the title ~~Improve Select Component a11y by adding combobox role and aria-controls attribute~~ [material-ui][Select] Improve a11y by adding combobox role and aria-controls attribute Sep 5, 2023

danilo-leal added accessibility a11y package: material-ui Specific to @mui/material labels Sep 5, 2023

replace button role with combobox role

f47dcd2

xulingzhihou commented Sep 14, 2023

View reviewed changes

packages/mui-material/test/integration/Select.test.js Show resolved Hide resolved

DiegoAndai reviewed Sep 15, 2023

View reviewed changes

packages/mui-material/src/Select/SelectInput.js Outdated Show resolved Hide resolved

packages/mui-material/test/integration/Select.test.js Show resolved Hide resolved

address comments and fix tests

5859dc3

xulingzhihou commented Sep 20, 2023

View reviewed changes

packages/mui-material-next/src/TablePagination/TablePagination.test.js Show resolved Hide resolved

DiegoAndai reviewed Sep 22, 2023

View reviewed changes

packages/mui-material-next/src/TablePagination/TablePagination.test.js Outdated Show resolved Hide resolved

DiegoAndai reviewed Sep 22, 2023

View reviewed changes

DiegoAndai added 2 commits September 22, 2023 14:37

Update table pagination tests

9f0e07d

Remove unnecessary hidden option in test queries

7e3539c

xulingzhihou commented Sep 22, 2023

View reviewed changes

packages/mui-material/src/Select/Select.test.js Outdated Show resolved Hide resolved

DiegoAndai added 2 commits September 22, 2023 15:04

Revert "Remove unnecessary hidden option in test queries"

88d5c68

This reverts commit 7e3539c.

Merge branch 'master' into xulingzhihou/fixSelectA11y

b233d35

DiegoAndai approved these changes Sep 22, 2023

View reviewed changes

DiegoAndai added 2 commits September 22, 2023 17:55

Update new test

1c43753

Merge branch 'master' into xulingzhihou/fixSelectA11y

1acf350

DiegoAndai merged commit 572449d into mui:master Sep 27, 2023
6 checks passed

xulingzhihou deleted the xulingzhihou/fixSelectA11y branch October 3, 2023 20:03

plee-fn reviewed Oct 6, 2023

View reviewed changes

LukasTy mentioned this pull request Oct 19, 2023

Bump MUI Core mui/mui-x#10604

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[material-ui][Select] Improve a11y by adding combobox role and aria-controls attribute #38785

[material-ui][Select] Improve a11y by adding combobox role and aria-controls attribute #38785

xulingzhihou commented Sep 3, 2023 •

edited by DiegoAndai

Loading

mui-bot commented Sep 3, 2023 •

edited

Loading

DiegoAndai commented Sep 5, 2023 •

edited

Loading

xulingzhihou commented Sep 5, 2023 •

edited

Loading

DiegoAndai commented Sep 6, 2023

oliviertassinari commented Sep 11, 2023 •

edited

Loading

DiegoAndai commented Sep 15, 2023

DiegoAndai commented Sep 21, 2023

xulingzhihou commented Sep 21, 2023

DiegoAndai left a comment •

edited

Loading

DiegoAndai left a comment

xulingzhihou commented Sep 22, 2023

plee-fn Oct 6, 2023

DiegoAndai Oct 6, 2023

mrharispe Dec 7, 2023

Lori-Becker Jun 11, 2024 •

edited

Loading

DiegoAndai Jun 12, 2024

[material-ui][Select] Improve a11y by adding combobox role and aria-controls attribute #38785

[material-ui][Select] Improve a11y by adding combobox role and aria-controls attribute #38785

Conversation

xulingzhihou commented Sep 3, 2023 • edited by DiegoAndai Loading

Changelog instructions

mui-bot commented Sep 3, 2023 • edited Loading

Netlify deploy preview

Bundle size report

DiegoAndai commented Sep 5, 2023 • edited Loading

xulingzhihou commented Sep 5, 2023 • edited Loading

DiegoAndai commented Sep 6, 2023

oliviertassinari commented Sep 11, 2023 • edited Loading

DiegoAndai commented Sep 15, 2023

DiegoAndai commented Sep 21, 2023

xulingzhihou commented Sep 21, 2023

DiegoAndai left a comment • edited Loading

Choose a reason for hiding this comment

DiegoAndai left a comment

Choose a reason for hiding this comment

xulingzhihou commented Sep 22, 2023

plee-fn Oct 6, 2023

Choose a reason for hiding this comment

DiegoAndai Oct 6, 2023

Choose a reason for hiding this comment

mrharispe Dec 7, 2023

Choose a reason for hiding this comment

Lori-Becker Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

DiegoAndai Jun 12, 2024

Choose a reason for hiding this comment

xulingzhihou commented Sep 3, 2023 •

edited by DiegoAndai

Loading

mui-bot commented Sep 3, 2023 •

edited

Loading

DiegoAndai commented Sep 5, 2023 •

edited

Loading

xulingzhihou commented Sep 5, 2023 •

edited

Loading

oliviertassinari commented Sep 11, 2023 •

edited

Loading

DiegoAndai left a comment •

edited

Loading

Lori-Becker Jun 11, 2024 •

edited

Loading