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.

Sign up for GitHub

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

Cover: Fix placeholder color options keyboard accessibility #68662

Merged
merged 11 commits into from
Feb 18, 2025
Merged

Cover: Fix placeholder color options keyboard accessibility #68662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
95c1e9a
Render the color options as buttons.
afercia Jan 13, 2025
fc45108
Make circular option picker get a group role and optional label when …
afercia Jan 14, 2025
373b0d8
Adjust tests.
afercia Jan 14, 2025
187e867
Adjust more tests.
afercia Jan 15, 2025
a26c125
Abstract logic to compute the common props.
afercia Feb 7, 2025
fcf3311
Update snapshot.
afercia Feb 7, 2025
cbf0929
Fix OptionGroup ARIA role.
afercia Feb 7, 2025
7599ff3
Rename.
afercia Feb 7, 2025
abfed4e
Adjust tests.
afercia Feb 7, 2025
142a586
Restore group ARIA role on OptionGroup.
afercia Feb 10, 2025
61e8228
Change Background color label to Overlay color.
afercia Feb 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ exports[`ColorPaletteControl matches the snapshot 1`] = `
class="components-circular-option-picker"
>
<div
aria-label="Custom color picker."
aria-label="Custom color picker"
id="components-circular-option-picker-0"
role="listbox"
>
Expand Down
2 changes: 2 additions & 0 deletions packages/block-library/src/cover/edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,8 @@ function CoverEdit( {
value={ overlayColor.color }
onChange={ onSetOverlayColor }
clearable={ false }
asButtons
aria-label={ __( 'Overlay color' ) }
/>
</div>
</CoverPlaceholder>
Expand Down
12 changes: 6 additions & 6 deletions packages/block-library/src/cover/test/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ async function setup( attributes, useCoreBlocks, customSettings ) {

async function createAndSelectBlock() {
await userEvent.click(
screen.getByRole( 'option', {
screen.getByRole( 'button', {
name: 'Black',
} )
);
Expand All @@ -72,7 +72,7 @@ describe( 'Cover block', () => {

test( 'can set overlay color using color picker on block placeholder', async () => {
const { container } = await setup();
const colorPicker = screen.getByRole( 'option', {
const colorPicker = screen.getByRole( 'button', {
name: 'Black',
} );
await userEvent.click( colorPicker );
Expand All @@ -96,7 +96,7 @@ describe( 'Cover block', () => {
await setup();

await userEvent.click(
screen.getByRole( 'option', {
screen.getByRole( 'button', {
name: 'Black',
} )
);
Expand Down Expand Up @@ -389,7 +389,7 @@ describe( 'Cover block', () => {
describe( 'isDark settings', () => {
test( 'should toggle is-light class if background changed from light to dark', async () => {
await setup();
const colorPicker = screen.getByRole( 'option', {
const colorPicker = screen.getByRole( 'button', {
name: 'White',
} );
await userEvent.click( colorPicker );
Expand All @@ -413,7 +413,7 @@ describe( 'Cover block', () => {
} );
test( 'should remove is-light class if overlay color is removed', async () => {
await setup();
const colorPicker = screen.getByRole( 'option', {
const colorPicker = screen.getByRole( 'button', {
name: 'White',
} );
await userEvent.click( colorPicker );
Expand All @@ -426,7 +426,7 @@ describe( 'Cover block', () => {
} )
);
await userEvent.click( screen.getByText( 'Overlay' ) );
// The default color is black, so clicking the black color option will remove the background color,
// The default color is black, so clicking the black color button will remove the background color,
// which should remove the isDark setting and assign the is-light class.
const popupColorPicker = screen.getByRole( 'option', {
name: 'White',
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/border-box-control/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ describe( 'BorderBoxControl', () => {
await waitFor( () =>
expect(
screen.getByRole( 'button', {
name: 'Custom color picker.',
name: 'Custom color picker',
} )
).toBeVisible()
);
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/border-control/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ describe( 'BorderControl', () => {

const customColorPicker = getButton( /Custom color picker/ );
const circularOptionPicker = screen.getByRole( 'listbox', {
name: 'Custom color picker.',
name: 'Custom color picker',
} );
const colorSwatchButtons =
within( circularOptionPicker ).getAllByRole( 'option' );
Expand Down
13 changes: 13 additions & 0 deletions packages/components/src/circular-option-picker/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,19 @@ Prevents keyboard interaction from wrapping around. Only used when `asButtons` i
- Required: No
- Default: `true`

### `aria-labelledby`: `string`

The ID reference list of one or more elements that label the wrapper element.

- Required: No

### `aria-label`: `string`

The label for the wrapper element. Not used if an 'aria-labelledby' is provided.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


- Required: No
- Default: `Custom color picker`

## Subcomponents

### `CircularOptionPicker.ButtonAction`
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/circular-option-picker/circular-option-picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ function ButtonsCircularOptionPicker(
);

return (
<div { ...additionalProps } id={ baseId }>
<div { ...additionalProps } role="group" id={ baseId }>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is OK. It should use the fallback label though, am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it — feel free to work on this improvement to OptionGroup in a follow-up PR and ping for reviews

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very minor, not sure it is worth it.

<CircularOptionPickerContext.Provider value={ contextValue }>
{ options }
{ children }
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/circular-option-picker/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@ export {
ButtonAction,
DropdownLinkAction,
} from './circular-option-picker-actions';
export { getComputeCircularOptionPickerCommonProps } from './utils';

export default CircularOptionPicker;
2 changes: 1 addition & 1 deletion packages/components/src/circular-option-picker/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ WithLoopingDisabled.parameters = {
docs: {
source: {
code: `<CircularOptionPicker
aria-label="${ WithLoopingDisabled.args[ 'aria-label' ] }"
'aria-label': 'Circular Option Picker',
loop={false}
options={<DefaultOptions />}
/>`,
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/circular-option-picker/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ describe( 'CircularOptionPicker', () => {

expect( screen.queryByRole( 'listbox' ) ).not.toBeInTheDocument();
expect( screen.queryByRole( 'option' ) ).not.toBeInTheDocument();
expect( screen.getByRole( 'group' ) ).toBeInTheDocument();
expect( screen.getByRole( 'button' ) ).toBeInTheDocument();
} );
} );
Expand Down
21 changes: 11 additions & 10 deletions packages/components/src/circular-option-picker/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,16 @@ type CommonCircularOptionPickerProps = {
* The child elements.
*/
children?: ReactNode;
/**
* The ID reference list of one or more elements that label the wrapper
* element.
*/
'aria-labelledby'?: string;
/**
* The label for the wrapper element. Defaults to 'Custom color picker'. Not
* used if an 'aria-labelledby' is provided.
*/
'aria-label'?: string;
};

type WithBaseId = {
Expand All @@ -59,16 +69,7 @@ type FullListboxCircularOptionPickerProps = CommonCircularOptionPickerProps & {
* @default true
*/
loop?: boolean;
} & (
| {
'aria-label': string;
'aria-labelledby'?: never;
}
| {
'aria-label'?: never;
'aria-labelledby': string;
}
);
};

export type ListboxCircularOptionPickerProps = WithBaseId &
Omit< FullListboxCircularOptionPickerProps, 'asButtons' >;
Expand Down
27 changes: 27 additions & 0 deletions packages/components/src/circular-option-picker/utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';

/**
* Computes the common props for the CircularOptionPicker.
*/
export function getComputeCircularOptionPickerCommonProps(
asButtons?: boolean,
loop?: boolean,
ariaLabel?: string,
ariaLabelledby?: string
) {
const metaProps = asButtons
? { asButtons: true }
: { asButtons: false, loop };

const labelProps = {
'aria-labelledby': ariaLabelledby,
'aria-label': ariaLabelledby
? undefined
: ariaLabel || __( 'Custom color picker' ),
};

return { metaProps, labelProps };
}
40 changes: 11 additions & 29 deletions packages/components/src/color-palette/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ import { useCallback, useMemo, useState, forwardRef } from '@wordpress/element';
*/
import Dropdown from '../dropdown';
import { ColorPicker } from '../color-picker';
import CircularOptionPicker from '../circular-option-picker';
import CircularOptionPicker, {
getComputeCircularOptionPickerCommonProps,
} from '../circular-option-picker';
import { VStack } from '../v-stack';
import { Truncate } from '../truncate';
import { ColorHeading } from './styles';
Expand Down Expand Up @@ -233,7 +235,7 @@ function UnforwardedColorPalette(
buttonLabelName,
displayValue
)
: __( 'Custom color picker.' );
: __( 'Custom color picker' );

const paletteCommonProps = {
clearColor,
Expand All @@ -251,33 +253,12 @@ function UnforwardedColorPalette(
</CircularOptionPicker.ButtonAction>
);

let metaProps:
| { asButtons: false; loop?: boolean; 'aria-label': string }
| { asButtons: false; loop?: boolean; 'aria-labelledby': string }
| { asButtons: true };

if ( asButtons ) {
metaProps = { asButtons: true };
} else {
const _metaProps: { asButtons: false; loop?: boolean } = {
asButtons: false,
loop,
};

if ( ariaLabel ) {
metaProps = { ..._metaProps, 'aria-label': ariaLabel };
} else if ( ariaLabelledby ) {
metaProps = {
..._metaProps,
'aria-labelledby': ariaLabelledby,
};
} else {
metaProps = {
..._metaProps,
'aria-label': __( 'Custom color picker.' ),
};
}
}
const { metaProps, labelProps } = getComputeCircularOptionPickerCommonProps(
asButtons,
loop,
ariaLabel,
ariaLabelledby
);

return (
<VStack spacing={ 3 } ref={ forwardedRef } { ...additionalProps }>
Expand Down Expand Up @@ -335,6 +316,7 @@ function UnforwardedColorPalette(
{ ( colors.length > 0 || actions ) && (
<CircularOptionPicker
{ ...metaProps }
{ ...labelProps }
actions={ actions }
options={
hasMultipleColorOrigins ? (
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/color-palette/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ describe( 'ColorPalette', () => {
expect( screen.queryByText( colorCode ) ).not.toBeInTheDocument();
expect(
screen.getByRole( 'button', {
name: /^Custom color picker.$/,
name: /^Custom color picker$/,
} )
).toBeInTheDocument();
} );
Expand Down
38 changes: 10 additions & 28 deletions packages/components/src/duotone-picker/duotone-picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ import { __, sprintf } from '@wordpress/i18n';
* Internal dependencies
*/
import ColorListPicker from './color-list-picker';
import CircularOptionPicker from '../circular-option-picker';
import CircularOptionPicker, {
getComputeCircularOptionPickerCommonProps,
} from '../circular-option-picker';
import { VStack } from '../v-stack';

import CustomDuotoneBar from './custom-duotone-bar';
Expand Down Expand Up @@ -127,33 +129,12 @@ function DuotonePicker( {
);
} );

let metaProps:
| { asButtons: false; loop?: boolean; 'aria-label': string }
| { asButtons: false; loop?: boolean; 'aria-labelledby': string }
| { asButtons: true };

if ( asButtons ) {
metaProps = { asButtons: true };
} else {
const _metaProps: { asButtons: false; loop?: boolean } = {
asButtons: false,
loop,
};

if ( ariaLabel ) {
metaProps = { ..._metaProps, 'aria-label': ariaLabel };
} else if ( ariaLabelledby ) {
metaProps = {
..._metaProps,
'aria-labelledby': ariaLabelledby,
};
} else {
metaProps = {
..._metaProps,
'aria-label': __( 'Custom color picker.' ),
};
}
}
const { metaProps, labelProps } = getComputeCircularOptionPickerCommonProps(
asButtons,
loop,
ariaLabel,
ariaLabelledby
);

const options = unsetable
? [ unsetOption, ...duotoneOptions ]
Expand All @@ -163,6 +144,7 @@ function DuotonePicker( {
<CircularOptionPicker
{ ...otherProps }
{ ...metaProps }
{ ...labelProps }
options={ options }
actions={
!! clearable && (
Expand Down
38 changes: 10 additions & 28 deletions packages/components/src/gradient-picker/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ import { useCallback, useMemo } from '@wordpress/element';
/**
* Internal dependencies
*/
import CircularOptionPicker from '../circular-option-picker';
import CircularOptionPicker, {
getComputeCircularOptionPickerCommonProps,
} from '../circular-option-picker';
import CustomGradientPicker from '../custom-gradient-picker';
import { VStack } from '../v-stack';
import { ColorHeading } from '../color-palette/styles';
Expand Down Expand Up @@ -128,37 +130,17 @@ 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 };

if ( asButtons ) {
metaProps = { asButtons: true };
} else {
const _metaProps: { asButtons: false; loop?: boolean } = {
asButtons: false,
loop,
};

if ( ariaLabel ) {
metaProps = { ..._metaProps, 'aria-label': ariaLabel };
} else if ( ariaLabelledby ) {
metaProps = {
..._metaProps,
'aria-labelledby': ariaLabelledby,
};
} else {
metaProps = {
..._metaProps,
'aria-label': __( 'Custom color picker.' ),
};
}
}
const { metaProps, labelProps } = getComputeCircularOptionPickerCommonProps(
asButtons,
loop,
ariaLabel,
ariaLabelledby
);

return (
<CircularOptionPicker
{ ...metaProps }
{ ...labelProps }
actions={ actions }
options={ options }
/>
Expand Down
Loading
Loading