Skip to content
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

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

Merged
49 changes: 48 additions & 1 deletion cypress/integration/Modal.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ function getModalTargetButton() {

describe('Modal', () => {
before(() => {
cy.viewport(500, 300);
Copy link
Member Author

Choose a reason for hiding this comment

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

Not actually sure why this is here.

h.stories.visit();
});

Expand Down Expand Up @@ -39,6 +38,30 @@ describe('Modal', () => {
h.modal.get().should('be.visible');
});

it('should place the portal as a child of the body element', () => {
cy.get('body').then($body => {
h.modal
.get()
.parent() // wrapper
.parent()
.should($el => {
expect($el[0]).to.equal($body[0]);
});
});
});

it('should hide non-modal content from assistive technology', () => {
h.modal
.get()
.parent()
.siblings()
.should($siblings => {
$siblings.each((_, $sibling) => {
expect($sibling).to.have.attr('aria-hidden', 'true');
});
});
});

it('should not have any axe errors', () => {
cy.checkA11y();
});
Expand Down Expand Up @@ -161,6 +184,30 @@ describe('Modal', () => {
h.modal.get().should('be.visible');
});

it('should place the portal as a child of the body element', () => {
cy.get('body').then($body => {
h.modal
.get()
.parent() // wrapper
.parent()
.should($el => {
expect($el[0]).to.equal($body[0]);
});
});
});

it('should hide non-modal content from assistive technology', () => {
h.modal
.get()
.parent()
.siblings()
.should($siblings => {
$siblings.each((_, $sibling) => {
expect($sibling).to.have.attr('aria-hidden', 'true');
});
});
});

it('should not have any axe errors', () => {
cy.checkA11y();
});
Expand Down
5 changes: 3 additions & 2 deletions modules/modal/react/lib/Modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ export interface ModalProps extends ModalContentProps {
open: boolean;
}

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can we revert this change?

};

Modal.Padding = PopupPadding;
Modal.Width = ModalWidth;
Expand Down
26 changes: 25 additions & 1 deletion modules/modal/react/lib/ModalContent.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as React from 'react';
import ReactDOM from 'react-dom';
import styled from '@emotion/styled';
import {keyframes} from '@emotion/core';
import tabTrappingKey from 'focus-trap-js';
Expand Down Expand Up @@ -172,7 +173,28 @@ const ModalContent = ({
}
};

return (
React.useEffect(() => {
const siblings = [...((document.body.children as any) as HTMLElement[])].filter(
Copy link
Member Author

Choose a reason for hiding this comment

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

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

el => el !== modalRef.current!.parentElement
Copy link
Contributor

Choose a reason for hiding this comment

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

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]);

Copy link
Member Author

Choose a reason for hiding this comment

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

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

);
const prevAriaHidden = siblings.map(el => el.getAttribute('aria-hidden'));
siblings.forEach(el => {
el.setAttribute('aria-hidden', 'true');
});

return () => {
siblings.forEach((el, index) => {
const prev = prevAriaHidden[index];
if (prev) {
el.setAttribute('aria-hidden', prev);
} else {
el.removeAttribute('aria-hidden');
}
});
};
}, []);

const content = (
<Container onClick={handleOutsideClick} {...elemProps}>
<Popup
popupRef={modalRef}
Expand All @@ -186,6 +208,8 @@ const ModalContent = ({
</Popup>
</Container>
);

return ReactDOM.createPortal(content, document.body);
};

export default ModalContent;