[Modal] Fix stale aria-hidden when the container changes - #48889
[Modal] Fix stale aria-hidden when the container changes#48889atharv-sys32 wants to merge 3 commits into
Conversation
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
|
Hi @ZeeshanTamboli, could you take a look when you get a chance? This fixes #48882 (stale aria-hidden when a Modal's container changes). All checks are passing, including a regression test that swaps the container after mount. A maintainer label on the PR would also let the remaining label check go green. |
|
Hi @ZeeshanTamboli @silviuaavram, quick nudge on this one. The fix is ready, all CI checks pass, and I added a regression test for the stale aria-hidden case. The |
|
@ZeeshanTamboli this seems to be in the same area as your current transition fix, that also checks whether the portal container changes. Can you sync the work and make sure there's no duplicated effort? |
@silviuaavram This is a bit different than the fix in the transition PR (#48881). I will review this PR later after the transition one is accepted and merged, incase if there is any duplication. |
github-actions
Bot
added
PR: out-of-date
1ef9f63 to
b7e5c8b
Compare
|
Hi @ZeeshanTamboli, I rebased this branch on the latest master and resolved the conflicts that came up after your transition fix (#48881) landed. Both tests are kept: yours for the exit transition and mine for the stale aria-hidden case, they cover different scenarios. |
|
See #48882 (comment). A proper reproduction is needed first. |
ZeeshanTamboli
left a comment
ZeeshanTamboli
left a comment
There was a problem hiding this comment.
I added this PR build in package.json:
"@mui/material": "https://pkg.pr.new/mui/material-ui/@mui/material@eb8e990",
to test it out and it does not work as expected: https://stackblitz.com/edit/xnjsefyz-bwdrxq7f.
It should work both when disablePortal is used and also when container changes dynamically by using a controlled component. There should be a root fix for these.
ariaHiddenSiblings skipped hiding a container child if it contained the modal, so a non-portaled modal (disablePortal) whose DOM node lives inside one of the container's children is no longer rendered inside an aria-hidden ancestor. Previously the container's children were hidden unconditionally, making such modals inaccessible to assistive tech.
The disablePortal case is the same pre-existing bug, just triggered differently: since the modal isn't portaled, its DOM node sits inside one of the siblings the manager hides, so that sibling gets aria-hidden and the dialog becomes inaccessible. I've extended the fix so we never hide a container child that contains the modal. Both disablePortal and the container-change case should work now, and the normal portaled behavior is unchanged. One edge case I left out of scope: a portaled modal and a disablePortal modal open at the same time. That's a very rare combo so I'm not including that in this PR. |
ZeeshanTamboli
left a comment
ZeeshanTamboli
left a comment
There was a problem hiding this comment.
Findings
-
[P1] Moving the top modal can leave it inside
aria-hidden.
The new remove/add migration fails when another modal remains in the old container. Moving the top body modal into#rootleaves#roothidden because the old body container is not fully restored; adding the modal to#rootcannot unhide the container itself. -
[P1] Container migration changes modal stack order.
Removing and re-adding a lower modal appends it toModalManager.modals, promoting it above a newer modal. Focus trapping and Escape handling then move to the visually underlying modal. In the reproduction, pressing Escape inside the migrated lower modal incorrectly invoked itsonClose. Migration must preserve the original global stack position. -
[P1] The ancestor guard also blocks restoration.
isNotModalAncestoris applied for both hiding and unhiding. If the portal node has actually moved inside the destination beforeremove, restoration skips that destination and leaves its stalearia-hidden="true". The new test claims to simulate this but only changesmodal.mount; it never reparentsmodalRef. -
[P2]
disablePortalleaves the modal background accessible.
Skipping the entire React root prevents the modal from being hidden, but also leaves every background sibling inside that root exposed to assistive technology. Two inline modals similarly leave the older modal accessible. The intended fix from #43318 hides siblings along each ancestor level and handles mixed inline/portaled stacks. The author explicitly excluded that stack case in the latest reply, but it is central to the linked issue. -
[P2]
containeris still honored by the manager whendisablePortalignores it.
Portal renders inline whendisablePortalis enabled, buthandleOpenregisters and scroll-locks against the suppliedcontainer. An explicit or changing container can therefore hide and lock an unrelated subtree. Accessibility topology should come from the actual inline DOM location; scroll-lock container selection should remain a separate concern.
5 participants
Related to #19450 . Another PR attempt to fix: #43318
Root cause
When a
Modal'scontainerprop changes while the modal stays open, the modal is moved into the new container but the previous container keeps thearia-hiddenstate that was computed for the previous sibling set. For example, a modal mounted indocument.body(default container) marks#rootasaria-hidden; switchingcontainerto a node inside#rootleaves#roothidden, making the dialog inaccessible to assistive technologies (getByRole('dialog')fails).Changes
useModal: track the container the modal is registered with (registeredContainerRef) and re-register the modal with theModalManagerwhen the resolved container changes while open.ModalManager: when removing the last modal of a container, restore siblings using the container itself as the blacklist entry instead ofmodal.mount. The mount node is already updated to the new container by the portal before the manager is notified, which previously kept the new container in the blacklist and left itaria-hidden.Tests
ModalManager.test.ts: unit test covering the re-registration flow to a nested container.Modal.test.js: regression test that changes thecontainerprop after mount and asserts the previous container losesaria-hidden.