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

pauljadam · 2017-03-22T21:07:30Z

Summary:

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

Steps to reproduce:

Open a test modal, https://reactcommunity.org/react-modal/examples/minimal.html.
Activate modal dialog.
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 · 2017-03-22T21:10:36Z

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

diasbruno · 2017-03-22T22:46:38Z

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

pauljadam · 2017-03-22T22:58:09Z

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 · 2017-03-22T22:59:46Z

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

marcysutton · 2017-03-22T23:03:25Z

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:

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.
Send focus to the dialog close button on open.
Send focus back to the triggering button on close.

Following the ARIA practices guide is a great suggestion!

diasbruno · 2017-03-22T23:07:59Z

Thank you so much @pauljadam and @marcysutton.

claydiffrient · 2017-03-23T01:05:40Z

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 2e806c7 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.

pauljadam · 2017-03-23T01:16:16Z

@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.

claydiffrient · 2017-03-23T04:43:12Z

@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.

pauljadam · 2017-03-23T05:07:45Z

@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.

claydiffrient · 2017-03-24T03:10:19Z

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.

sinabahram · 2017-03-24T03:55:29Z

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).

pauljadam · 2017-03-24T04:19:25Z

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 · 2017-03-24T04:40:49Z

@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.

claydiffrient · 2017-03-24T04:55:31Z

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.

pauljadam · 2017-03-24T05:02:50Z

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 :)

claydiffrient · 2017-03-24T05:04:44Z

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.

This makes getAppElement a required prop as well as makes it a function that will be called expecting a DOMElement. closes #287 This also takes some inspiration from #359 for handling arrays of objects. Upgrade Path: - If you had specified an appElement via `Modal.setAppElement`, then you need to convert that to a getAppElement prop on the modal, this should be a function that returns either a single element or an array of elements. - If you had nothing specified you will need to add the getAppElement element to prevent beakages.

sinabahram · 2017-03-24T16:15:52Z

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.

This makes getAppElement a required prop as well as makes it a function that will be called expecting a DOMElement. closes #287 This also takes some inspiration from #359 for handling arrays of objects. Upgrade Path: - If you had specified an appElement via `Modal.setAppElement`, then you need to convert that to a getAppElement prop on the modal, this should be a function that returns either a single element or an array of elements. - If you had nothing specified you will need to add the getAppElement element to prevent beakages.

afercia · 2017-04-13T15:09:41Z

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.

pauljadam · 2017-04-13T15:21:10Z

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.

diasbruno · 2017-11-06T12:51:30Z

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

So, what next to improve react-modal accessibility:

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

marcysutton · 2017-11-20T20:49:16Z

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.

diasbruno · 2017-11-22T12:29:15Z

This week, I'm going to start a new branch to address this accessibility issues. Testing and review would be really appreciated.

sheerun · 2017-11-29T18:09:46Z

@diasbruno aria-hidden is not removed at all, because you check for reference count in beforeClose:

https://github.com/reactjs/react-modal/blob/master/src/components/ModalPortal.js#L133

but you decrement this counter is afterClose:

https://github.com/reactjs/react-modal/blob/master/src/components/ModalPortal.js#L140

diasbruno · 2017-11-29T18:24:29Z

@sheerun It make sense. So we need to move the logic on beforeClose to afterClose. Can you open a PR?

- adds aria-modal="true" to modal portal - doesn't make body a default appElement - warns if appElement is not set in any way

sheerun · 2017-11-29T19:33:02Z

@diasbruno I've sent one. Could you finish it? I don't have any more time today

It adds aria-modal to modal portal and removes adding aria-hidden to body by default. Also fixes issue I mentioned.

diasbruno · 2017-11-29T19:34:35Z

Sure. Thank you!

- adds aria-modal="true" to modal portal - doesn't make body a default appElement - warns if appElement is not set in any way

sheerun · 2017-11-29T19:57:14Z

@pauljadam @marcysutton @afercia If you'd like to test it before @diasbruno releases it, I've published it under following package name on npm: @sheerun/react-modal

- adds aria-modal="true" to modal portal - doesn't make body a default appElement - warns if appElement is not set in any way

- adds aria-modal="true" to modal portal - doesn't make body a default appElement - warns if appElement is not set in any way fixes #359

diasbruno · 2017-11-30T13:36:47Z

Released v3.1.6.

