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

[docs] Fix a11y in Menu demos #30378

Merged

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented Dec 23, 2021

Closes #30125

Improves the Lighthouse accessibility score from 84 to 94 of this demo.

https://deploy-preview-30378--material-ui.netlify.app/components/menus/

@ZeeshanTamboli ZeeshanTamboli added accessibility a11y component: menu This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation labels Dec 23, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Dec 23, 2021

Details of bundle changes

Generated by 🚫 dangerJS against f930449

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

docs/src/pages/components/menus/BasicMenu.tsx Show resolved Hide resolved
@oliviertassinari oliviertassinari changed the title [docs] Fix aria-controls in Basic Menu demo. [docs] Fix aria-controls in menu demo Dec 23, 2021
@oliviertassinari oliviertassinari changed the title [docs] Fix aria-controls in menu demo [docs] Fix a11y in menu demo Dec 23, 2021
@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented Dec 23, 2021

The a11y of this demo https://deploy-preview-30378--material-ui.netlify.app/components/menus/#account-menu seems wrong too.

I improved a11y in the Menu demos which had low score and the score of each of them got better.

@ZeeshanTamboli ZeeshanTamboli changed the title [docs] Fix a11y in menu demo [docs] Fix a11y in Menu demos Dec 23, 2021
@oliviertassinari oliviertassinari removed their request for review December 23, 2021 13:57
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Looks great 👍

@@ -17,7 +17,7 @@ export default function BasicMenu() {
<div>
<Button
id="basic-button"
aria-controls="basic-menu"
aria-controls={open ? 'basic-menu' : undefined}
Copy link
Member

Choose a reason for hiding this comment

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

👍 Looks correct based on https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-controls

A combobox element has aria-controls set to a value that refers to the element that serves as the popup. The aria-controls only needs to be set when the popup is visible, but it is valid and easier to program to reference an element that is not visible.

Copy link
Member

@oliviertassinari oliviertassinari Dec 24, 2021

Choose a reason for hiding this comment

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

I guess that in our case, it's even more than non-visible => non-existent.

On a different note, the thread in w3c/aria#995 (comment) suggests that the attribute is close to poop.

@oliviertassinari oliviertassinari merged commit c0183d9 into mui:master Dec 24, 2021
@oliviertassinari
Copy link
Member

@ZeeshanTamboli Great, well done, thanks!

@iansjk
Copy link
Contributor

iansjk commented Dec 25, 2021

Thank you for submitting this 🙏🏼

@ZeeshanTamboli ZeeshanTamboli deleted the issue/30125-menu-accessibility branch December 29, 2021 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: menu This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs] Menu component examples appear to use aria-controls incorrectly
5 participants