Skip to content

Commit 38dc8f9

Browse files
sheerundiasbruno
authored andcommittedNov 30, 2017
[fixes] don't set aria-hidden if appElement is not defined.
- 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
1 parent 700eb4e commit 38dc8f9

File tree

5 files changed

+30
-43
lines changed

5 files changed

+30
-43
lines changed
 

‎README.md

-3
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,6 @@ to prevent assistive technologies such as screenreaders
5757
from reading content outside of the content of
5858
your modal.
5959

60-
It's optional and if not specified it will try to use
61-
`document.body` as your app element.
62-
6360
If you are doing server-side rendering, you should use
6461
this property.
6562

‎package.json

+2-1
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,8 @@
6262
},
6363
"dependencies": {
6464
"exenv": "^1.2.0",
65-
"prop-types": "^15.5.10"
65+
"prop-types": "^15.5.10",
66+
"warning": "^3.0.0"
6667
},
6768
"peerDependencies": {
6869
"react": "^0.14.0 || ^15.0.0 || ^16",

‎specs/Modal.spec.js

-9
Original file line numberDiff line numberDiff line change
@@ -339,15 +339,6 @@ export default () => {
339339
unmountModal();
340340
});
341341

342-
it("uses document.body for aria-hidden if no appElement", () => {
343-
ariaAppHider.documentNotReadyOrSSRTesting();
344-
const node = document.createElement("div");
345-
ReactDOM.render(<Modal isOpen />, node);
346-
document.body.getAttribute("aria-hidden").should.be.eql("true");
347-
ReactDOM.unmountComponentAtNode(node);
348-
should(document.body.getAttribute("aria-hidden")).not.be.ok();
349-
});
350-
351342
it("raises an exception if the appElement selector does not match", () => {
352343
should(() => ariaAppHider.setElement(".test")).throw();
353344
});

‎src/components/ModalPortal.js

+7-10
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,7 @@ export default class ModalPortal extends Component {
9999
}
100100

101101
componentWillUnmount() {
102-
// Remove body class
103-
bodyClassList.remove(this.props.bodyOpenClassName);
104-
this.beforeClose();
102+
this.afterClose();
105103
clearTimeout(this.closeTimer);
106104
}
107105

@@ -127,17 +125,16 @@ export default class ModalPortal extends Component {
127125
}
128126
}
129127

130-
beforeClose() {
128+
afterClose = () => {
131129
const { appElement, ariaHideApp } = this.props;
130+
131+
// Remove body class
132+
bodyClassList.remove(this.props.bodyOpenClassName);
133+
132134
// Reset aria-hidden attribute if all modals have been removed
133135
if (ariaHideApp && refCount.totalCount() < 1) {
134136
ariaAppHider.show(appElement);
135137
}
136-
}
137-
138-
afterClose = () => {
139-
// Remove body class
140-
bodyClassList.remove(this.props.bodyOpenClassName);
141138

142139
if (this.props.shouldFocusAfterRender) {
143140
if (this.props.shouldReturnFocusAfterClose) {
@@ -171,7 +168,6 @@ export default class ModalPortal extends Component {
171168
};
172169

173170
close = () => {
174-
this.beforeClose();
175171
if (this.props.closeTimeoutMS > 0) {
176172
this.closeWithTimeout();
177173
} else {
@@ -309,6 +305,7 @@ export default class ModalPortal extends Component {
309305
onClick={this.handleOverlayOnClick}
310306
onMouseDown={this.handleOverlayOnMouseDown}
311307
onMouseUp={this.handleOverlayOnMouseUp}
308+
aria-modal="true"
312309
>
313310
<div
314311
ref={this.setContentRef}

‎src/helpers/ariaAppHider.js

+21-20
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import warning from "warning";
2+
13
let globalElement = null;
24

35
export function assertNodeList(nodeList, selector) {
@@ -19,42 +21,41 @@ export function setElement(element) {
1921
return globalElement;
2022
}
2123

22-
export function tryForceFallback() {
23-
if (document && document.body) {
24-
// force fallback to document.body
25-
setElement(document.body);
26-
return true;
27-
}
28-
return false;
29-
}
30-
3124
export function validateElement(appElement) {
32-
if (!appElement && !globalElement && !tryForceFallback()) {
33-
throw new Error(
25+
if (!appElement && !globalElement) {
26+
warning(
27+
false,
3428
[
35-
"react-modal: Cannot fallback to `document.body`, because it is not",
36-
"ready or available. If you are doing server-side rendering, use this",
37-
"function to defined an element. `Modal.setAppElement(el)` to make",
38-
"this accessible"
29+
"react-modal: App element is not defined.",
30+
"Please use `Modal.setAppElement(el)` or set `appElement={el}`.",
31+
"This is needed so screen reades don't see main content",
Has a comment. Original line has a comment.
32+
"when modal is opened. It is not recommended, but you can opt-out",
33+
"by setting `ariaHideApp={false}`."
3934
].join(" ")
4035
);
36+
37+
return false;
4138
}
39+
40+
return true;
4241
}
4342

4443
export function hide(appElement) {
45-
validateElement(appElement);
46-
(appElement || globalElement).setAttribute("aria-hidden", "true");
44+
if (validateElement(appElement)) {
45+
(appElement || globalElement).setAttribute("aria-hidden", "true");
46+
}
4747
}
4848

4949
export function show(appElement) {
50-
validateElement(appElement);
51-
(appElement || globalElement).removeAttribute("aria-hidden");
50+
if (validateElement(appElement)) {
51+
(appElement || globalElement).removeAttribute("aria-hidden");
52+
}
5253
}
5354

5455
export function documentNotReadyOrSSRTesting() {
5556
globalElement = null;
5657
}
5758

5859
export function resetForTesting() {
59-
globalElement = document.body;
60+
globalElement = null;
6061
}

2 commit comments

Comments
 (2)

dbkingsb commented on Dec 1, 2017

@dbkingsb

This fixes a screen reader issue. Before this commit, the "aria-hidden" would remain on the app element "after" the close of the modal, because the refCount.totalCount() would always be at least 1 (due to the logic actually being executed before the modal closed). Going from "beforeClose" to "afterClose" fixed it.

diasbruno commented on Dec 1, 2017

@diasbruno
Collaborator

Great! Thanks, @dbkingsb, for validating this!

Please sign in to comment.