indiesquidge · 2018-07-14T01:02:46Z

Just to be clear, is it unacceptable in all situations to have ReactModal.setAppElement('body')?

I am asking because I'm building a component library for my company, and since many teams will be using it I cannot guess (or constrict) what their React root element is going to be. Even though this component library will mostly be used internally for the foreseeable future, I don't want to make things inaccessible if I can avoid it.

One solution I thought of was just using the appElement prop and requiring implementers of the component library modal to pass in the root element, but that's fairly tedious to do for every modal in an application.

Another solution was to have ReactModal as a named export of the component library so that implementors could have ReactModal.setAppElement(document.getElementById('root')) defined just once in their app, but that's a weird api to make them adhere to.

Basically my question is what is the status quo for setting app element when it is unknown what the app element is going to be?

UPDATE (7/16/2018):

The solution I landed on was actually just exposing the same static property in my custom modal

class MyCustomModal extends Component {
+   static setAppElement(element) {
+       if (element) {
+           ReactModal.setAppElement(element);
+       }
+   }
}

This way implementors of the modal will only have to set the app element once per application, rather than on every element instance (of course the appElement prop is still available as it is in ReactModal since we just forward all props). The best part here being that I avoid have to fall back to using document.body, which (hopefully) avoids the issues pointed out in this issue thread.

If someone sees a major issue with the solution I arrived at for my modal component wrapper, please let me know.

kylemh · 2020-02-02T22:07:32Z

This should be closed, no?

techieshark · 2020-08-12T07:01:32Z

@kylemh ~~no, I don't think this should be closed.~~

Google Lighthouse reports setting aria-hidden=true on the body as an accessibility fault: https://web.dev/aria-hidden-body/

Screen readers and other assistive technologies don't announce content that's marked as hidden. Applying the aria-hidden="true" attribute to your element hides your entire web page from assistive technology users.

kylemh · 2020-08-12T17:23:08Z

Sure, but that would only happen if you incorrectly do: ReactModal.setAppElement('body')

38dc8f9 removed the ability for appElement to be optional meaning if you have aria-hidden=true on the body it's because you've set the app element to be body.

Notice this warning that exists when neither appElement or globalElement is set.

kylemh · 2020-08-12T17:31:09Z

@diasbruno you closed and then reopened this, but announced a release as if the fix was out. I believe you meant for this to be closed because setting the appElement no longer was optional and you hoped people wouldn't set it to be document.body.

I think one way to definitely mark this as closed would be to expand documentation or the dev warning here to ensure that nobody is hiding the body.

techieshark · 2020-08-12T22:47:31Z

@kylemh apologies, I must have been looking at an older version of ariaAppHider.js:

react-modal/src/helpers/ariaAppHider.js

Line 1 in 5641f40

let globalElement = typeof document !== 'undefined' ? document.body : null;

but looking at the the newer code, I see it is fixed:

react-modal/src/helpers/ariaAppHider.js

Line 4 in 5189e5b

let globalElement = null;

I checked a small personal example and did see that aria-hidden is not set on <body> by react-modal, so yeah it seems this issue should be closed.

Perhaps a new issue should be opened to update the docs. For example, the "minimal example" doc links to this codepen which seems to indicate that aria-hidden is true on the <body>... but that example is using an old version of react-modal.

We also have docs/examples/set_app_element.md which says:

You'll notice in this example that the aria-hidden attribute is applied to the #main div rather than the document body.

kylemh · 2020-08-13T00:13:05Z

Thanks for confirming, @techieshark

diasbruno added the discussion label Mar 22, 2017

diasbruno added enhancement and removed discussion labels Mar 22, 2017

diasbruno added this to the 1.7 milestone Mar 22, 2017

diasbruno removed this from the 1.7 milestone Mar 23, 2017

claydiffrient mentioned this issue Mar 24, 2017

[changed] Replace appElement with getAppElement #360

Merged

5 tasks

sheerun added a commit to sheerun/react-modal that referenced this issue Nov 29, 2017

Improve accessibility, fixes reactjs#359

bd5e012

- adds aria-modal="true" to modal portal - doesn't make body a default appElement - warns if appElement is not set in any way

sheerun mentioned this issue Nov 29, 2017

Improve accessibility, fixes #359 #570

Merged

5 tasks

sheerun added a commit to sheerun/react-modal that referenced this issue Nov 29, 2017

