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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ColorPalette: Add an accessible color picker based on ChromePicker #10564

Merged
merged 38 commits into from Oct 19, 2018

Conversation

@ryelle
Contributor

ryelle commented Oct 12, 2018

This PR attempts to fork out the ChromePicker from react-color to make accessibility-related improvements. I've introduced a new component ColorPicker in components/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 @tofumatt

Current ChromePicker (on master)

chromepicker-master

New ColorPicker (this PR)

Hex input RGB input With alpha
color-picker-hex color-picker-rgb color-picker-w-alpha

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:

  • Text inputs are type=number so that arrow keys can increase/decrease values
  • Text inputs are correctly labelled, and grouped in fieldsets for RGB/HSL
  • Toggle input type button is labelled and a button so it supports keyboard interaction
  • Hue & Alpha bars have been implemented as sliders, and support arrow keys to increase/decrease values (along with shift+arrows, page up, page down, home, and end)
  • Saturation has 2 axis of data: up and down change brightness, while left + right change saturation. These also have keyboard support, but there isn't a matching aria role. Instead I've added a aria-describedby attribute 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:

  • Open the editor
  • Select a block with color settings, for example paragraph
  • In the block settings, open "Color settings"
  • Click on the multicolored "custom color picker" button
  • Interact with the color picker:
    • enter a hex color
    • attempt to change the color using just your keyboard (you can tab forward or shift+tab backwards between controls)
    • pick a color using your mouse (no regressions 馃檪 )
    • toggle to RGB input & enter a color in that format
    • try inputting invalid color values

Checklist:

  • My code is tested. I haven't added tests yet, but I'm happy to add that if this approach is valid.
  • My code follows the WordPress code style
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

This PR doesn't remove the react-color dependency, so we'll want to follow up with that (or I can add it here -- wasn't sure of the process).

@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 12, 2018

馃槷 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"

This comment has been minimized.

@dimadin

dimadin Oct 13, 2018

Contributor

Should this label be marked for translation?

@mtias

This comment has been minimized.

Contributor

mtias commented Oct 13, 2018

This is great, thank you @ryelle!

@youknowriad

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.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 15, 2018

(Looks like there's an outdated snapshot causing the unit tests to fail)

@mtias

This comment has been minimized.

Contributor

mtias commented Oct 15, 2018

Are some of our color utilities (contrast, etc) duplicating any of the libraries work (tinycolor2)? cc @jorgefilipecosta

Show resolved Hide resolved packages/components/src/color-picker/alpha.js Outdated
style={ gradient }
/>
{ /* eslint-disable jsx-a11y/no-static-element-interactions */ }
<div

This comment has been minimized.

@aduth

aduth Oct 15, 2018

Member

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">?

This comment has been minimized.

@ryelle

ryelle Oct 15, 2018

Contributor

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.

Show resolved Hide resolved packages/components/src/color-picker/alpha.js Outdated
import { isValidHex } from './utils';
import TextControl from '../text-control';
/* Wrapper for TextControl, only used to handle intermediate state while typing. */

This comment has been minimized.

@aduth

aduth Oct 15, 2018

Member

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.

This comment has been minimized.

@tofumatt

tofumatt Oct 18, 2018

Member

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 comment has been minimized.

@tofumatt

tofumatt Oct 23, 2018

Member

Filed #10974 馃槃

this.handleKeyDown = this.handleKeyDown.bind( this );
}
componentWillReceiveProps( nextProps ) {

This comment has been minimized.

@aduth

aduth Oct 15, 2018

Member

componentWillReceiveProps is effectively deprecated:

https://reactjs.org/docs/react-component.html#updating

Since we use StrictMode, this also logs an error in the console.

This comment has been minimized.

@ryelle

ryelle Oct 15, 2018

Contributor

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).

This comment has been minimized.

@tofumatt

tofumatt Oct 16, 2018

Member

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 馃し鈥嶁檪锔

Show resolved Hide resolved packages/components/src/color-picker/inputs.js Outdated
Show resolved Hide resolved packages/components/src/color-picker/inputs.js Outdated
@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Oct 15, 2018

Are some of our color utilities (contrast, etc) duplicating any of the libraries work (tinycolor2)? cc @jorgefilipecosta

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.

@tofumatt

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.

@tofumatt

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 }

This comment has been minimized.

@tofumatt

tofumatt Oct 16, 2018

Member

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.

This comment has been minimized.

@afercia

afercia Oct 17, 2018

Contributor

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.

This comment has been minimized.

@tofumatt

tofumatt Oct 17, 2018

Member

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.

This comment has been minimized.

@afercia

afercia Oct 17, 2018

Contributor

NVDA read off a lot of decimal places

