fix(modal): Use React portals for accessibility fixes #419

NicholasBoll · 2020-01-23T06:15:12Z

Summary

Modal now uses a portal for content to not be constrained by a parent container. This also allows for the hiding of non-modal content (visibly covered by modal overlay) to assistive technology.
Fixes #414
Fixes #330

Checklist

tests are changed or added
yarn test passes
all (dev)dependencies that the module needs is added to its package.json
code has been documented and, if applicable, usage described in README.md
design approved final implementation
a11y approved final implementation
code adheres to the API & Pattern guidelines

Modal now uses a portal for content to not be constrained by a parent container. This also allows for the hiding of non-modal content (visibly covered by modal overlay) to assistive technology. Fixes Workday#414 Fixes Workday#330

NicholasBoll · 2020-01-23T06:16:26Z

cypress/integration/Modal.spec.ts

@@ -6,7 +6,6 @@ function getModalTargetButton() {

 describe('Modal', () => {
  before(() => {
-    cy.viewport(500, 300);


Not actually sure why this is here.

cypress · 2020-01-23T06:25:19Z

Test summary

140 • 0 • 0 • 0

Run details


Project	canvas-kit
Status	Passed
Commit	`e8237ba`
Started	Feb 6, 2020 4:20 AM
Ended	Feb 6, 2020 4:22 AM
Duration	02:37 💡
OS	Linux Ubuntu Linux - 16.04
Browser	Electron 78

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

alexandrzavalii · 2020-02-07T10:05:28Z

modules/modal/react/lib/ModalContent.tsx

-  return (
+  React.useEffect(() => {
+    const siblings = [...((document.body.children as any) as HTMLElement[])].filter(
+      el => el !== modalRef.current!.parentElement


I wonder if we should actually check if current is not undefined?

const observed = useRef(null); useEffect(() => { if(observed.current) console.log(observed.current); }, [observed]);

Interesting... Will useEffect ever be called before the ref is applied? Technically this is an unsupported use-case. If this case happens, we cannot guarantee accessibility of the Modal

modules/modal/react/README.md

anicholls · 2020-02-24T18:14:55Z

modules/modal/react/stories/stories.tsx

+        <a href="#">Link</a>
+      </p>
+
+      <button type="button">Button</button>


Why do we need all this extra content?

Accessibility requested this for testing if content was really being hidden from assistive technology

should i add this back in?

Maybe do that in an accessible story? Maybe behind a knob setting called extraElements that defaults to false? I don't think it is strictly necessary. We don't have a way to test this automatically, but it is nice for manual verification.

anicholls · 2020-03-11T16:28:40Z

@NicholasBoll Should we be adding portals to all of our popups? If it happens for modals, I would also expect it to happen for Popup, Toast, etc.

Approved the wrong PR haha

modules/modal/react/lib/Modal.tsx

cypress · 2020-03-12T21:09:28Z

Test summary

198 • 0 • 0 • 0

Run details


Project	canvas-kit
Status	Passed
Commit	`34dd6bf` ℹ️
Started	Mar 17, 2020 6:37 PM
Ended	Mar 17, 2020 6:40 PM
Duration	02:57 💡
OS	Linux Ubuntu Linux - 16.04
Browser	Electron 78

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

anicholls · 2020-03-12T21:35:50Z

Something worth noting that I just noticed. InputProvider will not work with any portalled components because they are no longer a child of the element that it controls. Maybe we need to consider making InputProvider set its data attributes on the body element.

anicholls

Thanks for the updates @mannycarrera4 Do we want to make the portal changes to other popups in another PR? I wonder if we should just split these into two.

anicholls · 2020-03-13T18:25:18Z

modules/modal/react/lib/Modal.tsx

-const Modal = ({open, ...modalContentProps}: ModalProps): JSX.Element | null =>
-  open ? <ModalContent {...modalContentProps} /> : null;
+const Modal = ({open, ...modalContentProps}: ModalProps): JSX.Element | null => {
+  return open ? <ModalContent {...modalContentProps} /> : null;


Nit: Can we revert this change?

NicholasBoll · 2020-03-16T18:39:28Z

Maybe we need to consider making InputProvider set its data attributes on the body element.

This is an open question for all things we do in Canvas Kit that effect element outside a component's render tree. This current change set aria-hidden="true" on other elements outside the control of the Modal, but that is required to work properly. I made a compromise by allowing the "container" to be set to anything and defaulting to document.body. Hopefully that is a good compromise. Documentation should state this is happening in case it causes issues.

NicholasBoll · 2020-03-16T19:07:10Z

@NicholasBoll Should we be adding portals to all of our popups? If it happens for modals, I would also expect it to happen for Popup, Toast, etc.

I think this should happen for all Popups. I think portalling should be the default unless we have a good reason not to. Portalling can complicate focus management if the portalled content has focusable elements inside. For the most part, focusable elements should be avoided due to complications with accessibility anyways. If a focusable element is not avoidable, have a conversation with accessibility about how to handle this. One compromise was capturing a blur event outside the portalled content and forcing focus back on the target element and closing the portalled popup. That way the next tab key is the native browser focus management.

If browsers came with a focus manager, things would be different: https://discourse.wicg.io/t/proposal-focus-traversal-api/3427

NicholasBoll · 2020-03-16T20:34:55Z

modules/modal/react/stories/stories.tsx

@@ -24,50 +24,7 @@ const DefaultModalExample = () => {
    <>
      <DeleteButton buttonRef={buttonRef} onClick={openModal}>
        Delete Item
-      </Button>
-      <p>
-        <a href="#">Link</a>


Ooo. Removed in a merge commit. Sneaky

NicholasBoll · 2020-03-16T20:43:01Z

modules/modal/react/lib/ModalContent.tsx

@@ -172,7 +173,28 @@ const ModalContent = ({
    }
  };

-  return (
+  React.useEffect(() => {
+    const siblings = [...((document.body.children as any) as HTMLElement[])].filter(


I forgot to make this line and https://github.com/Workday/canvas-kit/pull/419/files#diff-3fe25d4d6af6314872e682fe13b7f7ccR212 use a configurable container instead of just document.body. Applications may need a different container than document.body

anicholls · 2020-03-16T22:51:26Z

I think this should happen for all Popups. I think portalling should be the default unless we have a good reason not to.

@NicholasBoll are we going to look at that in this PR or in a followup? If the latter, we should create an issue for it

NicholasBoll · 2020-03-17T16:49:54Z

I think this should happen for all Popups. I think portalling should be the default unless we have a good reason not to.

@NicholasBoll are we going to look at that in this PR or in a followup? If the latter, we should create an issue for it

I think the latter.

modules/modal/react/lib/ModalContent.tsx

NicholasBoll force-pushed the fix/modal-accessibility-touch branch from d21a395 to 7ebf3f0 Compare January 23, 2020 06:15

NicholasBoll commented Jan 23, 2020

View reviewed changes

test: Add more comprehensive background elements

6ec8881

anicholls added the bug Something isn't working label Jan 25, 2020

NicholasBoll changed the title ~~DNM: fix(modal): Hide background content from assistive technology~~ fix(modal): Hide background content from assistive technology Feb 6, 2020

NicholasBoll marked this pull request as ready for review February 6, 2020 04:00

Merge branch 'master' into fix/modal-accessibility-touch

bafb042

alexandrzavalii reviewed Feb 7, 2020

View reviewed changes

lychyi added the 4.x label Feb 18, 2020

anicholls reviewed Feb 24, 2020

View reviewed changes

anicholls changed the base branch from master to prerelease/v4 February 24, 2020 18:16

anicholls mentioned this pull request Feb 24, 2020

Canvas Kit 4.0 #483

Closed

12 tasks

jpante assigned NicholasBoll Mar 3, 2020

anicholls previously approved these changes Mar 11, 2020

View reviewed changes

jpante assigned mannycarrera4 and unassigned NicholasBoll Mar 12, 2020

fix: Default to use portal always for modals

6e4f137

mannycarrera4 reviewed Mar 12, 2020

View reviewed changes

modules/modal/react/lib/Modal.tsx Outdated Show resolved Hide resolved

Merge branch 'prerelease/v4' into fix/modal-accessibility-touch

b31e7ff

anicholls changed the title ~~fix(modal): Hide background content from assistive technology~~ fix(modal): Use React portals for accessibility fixes Mar 12, 2020

anicholls reviewed Mar 13, 2020

View reviewed changes

test(modal): Update test to use getByRole

033515c

fix(modal): Revert change to return statement

0b5ce67

NicholasBoll commented Mar 16, 2020

View reviewed changes

fix(modal): Add documentation for container prop

57b8d65

mannycarrera4 mentioned this pull request Mar 16, 2020

Default our Popups to use React Portal #517

Closed

NicholasBoll commented Mar 17, 2020

View reviewed changes

modules/modal/react/lib/ModalContent.tsx Outdated Show resolved Hide resolved

fix(modal): Update desc for container and add accessible story

ff3144b

NicholasBoll commented Mar 17, 2020

View reviewed changes

modules/modal/react/lib/ModalContent.tsx Outdated Show resolved Hide resolved

fix(modal): Update where container gets passed

10c16e2

NicholasBoll merged commit 7895f09 into Workday:prerelease/v4 Mar 17, 2020

NicholasBoll deleted the fix/modal-accessibility-touch branch March 17, 2020 19:18

This was referenced Mar 17, 2020

Modal: Component is constrained by overflow content #330

Closed

Modal is not fully accessible #414

Closed

anicholls mentioned this pull request Mar 31, 2020

chore: Release 4.0.0-beta.0 #536

Merged

anicholls added this to the v4.0.0 milestone Mar 31, 2020

anicholls mentioned this pull request Jun 15, 2020

chore: Release v4.0.0 #746

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(modal): Use React portals for accessibility fixes #419

fix(modal): Use React portals for accessibility fixes #419

NicholasBoll commented Jan 23, 2020 •

edited by mannycarrera4

Loading

NicholasBoll Jan 23, 2020

cypress bot commented Jan 23, 2020 •

edited

Loading

alexandrzavalii Feb 7, 2020

NicholasBoll Mar 16, 2020

anicholls Feb 24, 2020

NicholasBoll Mar 16, 2020

mannycarrera4 Mar 16, 2020

NicholasBoll Mar 17, 2020

anicholls commented Mar 11, 2020

cypress bot commented Mar 12, 2020 •

edited

Loading

anicholls commented Mar 12, 2020

anicholls left a comment

anicholls Mar 13, 2020

NicholasBoll commented Mar 16, 2020

NicholasBoll commented Mar 16, 2020

NicholasBoll Mar 16, 2020

NicholasBoll Mar 16, 2020

anicholls commented Mar 16, 2020

NicholasBoll commented Mar 17, 2020

fix(modal): Use React portals for accessibility fixes #419

fix(modal): Use React portals for accessibility fixes #419

Conversation

NicholasBoll commented Jan 23, 2020 • edited by mannycarrera4 Loading

Summary

Checklist

Choose a reason for hiding this comment

cypress bot commented Jan 23, 2020 • edited Loading

Test summary

Run details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anicholls commented Mar 11, 2020

cypress bot commented Mar 12, 2020 • edited Loading

Test summary

Run details

anicholls commented Mar 12, 2020

anicholls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicholasBoll commented Mar 16, 2020

NicholasBoll commented Mar 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anicholls commented Mar 16, 2020

NicholasBoll commented Mar 17, 2020

NicholasBoll commented Jan 23, 2020 •

edited by mannycarrera4

Loading

cypress bot commented Jan 23, 2020 •

edited

Loading

cypress bot commented Mar 12, 2020 •

edited

Loading