Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[material-ui][Backdrop] Add aria-hidden by default #34691

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

chapmanm3
Copy link

@chapmanm3 chapmanm3 commented Oct 9, 2022

This PR aims to solve Issue 33146 In which having the aria-hidden value on all Backdrops is no longer indented and instead it should only exist on the Backdrop that is behind a Modal. This is achieved by passing the aria-hidden: true value to the Modal's custom Backdrop component.

Fixes #33146

@mui-bot
Copy link

mui-bot commented Oct 9, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 1ab855a

@chapmanm3 chapmanm3 changed the title Backdrop aria hidden default 33146 [a11y] Backdrop aria-hidden default 33146 Oct 9, 2022
@michaldudak michaldudak added the component: backdrop This is the name of the generic UI component, not the React module! label Oct 12, 2022
@@ -37,6 +37,7 @@ const ModalRoot = styled('div', {
const ModalBackdrop = styled(Backdrop, {
name: 'MuiModal',
slot: 'Backdrop',
'aria-hidden': true,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'aria-hidden': true,

You need to add this where ModalBackdrop is used, what you are doing here doesn't make sense as the API is not supported.

Copy link
Author

Choose a reason for hiding this comment

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

This should be updated to use the new suggested format now

Copy link
Author

Choose a reason for hiding this comment

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

@chapmanm3 chapmanm3 requested review from mnajdova and removed request for michaldudak, siriwatknp and hbjORbj October 22, 2022 00:43
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 3, 2022
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Changes look good, but should we consider them breaking changes and hold off till v6? cc @michaldudak what do you think?

@mnajdova mnajdova added on hold There is a blocker, we need to wait breaking change labels Dec 13, 2022
@michaldudak
Copy link
Member

I suppose so. If an application relies on a Backdrop being excluded from the a11y tree, it may become much harder or impossible to navigate with screen readers after this change.
I'll add it to the v6 milestone.

@michaldudak michaldudak added this to the v6 milestone Dec 14, 2022
@mnajdova mnajdova changed the title [a11y] Backdrop aria-hidden default 33146 [a11y] Backdrop add aria-hidden by default Dec 14, 2022
@danilo-leal danilo-leal removed this from the Material UI: v7 draft milestone Dec 22, 2023
@danilo-leal danilo-leal added the package: material-ui Specific to @mui/material label Dec 22, 2023
@danilo-leal danilo-leal changed the title [a11y] Backdrop add aria-hidden by default [material-ui][Backdrop] Add aria-hidden by default Dec 22, 2023
@ZeeshanTamboli
Copy link
Member

@DiegoAndai, should we consider including this fix in v6?

@DiegoAndai
Copy link
Member

DiegoAndai commented Apr 30, 2024

Thanks for the callout @ZeeshanTamboli!

I wonder if we should do it, considering :

  • that the possible breaking change might affect navigation on the entire app: [material-ui][Backdrop] Add aria-hidden by default #34691 (comment)
  • that there's a workaround by providing aria-hidden={false} to the Backdrop component
  • that on the major after v6, we plan to build the components on top of Base UI, so I wouldn't want to release back-to-back Backdrop accessibility breaking changes.

What do you think?

@colmtuite @michaldudak, is there a plan to add a Backdrop component to Base UI alongside the Dialog? If so, would/should the Backdrop component have aria-hidden=true by default?

@colmtuite
Copy link
Contributor

colmtuite commented Apr 30, 2024

@DiegoAndai Our Dialog API design doc was just started this week and is still in early draft mode, so I can't be sure. Currently, there's no <Dialog.Backdrop /> component suggested in the doc.

However, I'm almost certain we will include a <Dialog.Backdrop /> component in the new Dialog, yes. Whether or not it has aria-hidden depends on whether or not <Dialog.Popup /> is inside the Backdrop or outside it. I'm less sure about that right now, but I would guess it will be rendered outside, with aria-hidden.

@ZeeshanTamboli
Copy link
Member

that on the major after v6, we plan to build the components on top of Base UI, so I wouldn't want to release back-to-back Backdrop accessibility breaking changes.

What do you think?

I agree it would be better to take this into account when building it with the new Base UI (depending on what Base UI does).

@DiegoAndai DiegoAndai added this to the Material UI with Base UI milestone May 2, 2024
@DiegoAndai
Copy link
Member

DiegoAndai commented May 2, 2024

I added this to the Material UI with Base UI milestone to keep coordination between both. We can keep it on hold, and revisit it when we do that refactor. I added a comment with the workaround to the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y breaking change component: backdrop This is the name of the generic UI component, not the React module! on hold There is a blocker, we need to wait package: material-ui Specific to @mui/material PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui][Backdrop] aria-hidden is set by default
8 participants