Yeah (see below(, same for JAWS. They literally announce all those decimals:

screenshot 105

This comment has been minimized.

@afercia

afercia Oct 17, 2018

Contributor

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.

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.

Show resolved Hide resolved packages/components/src/color-picker/hue.js Outdated
aria-valuemin="359"
aria-valuenow={ hsl.h }
aria-orientation="horizontal"
aria-label={ __( 'Hue value in degrees, from 0 to 359.' ) }

This comment has been minimized.

@tofumatt

tofumatt Oct 16, 2018

Member

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.

This comment has been minimized.

@afercia

afercia Oct 17, 2018

Contributor

labels: the shorter, the better 馃檪 extra stuff should go in aria-descriptions or visible descriptions

This comment has been minimized.

@ryelle

ryelle Oct 18, 2018

Contributor

Is a description necessary? up/down will also work for the 1-dimensional sliders (hue/alpha), and follows the expected behavior of a slider.

@sarahmonster

This comment has been minimized.

Contributor

sarahmonster commented Oct 16, 2018

Here's a suggested solution for the focus styling:

2018-10-16 20 13 06

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.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 16, 2018

Those styles work for me. Can you push them to GitHub or paste them as CSS here so we can apply them?

@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 17, 2018

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鈥揷an you spot what's not working here?

(I'm in Firefox for Windows, btw.)

@ryelle

This comment has been minimized.

Contributor

ryelle commented Oct 17, 2018

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.

@afercia

This comment has been minimized.

Contributor

afercia commented Oct 17, 2018

@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 keydown default in this specific case 炉_(銉)_/炉

Minor:
there are a few overflow: hidden values, including one from the popover component, that prevent the "dot" focus style to be fully visible when the dot is close to the edges of its container:

screen shot 2018-10-17 at 16 04 49

@afercia

This comment has been minimized.

Contributor

afercia commented Oct 17, 2018

Forgot to mention I'd propose to consider to expand the input fields labels:
r, g, b, a

and
h, s, l, a

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.

Show resolved Hide resolved packages/components/src/color-palette/index.js
Show resolved Hide resolved packages/components/src/color-picker/index.js Outdated
Show resolved Hide resolved packages/components/src/color-picker/utils.js
Show resolved Hide resolved packages/components/src/index.js
@@ -0,0 +1,93 @@
/**

This comment has been minimized.

@gziolo

gziolo Oct 18, 2018

Member

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.

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 18, 2018

All components were pulled from react-color, then updated to follow GB code standards, then updated with the accessibility improvements.

We need to include credits in files that were ported from react-color. See one of the previous cases:
https://github.com/WordPress/gutenberg/blob/master/packages/element/src/serialize.js#L1-L26

@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 19, 2018

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:

2018-10-19 02 46 55

@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 19, 2018

Forgot to mention I'd propose to consider to expand the input fields labels:
r, g, b, a

and
h, s, l, a

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.

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.

@tofumatt

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 馃槃

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 19, 2018

Something went wrong with merging master in. It needs to be fixed before we proceed.

nosolosw and others added some commits Oct 18, 2018

Normalize HSV / HSL values
H: 0 - 260
S, L, V: 0 - 100
Prevent key events on slider handles
Fixes the issue where VoiceOver grabs arrow-key input as navigation
Remove test as the component API surface changed
We have done some changes to the ChromePicker so this test
can no longer be replicated.

@tofumatt tofumatt force-pushed the ryelle:add/a11y-color-picker branch from edda669 to 330ff58 Oct 19, 2018

@tofumatt

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 git user 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. 馃憤

@tofumatt tofumatt merged commit 3fa55cb into WordPress:master Oct 19, 2018

1 of 2 checks passed

codecov/project 48.51% (-0.76%) compared to 48c1209
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 19, 2018

I'm going to file some follow-up issues here soon and link to them from here. We're just getting started! 馃槃

@afercia

This comment has been minimized.

Contributor

afercia commented Oct 19, 2018

For me, in Windows, the saturation/lightness control does not work with my keyboard

Weird, for me it works. What browser / screen reader combinations are you testing with? please specify the versions.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 19, 2018

I would be happy if it's just me. I'm on a Windows 10, x64 VMWare image (from Modern.IE) running:

  • Firefox 62.0.3 (64-bit)
  • Edge 42.17134.1.0
  • latest Chrome

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...

@afercia

This comment has been minimized.

Contributor

afercia commented Oct 19, 2018

I guess It's really your VM. I've re-tested on Windows 10 without a screen reader running:

  • Firefox latest
  • Chrome latest
  • Edge
  • IE 11
    they all work.

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.

antpb added a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018

ColorPalette: Add an accessible color picker based on ChromePicker (W鈥
鈥rdPress#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 鈥淚nput鈥 component to handle intermediate state when entering values

Specifically needed for hex colors, which can be invalid while they鈥檙e 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment