Add non-listbox functionality back to `CircularOptionPicker` #54290

andrewhayward · 2023-09-08T09:19:47Z

What?

This PR puts back the ability to use CircularOptionPicker as buttons, which was removed in #52255, while maintaining the new (now default) listbox behaviour. (Closes #54239.)

Why?

In #35292, CircularOptionPicker was refactored to present better semantics and keyboard navigation for picking options. However, as per #54239, it was discovered that CircularOptionPicker is not just used as an "option picker", but also as an access method for editing options. As such, we need to put the original functionality back in place, while maintaining the new functionality, so that the control can be used in either way.

How?

A new asButtons flag has been added to CircularOptionPicker, allowing component consumers to decide how it should be rendered, depending on context. This is passed on by ColorPalette, DuotonePicker and GradientPicker, the higher-order components that expose it.

Testing Instructions

Superficially, nothing should change, and CircularOptionPicker should continue to behave as before. Clicking each option should fire an onClick event, and in Storybook, the selected option should show as checked. Toggling asButtons shouldn't impact this behaviour.

Testing Instructions for Keyboard

With a keyboard, the default behaviour should also continue as before. The CircularOptionPicker control should still present as a single tab stop, and arrow keys should navigate between the different options. Semantically, it should still present as a listbox with multiple options, marked aria-selected as appropriate.

Enabling asButtons should change this, so that each option becomes its own tab stop. The listbox semantics should be removed, and each option should be a button, marked aria-pressed where needed.

Added some tests and a new story to go with the changes.

They all need to pass through the new `asButtons`/`disableLooping` prop following the `CircularOptionPicker` refactor.

alexstine · 2023-09-08T13:45:21Z

That's annoying that we have to maintain two versions.

andrewhayward · 2023-09-08T13:53:43Z

That's annoying that we have to maintain two versions.

Unfortunately, despite what CircularOptionPicker might be called, it was built in such a way that it could be used for purposes other than picking things. As such, it was used for purposes other than picking things. And here we are.

The joys of component libraries!

ciampo

Thank you for working on this (and adding unit tests too).

Left a few comments, let me know in case anything isn't clear.

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

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

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

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

ciampo · 2023-09-12T14:01:20Z

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

