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

Jump to bottom

[ModalManager] Fix aria-hidden of modal current node #13082

Merged
merged 5 commits into from
Oct 18, 2018
Merged

[ModalManager] Fix aria-hidden of modal current node #13082

merged 5 commits into from
Oct 18, 2018

Conversation

J-Kallunki
Copy link
Contributor

Hi!

This PR resolves #12710.
Fixed aria-hidden siblings functions to not alter current node and set proper for modal current node.
It uses aria-hidden='false' with modals instead of removing the property.

@J-Kallunki J-Kallunki changed the title Feature/drawer keepmounted aria hidden [ModalManager] fix aria hidden of modal current node Oct 3, 2018
@J-Kallunki J-Kallunki changed the title [ModalManager] fix aria hidden of modal current node [ModalManager] Fix aria hidden of modal current node Oct 3, 2018
@eps1lon eps1lon mentioned this pull request Oct 3, 2018
Closed
2 tasks
eps1lon
eps1lon reviewed Oct 3, 2018
View reviewed changes
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

This should include a test that would fail without the fix. It seems like we either don't test at all if the opened modal has aria-hidden="false" or the test is not constructed right.

packages/material-ui/src/Modal/manageAriaHidden.js Outdated
mount = [].concat(mount); // eslint-disable-line no-param-reassign
[].forEach.call(container.children, node => {
if (mount.indexOf(node) === -1 && isHidable(node)) {
if (mount.indexOf(node) === -1 && node !== currentNode && isHidable(node)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think mount.indexOf wanted to do the same as node !== currentNode. Could you find out what the difference between those two expressions is and add some documentation? That might be the actual bug here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That mount.indexOf works if we give it the currentNode instead of mountNode. Is that desired?

packages/material-ui/src/Modal/manageAriaHidden.js Outdated
@@ -20,14 +20,14 @@ export function ariaHidden(show, node) {
if (show) {
node.setAttribute('aria-hidden', 'true');
} else {
node.removeAttribute('aria-hidden');
node.setAttribute('aria-hidden', 'false');
Copy link
Member

Choose a reason for hiding this comment

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

Was this necessary for the fix? If not I'd rather not change this.

When the element is presented, authors MUST set the aria-hidden attribute to false or remove the attribute [...]
--- W3 WAI-ARIA

Seems like it shouldn't matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was in the 'Expected Behavior' of the issue

@oliviertassinari
Copy link
Member

Interesting issue, I will give a close look at what's happening in the documentation.

@oliviertassinari oliviertassinari self-assigned this Oct 4, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 5, 2018 •
edited
Loading

I gave a close look at the issue. Here is a quick overview of what's happening:

  1. The drawer's modal is mounted and closed during the first render, no aria-hidden is set.
  2. The drawer's modal is open, the did mount hook is called, set the aria-hidden on all the root elements.
  3. The drawer's modal is open, the portal hook is called, the modal is updated.

So, we rely on the double rendering phase to correctly set the aria-hidden attributes. It's brittle, it doesn't work if the modal is already mounted. Remembers me #12831.

I have also noticed another issue. If we use the disablePortal property, no aria-hidden attribute should be set.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 17, 2018 •
edited
Loading

@J-Kallunki Alright, let's get this fix in master. I'm looking at it now.

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: modal This is the name of the generic UI component, not the React module! labels Oct 18, 2018
@oliviertassinari oliviertassinari force-pushed the feature/drawer-keepmounted-aria-hidden branch from 4952711 to 7874372 Compare October 18, 2018 22:31
@oliviertassinari oliviertassinari force-pushed the feature/drawer-keepmounted-aria-hidden branch from 7874372 to 5f1e6f8 Compare October 18, 2018 22:42
@oliviertassinari oliviertassinari merged commit f1c6f98 into mui:master Oct 18, 2018
@oliviertassinari
Copy link
Member

@J-Kallunki Thank you! It's a shame we had this issue that long ♿️👼.

@oliviertassinari oliviertassinari changed the title [ModalManager] Fix aria hidden of modal current node [ModalManager] Fix aria-hidden of modal current node Oct 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eps1lon eps1lon eps1lon left review comments

Assignees

@oliviertassinari oliviertassinari

Labels
accessibility a11y bug 🐛 Something doesn't work component: modal This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@J-Kallunki @oliviertassinari @eps1lon