Skip to content

[ModalUnstyled] Apply aria-hidden to the correct Elements in the tree when disablePortal or container props are used - #34165

Closed
Greg-NetDuma wants to merge 2 commits into
mui:masterfrom
Greg-NetDuma:no-hidden-on-no-portal
Closed

[ModalUnstyled] Apply aria-hidden to the correct Elements in the tree when disablePortal or container props are used#34165
Greg-NetDuma wants to merge 2 commits into
mui:masterfrom
Greg-NetDuma:no-hidden-on-no-portal

Conversation

@Greg-NetDuma

@Greg-NetDuma Greg-NetDuma commented Sep 1, 2022

Copy link
Copy Markdown

Before if someone used disablePortal or container and opened a modal the whole page became inaccessible to Screen Readers. This PR aims to fix that.

Changes:

  • Make sure we pass in the correct container if disablePortal is used.
  • If the modal container is deep in the tree we need to make sure every sibling in every ancestor level is aria-hidden and and every aria-hidden we added gets cleaned up properly on unmount.
  • In addition because we don't differentiate between aria-hidden tags that got added by modals and added by devs, we need to pierce through (remove) every aria-hidden attribute in every ancestor of the modal container; this is to prevent no accessible elements on a page when multiple modals are present.

PS.: Existing tests were wrong, fixed them.

Problems I've discovered while fixing this:

  • disableEnforceFocus is not respected by ModalManager, it should not apply aria-hidden to other elements then (in my opinion)
  • TrapFocus sentinel elements should always be aria-hidden
  • TrapFocus sentinel elements should have tabIndex={-1} when disableEnforceFocus is set to prevent people tabbing to them.

Known Problems:

  • I only accounted for Modals that are either using disablePortal, container or using the default portal in body. Every other case will still break. I've not found a corner case like that yet.
  • If aria-hidden states were changed outside of Modal while the Modal was up the change is not tracked (same as before this PR)

@Greg-NetDuma Greg-NetDuma changed the title [ModalUnstyled] Do not aria-hide root if the top modal doesn't use the portal [ModalUnstyled] Do not aria-hide top-level container if the top modal doesn't use the portal Sep 1, 2022
@mui-bot

mui-bot commented Sep 1, 2022

Copy link
Copy Markdown

Netlify deploy preview

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

@material-ui/core: parsed: +0.08% , gzip: +0.13%
@material-ui/unstyled: parsed: +0.33% , gzip: +0.43%
@mui/joy: parsed: +0.06% , gzip: +0.10%

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 947cfa8

@Greg-NetDuma

Greg-NetDuma commented Sep 1, 2022

Copy link
Copy Markdown
Author

This clearly needs some new test cases, for example:

1. Modal added that doesn't use portal on after a Modal that uses the portal - toplevel is not aria-hidden
2. Modal added that doesn't use portal on after a Modal that doesn't use the portal - toplevel is still not aria-hidden
3. Modal added that uses portal on after a Modal that doesn't use the portal
4. Modal added that uses portal on after a Modal that uses the portal - previous behaviour

Technically I can get away just adding the 1. and 3. because the previous modal doesn't affect adding a new one on top of it.

Last Modal removed doesn't really need a test because it's a different codepath, aria-hidden is always removed.

I ended up fixing the existing tests because they were wrong.

@Greg-NetDuma
Greg-NetDuma marked this pull request as ready for review September 5, 2022 10:58
@Greg-NetDuma Greg-NetDuma changed the title [ModalUnstyled] Do not aria-hide top-level container if the top modal doesn't use the portal [ModalUnstyled] Apply aria-hidden to the correct Elements in the tree when disablePortal or container props are used Sep 5, 2022
@Greg-NetDuma
Greg-NetDuma force-pushed the no-hidden-on-no-portal branch from df92c4a to fbd4868 Compare September 5, 2022 12:12
@mnajdova mnajdova added accessibility a11y scope: modal Changes related to the modal. labels Sep 26, 2022
@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Dec 20, 2022
@mnajdova
mnajdova requested review from michaldudak and removed request for michaldudak and mnajdova December 21, 2022 13:45

@ZeeshanTamboli ZeeshanTamboli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Greg-NetDuma Can you merge with the latest master branch and fix the tests?

@Greg-NetDuma
Greg-NetDuma force-pushed the no-hidden-on-no-portal branch from 00d48d6 to ff9bef2 Compare May 12, 2023 12:03
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label May 12, 2023
@ZeeshanTamboli

Copy link
Copy Markdown
Member

Since there is no activity, I am closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accessibility a11y scope: modal Changes related to the modal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

4 participants