+			{ ...compositeState }
+			as={ Button }
+			className={ classnames( className, {
+				'is-pressed': isSelected,


Any reason why we're not passing isPressed as a prop here?

Doing so also sets aria-pressed on the control, which we don't want for role="option". I thought I'd tried to override that and failed, but clearly not hard enough. Definitely better to let Button worry about which classes to apply.

Sounds good. Maybe let's add an inline comment above the aria-pressed override explaining the reason for it.

Turns out I possibly had tried hard enough, because aria-pressed={ null } doesn't "work"...

Types of property ''aria-pressed'' are incompatible. Type 'null' is not assignable to type 'boolean | "true" | "false" | "mixed" | undefined'.

Setting it to null does properly override the value, and prevent aria-pressed from showing up as an attribute in the HTML, but TypeScript doesn't like it.

From what you say, it sounds like passing undefined would correctly remove the aria-pressed attribute from the button.

Maybe something like this would work?

{ ...( isSelected ? { 'aria-pressed': undefined } : {} ) }

Let's still add an inline comment in case.

Unfortunately passing undefined as a value for aria-pressed gets it ignored entirely. I've added a note to explain the class addition.

Note
Follow-up task: Update Button to check for role before setting aria-pressed, and set a more appropriate attribute (e.g. aria-selected) as needed.

Sounds good — could you open a new issue about it? You could work on it as a follow-up in the upcoming weeks.

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

ciampo · 2023-09-12T14:11:13Z

One more question:

This is passed on by ColorPalette, DuotonePicker and GradientPicker, the higher-order components that expose it.

Currently, do ColorPalette, DuotonePicker and GradientPicker all need to expose this prop? Do they all render in scenarios which required them to sometimes be a listbox, sometimes a list of separate buttons?

My instinct is usually to add the least amount of public APIs necessary.

andrewhayward · 2023-09-13T12:49:35Z

One more question:

This is passed on by ColorPalette, DuotonePicker and GradientPicker, the higher-order components that expose it.

Currently, do ColorPalette, DuotonePicker and GradientPicker all need to expose this prop? Do they all render in scenarios which required them to sometimes be a listbox, sometimes a list of separate buttons?

My instinct is usually to add the least amount of public APIs necessary.

ColorPalette and GradientPicker definitely do, in the theme style editor. I think DuotonePicker does too, in the same way, but that actually seems broken to me, having looked.

chad1008

I definitely feel like more of an observer on this PR but it looks good and tests well to me. Nothing new to add, but I do agree with @ciampo's comment on those two Omits.

This was fun to go through, thanks for all the work you've done on it!

andrewhayward · 2023-09-13T22:45:52Z

Definitely agreed with the Omit feedback. It's been a bit of a slog to refactor them out, but I think everything is happy now.

andrewhayward · 2023-09-13T23:14:10Z

I think DuotonePicker does too, in the same way, but that actually seems broken to me, having looked.

For whatever reason, DuotonePicker here is seemingly intentionally useless, with onChange={ noop }... not sure why!

gutenberg/packages/edit-site/src/components/global-styles/gradients-palette-panel.js

Lines 115 to 121 in fec5ffc

    
           <DuotonePicker 
        
           	duotonePalette={ duotonePalette } 
        
           	disableCustomDuotone={ true } 
        
           	disableCustomColors={ true } 
        
           	clearable={ false } 
        
           	onChange={ noop } 
        
           />

andrewhayward · 2023-09-14T09:05:21Z

Overrode the code sample for WithLoopingDisabled to make sure that loop={false} shows up. Would prefer to be able to override how loop is handled at the Story level so that the live sample works correctly too, but for now this will have to do.

ciampo

Almost there! Just a few minor comments left, but things are looking good.

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

ciampo · 2023-09-14T18:06:38Z

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

+			{ ...compositeState }
+			as={ Button }
+			className={ classnames( className, {
+				'is-pressed': isSelected,


From what you say, it sounds like passing undefined would correctly remove the aria-pressed attribute from the button.

Maybe something like this would work?

{ ...( isSelected ? { 'aria-pressed': undefined } : {} ) }

Let's still add an inline comment in case.

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

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

packages/components/src/composite/index.ts

ciampo

I believe something went wrong with the latest trunk merge.

Also, the last commit broke unit tests

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

… list

ciampo

🚀

ciampo · 2023-10-12T20:36:37Z

See #52255 for the dev note

Andrew Hayward added 2 commits September 8, 2023 09:34

Refactoring CircularOptionPicker

b976cb3

Added some tests and a new story to go with the changes.

Updating CircularOptionPicker consumers

a5ef383

They all need to pass through the new `asButtons`/`disableLooping` prop following the `CircularOptionPicker` refactor.

andrewhayward requested a review from ajitbohra as a code owner September 8, 2023 09:19

ciampo requested review from ciampo, chad1008 and brookewp September 8, 2023 10:30

ciampo assigned andrewhayward Sep 8, 2023

ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Sep 8, 2023

Andrew Hayward added 2 commits September 8, 2023 14:13

Merge branch 'trunk' into 54239/allow-circularoptionpicker-non-listbox

4d9c7df

Updating CHANGELOG.md

158207d

Fixing circular dependency

8de8875

ciampo reviewed Sep 12, 2023

View reviewed changes

Andrew Hayward added 9 commits September 12, 2023 16:43

Merge branch 'trunk' into 54239/allow-circularoptionpicker-non-listbox

bd811c1

Updating subcomponent filenames

d9cac8f

Simplifying selectedIconProps logic

954932b

Refactoring commonProps

e3ab6d2

Refactoring render functions into components

61c6762

Letting Button deal with classes

a65f483

Splitting state props into multiple effects

b446436

Reverting disableLooping prop to loop

33ef3a8

Adding note explaining aria-pressed={null}

77e38f7

Andrew Hayward added 2 commits September 13, 2023 15:41

Fixing some typing, removing any

96acbc5

Reverting back to manual is-pressed style

08dca39

chad1008 reviewed Sep 13, 2023

View reviewed changes

Refactoring Omit

eb916e0

Updating color-palette snapshot

5eb54b2

andrewhayward requested a review from ellatrix as a code owner September 13, 2023 23:07

Andrew Hayward added 2 commits September 14, 2023 00:46

Wrapping Option implementations with forwardRef

be7ce54

Overriding WithLoopingDisabled code sample to show loop

87cafaf

andrewhayward requested a review from ciampo September 14, 2023 08:36

ciampo reviewed Sep 14, 2023

View reviewed changes

Andrew Hayward added 3 commits September 15, 2023 10:57

Refactoring asButtons typing

0e4eb2a

Refactoring compositeState accessors

880a1a1

Updating various README docs with new props

3deef13

ciampo reviewed Sep 15, 2023

View reviewed changes

packages/components/src/circular-option-picker/types.ts Outdated Show resolved Hide resolved

Andrew Hayward added 4 commits September 15, 2023 16:01

Adding note to explain class additions

6fd9ba6

Refactoring listbox implementation to keep children out of option…

2f55f86

… list

Re-refactoring listbox implementation

a7b82e8

Removing redundant @default

87a3b2b

andrewhayward force-pushed the 54239/allow-circularoptionpicker-non-listbox branch from 3b1e00e to 87a3b2b Compare September 15, 2023 15:02

Merge branch 'trunk' into 54239/allow-circularoptionpicker-non-listbox

5007d78

ciampo approved these changes Sep 15, 2023

View reviewed changes

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

ciampo disabled auto-merge September 15, 2023 15:14

andrewhayward merged commit d2e3e29 into WordPress:trunk Sep 15, 2023

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

andrewhayward mentioned this pull request Oct 3, 2023

Update CircularOptionPicker consumers to use asButtons #55013

Open

3 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add non-listbox functionality back to `CircularOptionPicker` #54290

Add non-listbox functionality back to `CircularOptionPicker` #54290

andrewhayward commented Sep 8, 2023 •

edited

Loading

alexstine commented Sep 8, 2023

andrewhayward commented Sep 8, 2023

ciampo left a comment

ciampo Sep 12, 2023

andrewhayward Sep 13, 2023

ciampo Sep 13, 2023

andrewhayward Sep 13, 2023

ciampo Sep 14, 2023

andrewhayward Sep 15, 2023

ciampo Sep 15, 2023

ciampo commented Sep 12, 2023

andrewhayward commented Sep 13, 2023

chad1008 left a comment

andrewhayward commented Sep 13, 2023

andrewhayward commented Sep 13, 2023

andrewhayward commented Sep 14, 2023

ciampo left a comment

ciampo Sep 14, 2023

ciampo left a comment

ciampo left a comment

ciampo commented Oct 12, 2023

Add non-listbox functionality back to CircularOptionPicker #54290

Add non-listbox functionality back to CircularOptionPicker #54290

Conversation

andrewhayward commented Sep 8, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

alexstine commented Sep 8, 2023

andrewhayward commented Sep 8, 2023

ciampo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo commented Sep 12, 2023

andrewhayward commented Sep 13, 2023

chad1008 left a comment

Choose a reason for hiding this comment

andrewhayward commented Sep 13, 2023

andrewhayward commented Sep 13, 2023

andrewhayward commented Sep 14, 2023

ciampo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo left a comment

Choose a reason for hiding this comment

ciampo left a comment

Choose a reason for hiding this comment

ciampo commented Oct 12, 2023

Add non-listbox functionality back to `CircularOptionPicker` #54290

Add non-listbox functionality back to `CircularOptionPicker` #54290

andrewhayward commented Sep 8, 2023 •

edited

Loading