Skip to content

Commit

Permalink
[fixes] don't set aria-hidden if appElement is not defined.
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
sheerun authored and diasbruno committed Nov 30, 2017
1 parent 700eb4e commit 38dc8f9
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 43 deletions.
3 changes: 0 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,6 @@ to prevent assistive technologies such as screenreaders
from reading content outside of the content of
your modal.

It's optional and if not specified it will try to use
`document.body` as your app element.

If you are doing server-side rendering, you should use
this property.

Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@
},
"dependencies": {
"exenv": "^1.2.0",
"prop-types": "^15.5.10"
"prop-types": "^15.5.10",
"warning": "^3.0.0"
},
"peerDependencies": {
"react": "^0.14.0 || ^15.0.0 || ^16",
Expand Down
9 changes: 0 additions & 9 deletions specs/Modal.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -339,15 +339,6 @@ export default () => {
unmountModal();
});

it("uses document.body for aria-hidden if no appElement", () => {
ariaAppHider.documentNotReadyOrSSRTesting();
const node = document.createElement("div");
ReactDOM.render(<Modal isOpen />, node);
document.body.getAttribute("aria-hidden").should.be.eql("true");
ReactDOM.unmountComponentAtNode(node);
should(document.body.getAttribute("aria-hidden")).not.be.ok();
});

it("raises an exception if the appElement selector does not match", () => {
should(() => ariaAppHider.setElement(".test")).throw();
});
Expand Down
17 changes: 7 additions & 10 deletions src/components/ModalPortal.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,7 @@ export default class ModalPortal extends Component {
}

componentWillUnmount() {
// Remove body class
bodyClassList.remove(this.props.bodyOpenClassName);
this.beforeClose();
this.afterClose();
clearTimeout(this.closeTimer);
}

Expand All @@ -127,17 +125,16 @@ export default class ModalPortal extends Component {
}
}

beforeClose() {
afterClose = () => {
const { appElement, ariaHideApp } = this.props;

// Remove body class
bodyClassList.remove(this.props.bodyOpenClassName);

// Reset aria-hidden attribute if all modals have been removed
if (ariaHideApp && refCount.totalCount() < 1) {
ariaAppHider.show(appElement);
}
}

afterClose = () => {
// Remove body class
bodyClassList.remove(this.props.bodyOpenClassName);

if (this.props.shouldFocusAfterRender) {
if (this.props.shouldReturnFocusAfterClose) {
Expand Down Expand Up @@ -171,7 +168,6 @@ export default class ModalPortal extends Component {
};

close = () => {
this.beforeClose();
if (this.props.closeTimeoutMS > 0) {
this.closeWithTimeout();
} else {
Expand Down Expand Up @@ -309,6 +305,7 @@ export default class ModalPortal extends Component {
onClick={this.handleOverlayOnClick}
onMouseDown={this.handleOverlayOnMouseDown}
onMouseUp={this.handleOverlayOnMouseUp}
aria-modal="true"
>
<div
ref={this.setContentRef}
Expand Down
41 changes: 21 additions & 20 deletions src/helpers/ariaAppHider.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import warning from "warning";

let globalElement = null;

export function assertNodeList(nodeList, selector) {
Expand All @@ -19,42 +21,41 @@ export function setElement(element) {
return globalElement;
}

export function tryForceFallback() {
if (document && document.body) {
// force fallback to document.body
setElement(document.body);
return true;
}
return false;
}

export function validateElement(appElement) {
if (!appElement && !globalElement && !tryForceFallback()) {
throw new Error(
if (!appElement && !globalElement) {
warning(
false,
[
"react-modal: Cannot fallback to `document.body`, because it is not",
"ready or available. If you are doing server-side rendering, use this",
"function to defined an element. `Modal.setAppElement(el)` to make",
"this accessible"
"react-modal: App element is not defined.",
"Please use `Modal.setAppElement(el)` or set `appElement={el}`.",
"This is needed so screen reades don't see main content",

This comment has been minimized.

Copy link
@nerfologist

nerfologist Dec 4, 2017

I think it should read 'screen readers' (not 'screen reades') here.

"when modal is opened. It is not recommended, but you can opt-out",
"by setting `ariaHideApp={false}`."
].join(" ")
);

return false;
}

return true;
}

export function hide(appElement) {
validateElement(appElement);
(appElement || globalElement).setAttribute("aria-hidden", "true");
if (validateElement(appElement)) {
(appElement || globalElement).setAttribute("aria-hidden", "true");
}
}

export function show(appElement) {
validateElement(appElement);
(appElement || globalElement).removeAttribute("aria-hidden");
if (validateElement(appElement)) {
(appElement || globalElement).removeAttribute("aria-hidden");
}
}

export function documentNotReadyOrSSRTesting() {
globalElement = null;
}

export function resetForTesting() {
globalElement = document.body;
globalElement = null;
}

2 comments on commit 38dc8f9

@dbkingsb
Copy link

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great! Thanks, @dbkingsb, for validating this!

Please sign in to comment.