-
Notifications
You must be signed in to change notification settings - Fork 86
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
fix: add visible focus state to actions menu for A11y #839
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This is a great improvement. I would like to make a few small tweaks before we merge, though.
First, can we only show the outline on the menu button when the user uses keyboard navigation? I don't like the look when it's activated with the mouse. Maybe focus-visible
can work here.
Second, can you think of ways to support navigation with cursors in the popup menu? I noticed https://observablehq.com supports it in their menu.
The second isn't needed to merge, though.
Happy to make changes; I think maybe what you're seeing as outline on the menu button using the mouse might be a browser thing since on all the browsers I checked the mouse didn't add a focus ring, only the keyboard (I added some mouse hovering at the end of my screen recording gif above). Can you let me know know what browser/OS configuration you saw that behavior in so I can test? |
Here is what I am seeing in Safari on macOS looking at http://localhost:3000/test-vl.html. Screen.Recording.2022-01-18.at.12.07.21.mov |
Updated:
Here are screen grabs of safari and chrome on mac Screen.Recording.2022-01-18.at.10.25.11.PM.movScreen.Recording.2022-01-18.at.10.25.55.PM.mov |
While we are at it, I believe the color contrast fails A11y as well; the measured color of the Filed an issue #840 |
Separate pull request. I suspect the contrast is too low when the menu button is faded out. I'm not sure we want to change that, though. Maybe we can detect if the user requests high contrast. Anyway, let's discuss in a separate pull request. |
Did you try focus-visible? |
I looked into it; it seems like Safari doesn't support it yet (there is a development implementation of it so it looks promising it will be adopted). But I couldn't get it to work. |
Ahh, I just played a bit more with it and focus-visible seems like the thing we really need. It was actually enabled in Webkit by default 6 weeks ago so it could come in the next Safari release https://trac.webkit.org/changeset/286783/webkit. I think we can just show the focus ring in Safari until they add support for focus-visible. I'll make a pull request and you can take a look. |
Here #841. |
I can't move the focus to the action menu in Firefox. How are you doing it? |
Hi @domoritz -- you can see a discussion about Tab navigation in Firefox on MacOS here. Firefox is actually respecting MacOS keyboard accessibility settings. I was able to recreate your navigation issue locally and then resolve it by following the settings outlined in that stack overflow article. |
Done in #841 |
Description
This PR adds a visible focus state to the actions menu. This creates a more accessible experience for keyboard users who might be using the TAB key to navigate through websites, and matches the visual styling to the mouse user's "hover" state. More info on why this is required for accessibility reasons here: w3.org
Here is a screen recording showing behavior via keyboard and then via mouse for reference:
Specifically:
outline: none
which was preventing the browser's default focus indication on the<summary>
element<svg>
and to each focused<a>
option in the expanded menuType of change
How Has This Been Tested?
I have deployed a test react app here for testing: https://reverent-kilby-656ff9.netlify.app/ which uses my fork for
vega-embed
This has been tested on the following browsers, which all show the expected styling and built-in browser focus ring when tabbed into focus using the keyboard, and proper display for normal click/touch to expand menu. Note: I have not been able to test on Windows, ChromeOS or Android.
Mac OS11
iOS
Checklist:
next
branch:The input spec uses Vega-Lite v4, but the current version of Vega-Lite is v5.2.0.
)