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

michaelkro · 2018-12-04T21:57:30Z

BREAKING CHANGE: The reactRoot prop is required

Fixes #561

What: Adds keyboard and screen reader focus trapping

Notes/Resources:

react accessible modal (github.com/reactjs)
a11y dialog
The consumer must pass the react root HTMLElement as a prop.
When the modal is opened, aria-hidden=true is applied to the react root element, hiding it from screen readers.
When the modal is closed, the aria-hidden attribute is removed from the react root element

coveralls · 2018-12-04T22:10:04Z

Pull Request Test Coverage Report for Build 3518

2 of 4 (50.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.03%) to 82.202%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/patternfly-4/react-core/src/components/Modal/Modal.js	2	4	50.0%

Totals
Change from base Build 3512:	-0.03%
Covered Lines:	3983
Relevant Lines:	4584

💛 - Coveralls

patternfly-build · 2018-12-04T23:11:41Z

PatternFly-React preview: https://1011-pr-patternfly-react-patternfly.surge.sh

michaelkro · 2018-12-05T15:24:20Z

Having trouble writing tests that check that the aria-hidden attribute is applied/removed from the React root.

Using mount, it seems that componentDidUpdate fires at some unknown time. Taking a snapshot of document.body, I see something like the following

// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`the React root is hidden from screen readers when the modal is open 1`] = `
<body
  class="pf-c-backdrop__open"
>
  <div />
  <div />
  <div />
  <div />
  <div />
  <div />
</body>
`;

On a side note, I'm not sure why there are so many divs here

Notice that the pf-c-backdrop__open class is being applied. I can comment out the line that applies that style, and when I rerun the tests (with the mounted Modal being open), the snapshot does not error, i.e., the class is still somehow being applied.

Trying to see if componentDidUpdate is actually triggered, I added the following

  const view = mount(<Modal {...props} isOpen />);
  const spy = jest.spyOn(document.body.classList, 'add');

  view.update();

  expect(spy).toHaveBeenCalled();

which results in failure. I tried clearing the jest cache, and sadly that didn't help.

I was hoping to do the something like

test('the React root is hidden from screen readers when the modal is open', () => {
  document.body.appendChild(props.reactRoot);
  const view = mount(<Modal {...props} isOpen />);
  const spy = jest.spyOn(view.prop('reactRoot'), 'setAttribute');

  view.update();

  expect(spy).toHaveBeenCalledWith(stuff);
});

Any ideas/suggestions here?

jgiardino · 2018-12-06T23:42:06Z

Thanks for working on this! Everything looks good wrt screen readers. I tested in JAWS, NVDA, and VO, and all trap focus, and contents outside the modal are not accessible while the modal is visible.

jgiardino

Works as expected!

BREAKING CHANGE: The reactRoot prop is required Fixes patternfly#561

jschuler · 2018-12-11T18:01:27Z

packages/patternfly-4/react-core/src/components/Modal/Modal.js

-  isLarge: PropTypes.bool
+  isLarge: PropTypes.bool,
+  /** React application root element */
+  reactRoot: PropTypes.instanceOf(safeHTMLElement).isRequired


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

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

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

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:

https://github.com/edenspiekermann/a11y-dialog

https://github.com/gdkraus/accessible-modal-dialog

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

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.

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

tlabaj

LGTM

michaelkro added PF4 A11y labels Dec 4, 2018

michaelkro requested review from dmiller9911, jgiardino, jschuler, seanforyou23 and tlabaj December 4, 2018 21:57

michaelkro force-pushed the 561-modal-keyboard-interaction branch from 3dab5e5 to 9551ff0 Compare December 4, 2018 22:53

jgiardino mentioned this pull request Dec 6, 2018

Dropdown interaction issues #1014

Merged

jgiardino previously approved these changes Dec 6, 2018

View reviewed changes

jgiardino mentioned this pull request Dec 7, 2018

VoiceOver and modal/popover issues patternfly/patternfly#925

Closed

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

c0d834a

BREAKING CHANGE: The reactRoot prop is required Fixes patternfly#561

michaelkro dismissed jgiardino’s stale review via 304a00d December 10, 2018 11:49

michaelkro force-pushed the 561-modal-keyboard-interaction branch from 9551ff0 to 304a00d Compare December 10, 2018 11:49

Clicking outside of modal closes it

764d051

michaelkro force-pushed the 561-modal-keyboard-interaction branch from 304a00d to 764d051 Compare December 10, 2018 11:56

jgiardino mentioned this pull request Dec 11, 2018

feat(core): Popover positioning #1032

Merged

jschuler reviewed Dec 11, 2018

View reviewed changes

jschuler approved these changes Dec 11, 2018

View reviewed changes

tlabaj approved these changes Dec 12, 2018

View reviewed changes

tlabaj merged commit 7fa93b3 into patternfly:master Dec 12, 2018

michaelkro deleted the 561-modal-keyboard-interaction branch December 12, 2018 12:26

jgiardino mentioned this pull request Dec 12, 2018

Review methods for trapping focus #1037

Closed

jgiardino mentioned this pull request Dec 19, 2018

Modal aria attribute updates patternfly/patternfly#1139

Closed

tlabaj mentioned this pull request Jan 3, 2019

Update Basic Dropdown example to demonstrate focus management #839

Closed

jgiardino mentioned this pull request Mar 8, 2019

feat(ContextSelector): Add PF4 ContextSelector Component #1526

Merged

This was referenced Jun 28, 2019

AboutModal should trap focus #1389

Closed

Modal should trap screen reader focus #2405

Closed

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

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

Uh oh!

Conversation

michaelkro commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 3518

💛 - Coveralls

Uh oh!

patternfly-build commented Dec 4, 2018

Uh oh!

michaelkro commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgiardino commented Dec 6, 2018

Uh oh!

jgiardino left a comment

Choose a reason for hiding this comment

Uh oh!

jschuler Dec 11, 2018

Choose a reason for hiding this comment

Uh oh!

dmiller9911 Dec 11, 2018

Choose a reason for hiding this comment

Uh oh!

dmiller9911 Dec 11, 2018

Choose a reason for hiding this comment

Uh oh!

jgiardino Dec 11, 2018

Choose a reason for hiding this comment

Uh oh!

dmiller9911 Dec 12, 2018

Choose a reason for hiding this comment

Uh oh!

jgiardino Dec 12, 2018

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

michaelkro commented Dec 4, 2018 •

edited

Loading

coveralls commented Dec 4, 2018 •

edited

Loading

michaelkro commented Dec 5, 2018 •

edited

Loading