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

Add <Dialog> element #1416

Merged
merged 16 commits into from
Jan 30, 2019
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
1 change: 1 addition & 0 deletions .storybook/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { configure } from '@storybook/react';

function loadStories() {
require('../stories/button.jsx');
require('../stories/dialog.jsx');
require('../stories/header.jsx');
}

Expand Down
1 change: 1 addition & 0 deletions .storybook/preview-head.html
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<script src="https://polyfill.io/v3/polyfill.min.js?flags=gated&features=default"></script>
115 changes: 115 additions & 0 deletions assets/components/dialog/dialog.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
// @flow

// ----- Imports ----- //

import React, { Component, type Node } from 'react';

import { type Option } from 'helpers/types/option';
import { classNameWithModifiers } from 'helpers/utilities';

import './dialog.scss';


// ----- Props ----- //

export type PropTypes = {|
onStatusChange: (boolean) => void,
modal: boolean,
open: boolean,
'aria-label': Option<string>,
dismissOnBackgroundClick: boolean,
children: Node
|};


// ----- Component ----- //

class Dialog extends Component<PropTypes> {

static defaultProps = {
onStatusChange: () => {},
modal: true,
open: false,
dismissOnBackgroundClick: false,
}

componentDidMount() {
if (this.props.open) {
this.open();
}
}

componentDidUpdate(prevProps: PropTypes) {
if (prevProps.open === true && this.props.open === false) {
this.close();
} else if (prevProps.open === false && this.props.open === true) {
this.open();
}
}

open() {
if (this.ref && this.ref.showModal) {
if (this.props.modal) {
this.ref.showModal();
} else {
this.ref.show();
}
}
requestAnimationFrame(() => {
if (this.ref) {
this.ref.focus();
}
});
}

close() {
if (this.ref && this.ref.close) {
this.ref.close();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens to the focus after the modal is dismissed? Ideally it would return back to the button that opened it, or perhaps returned to the beginning of the page

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alas it depends on the implementer. the dialog does fire an event when it closes, so the component that fired it should be able to return the focus. In my testing it would seem that chrome makes a decent job at doing this on its own.

Something we could do could be to tweak the API so there's a nullable but required returnFocusAfterClose() prop to make it explicit that if you implement a dialog you have to take care of that? But I want to test default browser behaviour a bit more in case that just adds extra work

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, sounds like it would work!

As an aside, it's worth testing the following combos of browsers and screenreaders, as these are the most commonly used:

  • IE11 + JAWS
  • Firefox + NVDA
  • Firefox + JAWS
  • Safari + Voiceover

}

ref: ?(HTMLDialogElement & {focus: Function});

render() {
const {
open, modal, children, onStatusChange, dismissOnBackgroundClick, ...otherProps
} = this.props;

return (
<dialog // eslint-disable-line jsx-a11y/no-redundant-roles
className={classNameWithModifiers('component-dialog', [modal ? 'modal' : null, open ? 'open' : null])}
aria-modal={modal}
aria-hidden={!open}
Copy link
Contributor

@SiAdcock SiAdcock Jan 31, 2019

Choose a reason for hiding this comment

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

Should we also include the open attribute?

EDIT: I have just seen that the open attribute is provided by the wrapper. Is there a reason it is defined in the wrapper and not here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heya! it gets opened using the js callbacks in the lifecycle hooks a bit higher up the component. You can't actually use both. if you have open="true" and try to fire up dialog.show() that's a javascript warning, makes for a lovely imperative vs declarative debate!

(Also, open="true" doesn't support modals at all 😝. I think the usage scenario is for serving markup from a server. Even the MDN demos don't set it via js)

As all <dialog> elements support dialog.show() I chose to omit the open attr altogether and instead style the polyfill with traditional css

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. And so confusing! I'm sure it's contrary to what is outlined in the spec. Browsers are a law unto themselves 🙄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nono this is in the spec, see this:

screenshot 2019-01-31 at 10 29 50 am

tabIndex="-1"
Copy link
Contributor

Choose a reason for hiding this comment

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

The WHATWG spec states that

The tabindex attribute must not be specified on dialog elements.

Not sure why though. Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, since browser support for dialog is fiddly at best I have decided to implement focus management cheaply by just focusing the whole dialog after it opens – this seems to net sensible results in all tested browsers.

The native behaviour would be to focus on the first interactive element – however that requires more DOM fiddling, i'm not against exploring that solution but I'm not confident it's as portable as focusing the whole thing :(

I found healthy debate here about this all, my takeaways are:

  • The guidance refers more about a numberical tabindex than the magical -1 and 0 (because dialogs live outside of the element to a level)
  • Some user agents make dialog focusable? (thinking about using tabindex=0
  • Focusing the dialog itself vs focusing the first interactive element is an open discussion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks for sharing the discussion, really interesting!

The guidance we received from the accessibility consultant:

  • if there's an obvious candidate to receive focus in the dialog, use that, OTHERWISE
  • provide focusable empty divs at the top and bottom of the dialog and use JavaScript to return focus to the top div when tabbing out of the bottom div (and vice versa).

I can link you the notes he provided if you need it. But there's more than one way to peel a potato, and as long as the approach is tested, it's all good! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we do have a naively implemented oneline focus trap at the end but not one at the top 😢 I'm not sure how to implement both sides without a lot of code. it's something i want to look into though!

role="dialog"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this is needed for browsers that don't support the <dialog> element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes :(

onOpen={() => { onStatusChange(true); }}
onCancel={() => { onStatusChange(false); }}
ref={(d) => { this.ref = (d: any); }}
{...otherProps}
>
<div className="component-dialog__contents">
{children}
<div
tabIndex="0" // eslint-disable-line jsx-a11y/no-noninteractive-tabindex
onFocus={() => {
/* this acts as a cheap focus trap */
if (this.ref) { this.ref.focus(); }
}}
/>
</div>
{modal &&
<div
className="component-dialog__backdrop"
aria-hidden
onClick={() => dismissOnBackgroundClick && onStatusChange(false)}
/>
}
</dialog>
);
}
}


// ----- Exports ----- //

export default Dialog;
43 changes: 43 additions & 0 deletions assets/components/dialog/dialog.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
.component-dialog {
all: initial;
position: absolute;
visibility: hidden;
height: 0;
width: 0;
overflow: hidden;
}

.component-dialog--open {
visibility: visible;
position: fixed;
height: auto;
width: auto;
top: 0;
left: 0;
z-index: 100;
contain: content;
}

.component-dialog--modal.component-dialog--open {
display: flex;
bottom: 0;
right: 0;
justify-content: center;
align-items: center;
contain: strict;
}

.component-dialog__contents {
position: relative;
z-index: 10;
}

.component-dialog--modal .component-dialog__backdrop {
background: rgba(0, 0, 0, 0.6);
position: absolute;
top: 0;
left: 0;
bottom: 0;
right: 0;
z-index: 9;
}
71 changes: 71 additions & 0 deletions stories/dialog.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
// @flow

import React, { Component } from 'react';

import { storiesOf } from '@storybook/react';

import Dialog from 'components/dialog/dialog';
import Button from 'components/button/button';
import ProductPageTextBlock from 'components/productPage/productPageTextBlock/productPageTextBlock';
import { withCenterAlignment } from '../.storybook/decorators/withCenterAlignment';

// This is a barebones stateful wrapper - <Dialog/> needs to be controlled just like inputs
class ControlledDialogButton extends Component<{modal: boolean}, {open: boolean}> {
state = {
open: false,
}
render() {
return (
<div>
<Button
aria-haspopup="dialog"
aria-label={null}
appearance="greyHollow"
onClick={() => { this.setState({ open: true }); }}
>Open it up
</Button>
<Dialog
aria-label="Modal dialog"
modal={this.props.modal}
onStatusChange={(status) => { this.setState({ open: status }); }}
open={this.state.open}
>
<div style={{ padding: '1em', background: '#121212', color: '#fff' }}>
<ProductPageTextBlock title={'I\'m a dialog!'}>
I don&#39;t do much on my own :(
</ProductPageTextBlock>
<Button
icon={null}
aria-label={null}
appearance="primary"
onClick={() => { this.setState({ open: false }); }}
>Close
</Button>
</div>
</Dialog>
</div>
);
}
}

const stories = storiesOf('Dialogs', module)
.addDecorator(withCenterAlignment);

stories.add('Modal dialog', () => (
<ControlledDialogButton modal />
));

stories.add('Non-modal dialog', () => (
<div>
<ProductPageTextBlock title="This is a non-modal dialog example">
<p>It opens up but lets you interact
with the page under it while it is open
</p>
<p>You probably do not actually want this,
as this dialog does not have the click outside behaviour
and makes for a confusing experience for screen readers
</p>
<ControlledDialogButton modal={false} />
</ProductPageTextBlock>
</div>
));
Copy link
Contributor

@SiAdcock SiAdcock Jan 31, 2019

Choose a reason for hiding this comment

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

Why make this an option available through props? Non-modals are fiddly and require more complex focus management. Can you think of a use case for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good shout! I was thinking of stuff like the account menu but the more i think about it even that should be "modal" (you'd click anywhere on the background to close it but the page below doesnt receive that click – like context menus on mac)

We actually discourage against its use in the stories, I think it's wise to remove it until a usage scenario presents itself