Skip to content
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

Used color name in ColorPalette aria-label. #6423

Merged

Conversation

jorgefilipecosta
Copy link
Member

This improves accessibility as displaying color code is not very useful for users to understand the color.

Fixes part of: #2699

Description

In a theme that does not change the color palette or in the theme that sets the color palette with color names (mandatory now, but not using it was not yet deprecated), verify that the aria-labels for the items of the color palette specify the color names (before they always used color code).

@afercia
Copy link
Contributor

afercia commented Apr 25, 2018

Aahh @jorgefilipecosta this is awesome, thanks! 🙂

Now finally users would have a clue what that #cf2e2e color is!

screen shot 2018-04-25 at 17 53 32

A few things to consider:

  • how can we kindly invite developers to add a color name when they add their own colors? Should we make the color name required?
  • in the current implementation, when there's a name I'd strip out the Color name: part from the string and use just the name. It's pretty clear it's a color.
  • other users would need a visible label, for example speech recognition software users would need at least tooltips to discover the control name and voice a command like, for example: "Click vivid red". Can we add tooltips? e.g.

screen shot 2018-04-25 at 17 58 53

The Custom color picker would need a tooltip as well:

screen shot 2018-04-25 at 18 00 42

Good job! 🎉

P.S. I think the snapshots need to be regenerated.

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Apr 25, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the update/used-color-name-in-palette-aria-label branch 3 times, most recently from caff3e2 to 6fa1c8b Compare April 26, 2018 14:50
This improves as accessibility as displaying color code is not very useful for users to understand the color.
@jorgefilipecosta jorgefilipecosta force-pushed the update/used-color-name-in-palette-aria-label branch from 6fa1c8b to 6970dbb Compare April 26, 2018 15:34
@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Apr 26, 2018

Hi @afercia, thank you for your review. Your feedback was applied labels were changed and tooltips were added.
Regarding your question:

how can we kindly invite developers to add a color name when they add their own colors? Should we make the color name required?

Given the changes to the color mechanism to use classes, it is mandatory for theme developers to set a color name, but currently, we are still accepting colors without names (for two versions I think) in order to be compatible with existing code. Warnings appear in the console saying that a change will be required.

@jorgefilipecosta jorgefilipecosta self-assigned this Apr 26, 2018
@afercia
Copy link
Contributor

afercia commented Apr 26, 2018

Cool, thanks for the feedback @jorgefilipecosta

@jorgefilipecosta jorgefilipecosta requested a review from a team April 27, 2018 19:13
@jorgefilipecosta jorgefilipecosta added this to the 2.8 milestone Apr 30, 2018
@jorgefilipecosta jorgefilipecosta merged commit af190d5 into master Apr 30, 2018
@jorgefilipecosta jorgefilipecosta deleted the update/used-color-name-in-palette-aria-label branch April 30, 2018 10:51
@mtias
Copy link
Member

mtias commented May 3, 2018

how can we kindly invite developers to add a color name when they add their own colors? Should we make the color name required?

Yes, to confirm, this should be the required way to declare colors once we drop the back compat ability!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants