[Modal] aria-hidden should not be applied on non-portal modals

[pnarielwala-tc]

aria-hidden=true gets applied on top level container when a modal is opened. But if disablePortal is used, aria-hidden should not be set on that top level container as the modal will be created inline and most likely within that top level container

Running axe on the opened modal yields the following error:

"ARIA hidden element must not contain focusable elements (aria-hidden-focus)"

Fix all of the following:
Focusable content should have tabindex='-1' or be removed from the DOM

You can find more information on this issue here:
https://dequeuniversity.com/rules/axe/3.3/aria-hidden-focus?application=axeAPI

https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html

The dialog element is not a descendant of any element that has aria-hidden set to true.

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

1. Modal with disablePortal prop is opened and aria-hidden is set to true on top level html node

Expected Behavior 🤔

1. Modal with disablePortal prop is opened and aria-hidden is not set to true on top level html node
2. Modal without disablePortal prop is opened and aria-hidden is set to true on top level html node

Steps to Reproduce 🕹

https://codesandbox.io/s/material-mui-demo-9oddt?fontsize=14&hidenavigation=1&theme=dark

Context 🔦

Your Environment 🌎

Tech	Version
Material-UI	v4.9.0
React	v16.12.0
Browser
TypeScript	v3.8.0-dev.20200125
etc.

[oliviertassinari]

I could have sworn that we had a 6 months old duplicate issue about this problem, but I can't find it, fare enough. Thanks for the report.

[dkadrios]

Has anyone found a suitable workaround for this? It's wreaking havoc with our accessibility efforts.

It doesn't matter if disablePortal is true or false, aria-hidden="true" gets applied regardless. Premptively setting aria-hidden="false" doesn't help either, it just gets overwritten.

I also have a second root container, dedicated to tooltips, that also is getting set to aria-hidden when a dialog is open. I'm assuming it is being applied to all root body components, not just the react container (?)

[dkadrios]

Super hacky but this seems to be working in the short-term:

useEffect(() => {
    if (open) {
      document
        .getElementById('react-container')
        .setAttribute('aria-hidden', 'false')
    }
  }, [open])

[eps1lon]

It doesn't matter if disablePortal is true or false, aria-hidden="true" gets applied regardless.

@dkadrios That doesn't sound right. Could you open a separate issue and fill out the template so that we can isolate the problem?

[kylewilt]

This appears to still be an issue on 4.11.0. I've traced it down to here:
Modal.js Line 124

The isTopModal function returns false and open is true. It seems that the call to add the current modal to the ModalManager instance doesn't happen early enough.

In my case I'm rendering a element with open={true} all the time as well as disablePortal being set to true.

Is there a particular reason why this hasn't been addressed other than priority and/or resources?

[oliviertassinari]

@kylewilt What's your use case for using disablePortal?

[kylewilt]

@oliviertassinari if we don’t use disablePortal accessibility tools start reporting a lot of problems for screen reader support. Having the dialog appear in the dom outside of the root react node causes you to get errors like missing a “main” element and heading order problems etc. it also tends to complain about hidden input elements.

There’s really no sane work around in that situation because adding the missing nodes to the dialog creates new errors about repeating main nodes etc. it’s basically problematic to put it outside the rest of the tree that the react components for the page live in.

My “insane” workaround at the moment is to use a web pack plugin to replace the modalmanger.JS import with my own which is a verbatim copy of the original with the ariaHidden function basically being a noop.

[mattdemichele-pearson]

@oliviertassinari Just to chime in on a use case for the disablePortal - I am developing a react app that gets loaded and initialized into a separate page that has existing non-react content, including CSS. I use disablePortal to keep the menus self-contained within my app so that I can ensure proper CSS theming.

This really shouldn't be causing aria-hidden=true to be set to the entire root element. I hope a bug fix can be found

[franciscopuyo]

The aria-hidden=true makes the modal not navigable by a screen reader and also the code not testable by using getByRole in react-testing-library

[chittolina]

any updates on this one?