Cover: Fix placeholder color options keyboard accessibility #68662
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
afercia
added
[Type] Bug
|
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: -6 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 |
Mamaduka
changed the title
|
Pinging @WordPress/gutenberg-components as well for feedback. |
ciampo
reviewed
Jan 28, 2025
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.
Comment on lines
43
to
52
| } & ( | ||
| | { | ||
| '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.
Comment on lines
134
to
154
| | { 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 }
/>
afercia
force-pushed
the
fix/cover-placeholder-color-options-keyboard-accessibility
branch
2 times, most recently
from
013af9c to
3a78228
Compare
Mamaduka
reviewed
Feb 7, 2025
| /** | ||
| * Computes the common props for the CircularOptionPicker. | ||
| */ | ||
| export function useComputeCircularOptionPickerCommonProps( |
There was a problem hiding this comment.
Suggested change
| 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.
|
|
afercia
commented
Feb 7, 2025
| @@ -15,7 +15,7 @@ export function OptionGroup( { | |||
| }: OptionGroupProps ) { | |||
| 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:
|
ciampo
reviewed
Feb 7, 2025
| @@ -132,7 +132,7 @@ function ButtonsCircularOptionPicker( | |||
| ); | |||
|
|
|||
| 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.
6 tasks
…rendered as buttons.
afercia
force-pushed
the
fix/cover-placeholder-color-options-keyboard-accessibility
branch
from
bb291c3 to
e4c453e
Compare
|
I reverted the change to the |
carolinan
reviewed
Feb 14, 2025
| @@ -514,6 +514,8 @@ function CoverEdit( { | |||
| value={ overlayColor.color } | |||
| onChange={ onSetOverlayColor } | |||
| clearable={ false } | |||
| asButtons | |||
| aria-label={ __( 'Background color' ) } | |||
There was a problem hiding this comment.
Nit: In the cover block, it is not called background color but overlay.
Even if there is no media selected.
carolinan
reviewed
Feb 14, 2025
| .getByRole( 'listbox', { | ||
| name: 'Custom color picker.', | ||
| .getByRole( 'group', { | ||
| name: 'Background color', |
There was a problem hiding this comment.
Same here, this background color should be overlay.
|
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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