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

Storybook dropdown should be accessible 11724 #12295

Merged

Conversation

aitchiss
Copy link
Contributor

@aitchiss aitchiss commented Jan 15, 2021

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

This PR updates the Dropdown Storybook stories so that they are keyboard accessible. The dropdowns can now be opened and closed by keyboard (Enter / Esc), and can also be closed by clicking outside of the dropdown.

Notes have been added to the stories to give context on accessibility implications when using the dropdown component.

This PR ensures that our Storybook documents accessible usage of the Dropdown component, however wider work is needed to ensure all dropdowns used throughout the app are consistently handling keyboard events and focus in a way optimised for accessibility. This will need to be a generalised approach that accounts for both Preact and non-Preact usage. A new issue has been created for this here: #12297

Related Tickets & Documents

Closes #11724 - Storybook dropdown should be accessible

QA Instructions, Screenshots, Recordings

  • Open Storybook and navigate to Dropdowns
  • In each of the Dropdown stories, you should be able to:
    -- Focus the activator button by pressing the Tab key
    -- When pressing enter on the focused button, the dropdown content should appear, and focus should be sent to the link inside the content
    -- When the dropdown is open, pressing Escape should close it
    -- When the dropdown is open, clicking outside of the dropdown content should close it
    -- Clicking the button with the mouse should toggle the dropdown content open/closed
    -- When using a screen reader (I have tested this on Safari with VoiceOver), the expanded/collapsed state of the dropdown should be announced when the activator button is focused

UI accessibility concerns?

See above QA instructions.

Please also note I've disabled a jsx-a11y warning regarding the keyUp handler on the wrapping div in both Dropdown.stories.jsx and dropdown.html.stories.jsx. The event handler has been added to the containing (non-interactive) div to ensure Escape key presses are captured to collapse the dropdown content. As the containing div isn't expected to be interactive, the linter warning is in this case a false positive. A comment has been added to the code to this effect too.

Added tests?

  • Yes
  • No, and this is why: the underlying Dropdown component has not been changed, the code changes only impact the Storybook documentation
  • I need help with writing tests

Added to documentation?

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

a stoat pops its head up and down

@pr-triage pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label Jan 15, 2021
@@ -67,6 +67,7 @@ const themeSwitcherDecorator = (storyFn) => {
};

addDecorator(themeSwitcherDecorator);
addDecorator((Story) => <Story />);
Copy link
Contributor Author

@aitchiss aitchiss Jan 15, 2021

Choose a reason for hiding this comment

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

This allows us to use Preact hooks in Storybook

@aitchiss aitchiss marked this pull request as ready for review January 15, 2021 16:33
@aitchiss aitchiss requested review from a team and vaidehijoshi and removed request for a team January 15, 2021 16:33
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: draft bot applied label for PR's that are a work in progress labels Jan 15, 2021
@aitchiss aitchiss self-assigned this Jan 15, 2021
Copy link
Contributor

@vaidehijoshi vaidehijoshi left a comment

Choose a reason for hiding this comment

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

Works beautifully!! :)

-- When using a screen reader (I have tested this on Safari with VoiceOver), the expanded/collapsed state of the dropdown should be announced when the activator button is focused

Also, super impressed that you added screen reader QA instructions to this -- I love that! Thank you 💞

Comment on lines 13 to 24
### Dropdown accessibility

When a dropdown of options is opened, focus should be sent to the first
interactive item inside the dropdown content. When the dropdown is closed, focus
should return to the activator button. If there are no interactive items,
consider using an `aria-live` region to ensure the new content is announced to
screen-reader users when the dropdown appears.

A user should be able to open and close the dropdown by keyboard (e.g. using
Enter and Escape), and appropriate `aria` (e.g. `aria-haspopup`) should be used
to indicate the relationship between the activator button and the dropdown
content.
Copy link
Contributor

Choose a reason for hiding this comment

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

👏🏽👏🏽

app/javascript/crayons/Dropdown/__stories__/dropdowns.md Outdated Show resolved Hide resolved
@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 17, 2021
Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

Hi @aitchiss, love the idea!

I tried to test it and I wasn't able to use the keyboard, I'm 100% sure I'm doing something wrong though.

  1. I checked out the PR
  2. Ran yarn storybook
  3. Went to http://localhost:6006/?path=/story/components-dropdowns--default
  4. Tabbed a bit until the dropdown was in focus but as I hit the enter key, the page reloads (I tried both on Firefox and Chromium-based Edge)

Here's a gif if it can help:

dropdown

@@ -9,3 +9,16 @@ dependent on width:
- < 250px: 16px 251 - 320px: 24px

FYI: Dropdowns use “Box” component as background, with Level 3 elevation.

### Dropdown accessibility
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@aitchiss
Copy link
Contributor Author

aitchiss commented Jan 18, 2021

Hi @aitchiss, love the idea!

I tried to test it and I wasn't able to use the keyboard, I'm 100% sure I'm doing something wrong though.

  1. I checked out the PR
  2. Ran yarn storybook
  3. Went to http://localhost:6006/?path=/story/components-dropdowns--default
  4. Tabbed a bit until the dropdown was in focus but as I hit the enter key, the page reloads (I tried both on Firefox and Chromium-based Edge)

Here's a gif if it can help:

dropdown

Hey @rhymes - thanks for the review! This is strange as the gif you've posted doesn't include my changes! In the new version the button should read "click to trigger dropdown" 🤔 I'm not sure if maybe some weird caching has gotten you and maybe you could try re-running?

Either way, I've noticed there's a lack of visible focus style on the link I've added to the dropdown which I think could be potentially confusing too, so I'm going to take a look at that this morning

Co-authored-by: Vaidehi Joshi <vaidehi.sj@gmail.com>
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Jan 18, 2021
@rhymes
Copy link
Contributor

rhymes commented Jan 18, 2021

@aitchiss I managed to see your code, I had to remove the app/javascript/storybook-static folder and re-run Storybook. I guess it didn't pick the changes. Will review and report back

Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

I tried again with Edge, Chrome and Firefox and it works, thank you!

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 18, 2021
@aitchiss
Copy link
Contributor Author

thanks @rhymes - good to know about that storybook-static folder for future!

@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Jan 18, 2021
@aitchiss
Copy link
Contributor Author

@vaidehijoshi / @rhymes - I've pushed a commit since you approved - it's a minor CSS change to make the focus on the dropdown's link visible. Just in case you want to have a look at it.

@rhymes
Copy link
Contributor

rhymes commented Jan 18, 2021

@aitchiss re-tested, can't accept twice, but let's say: signed and sealed :D

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 18, 2021
@aitchiss aitchiss merged commit 516bab1 into master Jan 19, 2021
@aitchiss aitchiss deleted the aitchiss/storybook-dropdown-should-be-accessible-11724 branch January 19, 2021 08:51
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storybook: dropdown should be accessible
3 participants