Skip to content

feat(PF4-Modal): Adds keyboard and screen reader focus trapping #1011

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.

Sign up for GitHub

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
tlabaj merged 2 commits into from
Dec 12, 2018
Merged

feat(PF4-Modal): Adds keyboard and screen reader focus trapping #1011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c0d834a
feat(PF4-Modal): Adds keyboard and screen reader focus trapping
michaelkro Dec 4, 2018
764d051
Clicking outside of modal closes it
michaelkro Dec 10, 2018
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
11 changes: 9 additions & 2 deletions packages/patternfly-4/react-core/src/components/Modal/Modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from 'react';
import ReactDOM from 'react-dom';
import PropTypes from 'prop-types';
import ModalContent from './ModalContent';
import safeHTMLElement from '../../internal/safeHTMLElement';
import { canUseDOM } from 'exenv';
import { KEY_CODES } from '../../internal/constants';
import { css } from '@patternfly/react-styles';
Expand All @@ -23,7 +24,9 @@ const propTypes = {
/** A callback for when the close button is clicked */
onClose: PropTypes.func,
/** Creates a large version of the Modal */
isLarge: PropTypes.bool
isLarge: PropTypes.bool,
/** React application root element */
reactRoot: PropTypes.instanceOf(safeHTMLElement).isRequired
Copy link
Collaborator

@jschuler jschuler Dec 11, 2018

Choose a reason for hiding this comment

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

could we default to document.body and make it optional?

Copy link
Contributor

@dmiller9911 dmiller9911 Dec 11, 2018

Choose a reason for hiding this comment

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

I agree this seems pretty invasive for consumers. I haven't seen this before in others. It looks like the reasoning is for setting aria-hidden? It looks like that has been deprecated/replaced by aria-modal:
https://www.w3.org/TR/wai-aria-practices/#dialog_roles_states_props

Copy link
Contributor

@dmiller9911 dmiller9911 Dec 11, 2018

Choose a reason for hiding this comment

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

Maybe checking out atlaskits? It seems their focus trap works well, but I can't speak to screen readers: https://atlaskit.atlassian.com/packages/core/modal-dialog

Copy link
Contributor

@jgiardino jgiardino Dec 11, 2018

Choose a reason for hiding this comment

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

I had tested several implementations before to see what worked across the different screen reader / browser combinations we test in. The attributes role="dialog" and aria-modal don't seem to be supported as described by wai-aria. When I test their solution using JAWS / Chrome, focus is not trapped. It also is not trapped using focus-trap-react.

Solutions that did work were:

I did not test atlaskit, but will do that and let you know what the results were.

Copy link
Contributor

@dmiller9911 dmiller9911 Dec 12, 2018

Choose a reason for hiding this comment

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

Ah, bummer maybe aria-modal is not widely supported yet.

Here is another implementation to check: https://ui.reach.tech/dialog

It does not require a root, but sets aria-hidden on root elements.

Code here: https://github.com/reach/reach-ui/blob/master/packages/dialog/src/index.js#L7

Looks like it is just using document.querySelectorAll("body > *") and hides everything but the dialog root.

Copy link
Contributor

@jgiardino jgiardino Dec 12, 2018

Choose a reason for hiding this comment

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

I captured part of this discussion in a separate issue: #1037

};

