ColorPalette: Add an accessible color picker based on ChromePicker #10564
Conversation
|
😮 This is epic, you are awesome! I'll probably have to wait until the weekend to review this, but I'll look at it ASAP. Thank you several million for working on this! 👍 |
| <div className="components-color-picker__inputs-toggle"> | ||
| <IconButton | ||
| icon="arrow-down-alt2" | ||
| label="Toggle input type" |
There was a problem hiding this comment.
Should this label be marked for translation?
|
This is great, thank you @ryelle! |
mtias
added
[Focus] Accessibility (a11y)
There was a problem hiding this comment.
Awesome work here. I skimmed through the code (not deeply) and it's looking good to me.
Let's get rid of react-color as it's not used anymore. and yeah test would be great but I'm fine leaving for later to get this in the UI-freeze milestone. I'll defer to @tofumatt for accessibility validation.
|
(Looks like there's an outdated snapshot causing the unit tests to fail) |
|
Are some of our color utilities (contrast, etc) duplicating any of the libraries work ( |
| style={ gradient } | ||
| /> | ||
| { /* eslint-disable jsx-a11y/no-static-element-interactions */ } | ||
| <div |
There was a problem hiding this comment.
I may be missing some context (as will the hypothetical future maintainer): For what reason do we need to create a fake range, vs. using <input type="range">?
There was a problem hiding this comment.
This here was done to make the forking of react-color's ChromePicker as 1:1 as possible. Without looking into it more, I'm not sure if we can (as easily) style the gradient background on a range input.
| import { isValidHex } from './utils'; | ||
| import TextControl from '../text-control'; | ||
|
|
||
| /* Wrapper for TextControl, only used to handle intermediate state while typing. */ |
There was a problem hiding this comment.
We have a similar need for this behavior in date picker fields. Should we extract as a common component?
Regardless, I'd tend to discourage having more than one component defined per file, if even just for breaking convention.
There was a problem hiding this comment.
We should, but we should file a follow-up issue for that… I will set a reminder to do it; trying to fix issues in here first. 😄
| this.handleKeyDown = this.handleKeyDown.bind( this ); | ||
| } | ||
|
|
||
| componentWillReceiveProps( nextProps ) { |
There was a problem hiding this comment.
componentWillReceiveProps is effectively deprecated:
https://reactjs.org/docs/react-component.html#updating
Since we use StrictMode, this also logs an error in the console.
There was a problem hiding this comment.
I saw that while I was working, but I'm not sure how to best handle this - getDerivedStateFromProps doesn't give me access to previous props, and I only want the state to update if the prop has changed (as that indicates a change from interacting with the sliders).
There was a problem hiding this comment.
Would getSnapshotBeforeUpdate work? It has prevProps as the first argument. It seems to fire before DOM mutation and reading the docs that implies this.props inside that function would be the equivalent to nextProps here but I'm not sure 🤷♂️
Hi @mtias, previously we already made use of tinycolor2 in the color utils we build (they were just specific wrappers around the lib). ChromePicker also used tinycolor2 (it was one of the reasons we decided to use it instead of other color libraries), this PR is forking ChromePicker and continues to use tinycolor2, I think there is no duplication happening. |
There was a problem hiding this comment.
Sorry it took me so long to get to this!
So I took this for a spin and the keyboard accessibility is next-level awesome. It's intuitive and I dig it. I'm going to bed soon so I'm gonna run through it with screen-readers tomorrow, but as noted the lack of focus styles on the sliders is the main visual issue I'm having with this. We definitely need those, from maybe @karmatosed or @sarahmonster? Failing that we can just hack something together because it's not clear when I have one focused, but the colour picker itself works a treat.
I'll give a run through with a screenreader tomorrow, but for now I'd say we need a focus style on the slider dots.
There was a problem hiding this comment.
I tried this out with Firefox + NVDA and it largely worked pretty well, there are certainly plenty of little tweaks we could make, but I want to focus on the major ones for now. I've left inline comments everywhere I can so it's easier to address things.
One issue is that keyboard focus isn't trapped inside the modal, at least in Firefox + NVDA. Here's a video (contains audio from NVDA): https://www.dropbox.com/s/7ipcjfhn3gs9gdh/Screen%20Recording%202018-10-16%20at%2015.17.10.mov?dl=0
That's after tabbing to the control. You can here the aria-valuenow non-integer issue in there too.
Initial focus
One last issue is when I press Enter on the custom colour picker I don't have keyboard focus in the colour picker and can't control it. Or actually this could be related to the keyboard up/down/left/right not working in Firefox for the Hue/Lightness picker. It's hard to describe but I can send a screencast if needed, just let me know if I'm not being clear.
Overall though this is great. We should try to keep it simple for now and focus on what I've mentioned, I'll see if there's anything else after but this is so much better than before.
| role="slider" | ||
| aria-valuemax="1" | ||
| aria-valuemin="0" | ||
| aria-valuenow={ rgb.a } |
There was a problem hiding this comment.
We should limit the precision of these when read aloud as they can get pretty long and it's frustrating to hear NVDA read off a lot of decimal places.
I actually wonder if what we should read off is an integer rather than a float; instead of aria-valuenow="0.57" we could have aria-valuenow="57% opacity"?
Actually all that said I don't see this selector being used anywhere.
There was a problem hiding this comment.
aria-valuetext could be used to announce an extended text with an integer value but I'm not sure it's widely supported https://www.w3.org/TR/wai-aria-1.1/#aria-valuetext
I'd tend to agree an integer value would be the best option. Worth noting aria-valuenow can't be used for arbitrary text: https://www.w3.org/TR/wai-aria-1.1/#aria-valuenow
The value of aria-valuenow is a decimal number.
There was a problem hiding this comment.
Ah, right on. Since aria-valuetext is an alternative, even if not super-supported we could use that for the integer value and have aria-valuenow as the less-friendly-but-usable decimal number.
There was a problem hiding this comment.
NVDA read off a lot of decimal places
Yeah (see below(, same for JAWS. They literally announce all those decimals:
There was a problem hiding this comment.
Ah, right on. Since
aria-valuetextis an alternative, even if not super-supported we could use that for the integer value and havearia-valuenowas the less-friendly-but-usable decimal number.
I guess we can test it with the major browser / screen reader combos and if it has no good support revert to using aria-valuenow with the friendly value.
| aria-valuemin="359" | ||
| aria-valuenow={ hsl.h } | ||
| aria-orientation="horizontal" | ||
| aria-label={ __( 'Hue value in degrees, from 0 to 359.' ) } |
There was a problem hiding this comment.
It could be handy to provide instructions in this label too: __( 'Hue value in degrees, from 0 to 359. Slide left or right to adjust.' ). Some controls are left/right and some are up/down/left/right.
There was a problem hiding this comment.
labels: the shorter, the better 🙂 extra stuff should go in aria-descriptions or visible descriptions
There was a problem hiding this comment.
Is a description necessary? up/down will also work for the 1-dimensional sliders (hue/alpha), and follows the expected behavior of a slider.
|
Here's a suggested solution for the focus styling:
I'd also suggest applying a blue-outline focus styling to the "Toggle input type" drop-down button, which should maybe say "Change input type" rather than "toggle" as well, since toggle tends to refer to a binary switch and is represented by physical toggles in the UI. |
|
Those styles work for me. Can you push them to GitHub or paste them as CSS here so we can apply them? |
|
To clarify about focus not being trapped/keyboard up/down escaping the modal; here's a GIF of me pressing up/down/left/right (it's not logged on screen, sorry, but I'm doing it) a bunch in the colour picker after focusing it with the keyboard. You can see it's focused thanks to @sarahmonster's focus styles, but it's not moving and eventually you can notice elements behind the popover are being selected. Do we need to use another method to trap the keyboard in that colour picker? @aduth, I think you know a bunch about our keyboard/focus traps–can you spot what's not working here? (I'm in Firefox for Windows, btw.) |
|
Thanks everyone for all the feedback 🙂 just a status update: I've been at a conference the last few days, but should have time later today to apply these changes. |
|
@ryelle thanks so much for working on this. I've left some comments. Additionally, seems the saturation "dot" button and the slider buttons don't work as expected with Safari + VoiceOver. On Windows, NVDA and JAWS seem not affected. As discussed on Slack, an option could be attaching a keydown event on the buttons and prevent the default (but also allowing tabbing away). That fixed it for me. Although screen readers tend to intercept keystrokes and events might not behave as expected, I wonder why there's the need to prevent a Minor:
|
|
Forgot to mention I'd propose to consider to expand the input fields labels: and Having just one letter as accessible name of a control is not so ideal for all users, and is problematic for assistive technology users. On the other hand, I understand there's little space available. Maybe some CSS magic could help. |
| @@ -0,0 +1,93 @@ | |||
| /** | |||
There was a problem hiding this comment.
Can we add a corresponding README file with a simple usage example so it could be exposed in both Gutenberg Handbook and Calypso DevDocs?
A similar example: https://github.com/WordPress/gutenberg/blob/master/packages/components/src/color-palette/README.md.
We need to include credits in files that were ported from |
|
I'm not able to move the slider using a keyboard in Windows. Were you able to move the slider using the keyboard in Windows? Maybe it's just a bug in my VMWare setup. Here's me trying; note the focus leaves the colour picker:
|
This is a really good point, but the properly display these they'd need localisation, which is difficult for us to handle with string interpolation + React at the moment (see #9846). Unless we should always display H/S/L but have screen-reader elements in div alongside/as labels for each letter that explained the labels in a localised way… maybe that could work? I that that's worth doing but could be done after this gets into the UI freeze. There are a fair few follow-up issues to be extracted from this PR, but they're better-handled as iterations as this PR is already getting pretty tough to manage because GitHub hides a lot of comments. I will go through after it's merged and file follow-ups for anything we don't address here, but it's important we get this in, at least as a basis for iteration, for the UI freeze. |
There was a problem hiding this comment.
It looks like all that remains for this to be way better than what we have, an excellent basis for iteration, and something that covers most of the actual visual UI changes we'd want to make here, is to fix the package issues @gziolo mentioned. Let me know if there's anything I can do to help that part of the PR or to clarify anything else I might've missed. I tried to go through all the comments but it can sometimes be tough to tell in these big PRs what's left to do; just the way GitHub handles them 🤷♂️
If this is working in Windows for people I need to update my VMWare setup. It also means I think this is good to go and get merged.
If someone can confirm this works well for them on Windows and the new component pieces get added as per @gziolo's request I say we get this merged 👍
Thanks so much for all your work on this @ryelle; it's an epic patch and I am really pleased to see it. It was the big UI component in the Merge: Accessibility milestone I wasn't sure we'd manage to close; happy you proved my doubts wrong 😄
|
Something went wrong with merging master in. It needs to be fixed before we proceed. |
Fixes the issue where VoiceOver grabs arrow-key input as navigation
We have done some changes to the ChromePicker so this test can no longer be replicated.
tofumatt
force-pushed
the
add/a11y-color-picker
branch
from
edda669
to
330ff58
Compare
There was a problem hiding this comment.
For me, in Windows, the saturation/lightness control does not work with my keyboard. Focus is changed after keyboard input to things entirely outside the Colour Palette.
That said:
- this works on Mac fine
- keyboard users can use all of the controls except the saturation/lightness control (before they could use none)
- we use
speak()when the modes change
This is a starting point and should be improved upon going forward, but I want to get this merged for a few reasons:
- future updates should not impact visual UI at all and the rest of the UI minimally (basically we just need to fix keyboard on Windows and improve the UI for screen reader users 😄)
- this is on a fork rather than a branch and it's getting a bit difficult to deal with; we've already had some
gituser error that needed repair and I'd like to avoid future issues
I'll go through this and file the appropriate follow-up issues (or just make a PR if possible) that will land in 4.2.
Thanks all for your efforts here, this is way better. 👍
|
I'm going to file some follow-up issues here soon and link to them from here. We're just getting started! 😄 |
Weird, for me it works. What browser / screen reader combinations are you testing with? please specify the versions. |
|
I would be happy if it's just me. I'm on a Windows 10, x64 VMWare image (from Modern.IE) running:
All whilst running NVDA 2018.3.2 and not running any screenreader software. It happens in all of them, so maybe it is a VM bug... |
|
I guess It's really your VM. I've re-tested on Windows 10 without a screen reader running:
With a screen reader (NVDA, JAWS) running no, the saturation/lightness control does not work with a keyboard. It needs the fix suggested previously. Will prepare a quick PR. |
…ordPress#10564) * Add color picker component * Add color picker styles to build * Update to buttons for focus handles * Update styles * Update class name * Add color-picker component to exports * Update name to ColorPicker * Add labels to color input groups * Fix describedby prop name * Update view to use `getDerivedStateFromProps` * Add “Input” component to handle intermediate state when entering values Specifically needed for hex colors, which can be invalid while they’re being entered * Add space around text inputs * Translate toggle label * Add focus styles to colourpicker pointers. * Remove react-color dependency * Add class name to better find the component in the devtools * Add tinycolor2 dependency * Add reference to react-color license & copyright * Better label * Update snapshot * Decouple valueKey and label so we can use semantic labels. * Use KeyboardShortcuts component to avoid separate instances of Mousetrap * Use createRef * Normalize HSV / HSL values H: 0 - 260 S, L, V: 0 - 100 * Omit `valueKey` from being passed to input * Use createRef in all components * Update saturation handle label * Prevent key events on slider handles Fixes the issue where VoiceOver grabs arrow-key input as navigation * Use a focusable div for slider handle * Apply focus style to alpha handle * Add description to hue controls * Add speak commands for view change * Update snapshot * Remove test as the component API surface changed We have done some changes to the ChromePicker so this test can no longer be replicated. * Add README, tests and update Changelog * Remove react-color dependency * Add docs manifest file
This PR attempts to fork out the ChromePicker from
react-colorto make accessibility-related improvements. I've introduced a new componentColorPickerincomponents/src/color-picker, which is visually very similar to the existing color picker. All components were pulled from react-color, then updated to follow GB code standards, then updated with the accessibility improvements. cc @tofumattCurrent
ChromePicker(on master)New ColorPicker (this PR)
I've kept the alpha functionality in ChromePicker, as it was referenced in #4726 that we might want to enable this.
The accessibility improvements I made are:
type=numberso that arrow keys can increase/decrease valuesbuttonso it supports keyboard interactionsliders, and support arrow keys to increase/decrease values (along with shift+arrows, page up, page down, home, and end)aria-describedbyattribute to say "Use your arrow keys to change the base color. Move up to lighten the color, down to darken, left to increase saturation, and right to decrease saturation."I've tested this on Mac/Chrome, Mac/Safari, Mac/Safari/VoiceOver, Windows/Firefox/NVDA. I'm personally having some trouble with the sliders when using a screen reader, but I have the same problems on the examples, so that might be user error (since I don't use a screen reader daily). I'm also open to the idea of changing the saturation interaction if there is a better role/pattern we can use.
The toggle handles themselves (the little circles that show position over the color graphs) don't have a defined focus style at the moment, so getting a designer's feedback there would be great 🙂 Same for the text inputs, too.
To test yourself:
Checklist:
This PR doesn't remove the
react-colordependency, so we'll want to follow up with that (or I can add it here -- wasn't sure of the process).