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

[a11y] Make the “+ More” action for showing more filters keyboard accessible #311

Merged
merged 5 commits into from
Jul 9, 2019
Merged

[a11y] Make the “+ More” action for showing more filters keyboard accessible #311

merged 5 commits into from
Jul 9, 2019

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jul 2, 2019

Description

On the Facet/MultiCheckboxFacet components, the + More action for showing more filters/checkboxes was previous not accessible to keyboard users. This PR addresses that accessibility issue by converting the action to a button (see below screenshot).

List of changes

  • Converts MultiCheckboxFacet's + More action from a <div> to a <button>
  • Adds hover/focus styling to the + More action

Associated Github Issues

Fixes #307

- which overrides the default Chrome/browser-level focus ring, while still providing some UI indication/affordance to keyboard users tabbing through page
@cee-chen cee-chen requested a review from JasonStoltz July 2, 2019 23:28
@netlify
Copy link

netlify bot commented Jul 2, 2019

Deploy preview for search-ui-sandbox ready!

Built with commit 5a036c7

https://deploy-preview-311--search-ui-sandbox.netlify.com

@netlify
Copy link

netlify bot commented Jul 2, 2019

Deploy preview for search-ui-storybook ready!

Built with commit 5a036c7

https://deploy-preview-311--search-ui-storybook.netlify.com

@cee-chen
Copy link
Member Author

cee-chen commented Jul 2, 2019

⚠️ This is an extra/nice-to-have, but I thought I'd quickly spike making the "More" action more screen-reader-friendly as well:

voiceover

My goal was to create screen reader behavior similar to inclusive-component's To Do List example, where after any action (e.g. adding a new item, deleting an item), the action that you just performed gets read out to the user. That way visually impaired users aren't left wondering what happened after pressing a button.

You can check out the diff/extra code here.

I kept it separate from this PR for now (since it mostly addresses screen reader vs. keyboard accessibility), but I can merge it in if you feel comfortable @JasonStoltz.

@JasonStoltz
Copy link
Member

@constancecchen I like your separate changes. The only thing I'm thinking about, is custom styling. We are hiding that div in our own stylesheet, but users will also have to know to do that in their custom stylesheets.

I wonder if it would make sense to have a specific class name that indicates that something should be hidden for accessibility, then mention it in a styling guide?

@cee-chen
Copy link
Member Author

cee-chen commented Jul 3, 2019

@JasonStoltz Oh man, excellent catch/point! I'm actually slightly leaning towards making the visually-hidden styling inline in that case, since there is basically no use case where developers/users would want that section to be visible. Would you be cool with that?

@JasonStoltz
Copy link
Member

@constancecchen Well, here's the only catch. I'm trying to make this library work with a strict CSP: #93. These policies will disallow inline CSS by default.

However, I think there are reasonable ways that users can possibly work around this and we already have some inline css from react-select, so it's a larger problem we'll need to tackle at some point anyway.

So yes, just thinking out loud there, I think I'm leaning towards the inline css approach. It's probably fine for now and we could always change it later when we address CSP as a whole.

@cee-chen
Copy link
Member Author

cee-chen commented Jul 3, 2019

Oh wow, that's super fascinating. I wonder how CSS-in-JS libraries like emotion get around this (or maybe they don't address CSP?)

I'm seeing using CSSOM as a workaround (i.e. querySelecting it and adding styles with JS) for getting around the inline-style warning - would that be something you're good with doing as a workaround?

The more I think about this also, the more I think I want to tackle the screen reader announcements in a single live area that's present just once in the app, rather than repeating it multiple times in various locations. For that reason I might merge this PR as-is and open a brand new one for screen reader accessibility/announcements that uses the CSSOM method of setting the visually hidden styles.

@JasonStoltz
Copy link
Member

@constancecchen Sounds good to me. Also, I try to squash + merge PRs on Github for this project to try to keep a clean git history. Feel free to merge at your leisure.

@cee-chen cee-chen merged commit 67ef8cd into elastic:master Jul 9, 2019
@cee-chen cee-chen deleted the keyboard-accessible-more branch July 9, 2019 17:52
@cee-chen cee-chen changed the title Make the “+ More” action for showing more filters/checkboxes keyboard accessible [a11y] Make the “+ More” action for showing more filters keyboard accessible Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The “+more” action for showing more filters/checkboxes is not keyboard accessible
2 participants