[fixes] Improve accessibility, fixes reactjs#359

a6473ca

- adds aria-modal="true" to modal portal - doesn't make body a default appElement - warns if appElement is not set in any way

sheerun added a commit to sheerun/react-modal that referenced this issue Nov 29, 2017

[fixes] Improve accessibility, fixes reactjs#359

ce9ce75

- adds aria-modal="true" to modal portal - doesn't make body a default appElement - warns if appElement is not set in any way

sheerun added a commit to sheerun/react-modal that referenced this issue Nov 29, 2017

[fixes] Improve accessibility, fixes reactjs#359

7c36750

- adds aria-modal="true" to modal portal - doesn't make body a default appElement - warns if appElement is not set in any way

sheerun added a commit to sheerun/react-modal that referenced this issue Nov 30, 2017

[fixes] Improve accessibility, fixes reactjs#359

4879c5b

- adds aria-modal="true" to modal portal - doesn't make body a default appElement - warns if appElement is not set in any way

sheerun added a commit to sheerun/react-modal that referenced this issue Nov 30, 2017

[fixes] Improve accessibility, fixes reactjs#359

968e992

- adds aria-modal="true" to modal portal - doesn't make body a default appElement - warns if appElement is not set in any way

diasbruno closed this as completed in #570 Nov 30, 2017

diasbruno pushed a commit that referenced this issue Nov 30, 2017

[fixes] don't set aria-hidden if appElement is not defined.

38dc8f9

- adds aria-modal="true" to modal portal - doesn't make body a default appElement - warns if appElement is not set in any way fixes #359

diasbruno reopened this Nov 30, 2017

ryelle mentioned this issue Jan 25, 2018

Update react-modal to fix accessibility bug Automattic/wp-calypso#21838

Merged

afercia mentioned this issue Apr 29, 2018

Improvements for aria-modal #654

Closed

chawes13 mentioned this issue Oct 22, 2021

Defaulting to the body element breaks a11y on Firefox LaunchPadLab/lp-components#496

Closed

joaquild03 mentioned this issue Jan 20, 2023

Cannot read properties of undefined Klerith/react-mern-calendar#1

Open

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

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

Comments

pauljadam commented Mar 22, 2017

Summary:

Steps to reproduce:

Expected behavior:

Actual behavior:

pauljadam commented Mar 22, 2017

diasbruno commented Mar 22, 2017 • edited Loading

pauljadam commented Mar 22, 2017

pauljadam commented Mar 22, 2017

marcysutton commented Mar 22, 2017

diasbruno commented Mar 22, 2017

claydiffrient commented Mar 23, 2017

pauljadam commented Mar 23, 2017 • edited Loading

claydiffrient commented Mar 23, 2017

pauljadam commented Mar 23, 2017

claydiffrient commented Mar 24, 2017

sinabahram commented Mar 24, 2017

pauljadam commented Mar 24, 2017

pauljadam commented Mar 24, 2017

claydiffrient commented Mar 24, 2017

pauljadam commented Mar 24, 2017

claydiffrient commented Mar 24, 2017

sinabahram commented Mar 24, 2017 via email

afercia commented Apr 13, 2017

pauljadam commented Apr 13, 2017

diasbruno commented Nov 6, 2017

marcysutton commented Nov 20, 2017

diasbruno commented Nov 22, 2017

sheerun commented Nov 29, 2017

diasbruno commented Nov 29, 2017

sheerun commented Nov 29, 2017

diasbruno commented Nov 29, 2017

sheerun commented Nov 29, 2017

diasbruno commented Nov 30, 2017 • edited Loading

indiesquidge commented Jul 14, 2018 • edited Loading

kylemh commented Feb 2, 2020

techieshark commented Aug 12, 2020 • edited Loading

kylemh commented Aug 12, 2020 • edited Loading

kylemh commented Aug 12, 2020

techieshark commented Aug 12, 2020

kylemh commented Aug 13, 2020

diasbruno commented Mar 22, 2017 •

edited

Loading

pauljadam commented Mar 23, 2017 •

edited

Loading

diasbruno commented Nov 30, 2017 •

edited

Loading

indiesquidge commented Jul 14, 2018 •

edited

Loading

techieshark commented Aug 12, 2020 •

edited

Loading

kylemh commented Aug 12, 2020 •

edited

Loading