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

Remove role="document" from the dialog and add aria-modal="true" #30687

Closed
DavidMacDonald opened this issue Apr 29, 2020 · 5 comments · Fixed by #30755
Closed

Remove role="document" from the dialog and add aria-modal="true" #30687

DavidMacDonald opened this issue Apr 29, 2020 · 5 comments · Fixed by #30755

Comments

@DavidMacDonald
Copy link

DavidMacDonald commented Apr 29, 2020

Current Code

<div class="modal" tabindex="-1" role="dialog">
<div class="modal-dialog" role="document">

Solution code

<div class="modal" tabindex="-1" role="dialog" aria-modal="true">
 <div class="modal-dialog" >

Rational

Document role is only necessary if there role="application" on a parent that overrides screen reader short cut keys.
The role="dialog" doesn't do that, so it's fine without it ... see aria-practices example
https://w3c.github.io/aria-practices/examples/dialog-modal/dialog.html

aria-modal="true" ensure the screen reader user cannot arrow out of the modal inadvertently and replaces the old technique (not used in bootstrap) of aria-hidden on all background content.

@XhmikosR
Copy link
Member

/CC @patrickhlauke

@patrickhlauke
Copy link
Member

role="document" was necessary at the time to work around a bug in, if I recall correctly, NVDA which assumed that a role="dialog" only contained focusable elements and no content, and went directly into forms mode. this has likely been resolved since.

not sure about how widespread support for aria-modal is, but we can add that, sure...no harm either way.

@rohit2sharma95
Copy link
Collaborator

@patrickhlauke So now, role="document" should be removed from .modal-dialog and aria-modal="true" should be added to .modal?

May I do it if the above is right?

@patrickhlauke
Copy link
Member

@rohit2sharma95 yes. sure, go for it

@patrickhlauke
Copy link
Member

oh, as was pointed out separately in #30755 discussion, our script already dynamically adds aria-modal="true" when you trigger an actual modal. so this is really just about removing the redundant role="document"

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

Successfully merging a pull request may close this issue.

4 participants