Making Circular Option Picker a `listbox` #52255

andrewhayward · 2023-07-03T16:07:33Z

What?

As per #35292, This patch modifies CircularOptionPicker to become a listbox, but using the Composite package. Additionally, it updates the various components that use CircularOptionPicker.

Why?

Currently, keyboard interaction with CircularOptionPickers is difficult, as each option presents as an individual tab stop. By changing the component to behave as a listbox, the entire control becomes a single tab stop, with individual colour options accessed using arrow keys.

How?

The CircularOptionPicker has been partially rebuilt using the various Composite elements, which reduces any complex behavioural additions on our part.

Testing Instructions

Nothing should visually change, as the underlying base components are still being used.

Testing Instructions for Keyboard

Every use of the CircularOptionPicker (ColorPalette, for example) should now present as a single tab stop, with options being picked using arrow keys.

Additional Notes

~~Initial values aren't currently picked up correctly.~~

✍️ Dev note

To improve CircularOptionPicker's semantics and keyboard navigation, the component has been tweaked to render and behave as a listbox by default. This change also causes the component to become a single tab stop, with the individual color options accessed using arrow keys.

In the (few) instances in which it makes sense for CircularOptionPicker to still render as a list of individual buttons, consumers of the component can use the asButtons prop to switch back to the legacy behavior.

Using the `Composite` component to make the Circular Option Picker present as a listbox.

github-actions · 2023-07-03T16:07:58Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @andrewhayward! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

mirka

Thank you for working on this big enhancement!

Based on my first-pass review, this approach looks viable to me. What do you think? If you agree, let's address the remaining issues and add some unit tests.

We also need to check for RTL support. I think we need to map @wordpress/i18n's isRTL() to the Composite store's rtl.

packages/components/src/circular-option-picker/types.ts

packages/components/src/circular-option-picker/index.tsx

packages/components/src/color-palette/index.tsx

mirka · 2023-07-04T12:33:53Z

