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

Removed role="document" from the modal dialog #30755

Merged
merged 1 commit into from May 11, 2020

Conversation

rohit2sharma95
Copy link
Collaborator

Closes #30687

Updated the document for the modal component.

@XhmikosR
Copy link
Member

XhmikosR commented May 7, 2020

Not sure if this applies to v4 too @patrickhlauke

@ffoodd
Copy link
Member

ffoodd commented May 7, 2020

I'm not sure aria-modal should be shown in the examples markup: our scripts adds it when showing modal and removes it when hiding it. @patrickhlauke @Johann-S ?

@rohit2sharma95
Copy link
Collaborator Author

I'm not sure aria-modal should be shown in the examples markup: our scripts adds it when showing modal and removes it when hiding it.

@ffoodd is right though. @patrickhlauke and @Johann-S?

@patrickhlauke
Copy link
Member

indeed, i forgot our scripts already handle aria-modal="true". so this really is just about removing the redundant role="document".

and yes should be done for v4 as well.

Copy link
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

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

leave out all the additions of aria-modal="true". just keep the removal of role="document" from the tests, code examples, and the explanatory paragraph of text

@rohit2sharma95
Copy link
Collaborator Author

Okay @patrickhlauke! Let me do it.

@rohit2sharma95 rohit2sharma95 changed the title Removed role="document" from the modal dialog and added aria-modal="true" to the modal Removed role="document" from the modal dialog May 10, 2020
@XhmikosR XhmikosR added this to Inbox in v5 via automation May 11, 2020
@XhmikosR XhmikosR added this to Inbox in v4.5.0 via automation May 11, 2020
@XhmikosR
Copy link
Member

@patrickhlauke please check again so that I backport this and include it in the upcoming v4.4.2 🙂

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

LGTM

v5 automation moved this from Inbox to Approved May 11, 2020
@patrickhlauke
Copy link
Member

@XhmikosR looks good to me. possibly in future we could even consider injecting the role="dialog" via scripting (the same way aria-modal="true" is being injected. one less thing for authors to worry about.

@XhmikosR
Copy link
Member

Sounds good to me! Just make an issue about it so that we don't forget 🙂

@XhmikosR XhmikosR merged commit ec3cfae into twbs:master May 11, 2020
v5 automation moved this from Approved to Shipped May 11, 2020
@XhmikosR XhmikosR moved this from Inbox to Cherry-picked in v4.5.0 May 11, 2020
@rohit2sharma95 rohit2sharma95 deleted the modal-accessibility branch May 11, 2020 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v4.5.0
  
Shipped
v5
  
Shipped
Development

Successfully merging this pull request may close these issues.

Remove role="document" from the dialog and add aria-modal="true"
4 participants