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

[ModalUnstyled] Fix errors from the W3C validator about incorrect aria-hidden attribute on some elements #30920

Merged
merged 4 commits into from Jun 8, 2022

Conversation

mkrtchian
Copy link
Contributor

When using the Dialog component with a container having siblings like <noscript>, the generated code gets an error from the W3C validator saying:

Error: The aria-hidden attribute must not be specified on the noscript element.

This happens for everyone using Create React App with the default HTML code in the public/ directory, and letting the default modal container to <body>.

Here is a reproduction from Create React App and Material UI example, that has a simple Dialog and a few sibling elements that get the aria-hidden attribute but should not.

Some tags were already excluded from getting aria-hidden on them, so it was easy for me to add more of them. I went through the ARIA in HTML specification and took all the tags that could be children of body with the mention No role or aria-* attributes, to exclude them too.

@mui-bot
Copy link

mui-bot commented Feb 5, 2022

Details of bundle changes

Generated by 🚫 dangerJS against b347680

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.

The logic looks good, left a few comments for improving the implementation.

packages/mui-base/src/ModalUnstyled/ModalManager.test.js Outdated Show resolved Hide resolved
packages/mui-base/src/ModalUnstyled/ModalManager.ts Outdated Show resolved Hide resolved
packages/mui-base/src/ModalUnstyled/ModalManager.ts Outdated Show resolved Hide resolved
packages/mui-base/src/ModalUnstyled/ModalManager.ts Outdated Show resolved Hide resolved
packages/mui-base/src/ModalUnstyled/ModalManager.ts Outdated Show resolved Hide resolved
@mkrtchian
Copy link
Contributor Author

I've done the changes.

@mkrtchian mkrtchian requested a review from mnajdova May 18, 2022 16:33
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.

It's a great first pull request on Material UI, thank you for working on it! 👌

@mnajdova mnajdova added accessibility a11y component: modal This is the name of the generic UI component, not the React module! labels Jun 8, 2022
@mnajdova mnajdova merged commit 89798b2 into mui:master Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: modal This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants