react-modal icon indicating copy to clipboard operation
react-modal copied to clipboard

aria-hidden=true on body breaks Accessibility <body class="ReactModal__Body--open" aria-hidden="true">

Open pauljadam opened this issue 7 years ago • 44 comments

Summary:

Screen reader users cannot interact with any modal content because aria-hidden=true is incorrectly placed on the body tag.

Steps to reproduce:

  1. Open a test modal, https://reactcommunity.org/react-modal/examples/minimal.html.
  2. Activate modal dialog.
  3. Check that aria-hidden=true is incorrectly placed on the body tag of the modal's html document.

Expected behavior:

Screen reader user is trapped inside the modal dialog.

Actual behavior:

Screen reader user can't interact with anything on the page at all because of aria-hidden=true on the body.

pauljadam avatar Mar 22 '17 21:03 pauljadam

There's a new aria-modal=true attribute that should be placed on the modal dialog which could remove the need for aria-hidden.

pauljadam avatar Mar 22 '17 21:03 pauljadam

@pauljadam can you elaborate and give some advises about accessibility? Just to keep it registered here. :)

diasbruno avatar Mar 22 '17 22:03 diasbruno

http://w3c.github.io/aria/aria/aria.html#aria-hidden

aria-hidden (state)

Indicates whether the element is exposed to an accessibility API.

If the body is hidden,

then the whole body tag is no longer exposed to an Accessibility API. You only want to hide the main content container(plus header/footer) of the page not the entire tag.

pauljadam avatar Mar 22 '17 22:03 pauljadam

http://w3c.github.io/aria-practices/#dialog_modal

This demo uses aria-modal=true rather than aria-hidden. http://w3c.github.io/aria-practices/examples/dialog-modal/dialog.html

pauljadam avatar Mar 22 '17 22:03 pauljadam

I can confirm this behavior. It also looks like focus management isn't happening on the modal, and it's missing the dialog role (though those could be separate issues). Here's how to fix it:

  1. When the modal opens, put aria-hidden="true" on sibling element(s) to the dialog instead of the body, and change it back to false when it closes.
  2. Send focus to the dialog close button on open.
  3. Send focus back to the triggering button on close.

Following the ARIA practices guide is a great suggestion!

marcysutton avatar Mar 22 '17 23:03 marcysutton

Thank you so much @pauljadam and @marcysutton.

diasbruno avatar Mar 22 '17 23:03 diasbruno

Just wanna weigh in here for a moment. There is a lot of confusion around this behavior in the modal. I've been working to improve documentation and make things more clear in our 2.0 release which is overdue unfortunately. I can assure you that when used properly the modal is accessible. We've used it extensively in Canvas.

@pauljadam The example you linked to is the minimal example. It is the minimal needed to make the modal show up and function. react-modal uses the concept of the appElement to determine how it hides things from screen readers. The modal is appended to the body element (by default). We then hide the element that was provided by Modal.setAppElement(el). At some point in time that was made to default to the body (a behavior I hate vehemently because it will 100% break screenreaders). In order to use the modal properly users must set the app element otherwise the modal has no idea what to hide. For example:

 <body>
   <div id="application">
       ....
   </div>
</body>

In this case, we'd want to set the appElement to div#application such that it would get aria-hidden applied to it. And our final markup would look similar to:

 <body>
   <div id="application" aria-hidden="true">
       ....
   </div>
   <div class="ReactModalPortal">
       ...
   </div>
</body>

That would keep screen readers inside the modal.

In version 2.0 of the modal appElement will be a required prop called getAppElement which should be a function that returns the DOM element that should be aria-hidden.

Thanks also for pointing out aria-modal. It appears that that is an attribute that can be added to a role="dialog" to force them to be modal in nature. I'd not seen that before. It's interesting.

@marcysutton Thanks for adding your input here. It's super valuable! When Ryan initially created the modal the state of role="dialog" was a bit interesting. See his writeup about why not role="dialog" here. We added role="dialog" to the modal back in 2e806c724e8c0bbd6bc7dc49a8a2e4ba592e9ca6 and it was released as part of version 1.5.0. It was however removed and made optional because of the concerns listed in #237. So in short, if people want the dialog role added, they are more than welcome to add it themselves.

I'm not sure why we default to setting the focus to the body. I'm sure @ryanflorence might recall why it was done. That being said, we do provide an onAfterOpen prop that can be used to set focus to the close button. We also provide an onRequestClose prop that can be used to set focus back to the calling element. This is going to change to an afterClose prop in 2.0 for additional clarity. There are enough cases I've seen where the element used to open the modal doesn't exist when the modal gets closed for me to feel comfortable with always returning to what triggered the modal.

Sorry all, this was a bit long winded. Issues like this are very tough for me. They open my eyes more to the need to have better documentation around different assumptions the modal makes.

@diasbruno - I don't think there's much we can do about this on our 1.x branch. I think things will be much better and clearer in 2.0.

claydiffrient avatar Mar 23 '17 01:03 claydiffrient

@claydiffrient what about when you have a header and footer that you need to hide also? Can you do setAppElement on multiple html elements? e.g. http://codepen.io/anon/pen/jBxwZr

aria-modal=true looks like it would eliminate the need to manage aria-hidden

So far all the client sites I've tested out in the wild are not using setAppElement and it just hides the body so kills the whole page when the modal appears.

pauljadam avatar Mar 23 '17 01:03 pauljadam

@pauljadam Currently, it doesn't in all the use cases I've needed it for, there as always been a root-level non-body application container. What we can do about that though is make it so getAppElement in version 2.0 will handle receiving an array of elements to hide.

aria-modal does look really promising, but that also requires that the modal have a role of dialog which has concerns with screen readers that I mentioned in my lat comment.

I'm sad to hear that about the client sites. I want to fix that so badly, but it now ends up being a breaking change which necessitates a major version bump. We've done a poor job of documenting the need for that, something I hope to rectify.

claydiffrient avatar Mar 23 '17 04:03 claydiffrient

@claydiffrient I would still recommend role=dialog for normal dialogs and role=alertdialog for error type dialogs. This is what the aria spec and practices recommends.

I read the post from 2014 about the worries of role=dialog, that issue has no affect on macOS VoiceOver screen reader or mobile screen readers VoiceOver iOS or Android TalkBack. Only JAWS and NVDA include the automatic switching to application mode vs browse mode and they can switch back to browse mode if they're in application mode, I cover that issue in this blog post under the NVDA Forms Mode Behavior & Insert + Spacebar Key heading http://www.deque.com/blog/aria-modal-alert-dialogs-a11y-support-series-part-2/

In a normal role=”dialog” or “alertdialog” NVDA will go into Forms Mode when focus is sent into the dialog. Forms Mode means NVDA users an only tab around the dialog. They can’t press the up/down arrow keys to read through the dialog with linear navigation.

To exit forms mode NVDA users can press Insert + Spacebar keys and then enter Browse Mode where they can use the arrow keys and quick navigation keys to read through the dialog rather than just the TAB key.

Most accessibility folks are confused by this behavior and wonder if screen reader users understand how the read the dialog or if the dialog code is broken because it’s not working properly with NVDA. This is similar to the problems with using role=”application”. Forms Mode/Browse mode issues don’t affect other screen readers like VoiceOver and TalkBack. I’ve heard that JAWS isn’t affected by the role=”alertdialog” Forms Mode issue like NVDA.

pauljadam avatar Mar 23 '17 05:03 pauljadam

So arguably it's a better experience for SR users to not deal with switching modes. I worked at one point with a blind engineer who suggested doing it the way we currently have it implemented.

I guess it's all a matter of which is better to default to. Currently we default to no role and allow consumers to add a role. We could in theory default to having the role=dialog and then allow overrides from users to take if off if they felt things would be better the other way.

I personally like the idea of not having to worry about switching modes for any users. Sure it's not exactly in line with the ARIA Authoring Practices, but... as you said in your article, "Sometimes it makes sense to bend the rules of the ARIA Authoring Practices." I think this is perhaps one of those times.

claydiffrient avatar Mar 24 '17 03:03 claydiffrient

Please note that this example with aria-modal: http://w3c.github.io/aria-practices/examples/dialog-modal/dialog.html

does not work for users of IE11. While that might be tempting to justify, IE11 is a browser used quite a bit by blind users, especially given that Edge is nowhere near ready for production for these users.

As a result, I would strongly recommend making sure everything that is not modal receive an aria-hidden="true".

The above example does work with Jaws 18 and the latest Firefox (I did not test with Chrome nor with NVDA and other browsers, though I could if desired).

sinabahram avatar Mar 24 '17 03:03 sinabahram

I would just say that I think it's very important to use role=dialog and role=alertdialog to convey the accessibility semantics of the element. It's also the only way to give the dialog an accessible name and description via aria-labelledby and aria-describedby. This is something I regularly treat as an accessibility failure for client websites if it's not present. The 2 roles speak different semantics to a screen reader user, e.g. alertdialog says "Alert" on some screen readers when you enter the dialog.

pauljadam avatar Mar 24 '17 04:03 pauljadam

@sinabahram I was able to get the example working in IE 11 Win 10 using NVDA latest, it appears to trap the modal dialog with NVDA whereas with Narrator you can still navigate to the grayed out content so maybe NVDA likes aria-modal="true" and Narrator still not supporting it. Narrator also has no issues with forms mode or browse mode.

pauljadam avatar Mar 24 '17 04:03 pauljadam

I can see that, but given the two roles which do we use? The modal is meant to be general purpose so it could potentially be used with either option. So do we continue with what we have with role being optional and up to the client to implement? I think perhaps that might be the best course of action. Consumers will be able to specify if they want role=dialog or role=alertdialog. In absence of a role prop being passed in we could add a warning and give reference to possible a11y issues without a defined role.

claydiffrient avatar Mar 24 '17 04:03 claydiffrient

If they're setting the role they should also set the accessible name either via aria-labelledby pointing to the h1 in the dialog or an aria-label if there is no visible title for the dialog. Accessible description via aria-describedby should also be set if possible. If you're adding the accessible name, role, and description, should also do the ARIA 1.1 aria-modal=true attribute.

The biggest problem is still that aria-hidden=true is ending up on the body tag in all of the examples of this modal dialog I've seen in the wild during Accessibility testing. I figured it had to be in a widely used framework after seeing on different sites so I googled "ReactModal__Body--open" and ended up here :) I thought maybe I could fix it at the source rather than telling all my clients individually that aria-hidden has broken their entire application :)

pauljadam avatar Mar 24 '17 05:03 pauljadam

I can totally see that. I'll see what I can do about the role and other aria attributes. In version 2.0 of the modal body will not be hidden, that I can for sure promise.

claydiffrient avatar Mar 24 '17 05:03 claydiffrient

Testing was done with Jaws and IE11, a very popular combination. For nvDA, of course one uses Firefox and Chrome instead of IE given the myriad of performance issues and far better support on Firefox and Chrome.

Jaws18+IE11 does not hide the virtual content outside of the modal.

sinabahram avatar Mar 24 '17 16:03 sinabahram

Worth nothing aria-modal is part of the ARIA 1.1 spec which is still a Candidate Recommendation: https://www.w3.org/TR/wai-aria-1.1/#aria-modal that means support is not guaranteed at all, so maybe it's an option for the future. 🙂 Personally, I don't know anything about browsers/Assistive Technologies support for aria-modal, if anyone has data to share that would be super useful.

About the role=dialog debate, that really depends on the content of the modal. I'd suggest to have a look at this post from Mr. Marco Zehe, who's a developer, Mozillian, and a blind person: https://www.marcozehe.de/2015/02/05/advanced-aria-tip-2-accessible-modal-dialogs/

Basically, when using role=dialog then any text within the modal should be associated to some focusable element with aria-describedby, otherwise it will simply be ignored by screen reader in forms/focus mode. This would be very hard or maybe impossible to do programmatically, since we can't predict the nature of the modal content. VoiceOver (and TalkBack?) is an exception to this de-facto standard behaviour.

Think at a modal with content made exclusively by some long text or images without any interactive elements. It could be a warning, or some long inline help, whatever. Using role=dialog by default would make that content completely ignored by screen readers. A resounding silence. Yes, users could manually switch back to browse mode, but that's not a strong argument. I even doubt users would have any clue they have to switch back to browse mode.

Also about alertdialog opinions may differ 🙂 Personally, I'd second Mr. Zehe when he says:

Don’t use it. Seriously, I’ve been working with ARIA for over seven years now [the article is from 2015 --Ed.], and I still haven’t figured out why role “alertdialog” was ever invented.

afercia avatar Apr 13 '17 15:04 afercia

aria-modal=true works in VoiceOver on iOS and macOS. It worked in NVDA also. Have you tested the ARIA authoring practices demo?

aria-hidden=true on the body is still the major problem here.

pauljadam avatar Apr 13 '17 15:04 pauljadam

Interesting. Yep, also according to what Mr. Bryan Garaventa says here http://www.ssbbartgroup.com/blog/differences-aria-1-0-1-1-deprecations-additions/ it's only supported on iOS using VoiceOver iOS 9.X [data from January 6th, 2017].

