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

Use aria-describedBy to communicate border button style and color state #54469

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

Use aria-describedBy to communicate border button style and color state #54469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a5562bd
Use aria-describedBy to communicate border button style and color state
jeryj Sep 14, 2023
d5ce7dd
Use VisuallyHidden instead of display: none
jeryj Sep 15, 2023
b5173c2
Move description element outside of renderToggle
jeryj Sep 15, 2023
64dd3c8
Fix typscript errors
jeryj Sep 15, 2023
60bd713
Replace ToggleDescribedBy component with inlined
jeryj Sep 15, 2023
19574be
Revert back to display: none method for mounting aria-describedBy con…
jeryj Oct 13, 2023
32394a8
Update tests to reflect new aria-describedby namings
jeryj Oct 20, 2023
ddbf1f1
Remove whitespace.
jeryj Oct 20, 2023
9315c6a
test updates
draganescu Dec 1, 2023
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/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

## Unreleased

## 25.13.0 (2023-11-29)
- `BorderControlDropdown`: Switches from appending state to the aria-label and adds an aria-describedBy attribute with hidden text (via display: none;, as the text will be read via focus on the button and doesn't need to announced otherwise ([#54469](https://github.com/WordPress/gutenberg/pull/54469)).

### Enhancements

Expand Down
4 changes: 2 additions & 2 deletions 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 @@ -46,8 +46,8 @@ const props = {
value: undefined,
};

const toggleLabelRegex = /Border color( and style)* picker/;
const colorPickerRegex = /Border color picker/;
const toggleLabelRegex = /The currently selected color has/;
const colorPickerRegex = /The currently selected color is called/;

describe( 'BorderBoxControl', () => {
describe( 'Linked view rendering', () => {
Expand Down
78 changes: 48 additions & 30 deletions packages/components/src/border-control/border-control-dropdown/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type { CSSProperties } from 'react';
/**
* WordPress dependencies
*/
import { useInstanceId } from '@wordpress/compose';
import { __, sprintf } from '@wordpress/i18n';
import { closeSmall } from '@wordpress/icons';

Expand All @@ -24,13 +25,12 @@ import { contextConnect } from '../../context';
import { useBorderControlDropdown } from './hook';
import { StyledLabel } from '../../base-control/styles/base-control-styles';
import DropdownContentWrapper from '../../dropdown/dropdown-content-wrapper';

import type { ColorObject } from '../../color-palette/types';
import { isMultiplePaletteArray } from '../../color-palette/utils';
import type { DropdownProps as DropdownComponentProps } from '../../dropdown/types';
import type { ColorProps, DropdownProps } from '../types';

const getAriaLabelColorValue = ( colorValue: string ) => {
const getAriaDescriptionColorValue = ( colorValue: string ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function should be renamed to remove anything to do with aria. Small point but this seems to just be stripped var() from any CSS vars am I right? In which case let's

  1. Rename to better advertise it's purpose
  2. Consider extracting to a file and adding some quick unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree this could be made into a more generic utility function. Looks like it would be useful elsewhere too. Would you be OK with this also being a follow-up? It's starting to expand the purpose of this PR a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have a number of tasks that should happen as follow ups. Whilst I'm fine with them being follow ups I really think that whilst we're in the guts of this it would be good to leave the code in a better place than we found it if possible 🙏

// Leave hex values as-is. Remove the `var()` wrapper from CSS vars.
return colorValue.replace( /^var\((.+)\)$/, '$1' );
};
Expand Down Expand Up @@ -65,68 +65,77 @@ const getColorObject = (
return colors.find( ( color ) => color.color === colorValue );
};

const getToggleAriaLabel = (
const getToggleDescription = (
Copy link
Contributor

@getdave getdave Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think it would be good to extract this function and write some simple tests for the various conditionals. These are outlined in your test instructinos for the PR and it would provide a lot of confidence if they were encoded into tests:

  • No style, no color
  • Style, no color
  • Named color, no style
  • Named color, with style
  • Hex/rgba color, no style
  • Hex/rgba color, with style

I would also say it's worth refactoring it with the aim of removing the branches and conditionals but that could be done in a follow up and should not even be attempted unless we have tests in place 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this should all get refactored. As I was working within the context of what was here, I didn't want to change up things too much since I only wanted to provide a better description.

Copy link
Contributor

@getdave getdave Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It needs tidying up. I appreciate that's a big ask for this PR though. Feel free to add as a follow up task if you like.

I would help but I don't have bandwidth at this time 🙇

colorValue: CSSProperties[ 'borderColor' ],
colorObject: ColorObject | undefined,
Comment on lines 69 to 70
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this function need to know so much? Can it not simply accept a colorValue and the consumer can pass the appropriate value. This could reduce the number of confusing conditionals.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see it needs to get the name from the object....

Copy link
Contributor

Choose a reason for hiding this comment

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

In which case it could accept

  • colorValue
  • colorName

That way it would not need to know about the structure of a colorObject. It can then simply compute appropriate strings based on what you provide it.

Again let's not do this refactoring without tests 🙏

style: CSSProperties[ 'borderStyle' ],
isStyleEnabled: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this boolean signify? What is "style enabled"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm honestly not sure -- I only renamed it to feel more accurately named and used what I needed from it. I haven't looked into it further. I do agree a refactor would be nice, but I'd prefer it as a follow-up.

) => {
if ( isStyleEnabled ) {
if ( colorObject ) {
const ariaLabelValue = getAriaLabelColorValue( colorObject.color );
const ariaDescriptionValue = getAriaDescriptionColorValue(
colorObject.color
);
return style
? sprintf(
// translators: %1$s: The name of the color e.g. "vivid red". %2$s: The color's hex code e.g.: "#f00:". %3$s: The current border style selection e.g. "solid".
'Border color and style picker. The currently selected color is called "%1$s" and has a value of "%2$s". The currently selected style is "%3$s".',
'The currently selected color is called "%1$s" and has a value of "%2$s". The currently selected style is "%3$s".',
colorObject.name,
ariaLabelValue,
ariaDescriptionValue,
style
)
: sprintf(
// translators: %1$s: The name of the color e.g. "vivid red". %2$s: The color's hex code e.g.: "#f00:".
'Border color and style picker. The currently selected color is called "%1$s" and has a value of "%2$s".',
'The currently selected color is called "%1$s" and has a value of "%2$s".',
colorObject.name,
ariaLabelValue
ariaDescriptionValue
);
}

if ( colorValue ) {
const ariaLabelValue = getAriaLabelColorValue( colorValue );
const ariaDescriptionValue =
getAriaDescriptionColorValue( colorValue );
return style
? sprintf(
// translators: %1$s: The color's hex code e.g.: "#f00:". %2$s: The current border style selection e.g. "solid".
'Border color and style picker. The currently selected color has a value of "%1$s". The currently selected style is "%2$s".',
ariaLabelValue,
'The currently selected color has a value of "%1$s". The currently selected style is "%2$s".',
ariaDescriptionValue,
style
)
: sprintf(
// translators: %1$s: The color's hex code e.g: "#f00".
'Border color and style picker. The currently selected color has a value of "%1$s".',
ariaLabelValue
'The currently selected color has a value of "%1$s".',
ariaDescriptionValue
);
}

return __( 'Border color and style picker.' );
if ( style ) {
return sprintf(
// translators: %1$s: The current border style selection e.g. "solid".
'The currently selected style is "%1$s". No color is selected. Select a color to display the border.',
style
);
}
}

if ( colorObject ) {
return sprintf(
// translators: %1$s: The name of the color e.g. "vivid red". %2$s: The color's hex code e.g: "#f00".
'Border color picker. The currently selected color is called "%1$s" and has a value of "%2$s".',
'The currently selected color is called "%1$s" and has a value of "%2$s".',
colorObject.name,
getAriaLabelColorValue( colorObject.color )
getAriaDescriptionColorValue( colorObject.color )
);
}

if ( colorValue ) {
return sprintf(
// translators: %1$s: The color's hex code e.g: "#f00".
'Border color picker. The currently selected color has a value of "%1$s".',
getAriaLabelColorValue( colorValue )
'The currently selected color has a value of "%1$s".',
getAriaDescriptionColorValue( colorValue )
);
}

return __( 'Border color picker.' );
return __( 'Select a border color and style.' );
};

const BorderControlDropdown = (
Expand Down Expand Up @@ -155,8 +164,12 @@ const BorderControlDropdown = (

const { color, style } = border || {};
const colorObject = getColorObject( color, colors );
const toggleDescriptionId = useInstanceId(
BorderControlDropdown,
'border-control-dropdown'
);

const toggleAriaLabel = getToggleAriaLabel(
const toggleDescription = getToggleDescription(
color,
colorObject,
style,
Expand All @@ -174,10 +187,10 @@ const BorderControlDropdown = (
<Button
onClick={ onToggle }
variant="tertiary"
aria-label={ toggleAriaLabel }
tooltipPosition={ dropdownPosition }
label={ __( 'Border color and style picker' ) }
showTooltip={ true }
aria-describedby={ toggleDescriptionId }
>
<span className={ indicatorWrapperClassName }>
<ColorIndicator
Expand Down Expand Up @@ -243,15 +256,20 @@ const BorderControlDropdown = (
);

return (
<Dropdown
renderToggle={ renderToggle }
renderContent={ renderContent }
popoverProps={ {
...__unstablePopoverProps,
} }
{ ...otherProps }
ref={ forwardedRef }
/>
<>
<div id={ toggleDescriptionId } style={ { display: 'none' } }>
{ toggleDescription }
</div>
<Dropdown
renderToggle={ renderToggle }
renderContent={ renderContent }
popoverProps={ {
...__unstablePopoverProps,
} }
{ ...otherProps }
ref={ forwardedRef }
/>
</>
);
};

Expand Down
28 changes: 14 additions & 14 deletions 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 @@ -186,7 +186,7 @@ describe( 'BorderControl', () => {
render( <BorderControl { ...props } /> );

expect(
screen.getByLabelText( 'Border color and style picker.' )
screen.getByText( 'Select a border color and style.' )
).toBeInTheDocument();
} );

Expand All @@ -195,8 +195,8 @@ describe( 'BorderControl', () => {
render( <BorderControl { ...props } /> );

expect(
screen.getByLabelText(
'Border color and style picker. The currently selected color is called "Blue" and has a value of "#72aee6".'
screen.getByText(
'The currently selected color is called "Blue" and has a value of "#72aee6".'
)
).toBeInTheDocument();
} );
Expand All @@ -206,8 +206,8 @@ describe( 'BorderControl', () => {
render( <BorderControl { ...props } /> );

expect(
screen.getByLabelText(
'Border color and style picker. The currently selected color has a value of "#4b1d80".'
screen.getByText(
'The currently selected color has a value of "#4b1d80".'
)
).toBeInTheDocument();
} );
Expand All @@ -219,8 +219,8 @@ describe( 'BorderControl', () => {
render( <BorderControl { ...props } /> );

expect(
screen.getByLabelText(
'Border color and style picker. The currently selected color is called "Blue" and has a value of "#72aee6". The currently selected style is "dotted".'
screen.getByText(
'The currently selected color is called "Blue" and has a value of "#72aee6". The currently selected style is "dotted".'
)
).toBeInTheDocument();
} );
Expand All @@ -232,8 +232,8 @@ describe( 'BorderControl', () => {
render( <BorderControl { ...props } /> );

expect(
screen.getByLabelText(
'Border color and style picker. The currently selected color has a value of "#4b1d80". The currently selected style is "dashed".'
screen.getByText(
'The currently selected color has a value of "#4b1d80". The currently selected style is "dashed".'
)
).toBeInTheDocument();
} );
Expand All @@ -248,7 +248,7 @@ describe( 'BorderControl', () => {
render( <BorderControl { ...props } /> );

expect(
screen.getByLabelText( 'Border color picker.' )
screen.getByText( 'Select a border color and style.' )
).toBeInTheDocument();
} );

Expand All @@ -260,8 +260,8 @@ describe( 'BorderControl', () => {
render( <BorderControl { ...props } /> );

expect(
screen.getByLabelText(
'Border color picker. The currently selected color is called "Blue" and has a value of "#72aee6".'
screen.getByText(
'The currently selected color is called "Blue" and has a value of "#72aee6".'
)
).toBeInTheDocument();
} );
Expand All @@ -274,8 +274,8 @@ describe( 'BorderControl', () => {
render( <BorderControl { ...props } /> );

expect(
screen.getByLabelText(
'Border color picker. The currently selected color has a value of "#4b1d80".'
screen.getByText(
'The currently selected color has a value of "#4b1d80".'
)
).toBeInTheDocument();
} );
Expand Down
Loading