Add <Dialog> element #1416

walaura · 2019-01-30T12:34:33Z

Why are you doing this?

We need an accessible (as possible) way to display dialogs that exit the page flow, float over it, and can be dismissed. This component wraps a native <dialog> element (which should take care of a11y for us) and polyfills it as much as possible.

There's some barebones styling but the idea is that every dialog client would implement its own styling.

Tested it with voiceover in Chrome, Safari & Firefox, as well as with my own keyboard. A known issue is that the focus trap only works going down, this is for simplicity reasons in this PR, implementing a better focus trap can be done without altering the public interface.

Trello Card

Screenshots

rupertbates · 2019-01-30T14:06:19Z

I'm anticipating what Jamie would say if he were here: "Is it not possible to achieve the same thing using stateless components?"

walaura · 2019-01-30T14:41:16Z

@rupertbates I can't think of a way :( FWIW react's own section on a11y asks you to use props and state, notably being the 1 part of the react site that doesn't tell you to not use them

rupertbates

Great work!

prout-bot · 2019-01-30T16:35:51Z

Seen on PROD (merged by @walaura 10 minutes and 40 seconds ago)

Keep an eye out for post deployment test results on Travis

Sentry Release: support-client-side, support

prout-bot · 2019-01-30T16:44:50Z

✅ Testing in PROD passed! Details

SiAdcock · 2019-01-31T08:32:11Z

stories/dialog.jsx

+      <ControlledDialogButton modal={false} />
+    </ProductPageTextBlock>
+  </div>
+));


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?

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

SiAdcock · 2019-01-31T08:40:33Z

assets/components/dialog/dialog.jsx

+        className={classNameWithModifiers('component-dialog', [modal ? 'modal' : null, open ? 'open' : null])}
+        aria-modal={modal}
+        aria-hidden={!open}
+        tabIndex="-1"


The WHATWG spec states that

The tabindex attribute must not be specified on dialog elements.

Not sure why though. Is this necessary?

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?

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! 👍

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!

SiAdcock · 2019-01-31T08:42:12Z

assets/components/dialog/dialog.jsx

+      <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}


~~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?

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

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

nono this is in the spec, see this:

SiAdcock · 2019-01-31T08:49:23Z

assets/components/dialog/dialog.jsx

+        aria-modal={modal}
+        aria-hidden={!open}
+        tabIndex="-1"
+        role="dialog"


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

SiAdcock · 2019-01-31T08:59:02Z

assets/components/dialog/dialog.jsx

+  close() {
+    if (this.ref && this.ref.close) {
+      this.ref.close();
+    }


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

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

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

SiAdcock

This looks great! 💎

Sorry for the post-merge question spam. Sources for my questions:

walaura · 2019-01-31T09:33:57Z

@SiAdcock not at all! you raise many super valid points and have given me food for thought, will tag you in a subsequent PR.

Laura added 12 commits January 29, 2019 19:45

barebones dialog

c635f84

stories

efc2789

dialog stories

c510b84

css containment

f7486a1

update story

e84cc99

simplify

f98225b

vegi supreme

ae8a156

dialog types

8ff277a

aria stuff

8143bf8

more aria stuff

a922e3d

a11y

6e21d48

undo mobile header

308af8a

walaura requested a review from rupertbates January 30, 2019 12:34

Laura added 4 commits January 30, 2019 12:35

not u

4ade05d

cleanup

4abee15

fix aria button

e1dccce

add pf.io

7896644

walaura requested a review from SiAdcock January 30, 2019 13:38

rupertbates approved these changes Jan 30, 2019

View reviewed changes

walaura merged commit 7c2b3cf into master Jan 30, 2019

prout-bot added Pending-on-PROD and removed Pending-on-PROD labels Jan 30, 2019

prout-bot added the Seen-on-PROD label Jan 30, 2019

prout-bot added the Pass-in-PROD label Jan 30, 2019

walaura deleted the lg-what-are-dialogs-even branch January 31, 2019 07:39

SiAdcock reviewed Jan 31, 2019

View reviewed changes

walaura mentioned this pull request Jan 31, 2019

Dialog tweaks #1421

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add <Dialog> element #1416

Add <Dialog> element #1416

walaura commented Jan 30, 2019

rupertbates commented Jan 30, 2019

walaura commented Jan 30, 2019

rupertbates left a comment

prout-bot commented Jan 30, 2019

prout-bot commented Jan 30, 2019

SiAdcock Jan 31, 2019 •

edited

Loading

walaura Jan 31, 2019

SiAdcock Jan 31, 2019

walaura Jan 31, 2019

SiAdcock Jan 31, 2019

walaura Jan 31, 2019

SiAdcock Jan 31, 2019 •

edited

Loading

walaura Jan 31, 2019

SiAdcock Jan 31, 2019

walaura Jan 31, 2019

SiAdcock Jan 31, 2019

walaura Jan 31, 2019

SiAdcock Jan 31, 2019

walaura Jan 31, 2019

SiAdcock Jan 31, 2019

SiAdcock left a comment •

edited

Loading

walaura commented Jan 31, 2019 •

edited

Loading

Add <Dialog> element #1416

Add <Dialog> element #1416

Conversation

walaura commented Jan 30, 2019

Why are you doing this?

Screenshots

rupertbates commented Jan 30, 2019

walaura commented Jan 30, 2019

rupertbates left a comment

Choose a reason for hiding this comment

prout-bot commented Jan 30, 2019

Sentry Release: support-client-side, support

prout-bot commented Jan 30, 2019

SiAdcock Jan 31, 2019 • edited Loading

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

SiAdcock Jan 31, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SiAdcock left a comment • edited Loading

Choose a reason for hiding this comment

walaura commented Jan 31, 2019 • edited Loading

SiAdcock Jan 31, 2019 •

edited

Loading

SiAdcock Jan 31, 2019 •

edited

Loading

SiAdcock left a comment •

edited

Loading

walaura commented Jan 31, 2019 •

edited

Loading