Fixing color picker accessibility #11303
Conversation
* Replacing some abbriviations with proper abbr tags * Other abbriviations have been replaced with full names
Soean
added
the
[Focus] Accessibility (a11y)
Note: there is no VoiceOver bug. The issue asked to test if what as mentioned in #10564 (review) could be reproduced. However, that testing was made on a Windows VM which is a non reliable environment to test with. What was needed there was merged in #10807 |
| @@ -174,10 +174,11 @@ export class Inputs extends Component { | |||
| <fieldset> | |||
| <legend className="screen-reader-text"> | |||
| { __( 'Color value in RGB' ) } | |||
| <abbr title={ __( 'Red Green Blue' ) }>RGB</abbr> | |||
There was a problem hiding this comment.
Translatable strings shouldn't be split in two parts. Also, considering this is a legend and the field labels are expanded, I'd recommend to not use an <abbr> element at all, which has little real effect.
| </legend> | ||
| <div className="components-color-picker__inputs-fields"> | ||
| <Input | ||
| label="r" | ||
| label={ __( 'Red' ) } |
There was a problem hiding this comment.
Should use _x() to specify it refers to 'RGB color format'.
| @@ -186,7 +187,7 @@ export class Inputs extends Component { | |||
| max="255" | |||
| /> | |||
| <Input | |||
| label="g" | |||
| label={ __( 'Green' ) } | |||
There was a problem hiding this comment.
Should use _x() to specify it refers to 'RGB color format'.
| @@ -195,7 +196,7 @@ export class Inputs extends Component { | |||
| max="255" | |||
| /> | |||
| <Input | |||
| label="b" | |||
| label={ __( 'Blue' ) } | |||
There was a problem hiding this comment.
Should use _x() to specify it refers to 'RGB color format'.
| @@ -205,7 +206,7 @@ export class Inputs extends Component { | |||
| /> | |||
| { disableAlpha ? null : ( | |||
| <Input | |||
| label="a" | |||
| label={ __( 'Alpha' ) } | |||
There was a problem hiding this comment.
Should use _x() to specify it refers to 'RGBA transparency'.
| @@ -222,11 +223,12 @@ export class Inputs extends Component { | |||
| return ( | |||
| <fieldset> | |||
| <legend className="screen-reader-text"> | |||
| { __( 'Color value in HSL' ) } | |||
| { __( 'Color value in' ) } | |||
| <abbr title={ 'Hue Saturation Lightness' }>{ __( 'HSL' ) }</abbr> | |||
There was a problem hiding this comment.
See above, I'd recommend to not use <abbr>. (also note the title is not translatable but let's remove the abbr all together)
There was a problem hiding this comment.
You should use i18n %s placeholders. Use Color value in %s. This way, translators can control the location of the %s in the string when they translate to local languages.
| </legend> | ||
| <div className="components-color-picker__inputs-fields"> | ||
| <Input | ||
| label="h" | ||
| label={ __( 'Hue' ) } |
There was a problem hiding this comment.
Should use _x() to specify it refers to 'HSL color format'.
| @@ -235,7 +237,7 @@ export class Inputs extends Component { | |||
| max="359" | |||
| /> | |||
| <Input | |||
| label="s" | |||
| label={ __( 'Saturation' ) } | |||
There was a problem hiding this comment.
Should use _x() to specify it refers to 'HSL color format'.
| @@ -244,7 +246,7 @@ export class Inputs extends Component { | |||
| max="100" | |||
| /> | |||
| <Input | |||
| label="l" | |||
| label={ __( 'Lightness' ) } | |||
There was a problem hiding this comment.
Should use _x() to specify it refers to 'HSL color format'.
| @@ -254,7 +256,7 @@ export class Inputs extends Component { | |||
| /> | |||
| { disableAlpha ? null : ( | |||
| <Input | |||
| label="a" | |||
| label={ __( 'Alpha' ) } | |||
There was a problem hiding this comment.
Should use _x() to specify it refers to 'HSLA transparency'.
|
@aldavigdis thanks for your PR 🙂 I've left some comments about a few points that should be addressed. Also, I've asked on Slack #polyglots and #accessibility if terms like However, this creates a potential issue with longer strings provided by some languages. For example. for
With longer translations, the problem would be more noticeable:
This needs to be addressed in some way, as the design didn't take internationalization into account. |
|
@aldavigdis are you going to address feedback shared by @afercia? It also looks like this PR needs to be rebased with the latest changes from master to resolve failures on Travis CI. |
gziolo
added
[Type] Copy
|
Is there any editorial copy review required here, or is this a translation and design issue? |
gziolo
removed
the
Needs Copy Review
|
@michelleweber - you are right, it looks like it doesn't need editorial copy. My mistake, sorry about that. |
|
@aldavigdis where are we with this PR? Will you continue working on this, or should we close it for now? |
aduth
added
the
[Status] Stale
|
Closing as stale. Please propose a new pull request if you plan to continue the work here. |
Description
Attempts to cover part 1 of #10975.
How has this been tested?
Manual testing. I did not attempt to fix the VoiceOver bug in #10975
Screenshots
Types of changes
No breaking changes. Those are only markup-related. No testing required as far as I know, unless the CI breaks. 🤞
Checklist: