Skip to content

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

Closed
chapmanm3 wants to merge 7 commits intomui:v5.xfrom
chapmanm3:BackdropAriaHiddenDefault_33146
Closed

[material-ui][Backdrop] Add aria-hidden by default#34691
chapmanm3 wants to merge 7 commits intomui:v5.xfrom
chapmanm3:BackdropAriaHiddenDefault_33146

Conversation

@chapmanm3
Copy link
Copy Markdown

@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
Copy Markdown

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 scope: backdrop This is the name of the generic UI component, not the React module! label Oct 12, 2022
const ModalBackdrop = styled(Backdrop, {
name: 'MuiModal',
slot: 'Backdrop',
'aria-hidden': true,
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.

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
Copy Markdown
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
Copy Markdown
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 hbjORbj, michaldudak and siriwatknp 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
Copy Markdown
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 Introduces changes that are not backward compatible. labels Dec 13, 2022
@michaldudak
Copy link
Copy Markdown
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 changed the title [a11y] Backdrop add aria-hidden by default [material-ui][Backdrop] Add aria-hidden by default Dec 22, 2023
@ZeeshanTamboli
Copy link
Copy Markdown
Member

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

@DiegoAndai
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@silviuaavram
Copy link
Copy Markdown
Member

Closing it for #47798

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

Labels

accessibility a11y breaking change Introduces changes that are not backward compatible. on hold There is a blocker, we need to wait. PR: out-of-date The pull request has merge conflicts and can't be merged. scope: backdrop This is the name of the generic UI component, not the React module!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

10 participants