material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

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

Open Greg-NetDuma opened this issue 2 years ago • 2 comments

Fixes https://github.com/mui/material-ui/issues/19450

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.

Greg-NetDuma avatar Sep 01 '22 11:09 Greg-NetDuma

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 :no_entry_sign: dangerJS against 947cfa880c831394a68e9ca644cf2b47e3f2b744

mui-bot avatar Sep 01 '22 11:09 mui-bot

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 avatar Sep 01 '22 11:09 Greg-NetDuma

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

ZeeshanTamboli avatar Jul 29 '23 10:07 ZeeshanTamboli