spectrum icon indicating copy to clipboard operation
spectrum copied to clipboard

Add keyboard navigation to the color palette, for accessibility.

Open ajssd opened this issue 9 years ago • 16 comments

Allow tabbing into the color picker, and focus to be set on a square in the color picker. Allow using arrow keys to move around the color palette and set focus. Allow Enter key to select the focused color. This is done with a new keydown handler.

This is needed in order to comply with accessibility requirements. Note that the color gradient is not yet accessible, just the palette.

https://github.com/bgrins/spectrum/issues/295

ajssd avatar Mar 27 '15 16:03 ajssd

Thanks for this. I've pulled this down and played with it, it's a very good start. As this kind of navigation gets tricky, I've noticed a couple of issues:

  • when I click on a swatch, it'd be nice if arrow navigation started working for consistency
  • when I press enter to select a swatch, it'd be nice if arrow navigation continued to work
  • when trying to go up/down between nodes within a single row (but that have wrapped visually like in the top colorpicker on http://bgrins.github.io/spectrum/) it doesn't move as I'd expect. This is probably really hard to get right, since it's not obvious which node is visually below the other - they are all siblings. The only way I can think of to address this would be to use elementFromPoint to try and find an element if possible, and if not fall back to the current algorithm. Let me try and put something together with that and respond later.

Here are a couple of other bugs that are maybe not as important in a first version:

  • when I press escape, I'd like the colorpicker to close (this is an existing general issue in https://github.com/bgrins/spectrum/issues/281, but it'd be great to get it fixed)
  • when pressing 'left' on the first swatch in a row or 'right' on the last, it'd be nice if it continued wrapping into the previous/next row.

bgrins avatar Apr 25 '15 16:04 bgrins

OK, I've updated the https://github.com/bgrins/spectrum/compare/ajssd-master branch to handle the left/right and up/down behavior I was discussing. Let me know what you think so far.

I'd still like if we could get navigation to work after pressing Enter, and also if it was enabled after clicking on a swatch. I think we also need to make escape close the picker so people have a way to get out of the picker if they want to use the keyboard to scroll the page, for instance.

We also need some basic test coverage for this in https://github.com/bgrins/spectrum/blob/master/test/tests.js.

bgrins avatar Apr 25 '15 17:04 bgrins

I think we also need to make escape close the picker so people have a way to get out of the picker if they want to use the keyboard to scroll the page, for instance.

Just landed ESC closing the picker on master for #281

bgrins avatar Apr 27 '15 00:04 bgrins

I added two more minor accessibility-related changes, both aria-labels on user-navigable elements (the initial/current color swatches, and the input field). An aria-label is read aloud by screen readers but is otherwise not visible; it is required for accessibility when there isn't an explicit label.

ajssd avatar Sep 01 '15 04:09 ajssd

I made one additional change -- instead of putting the tabindex on the focused swatch (sp-thumb-el) now the tabindex is on on the palette itself. This way, we don't have to add and remove tab stops as the user tabs around. Also, if a color is chosen from the rgb picker on the right, and that color is not in the palette, previously there was no way to regain focus in the palette. Now it is possible. Please consider accepting this pull request in order to make Spectrum keyboard-navigable and compliant with Accessibility requirements.

ajssd avatar Sep 01 '15 16:09 ajssd

This still needs tests, you can add a new test function at the bottom of https://github.com/bgrins/spectrum/blob/master/test/tests.js and run them locally either by loading the test/index.html page in a browser or by running grunt test after setting it up with: https://github.com/bgrins/spectrum#building-spectrum-locally.

jQuery allows you to create new events to trigger custom key presses like this: http://api.jquery.com/category/events/event-object/

bgrins avatar Sep 01 '15 17:09 bgrins

OK I've gone through and made a few notes. I will still need to apply this locally and do a test, but I think getting basic accessibility in place (once there is some test coverage) will be a great start and we can handle enhancements in follow up work. Thanks for following up with this!

bgrins avatar Sep 01 '15 17:09 bgrins

What's missing in this PR to hit master?

I could help

p4bl1t0 avatar Dec 17 '15 12:12 p4bl1t0

Nothing that I know of ... we have been pulling from this branch since Sep 1, without issue. I would recommend Brian fold this into master and release 1.7.1.

ajssd avatar Jan 04 '16 19:01 ajssd

Brian, would it be possible to merge these changes into the forthcoming 1.8.0 release? My team is interested in leveraging Spectrum in our product, however we will most likely be unable to take the dependency until accessibility support has been added. Thanks!

SirRidesAlot avatar Jan 07 '16 23:01 SirRidesAlot

I'm sorry for dropping this. I've asked @miketoth if he could review the patch and advise if we can land it straight away or if it needs changes.

bgrins avatar May 05 '19 16:05 bgrins

Thanks bgrins! I should be able to review this tomorrow

miketoth avatar May 08 '19 05:05 miketoth

Hey guys. I'm fairly new to github branching stuff. Is this accessibility stuff checked into the main branch? E.g. if I get the main branch, it will have accessibility already. If it isn't checked into the main branch, this branch has no conflicts with the main. So I could also use this child branch? Thanks

NickersWeb avatar Sep 11 '19 11:09 NickersWeb

Downloaded the branch. Would like better keyboard functionality, unless I've missed something. Going to branch this branch.

NickersWeb avatar Sep 16 '19 08:09 NickersWeb

Hi guys, I appreciate your effort for doing these accessibility updates. I'm interested to use this updated version. Can it be already merged with master?

The thing is I'm managing my project with Composer by using asset-packagist and there is no option to use these specific branches as you can see https://asset-packagist.org/package/npm-asset/spectrum-colorpicker

Thanks for understanding!

dekisha avatar Jul 03 '20 13:07 dekisha

@dekisha if they do merge it. I hope they merge the changes on my fork. I did some more accessibility changes. Check my merge request.

NickersWeb avatar Jul 03 '20 13:07 NickersWeb