[material-ui][Modal] Apply aria-hidden to the correct elements when Modals are mounted to other places than document.body - #43318
[material-ui][Modal] Apply aria-hidden to the correct elements when Modals are mounted to other places than document.body#43318Gr3q wants to merge 53 commits into
Conversation
Netlify deploy previewhttps://deploy-preview-43318--material-ui.netlify.app/ Bundle size report@mui/material parsed: 🔺+320B(+0.06%) gzip: 🔺+160B(+0.10%) DetailsShow details for 100 more bundles (86 more not shown)@mui/material/Dialog parsed: 🔺+326B(+0.34%) gzip: 🔺+135B(+0.40%) |
Gr3q
changed the title
Gr3q
changed the title
aarongarciah
added
scope: modal
aarongarciah
changed the title
|
Your argos check seems to be borked especially I made no changed that would affect visuals. |
There was a problem hiding this comment.
@Gr3q, can you point the PR to the master branch, our current development branch for releases? I'll review it once that's done.
Your argos check seems to be borked especially I made no changed that would affect visuals.
I think its because the target branch is v5 and argos checks against master.
a7e66a1 to
59f21c9
Compare
|
Should I not touch the unstable folder in the future? Could you explain why does it exist? |
I reverted the changes in Base UI ( |
ZeeshanTamboli
left a comment
ZeeshanTamboli
left a comment
There was a problem hiding this comment.
@Gr3q Thanks for the pull request.
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 last modal container; this is to prevent no accessible elements on a page when multiple modals are present.
I didn't understand this. Can you please explain more clearly with an example?
Previous bug still stands when you mess with aria-hidden states in the tree outside your modal, when the modal gets removed your changes won't be preserved (because the modal will restore the state when it was added)
What bug is this? Can you give a bug reproduction in the form of StackBlitz or CodeSandbox?
Co-authored-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com> Signed-off-by: Attila Greguss <floyd0122@gmail.com>
Co-authored-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com> Signed-off-by: Attila Greguss <floyd0122@gmail.com>
Co-authored-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com> Signed-off-by: Attila Greguss <floyd0122@gmail.com>
I'm changing the code, but I want to summarize the bugs this PR had, then what you want. Bugs:
So let me clarify what you want: You want me to change it that scroll lock works peropely when:
and it shouldn't work properly (aka scroll lock applies to the container that effectively does nothing to prevent page scrolling):
I want you to confirm this because this is the only way bugs 1 and 2 can be resolved on the Modal docs page at the same time. (Edit: I'm asking because I will need to implement some workarounds to handle that) |
|
@Gr3q It's been a while, but from what I remember you're right—that's what's needed. |
|
@ZeeshanTamboli I fixed the doc page, it does what you want (so no breaking changes). Added a comment that technically the behaviour around the scrolllock+container is still a bug, hopefully someone will remove it when the next major version comes. Fixed the existing tests and added new tests for this too. So now the PR should be ready. 🤞 |
ZeeshanTamboli
left a comment
ZeeshanTamboli
left a comment
There was a problem hiding this comment.
@Gr3q This looks good to me. I left one comment.
I'm unsure if this will be considered since the focus is shifting to re-implementing Material UI with the new Base UI in the next major release or the one after. I appreciate the time you've invested in this, and I apologize in advance if it doesn’t move forward. However, it might still be possible to release this now or in the next major version. This will definitely need further review. @DiegoAndai, could you take a look?
I think it's still be a while that is released, so it's worth fixing the current version now. |
|
Hey @Gr3q and @ZeeshanTamboli. First of all, thanks for working on this 🙌🏼
While true, we can still accept and review improvements from the community regarding these topics. So, let's keep working on this. We can land it in v7, which will help many people (given the number of upvotes in the issue). The first thing I would say is that we should change to using It will be a breaking change, but we're already on v7 on the Let me know what you think. |
See conversation from #43318 (review). That wasn't planned back then but maybe we can consider it now for a major release. |
There was a problem hiding this comment.
Ok, I checked the discussion on this PR and also reached out to the Base UI team.
In conclusion, we are not able to stop using aria-hidden as I suggested. So I would say we should move forward with this PR's intention: fix aria-hidden handling when disablePortal is true.
Here's my initial review.
By the way, thanks for working on this @Gr3q
| const isPreviousElement = element === previousElement; | ||
|
|
||
| // We came from here | ||
| if (isPreviousElement) { |
There was a problem hiding this comment.
If isPreviousElement is true, it means that we're on the modal or one of its ancestors, right?
There was a problem hiding this comment.
Yes, that is correct. Can happen if container is set to the same thing as the other args.
| // If any ancestor has aria-hidden applied (e.g. by another modal), the current modal could become inaccessible. | ||
| // We remove aria-hidden from ancestors to ensure the current modal is accessible, even though this might not be ideal if aria-hidden wasn't added by another modal (For example, if a developer manually applied aria-hidden to hide certain content, removing it could lead to unintended accessibility issues.). |
There was a problem hiding this comment.
We shouldn't do this:
If multiple modals are open, or the developer applied aria-hidden to one of the modal's ancestors, it's the developer responsibility to fix it.
This is not an acceptable side-effect:
[...] if a developer manually applied aria-hidden to hide certain content, removing it could lead to unintended accessibility issues.
There was a problem hiding this comment.
This is the most important part of the fix. How do you propose me fixing this problem:
- Open a Modal with
disablePortal - Open another Modal with no
disablePortal
No need for nesting modals etc, just do it programmatically. Now step one applied aria-hidden to the ModalManager's parent container, step 2 will apply aria-hidden to the parent of step one's Modal. Without that part it's completely inaccessible, no need for developer error.
If multiple modals are open
, or the developer applied aria-hidden to one of the modal's ancestors,it's the developer responsibility to fix it.
Why would you expect the dev's to fix something that the library is doing to itself? How the hell would you fix this as a dev? ModalManager is not exposed, you can't replace it with something that actually works.
If
multiple modals are open, orthe developer applied aria-hidden to one of the modal's ancestors, it's the developer responsibility to fix it.
This one makes sense. Tell me, how would I go about tracking which ones were added by ModalManager, which ones are added by the dev, or my favorite case: when both are true?
| // Implement workaround according to | ||
| // https://github.com/mui/material-ui/pull/43318#issuecomment-2553509176 | ||
| // Technically applying scrollLock to the container does nothing, but | ||
| // we preserve the original buggy behavior because fixing it would be | ||
| // a breaking change. | ||
| // Original behavior: Apply scroll lock to container if it's set, | ||
| // otherwise apply to the body element. Because disablePortal | ||
| // passes in the correct `containerInfo.container`, the easiest way to | ||
| // make it apply to the correct element is to do this. | ||
| const container = props.container ? containerInfo.container : document.body; |
There was a problem hiding this comment.
Is this related to scroll locking and not aria-hidden? If so, we should move it to a separate PR
There was a problem hiding this comment.
If you do that scroll locking will be broken for disablePortal and you will still need to merge them in together.
This PR changed containerInfo.container so it's resolved correctly when disablePortal is used, making scroll locking ineffective (because basically it was always applied to body - which works - unless container was passed in, where it was also ineffective)
| const resolvedContainer = disablePortal | ||
| ? ((mountNodeRef.current ?? modalRef.current)?.parentElement ?? getDoc().body) | ||
| : getContainer(container) || getDoc().body; |
There was a problem hiding this comment.
If providing the container prop was required when using disablePortal, this could be simplified to
| const resolvedContainer = disablePortal | |
| ? ((mountNodeRef.current ?? modalRef.current)?.parentElement ?? getDoc().body) | |
| : getContainer(container) || getDoc().body; | |
| const resolvedContainer = getContainer(container) || getDoc().body; |
Right?
There was a problem hiding this comment.
But it's not required and it shouldn't be, because if you don't use Portal how could you mount it to a different container?
That is also the reason disablePortal takes priority over container here.
github-actions
Bot
added
the
PR: out-of-date
github-actions
Bot
removed
the
PR: out-of-date
|
We ran into this bug again today. Is there anything I can do to help move the PR along? Thank you. |
github-actions
Bot
added
the
PR: out-of-date
7 participants
Fixes #19450
Lets do #34165 again. Before if someone used
disablePortaland opened a modal the whole page became inaccessible to Screen Readers becausearia-hiddenonly got applied to siblings of document.body, in this case all of them. This PR fixes that.Changes:
disablePortalis used.Known problems:
I'll port this to
nexttoo when I have time.