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

Fix/cover placeholder color options keyboard accessibility #68662

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open

Fix/cover placeholder color options keyboard accessibility #68662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b2f40e5
Render the color options as buttons.
afercia Jan 13, 2025
e1515e0
Make circular option picker get a group role and optional label when …
afercia Jan 14, 2025
b99ccad
Adjust tests.
afercia Jan 14, 2025
24863a8
Adjust more tests.
afercia Jan 15, 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: 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={ __( 'Background 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/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 }>
<CircularOptionPickerContext.Provider value={ contextValue }>
{ options }
{ children }
Expand Down
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
22 changes: 11 additions & 11 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,7 +40,16 @@ type CommonCircularOptionPickerProps = {
* The child elements.
*/
children?: ReactNode;
};
} & (
| {
'aria-label': string;
'aria-labelledby'?: never;
}
| {
'aria-label'?: never;
'aria-labelledby': string;
}
);

type WithBaseId = {
baseId: string;
Expand All @@ -59,16 +68,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
21 changes: 19 additions & 2 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 @@ -254,10 +254,27 @@ function UnforwardedColorPalette(
let metaProps:
| { asButtons: false; loop?: boolean; 'aria-label': string }
| { asButtons: false; loop?: boolean; 'aria-labelledby': string }
| { asButtons: true };
| { 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.' ),
};
}
} else {
const _metaProps: { asButtons: false; loop?: boolean } = {
asButtons: false,
Expand Down
21 changes: 19 additions & 2 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 @@ -130,10 +130,27 @@ function DuotonePicker( {
let metaProps:
| { asButtons: false; loop?: boolean; 'aria-label': string }
| { asButtons: false; loop?: boolean; 'aria-labelledby': string }
| { asButtons: true };
| { 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.' ),
};
}
} else {
const _metaProps: { asButtons: false; loop?: boolean } = {
asButtons: false,
Expand Down
21 changes: 19 additions & 2 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 @@ -131,10 +131,27 @@ function Component( props: PickerProps< any > ) {
let metaProps:
| { asButtons: false; loop?: boolean; 'aria-label': string }
| { asButtons: false; loop?: boolean; 'aria-labelledby': string }
| { asButtons: true };
| { 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.' ),
};
}
} else {
const _metaProps: { asButtons: false; loop?: boolean } = {
asButtons: false,
Expand Down
10 changes: 5 additions & 5 deletions test/e2e/specs/editor/blocks/cover.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ test.describe( 'Cover', () => {
} );

// Locate the Black color swatch.
const blackColorSwatch = coverBlock.getByRole( 'option', {
const blackColorSwatch = coverBlock.getByRole( 'button', {
name: 'Black',
} );
await expect( blackColorSwatch ).toBeVisible();
Expand Down Expand Up @@ -105,7 +105,7 @@ test.describe( 'Cover', () => {
// Choose a color swatch to transform the placeholder block into
// a functioning block.
await coverBlock
.getByRole( 'option', {
.getByRole( 'button', {
name: 'Black',
} )
.click();
Expand All @@ -128,7 +128,7 @@ test.describe( 'Cover', () => {
name: 'Block: Cover',
} );
await coverBlock
.getByRole( 'option', {
.getByRole( 'button', {
name: 'Black',
} )
.click();
Expand Down Expand Up @@ -240,7 +240,7 @@ test.describe( 'Cover', () => {
// Choose a color swatch to transform the placeholder block into
// a functioning block.
await coverBlock
.getByRole( 'option', {
.getByRole( 'button', {
name: 'Black',
} )
.click();
Expand All @@ -266,7 +266,7 @@ test.describe( 'Cover', () => {
// Choose a color swatch to transform the placeholder block into
// a functioning block.
await secondCoverBlock
.getByRole( 'option', {
.getByRole( 'button', {
name: 'Black',
} )
.click();
Expand Down
6 changes: 3 additions & 3 deletions test/e2e/specs/editor/various/list-view.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,10 @@ test.describe( 'List View', () => {
// make the inner blocks appear.
await editor.canvas
.getByRole( 'document', { name: 'Block: Cover' } )
.getByRole( 'listbox', {
name: 'Custom color picker.',
.getByRole( 'group', {
name: 'Background color',
} )
.getByRole( 'option' )
.getByRole( 'button' )
.first()
.click();

Expand Down
Loading