const defaultProps = {
Expand Down Expand Up @@ -62,8 +65,10 @@ class Modal extends React.Component {
componentDidUpdate() {
if (this.props.isOpen) {
document.body.classList.add(css(styles.backdropOpen));
this.props.reactRoot.setAttribute('aria-hidden', true);
} else {
document.body.classList.remove(css(styles.backdropOpen));
this.props.reactRoot.removeAttribute('aria-hidden');
}
}

Expand All @@ -73,6 +78,8 @@ class Modal extends React.Component {
}

render() {
const { reactRoot, ...props } = this.props;

if (!canUseDOM) {
return null;
}
Expand All @@ -81,7 +88,7 @@ class Modal extends React.Component {
this.container = document.createElement('div');
}

return ReactDOM.createPortal(<ModalContent {...this.props} id={this.id} />, this.container);
return ReactDOM.createPortal(<ModalContent {...props} id={this.id} />, this.container);
}
}

Expand Down
3 changes: 2 additions & 1 deletion packages/patternfly-4/react-core/src/components/Modal/Modal.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ const props = {
title: 'Modal',
onClose: jest.fn(),
isOpen: false,
children: 'modal content'
children: 'modal content',
reactRoot: document.createElement('div')
};

test('Modal creates a container element once for div', () => {
Expand Down
19 changes: 11 additions & 8 deletions packages/patternfly-4/react-core/src/components/Modal/ModalContent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React from 'react';
import PropTypes from 'prop-types';
import FocusTrap from 'focus-trap-react';
import ModalBoxBody from './ModalBoxBody';
import ModalBoxHeader from './ModalBoxHeader';
import ModalBoxHCloseButton from './ModalBoxCloseButton';
Expand Down Expand Up @@ -47,14 +48,16 @@ const ModalContent = ({ children, className, isOpen, title, hideTitle, actions,
return (
<Backdrop>
<Bullseye>
<ModalBox className={className} isLarge={isLarge} title={title} id={id}>
<ModalBoxHCloseButton onClose={onClose} />
{modalBoxHeader}
<ModalBoxBody {...props} id={id}>
{children}
</ModalBoxBody>
{modalBoxFooter}
</ModalBox>
<FocusTrap focusTrapOptions={{ clickOutsideDeactivates: true }}>
<ModalBox className={className} isLarge={isLarge} title={title} id={id}>
<ModalBoxHCloseButton onClose={onClose} />
{modalBoxHeader}
<ModalBoxBody {...props} id={id}>
{children}
</ModalBoxBody>
{modalBoxFooter}
</ModalBox>
</FocusTrap>
</Bullseye>
</Backdrop>
);
Expand Down
64 changes: 38 additions & 26 deletions ...ages/patternfly-4/react-core/src/components/Modal/__snapshots__/ModalContent.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,36 +8,48 @@ exports[`Modal Content Test isOpen 1`] = `
className=""
component="div"
>
<ModalBox
className=""
id="id"
isLarge={false}
title="Test Modal Content title"
<FocusTrap
_createFocusTrap={[Function]}
active={true}
focusTrapOptions={
Object {
"clickOutsideDeactivates": true,
}
}
paused={false}
tag="div"
>
<ModalBoxCloseButton
className=""
onClose={[Function]}
/>
<ModalBoxHeader
className=""
>

Test Modal Content title

</ModalBoxHeader>
<ModalBoxBody
<ModalBox
className=""
id="id"
isLarge={false}
title="Test Modal Content title"
>
This is a ModalBox header
</ModalBoxBody>
<ModalBoxFooter
className=""
>


</ModalBoxFooter>
</ModalBox>
<ModalBoxCloseButton
className=""
onClose={[Function]}
/>
<ModalBoxHeader
className=""
>

Test Modal Content title

</ModalBoxHeader>
<ModalBoxBody
className=""
id="id"
>
This is a ModalBox header
</ModalBoxBody>
<ModalBoxFooter
className=""
>


</ModalBoxFooter>
</ModalBox>
</FocusTrap>
</Bullseye>
</Backdrop>
`;
Expand Down
1 change: 1 addition & 0 deletions packages/patternfly-4/react-core/src/components/Modal/examples/LargeModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class LargeModal extends React.Component {
Confirm
</Button>
]}
reactRoot={document.querySelector('#___gatsby')}
>
Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore
magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo
Expand Down
1 change: 1 addition & 0 deletions packages/patternfly-4/react-core/src/components/Modal/examples/SimpleModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class SimpleModal extends React.Component {
Confirm
</Button>
]}
reactRoot={document.querySelector('#___gatsby')}
>
Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore
magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo
Expand Down
4 changes: 4 additions & 0 deletions packages/patternfly-4/react-core/src/internal/safeHTMLElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// https://github.com/reactjs/react-modal/blob/master/src/helpers/safeHTMLElement.js
import { canUseDOM } from 'exenv';

export default (canUseDOM ? window.HTMLElement : {});