I would wait for ARIA 1.1 to become an approved Recommendation. Then, I'd agree modal dialogs that use role=dialog should also use aria-modal=true to ensure forward compatibility.

Have you tested the ARIA authoring practices demo?

Time (the lack of) is my biggest problem 🙂

afercia avatar Apr 13 '17 15:04 afercia

I really want to record an accessibility demo with react-modal, but aria-hidden="true" on the body is preventing me from doing that. It will also prevent screen reader users from accessing webpages with react-modal open. Do you have an ETA on when a fix for this will ship?

marcysutton avatar Apr 24 '17 00:04 marcysutton

@marcysutton Sorry for the delay on getting this out. I was hoping that v2 would ship sooner, but I haven't been able to get it out the door, but it's much closer. I'm thinking we may pare down a few things and ship with just the biggest things initially. I wish I could give you a more concrete idea of when it might happen, but I can't really offer much more than two weeks or so perhaps.

claydiffrient avatar Apr 24 '17 04:04 claydiffrient

Another alternative you could do is use the Modal.setAppElement() method to declare a different appElement to apply aria-hidden="true" to. This will prevent it from being applied to the body.

claydiffrient avatar Apr 24 '17 04:04 claydiffrient

Ooh, that should work for now. Thanks!

marcysutton avatar Apr 24 '17 15:04 marcysutton

Really great insights everyone, thank you so much.

For anyone else like me stumbling across the issue....

RE allowing the modal to be read out by screenreaders/voiceover, whilst the body has aria-hidden="true", with the currently latest version (2.2.4) and react version 15.4.2:

Nothing suggested was working for me, I was probably doing something wrong. I tried Modal.setAppElement(), role="dialog", aria-modal, all in various places.

I am now declaring a false ariaHideApp prop, which solves it, with a side effect.

<Modal
  isOpen
  onAfterOpen={this.handleOnAfterOpen}
  ariaHideApp={false}>
    {children}
</Modal>

ariaHideApp is true by default - therefore aria-hidden will be applied to the appElement (default appElement is the body).

Without setting the appElement to something other than the default body, and by also setting ariaHideApp to false, when a modal is open, all content in or outside of the modal could be read out. For my use case this is the desired behaviour. We may manually set some aria-hidden attributes to a couple of elements that we don't want read out in certain instances.

Is there a reason why ariaHideApp isn't documented?

Looking forward to the next version so this can be cleaner, great work! 👍

ttbarnes avatar Aug 31 '17 14:08 ttbarnes

Sigh....I just wasted a couple days before finding this. I was totally confused about how to use the app element - and now I get this a bit more, but Marcy Sutton is right that there should just be a way to apply it to all sibling elements.

If react-modal didn't assume there should only be 1 "app element", you could probably get the correct behavior by using querySelectorAll and body > :not(.modal-class) or something similar. That's not super simple, but it'd work OK.

IMO, the current default is 100% wrong - the default experience is guaranteed to be inaccessible. If you provide no app element, the screen reader experience isn't great, but it's better than nothing being being accessible.

cycomachead avatar Sep 07 '17 01:09 cycomachead

Hello,

forgive my possible ignorance, however this seems more than just an enhancement to me. A lot of pages, e. g. twitch, some local service pages, are completely inaccessible because of inaccessible pop overs resulting from aria-hidden being set to true. This includes login forms, etc. No screen reader can read the pages and frustrating workarounds have to be used to guesstimate what you're supposed to be doing. Since the webpages in question use react I thought this might be the right place to comment about this. Imagine if you click a pop over the entire screen goes blank. Not exactly pretty.

Is there a chance anything could be done? I know that most devs will take a while to update their libraries but maybe this will lessen the blow in the future. I feel like this is more important than people make it out to be, especially seeing this issue has been open since march. I could be wrong, I'm new to react development in general so I'm speaking mostly from a user's standpoint. I just know that a lot of websites are incredibly frustrating to use because of probably this little oversight.

Ghorthalon avatar Nov 06 '17 09:11 Ghorthalon

Hi @Ghorthalon, maybe it was missing to do something about it.

So, what next to improve react-modal accessibility:

  • [x] allow define role attribute.
  • [x] allow to pass custom aria-* attributes.
  • [ ] improve aria-hide-app mechanism.

diasbruno avatar Nov 06 '17 12:11 diasbruno

This issue is still open, was there ever a resolution? It's a pretty big show-stopper, so it would be good to get the docs updated or whatever else needs to happen.

marcysutton avatar Nov 20 '17 20:11 marcysutton