-
Notifications
You must be signed in to change notification settings - Fork 368
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
Conversation
- which overrides the default Chrome/browser-level focus ring, while still providing some UI indication/affordance to keyboard users tabbing through page
Deploy preview for search-ui-sandbox ready! Built with commit 5a036c7 |
Deploy preview for search-ui-storybook ready! Built with commit 5a036c7 |
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. |
@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? |
@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? |
@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. |
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. |
@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. |
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
+ More
action from a<div>
to a<button>
+ More
actionAssociated Github Issues
Fixes #307