packages/components/src/color-palette/index.tsx


 	return (
-		<CircularOptionPicker
+		<CircularOptionPicker.OptionGroup


I'm unsure about modeling the multiple palette case as groups in a single listbox. From reading the WAI guidance, I think a listbox is assumed to be one-dimensional (i.e. a list not a grid), with a aria-orientation to dictate which arrow keys to use.

What do you think about dropping the group construct and modeling them as separate listboxes?

What do you think about dropping the group construct and modeling them as separate listboxes?

After interacting with the ColorPalette storybook demo with multiple origins, I also got the feeling that having each OptionGroup as a separate tab stop feels better.

After further discussion, we've decided that we're going to keep the current behaviour, and iterate if/when necessary in a future PR.

mirka · 2023-07-04T12:36:34Z

packages/components/src/circular-option-picker/index.tsx

 function CircularOptionPicker( props: CircularOptionPickerProps ) {
-	const { actions, className, options, children } = props;
+	const { actions, className, options, children, loop = true } = props;
+	const compositeState = useCompositeState( { loop } );


I'm actually not up to date on this, but is there a reason why we're using useCompositeState instead of useCompositeStore?

We're still using reakit under the hood for this, which is where useCompositeState is coming from. As and when we move composite to ariakit we'll have to change that.

packages/components/src/circular-option-picker/index.tsx

mirka · 2023-07-04T15:18:12Z

packages/components/src/color-palette/stories/index.tsx

 	...args
 } ) => {
-	const [ color, setColor ] = useState< string | undefined >();
+	const [ color, setColor ] = useState< string | undefined >( value );


(Marking this as TODO so we don't forget to address it)

Out of curiosity, what exactly was the TODO item related to this line of code?

@andrewhayward would you happen to know what needed to be done around this line?

…ard_accessiblity--listbox

– Adding support for RTL - Adding support for preselected values - Adjusting how groups behave - Addressing some feedback

…iblity--listbox

Using `useEffect` to fix updates during render cycle

…iblity--listbox

ciampo

Great job on this one so far, @andrewhayward !

I gave a quick look at Storybook and at the code and left some comments.

I'll have a closer look at how the component behaves in the editor in the next review round

ciampo · 2023-08-31T12:59:57Z

packages/components/src/border-control/test/index.js

 };

+const selectColorOption = ( name ) => {
+	fireEvent.click( getColorOption( name ) );


Another idea for a follow-up PR: refactor BorderControl's unit tests from fireEvent to testing-library/user-event

See #54155.

ciampo · 2023-08-31T13:48:51Z

packages/components/src/color-palette/index.tsx


 	return (
-		<CircularOptionPicker
+		<CircularOptionPicker.OptionGroup


What do you think about dropping the group construct and modeling them as separate listboxes?

After interacting with the ColorPalette storybook demo with multiple origins, I also got the feeling that having each OptionGroup as a separate tab stop feels better.

packages/components/src/color-palette/index.tsx

ciampo · 2023-08-31T14:19:50Z

packages/components/src/color-palette/types.ts

+	/**
+	 * A label to identify the purpose of the control.
+	 *
+	 * @todo Either this or `aria-labelledby` should be required
+	 */
+	'aria-label'?: string;
+	/**
+	 * An ID of an element to provide a label for the control.
+	 *
+	 * @todo Either this or `aria-label` should be required
+	 */
+	'aria-labelledby'?: string;


If you want either prop a or prop b to be required, you should be able to do so by using the never keyword — see this playground example that I created to illustrate how to.

For the purposes of this PR, I wanted to leave both aria-label and aria-labelledby as optional props here, because of the number of components that use this and don't provide either; fixing all of those would dramatically increase the scope of the ticket. I created a follow-up ticket to track that work.

The desired mutual exclusivity could probably be clearer here though, and it may be worth adopting the never union syntax, even if they both remain optional...

export type ColorPaletteProps = Pick< PaletteProps, 'onChange' > & { ... } & ( | { 'aria-label'?: string; 'aria-labelledby'?: never; } | { 'aria-label'?: never; 'aria-labelledby'?: string; } );

Sounds good, we can do this as a follow-up

packages/components/src/duotone-picker/duotone-picker.tsx

packages/components/src/duotone-picker/types.ts

packages/components/src/gradient-picker/types.ts

packages/components/src/circular-option-picker/types.ts

packages/components/CHANGELOG.md

…iblity--listbox

ciampo

LGTM 🚀

Code changes look good and test well as per instructions.

…iblity--listbox

ciampo · 2023-09-04T14:42:44Z

Noticed #54156 while testing, @andrewhayward this is also another good follow-up task

ciampo · 2023-10-12T20:35:51Z

Added a dev note in the PR description

Making Circular Option Picker a listbox

9232f14

Using the `Composite` component to make the Circular Option Picker present as a listbox.

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jul 3, 2023

andrewhayward mentioned this pull request Jul 3, 2023

Color Palette: improve keyboard navigation #35292

Closed

3 tasks

mirka assigned andrewhayward Jul 3, 2023

mirka added the [Package] Components /packages/components label Jul 3, 2023

mirka self-requested a review July 3, 2023 16:32

mirka reviewed Jul 4, 2023

View reviewed changes

andrewhayward and others added 3 commits August 1, 2023 23:29

Merge branch 'WordPress:trunk' into 35292/improve_color_palette_keybo…

1f87fc5

…ard_accessiblity--listbox

Various updates

309e523

– Adding support for RTL - Adding support for preselected values - Adjusting how groups behave - Addressing some feedback

Merge branch 'trunk' into 35292/improve_color_palette_keyboard_access…

84ac262

…iblity--listbox

brookewp mentioned this pull request Aug 15, 2023

ColorPalette: refine test query #53704

Merged

mirka added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). and removed [Type] Enhancement A suggestion for improvement. labels Aug 18, 2023

Andrew Hayward added 12 commits August 21, 2023 15:04

Merge branch 'trunk' into 35290/improve_color_palette_keyboard_access…

37016b3

…iblity--listbox

Fixing tests for core/cover

72e8889

Fixing tests for BorderControl

d932389

Fixing tests for BorderControl

b331d7c

Adding default values for ARIA labels

a90e82f

Using `useEffect` to fix updates during render cycle

Fixing merge conflict

142b7c2

Removing redundant ColorPalette test

7336b69

Updating custom color tests

3692ccd

Removing redundant snapshots

fc000ee

Updating broken snapshot

17f8837

Merge branch 'trunk' into 35292/improve_color_palette_keyboard_access…

a61af94

…iblity--listbox

Updating CHANGELOG.md

5385c6e

andrewhayward marked this pull request as ready for review August 29, 2023 16:24

andrewhayward requested review from ajitbohra and ellatrix as code owners August 29, 2023 16:24

andrewhayward requested review from Mamaduka and kevin940726 as code owners August 29, 2023 19:25

Updating test for new keyboard navigation

718f81f

andrewhayward requested review from nerrad and ntwb as code owners August 29, 2023 19:54

andrewhayward requested a review from mirka August 29, 2023 19:55

Andrew Hayward added 4 commits August 30, 2023 00:09

Updating E2E tests

d20d985

Fixing navigation-colors E2E test

432af69

Fixing E2E tests for cover and heading blocks

65a74fb

Removing option constant

01693bb

andrewhayward mentioned this pull request Aug 30, 2023

Ensure appropriate labels are provided for color/gradient picker controls #54055

Open

3 tasks

ciampo reviewed Aug 31, 2023

View reviewed changes

Andrew Hayward added 5 commits September 1, 2023 14:22

Removing redundant type coercion

aaa8c45

Enforcing mutual exclusivity of -label/-labelledby

149c4da

Making label exclusivity clearer

8c22df2

Merge branch 'trunk' into 35292/improve_color_palette_keyboard_access…

78d88d0

…iblity--listbox

Updating CHANGELOG.md

e808582

andrewhayward requested a review from ciampo September 1, 2023 17:03

Fixing broken test

b15497f

ciampo approved these changes Sep 4, 2023

View reviewed changes

Merge branch 'trunk' into 35292/improve_color_palette_keyboard_access…

717469b

…iblity--listbox

ciampo enabled auto-merge (squash) September 4, 2023 14:15

ciampo merged commit 3434e37 into WordPress:trunk Sep 4, 2023

github-actions bot added this to the Gutenberg 16.7 milestone Sep 4, 2023

This was referenced Sep 7, 2023

Allow CircularOptionPicker to work as a non-listbox #54239

Closed

Add non-listbox functionality back to CircularOptionPicker #54290

Merged

ciampo added the has dev note when dev note is done (for upcoming WordPress release) label Oct 12, 2023

ciampo added the [Type] Enhancement A suggestion for improvement. label Oct 12, 2023

Making Circular Option Picker a listbox #52255

Making Circular Option Picker a listbox #52255

Uh oh!

Conversation

andrewhayward commented Jul 3, 2023 • edited by ciampo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Additional Notes

✍️ Dev note

Uh oh!

github-actions bot commented Jul 3, 2023

Uh oh!

mirka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ciampo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ciampo left a comment

Choose a reason for hiding this comment

Uh oh!

ciampo commented Sep 4, 2023

Uh oh!

ciampo commented Oct 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Making Circular Option Picker a `listbox` #52255

Making Circular Option Picker a `listbox` #52255

andrewhayward commented Jul 3, 2023 •

edited by ciampo

Loading