Cover: Fix placeholder color options keyboard accessibility#68662
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -8 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
|
Since any UI in the editor should be operable with the keyboard at least for a basic usage, and this UI isn't, I would appreciate this PR to be considered for the next WordPress release. Cc @WordPress/gutenberg-core |
|
Pinging @WordPress/gutenberg-components as well for feedback. |
ciampo
left a comment
There was a problem hiding this comment.
Thank you for working on this, @afercia .
I think it would be best to split the changes to a few separate PR:
- one improving the accessibility (grouping and labelling
CircularOptionPickeroptions whenasButtons) - another one switching the Cover block placeholder to use the
asButtonsprop
It makes code changes easier to follow and test in isolation, especially when they happen across separate packages.
| } & ( | ||
| | { | ||
| 'aria-label': string; | ||
| 'aria-labelledby'?: never; | ||
| } | ||
| | { | ||
| 'aria-label'?: never; | ||
| 'aria-labelledby': string; | ||
| } | ||
| ); |
There was a problem hiding this comment.
This seems technically correct, but let's watch out for TS computed types since combining type "unions" can quickly result in a high number or resulting types
There was a problem hiding this comment.
First time for me familiarizing with 'unions' so any advice is more than welcome.
Also, there's a little bit of duplication in this PR, I'd appreciate any feedback about how to prevent it and abstract a bit.
| ..._metaProps, | ||
| 'aria-label': __( 'Custom color picker.' ), | ||
| }; | ||
| } |
There was a problem hiding this comment.
The whole logic for building the metaProps object feels more complicated than it could be, potentially with less if statements and without using an intermediate _metaProps object.
I don't have much to take a look at it today, but if needed I can try to help in the next days.
There was a problem hiding this comment.
Yes totally agree, I know. I just followed the existing pattern but I did see there's room for improvements. Also, there's now some duplication. I'd be glad if you want to make improvements here and make this PR yours, when you have a chance.
| | { asButtons: true; 'aria-label': string } | ||
| | { asButtons: true; 'aria-labelledby': string }; | ||
|
|
||
| if ( asButtons ) { | ||
| metaProps = { asButtons: true }; | ||
| const _metaProps: { asButtons: true } = { | ||
| asButtons: true, | ||
| }; | ||
|
|
||
| if ( ariaLabel ) { | ||
| metaProps = { ..._metaProps, 'aria-label': ariaLabel }; | ||
| } else if ( ariaLabelledby ) { | ||
| metaProps = { | ||
| ..._metaProps, | ||
| 'aria-labelledby': ariaLabelledby, | ||
| }; | ||
| } else { | ||
| metaProps = { | ||
| ..._metaProps, | ||
| 'aria-label': __( 'Custom color picker.' ), | ||
| }; | ||
| } |
There was a problem hiding this comment.
I know that this was an existing issue before this PR, but it looks like we have the same exact logic of DuotonePicker repeated in GradientPicker — something to look into and understand if it's worth de-duping.
There was a problem hiding this comment.
Yes totally agree, as mentioned above. I was just unsure abou tthe best way to abstract it and de-dupe.
There was a problem hiding this comment.
We could do something like the following:
- simplify the logic by splitting the accessible label computation from the
asButtons+loop, which already enables a lot of de-duping - move the logic to a shared place, eg a utils file in the
CircularOptionPickerfolder
Example code changes:
diff --git a/packages/components/src/color-palette/index.tsx b/packages/components/src/color-palette/index.tsx
index 68c4d339120..93f1dc6c576 100644
--- a/packages/components/src/color-palette/index.tsx
+++ b/packages/components/src/color-palette/index.tsx
@@ -39,6 +39,7 @@ import {
isMultiplePaletteArray,
normalizeColorValue,
} from './utils';
+import { useComputeCircularOptionPickerCommonProps } from '../circular-option-picker/utils';
extend( [ namesPlugin, a11yPlugin ] );
@@ -251,50 +252,15 @@ function UnforwardedColorPalette(
</CircularOptionPicker.ButtonAction>
);
- let metaProps:
- | { asButtons: false; loop?: boolean; 'aria-label': string }
- | { asButtons: false; loop?: boolean; 'aria-labelledby': string }
- | { asButtons: true; 'aria-label': string }
- | { asButtons: true; 'aria-labelledby': string };
-
- if ( asButtons ) {
- const _metaProps: { asButtons: true } = {
- asButtons: true,
- };
-
- if ( ariaLabel ) {
- metaProps = { ..._metaProps, 'aria-label': ariaLabel };
- } else if ( ariaLabelledby ) {
- metaProps = {
- ..._metaProps,
- 'aria-labelledby': ariaLabelledby,
- };
- } else {
- metaProps = {
- ..._metaProps,
- 'aria-label': __( 'Custom color picker.' ),
- };
- }
- } else {
- const _metaProps: { asButtons: false; loop?: boolean } = {
- asButtons: false,
+ const { metaProps, labelProps } = useComputeCircularOptionPickerCommonProps(
+ {
+ asButtons,
loop,
- };
-
- if ( ariaLabel ) {
- metaProps = { ..._metaProps, 'aria-label': ariaLabel };
- } else if ( ariaLabelledby ) {
- metaProps = {
- ..._metaProps,
- 'aria-labelledby': ariaLabelledby,
- };
- } else {
- metaProps = {
- ..._metaProps,
- 'aria-label': __( 'Custom color picker.' ),
- };
+ ariaLabel,
+ ariaLabelledby,
+ fallbackLabel: __( 'Custom color picker.' ),
}
- }
+ );
return (
<VStack spacing={ 3 } ref={ forwardedRef } { ...additionalProps }>
@@ -352,6 +318,7 @@ function UnforwardedColorPalette(
{ ( colors.length > 0 || actions ) && (
<CircularOptionPicker
{ ...metaProps }
+ { ...labelProps }
actions={ actions }
options={
hasMultipleColorOrigins ? (
diff --git a/packages/components/src/duotone-picker/duotone-picker.tsx b/packages/components/src/duotone-picker/duotone-picker.tsx
index ee8319b1618..f6348bf1129 100644
--- a/packages/components/src/duotone-picker/duotone-picker.tsx
+++ b/packages/components/src/duotone-picker/duotone-picker.tsx
@@ -20,6 +20,7 @@ import CustomDuotoneBar from './custom-duotone-bar';
import { getDefaultColors, getGradientFromCSSColors } from './utils';
import { Spacer } from '../spacer';
import type { DuotonePickerProps } from './types';
+import { useComputeCircularOptionPickerCommonProps } from '../circular-option-picker/utils';
/**
* ```jsx
@@ -127,50 +128,15 @@ function DuotonePicker( {
);
} );
- let metaProps:
- | { asButtons: false; loop?: boolean; 'aria-label': string }
- | { asButtons: false; loop?: boolean; 'aria-labelledby': string }
- | { asButtons: true; 'aria-label': string }
- | { asButtons: true; 'aria-labelledby': string };
-
- if ( asButtons ) {
- const _metaProps: { asButtons: true } = {
- asButtons: true,
- };
-
- if ( ariaLabel ) {
- metaProps = { ..._metaProps, 'aria-label': ariaLabel };
- } else if ( ariaLabelledby ) {
- metaProps = {
- ..._metaProps,
- 'aria-labelledby': ariaLabelledby,
- };
- } else {
- metaProps = {
- ..._metaProps,
- 'aria-label': __( 'Custom color picker.' ),
- };
- }
- } else {
- const _metaProps: { asButtons: false; loop?: boolean } = {
- asButtons: false,
+ const { metaProps, labelProps } = useComputeCircularOptionPickerCommonProps(
+ {
+ asButtons,
loop,
- };
-
- if ( ariaLabel ) {
- metaProps = { ..._metaProps, 'aria-label': ariaLabel };
- } else if ( ariaLabelledby ) {
- metaProps = {
- ..._metaProps,
- 'aria-labelledby': ariaLabelledby,
- };
- } else {
- metaProps = {
- ..._metaProps,
- 'aria-label': __( 'Custom color picker.' ),
- };
+ ariaLabel,
+ ariaLabelledby,
+ fallbackLabel: __( 'Custom color picker.' ),
}
- }
+ );
const options = unsetable
? [ unsetOption, ...duotoneOptions ]
@@ -180,6 +146,7 @@ function DuotonePicker( {
<CircularOptionPicker
{ ...otherProps }
{ ...metaProps }
+ { ...labelProps }
options={ options }
actions={
!! clearable && (
diff --git a/packages/components/src/gradient-picker/index.tsx b/packages/components/src/gradient-picker/index.tsx
index 544faee8481..7d9ecdff8bd 100644
--- a/packages/components/src/gradient-picker/index.tsx
+++ b/packages/components/src/gradient-picker/index.tsx
@@ -18,6 +18,7 @@ import type {
OriginObject,
GradientObject,
} from './types';
+import { useComputeCircularOptionPickerCommonProps } from '../circular-option-picker/utils';
// The Multiple Origin Gradients have a `gradients` property (an array of
// gradient objects), while Single Origin ones have a `gradient` property.
@@ -128,54 +129,20 @@ function Component( props: PickerProps< any > ) {
<SingleOrigin { ...additionalProps } />
);
- let metaProps:
- | { asButtons: false; loop?: boolean; 'aria-label': string }
- | { asButtons: false; loop?: boolean; 'aria-labelledby': string }
- | { asButtons: true; 'aria-label': string }
- | { asButtons: true; 'aria-labelledby': string };
-
- if ( asButtons ) {
- const _metaProps: { asButtons: true } = {
- asButtons: true,
- };
-
- if ( ariaLabel ) {
- metaProps = { ..._metaProps, 'aria-label': ariaLabel };
- } else if ( ariaLabelledby ) {
- metaProps = {
- ..._metaProps,
- 'aria-labelledby': ariaLabelledby,
- };
- } else {
- metaProps = {
- ..._metaProps,
- 'aria-label': __( 'Custom color picker.' ),
- };
- }
- } else {
- const _metaProps: { asButtons: false; loop?: boolean } = {
- asButtons: false,
+ const { metaProps, labelProps } = useComputeCircularOptionPickerCommonProps(
+ {
+ asButtons,
loop,
- };
-
- if ( ariaLabel ) {
- metaProps = { ..._metaProps, 'aria-label': ariaLabel };
- } else if ( ariaLabelledby ) {
- metaProps = {
- ..._metaProps,
- 'aria-labelledby': ariaLabelledby,
- };
- } else {
- metaProps = {
- ..._metaProps,
- 'aria-label': __( 'Custom color picker.' ),
- };
+ ariaLabel,
+ ariaLabelledby,
+ fallbackLabel: __( 'Custom color picker.' ),
}
- }
+ );
return (
<CircularOptionPicker
{ ...metaProps }
+ { ...labelProps }
actions={ actions }
options={ options }
/>
013af9c to
3a78228
Compare
| /** | ||
| * Computes the common props for the CircularOptionPicker. | ||
| */ | ||
| export function useComputeCircularOptionPickerCommonProps( |
There was a problem hiding this comment.
| export function useComputeCircularOptionPickerCommonProps( | |
| export function getComputeCircularOptionPickerCommonProps( |
Should we use the get prefix instead of use? The latter is a reserved prefix in React for hooks, but since this method isn't a hook, it does not need to adhere to the "Rules of Hook" enforced by ESLint/React.
|
|
| const role = | ||
| 'aria-label' in additionalProps || 'aria-labelledby' in additionalProps | ||
| ? 'group' | ||
| ? 'listbox' |
There was a problem hiding this comment.
This aims to fix an additional issue with the CircularOptionPicker.OptionGroup. To my understanding, it always contains buttons with a role="option" so it should be a listbox.
This is different from the behavior of the parent component CircularOptionPicker that changes role depending on the asButton prop, which actually renders two alternative components: ListboxCircularOptionPicker or ButtonsCircularOptionPicker.
There was a problem hiding this comment.
it always contains buttons with a
role="option"so it should be a listbox.
Not 100% about that, but for sure this component is already rendered inside an element with role="listbox", see this example:
There was a problem hiding this comment.
Hm so this is intended to render one or more groups inside a listbox? Then I'm not understanding why the CircularOptionPicker in the ColorPalette is not renderinf the role=listbox.
To recapt:
- role=listbox > role=group > series of role=option is a valid structure
- role=group > series of role=option isn't.
I can revert this change but this should be investigated to understand how to fix the scenario 2).
There was a problem hiding this comment.
The example above is from ColorPalette
Did you come across an instance of ColorPalette without a listbox parent?
There was a problem hiding this comment.
I mentioned on the issue: Paragraph block > Color > Text.
Notice there's also a difference beween single palette and multiple palette.
There was a problem hiding this comment.
Thank you for signaling — I think it deserves a separate investigation, and I would otherwise leave the group role for the time being.
There was a problem hiding this comment.
Agreed, will revert on Monday and create a separate issue. Thanks for the review.
|
I tried to do my best to abstract the computation of the props. Please do let me know if this PR needs a changelog entry for the components package. Not sure because it started as a fix for the Cover block but then required changes to:
I'd appreciate some eyes on the Typescript changes, really not my area of expertise. I tried to simplify. To recap:
|
|
|
||
| return ( | ||
| <div { ...additionalProps } id={ baseId }> | ||
| <div { ...additionalProps } role="group" id={ baseId }> |
There was a problem hiding this comment.
Is it ok to have a role="group" even when the element is not labelled? Should we add a check like in the OptionGroup component?
There was a problem hiding this comment.
Yes it is OK. It should use the fallback label though, am I missing something?
There was a problem hiding this comment.
It will when used in ColorPalette / DuotonePicker / GradientPicker, but when the CircularOptionPicker is used standalone, we're not guaranteed to have a fallback label, right?
Yes it is OK.
If it is OK, should we remove the equivalent check in OptionGroup ?
There was a problem hiding this comment.
A group role is a very generic role for signaling logically grouped controls. In a way, it's similar to a <fieldset> element that, without a <legend> element would only signal the grouping without communicating the 'what' the group is about. It is acceptable but it would be good to label the group.
Rather, I think the group role should not depend on the presence of aria-label or ariar-labelledby. To me, a listbox that contains only one set of options should not have a group. Instead, when a listbox contains two or more set of options, they should be groups and be labeled.
There was a problem hiding this comment.
Got it — feel free to work on this improvement to OptionGroup in a follow-up PR and ping for reviews
There was a problem hiding this comment.
It's very minor, not sure it is worth it.
bb291c3 to
e4c453e
Compare
|
I reverted the change to the |
|
I am not able to do the code review, but I have confirmed that I can use the tab key or the right and left arrow keys to navigate inside the cover block placeholder to select different colors or media options. |
e4c453e to
61e8228
Compare
|
Thanks @carolinan I updated the label and selector in the test file. |
|
Flaky tests detected in 61e8228. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/13366485276
|
ciampo
left a comment
There was a problem hiding this comment.
Changes are testing good, overall. Left a couple of comments but I think once those are solved, we can do a final testing round and approve/merge
|
|
||
| ### `aria-label`: `string` | ||
|
|
||
| The label for the wrapper element. Not used if an 'aria-labelledby' is provided. |
There was a problem hiding this comment.
Question — is it a good practice to add extra logic to labels priority?
What I mean is, should we remove the custom logic in getComputeCircularOptionPickerCommonProps that ignores aria-label if aria-labelledby is defined? From what I understand, it's already the default behaviour applied by browsers, which means that aria-label would be automatically ignored if aria-labelledby is also defined.
In that sense, it wouldn't make sense to document the behaviour either, since it's standard DOM and not a particular behaviour of this component.
There was a problem hiding this comment.
Yes, aria-labelledby overrides aria-label so that's what browsers already do when exposing information in the accessibility tree. I think the logic and documentation do add value because developers need to be educated. They may set both props and think aria-label does something, while it wouldn't do anything,
Anyways, I would say it's unrelated to the scope of this PR and the previous implementation made sure only one of the two was in use anyways.
There was a problem hiding this comment.
I agree with the point that this behavior was already part of the component's logic.
In general, though, I'd prefer not to document standard DOM features. Given that these are web components, consumers should assume Web Standards and platform behavior as a pre-requisite unless specified otherwise. Otherwise, where do we draw the line between what need to be re-documented and what can be left implied?
|
|
||
| return ( | ||
| <div { ...additionalProps } id={ baseId }> | ||
| <div { ...additionalProps } role="group" id={ baseId }> |
There was a problem hiding this comment.
Got it — feel free to work on this improvement to OptionGroup in a follow-up PR and ping for reviews
ciampo
left a comment
There was a problem hiding this comment.
LGTM 🚀 Thank you all for working on this fix
|
Thanks for your review @ciampo
I would love contributors and consumers of this project to be knowledgeable of Web Standards and basic specifications such as HTML, WCAG, and ARIA. Unfortunately, what I've been observing in eight years of contribution to this project is that some of them don't. As such, either we enforce via code standards and best practices in the base components (which is a topic we had the opportunity to discuss in person but there's no progress about that) or we document the usage best practices in the docs and comments. |
…s#68662) * Render the color options as buttons. * Make circular option picker get a group role and optional label when rendered as buttons. * Adjust tests. * Adjust more tests. * Abstract logic to compute the common props. * Update snapshot. * Fix OptionGroup ARIA role. * Rename. * Adjust tests. * Restore group ARIA role on OptionGroup. * Change Background color label to Overlay color. Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: Mayank-Tripathi32 <mayanktripathi32@git.wordpress.org>
…s#68662) * Render the color options as buttons. * Make circular option picker get a group role and optional label when rendered as buttons. * Adjust tests. * Adjust more tests. * Abstract logic to compute the common props. * Update snapshot. * Fix OptionGroup ARIA role. * Rename. * Adjust tests. * Restore group ARIA role on OptionGroup. * Change Background color label to Overlay color. Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: Mayank-Tripathi32 <mayanktripathi32@git.wordpress.org>
Fixes #68639
What?
asButtonsset to true, it would be best for it to have arole=groupand appropriate labeling.Why?
How?
In the fist commit
Setting
asButtonsto true makes the circular options picker render the buttons as a a list of buttons that are all focusable and tabbables. This basically removes the arrow keys navigation that is in use when the options are rendered as a listbox, which conflicts with WritingFlow.In the second commit.
I just wanted to set a
role="group"and a meaningfularia-labelwhenasButtonsis true. This is important because otherwise the options are just a series of buttons. Adding arole="group"to the wrapper makes them logically grouped and thearia-labelcommunicates users what this group of buttons is about.However, I discovered that adding
aria-labeldidn't render an aria-label attribute whenasButtonsis true. Turned out the aria-label and aria-labelledby props were only allowed for theListboxCircularOptionPickerand not forButtonsCircularOptionPicker.I'm not that familiar with TypeScript and I had to make a series of changes to make this work that honestly I don't fully understand. I would greatly appreciate some eyes from contributors more familiar than me with TypeScript.
Note that I had to make changes to the allowed types also for the components that consume
CircularOptionPickerwhich are:ColorPalette,DuotonePickerandGradientPicker.To recap;
on trunk, by default the circular option picker renders:
role="listbox"aria-label="Custom color picker."that cab be passed a custom aria-labelInstead, when rendered as normal buttons via
asButton, it renders:aria-labeldoesn't workI just want the
asButtonsversion have arole=groupand anaria-label.Note: to my understanding, this old issue #54055 was aiming to provide a better labeling mechanism for these components but it seems it still needs to be addressed. It would be nice to consider further improvements in the context of that issue.
Testing Instructions
role="group"and anaria-label="Background color".Testing Instructions for Keyboard
Screenshots or screencast