Skip to content

[Fixed issue #464] #540

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

Merged
merged 3 commits into from
Oct 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ import ReactModal from 'react-modal';
Note: By disabling the esc key from closing the modal you may introduce an accessibility issue.
*/
shouldCloseOnEsc={true}
/*
Boolean indicating if the modal should restore focus to the element that
had focus prior to its display.
*/
shouldReturnFocusAfterClose={true}
/*
String indicating the role of the modal, allowing the 'dialog' role to be applied if desired.
*/
Expand Down
8 changes: 4 additions & 4 deletions specs/Modal.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ export default () => {
document.activeElement.should.be.eql(mcontent(modal));
});

it("does not focus the modal content when shouldFocusAfterRender is false", () => {
it("does not focus modal content if shouldFocusAfterRender is false", () => {
const modal = renderModal(
{ isOpen: true, shouldFocusAfterRender: false },
null
Expand Down Expand Up @@ -317,7 +317,7 @@ export default () => {
isBodyWithReactModalOpenClass("testBodyClass").should.not.be.ok();
});

it("should not remove classes from document.body when rendering unopened modal", () => {
it("should not remove classes from document.body if modal is closed", () => {
renderModal({ isOpen: true });
isBodyWithReactModalOpenClass().should.be.ok();
renderModal({ isOpen: false, bodyOpenClassName: "testBodyClass" });
Expand All @@ -340,7 +340,7 @@ export default () => {
unmountModal();
});

it("adding/removing aria-hidden without an appElement will try to fallback to document.body", () => {
it("uses document.body for aria-hidden if no appElement", () => {
ariaAppHider.documentNotReadyOrSSRTesting();
const node = document.createElement("div");
ReactDOM.render(<Modal isOpen />, node);
Expand All @@ -349,7 +349,7 @@ export default () => {
should(document.body.getAttribute("aria-hidden")).not.be.ok();
});

it("raise an exception if appElement is a selector and no elements were found.", () => {
it("raises an exception if the appElement selector does not match", () => {
should(() => ariaAppHider.setElement(".test")).throw();
});

Expand Down
2 changes: 1 addition & 1 deletion specs/Modal.style.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export default () => {
mcontent(modal).style.top.should.be.eql("");
});

it("overrides the default styles when a custom overlayClassName is used", () => {
it("overrides the default styles when using custom overlayClassName", () => {
const modal = renderModal({
isOpen: true,
overlayClassName: "myOverlayClass"
Expand Down
4 changes: 3 additions & 1 deletion src/components/Modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@ export default class Modal extends Component {
onRequestClose: PropTypes.func,
closeTimeoutMS: PropTypes.number,
ariaHideApp: PropTypes.bool,
shouldFocusAfter: PropTypes.bool,
shouldFocusAfterRender: PropTypes.bool,
shouldCloseOnOverlayClick: PropTypes.bool,
shouldReturnFocusAfterClose: PropTypes.bool,
parentSelector: PropTypes.func,
aria: PropTypes.object,
role: PropTypes.string,
Expand All @@ -71,6 +72,7 @@ export default class Modal extends Component {
shouldFocusAfterRender: true,
shouldCloseOnEsc: true,
shouldCloseOnOverlayClick: true,
shouldReturnFocusAfterClose: true,
parentSelector() {
return document.body;
}
Expand Down
27 changes: 23 additions & 4 deletions src/components/ModalPortal.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export default class ModalPortal extends Component {
closeTimeoutMS: PropTypes.number,
shouldFocusAfterRender: PropTypes.bool,
shouldCloseOnOverlayClick: PropTypes.bool,
shouldReturnFocusAfterClose: PropTypes.bool,
role: PropTypes.string,
contentLabel: PropTypes.string,
aria: PropTypes.object,
Expand Down Expand Up @@ -137,8 +138,23 @@ export default class ModalPortal extends Component {
afterClose = () => {
// Remove body class
bodyClassList.remove(this.props.bodyOpenClassName);
focusManager.returnFocus();
focusManager.teardownScopedFocus();

if (this.shouldReturnFocus()) {
focusManager.returnFocus();
focusManager.teardownScopedFocus();
}
};

shouldReturnFocus = () => {
// Don't restore focus to the element that had focus prior to
// the modal's display if:
// 1. Focus was never shifted to the modal in the first place
// (shouldFocusAfterRender = false)
// 2. Explicit direction to not restore focus
return (
this.props.shouldFocusAfterRender ||
this.props.shouldReturnFocusAfterClose
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be && not ||. Currently I have to pass both shouldFocusAfterRender={false} and shouldReturnFocusAfterClose={false} to opt out of refocusing.

Copy link
Collaborator

@diasbruno diasbruno Nov 6, 2017

Choose a reason for hiding this comment

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

Looking at this now, if shouldReturnFocusAfterClose=false, it would also disable the 'focus after render'. They must be independent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

NOTES:

If shouldFocusAfterRender=true and shouldReturnFocusAfterClose=false, we put the element in the focus manager and on close we just pop it out.

shouldFocusAfterRender=false and shouldReturnFocusAfterClose=true must be noop just like shouldFocusAfterRender=false and shouldReturnFocusAfterClose=false.

shouldFocusAfterRender=true and shouldReturnFocusAfterClose=true is the default behavior.

Does this make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, sounds reasonable

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, what are the usecases for shouldFocusAfterRender? Looks like it just focuses a div without any keyboard handlers

Copy link
Collaborator

@diasbruno diasbruno Nov 6, 2017

Choose a reason for hiding this comment

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

To set the scope for tabbing, so it won't leave the modal when it's open.

);
};

open = () => {
Expand All @@ -147,8 +163,11 @@ export default class ModalPortal extends Component {
clearTimeout(this.closeTimer);
this.setState({ beforeClose: false });
} else {
focusManager.setupScopedFocus(this.node);
focusManager.markForFocusLater();
if (this.shouldReturnFocus()) {
focusManager.setupScopedFocus(this.node);
focusManager.markForFocusLater();
}

this.setState({ isOpen: true }, () => {
this.setState({ afterOpen: true });

Expand Down