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

ToggleGroupControl: support disabled options #63450

Merged
merged 11 commits into from
Jul 16, 2024
Merged

ToggleGroupControl: support disabled options #63450

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
c01f777
Add disabled option tests
ciampo Jul 11, 2024
9546243
Fix handling of disabled prop on ToggleGroupControl options
ciampo Jul 11, 2024
ceebf66
Add basic disabled styles
ciampo Jul 11, 2024
a1cf315
Update snapshots
ciampo Jul 11, 2024
ea67533
CHANGELOG
ciampo Jul 11, 2024
62959f0
Support accessibleWhenDisabled for ratio option items
ciampo Jul 15, 2024
1d71066
Support accessibleWhenDisabled for button option items
ciampo Jul 15, 2024
8515e58
Update snapshots
ciampo Jul 15, 2024
0ee013e
Update tests to reflect that options are keyboard focusable while dis…
ciampo Jul 15, 2024
8172ca6
Move CHANGELOG entry to enhancements section
ciampo Jul 15, 2024
fe854cf
Revert "Support accessibleWhenDisabled for ratio option items"
ciampo Jul 16, 2024
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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

### Enhancements

- `ToggleGroupControl`: support disabled options ([#63450](https://github.com/WordPress/gutenberg/pull/63450)).
- `CustomSelectControl`: Stabilize `__experimentalShowSelectedHint` and `options[]. __experimentalHint` props ([#63248](https://github.com/WordPress/gutenberg/pull/63248)).

## 28.3.0 (2024-07-10)
Expand Down
42 changes: 36 additions & 6 deletions packages/components/src/toggle-group-control/test/__snapshots__/index.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@ exports[`ToggleGroupControl controlled should render correctly with icons 1`] =
border: 0;
}

.emotion-12[disabled] {
opacity: 0.4;
cursor: default;
}

.emotion-12:active {
background: #fff;
}
Expand Down Expand Up @@ -213,6 +218,11 @@ exports[`ToggleGroupControl controlled should render correctly with icons 1`] =
border: 0;
}

.emotion-18[disabled] {
opacity: 0.4;
cursor: default;
}

.emotion-18:active {
background: #fff;
}
Expand All @@ -238,7 +248,7 @@ exports[`ToggleGroupControl controlled should render correctly with icons 1`] =
class="components-toggle-group-control emotion-8 emotion-9"
data-wp-c16t="true"
data-wp-component="ToggleGroupControl"
id="toggle-group-control-as-radio-group-10"
id="toggle-group-control-as-radio-group-11"
role="radiogroup"
>
<div
Expand All @@ -252,7 +262,7 @@ exports[`ToggleGroupControl controlled should render correctly with icons 1`] =
data-value="uppercase"
data-wp-c16t="true"
data-wp-component="ToggleGroupControlOptionBase"
id="toggle-group-control-as-radio-group-10-24"
id="toggle-group-control-as-radio-group-11-30"
role="radio"
type="button"
value="uppercase"
Expand Down Expand Up @@ -291,7 +301,7 @@ exports[`ToggleGroupControl controlled should render correctly with icons 1`] =
data-value="lowercase"
data-wp-c16t="true"
data-wp-component="ToggleGroupControlOptionBase"
id="toggle-group-control-as-radio-group-10-25"
id="toggle-group-control-as-radio-group-11-31"
role="radio"
type="button"
value="lowercase"
Expand Down Expand Up @@ -452,6 +462,11 @@ exports[`ToggleGroupControl controlled should render correctly with text options
border: 0;
}

.emotion-12[disabled] {
opacity: 0.4;
cursor: default;
}

.emotion-12:active {
background: #fff;
}
Expand Down Expand Up @@ -486,7 +501,7 @@ exports[`ToggleGroupControl controlled should render correctly with text options
class="components-toggle-group-control emotion-8 emotion-9"
data-wp-c16t="true"
data-wp-component="ToggleGroupControl"
id="toggle-group-control-as-radio-group-9"
id="toggle-group-control-as-radio-group-10"
role="radiogroup"
>
<div
Expand All @@ -499,7 +514,7 @@ exports[`ToggleGroupControl controlled should render correctly with text options
data-value="rigas"
data-wp-c16t="true"
data-wp-component="ToggleGroupControlOptionBase"
id="toggle-group-control-as-radio-group-9-22"
id="toggle-group-control-as-radio-group-10-28"
role="radio"
type="button"
value="rigas"
Expand All @@ -521,7 +536,7 @@ exports[`ToggleGroupControl controlled should render correctly with text options
data-value="jack"
data-wp-c16t="true"
data-wp-component="ToggleGroupControlOptionBase"
id="toggle-group-control-as-radio-group-9-23"
id="toggle-group-control-as-radio-group-10-29"
role="radio"
type="button"
value="jack"
Expand Down Expand Up @@ -677,6 +692,11 @@ exports[`ToggleGroupControl uncontrolled should render correctly with icons 1`]
border: 0;
}

.emotion-12[disabled] {
opacity: 0.4;
cursor: default;
}

.emotion-12:active {
background: #fff;
}
Expand Down Expand Up @@ -758,6 +778,11 @@ exports[`ToggleGroupControl uncontrolled should render correctly with icons 1`]
border: 0;
}

.emotion-18[disabled] {
opacity: 0.4;
cursor: default;
}

.emotion-18:active {
background: #fff;
}
Expand Down Expand Up @@ -991,6 +1016,11 @@ exports[`ToggleGroupControl uncontrolled should render correctly with text optio
border: 0;
}

.emotion-12[disabled] {
opacity: 0.4;
cursor: default;
}

.emotion-12:active {
background: #fff;
}
Expand Down
114 changes: 114 additions & 0 deletions packages/components/src/toggle-group-control/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,13 @@ const optionsWithTooltip = (
/>
</>
);
const optionsWithDisabledOption = (
<>
<ToggleGroupControlOption value="pizza" label="Pizza" />
<ToggleGroupControlOption value="rice" label="Rice" disabled />
<ToggleGroupControlOption value="pasta" label="Pasta" />
Comment on lines +85 to +87
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ramonjd just making sure you're ok with those food options

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I was away last week.

I would have preferred risotto instead of just "rice", but I appreciate you asking. 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point. Feel free to open a PR

</>
);

describe.each( [
[ 'uncontrolled', ToggleGroupControl ],
Expand Down Expand Up @@ -351,6 +358,53 @@ describe.each( [

expect( expectedFocusTarget ).toHaveFocus();
} );

it( 'should ignore disabled radio options', async () => {
const mockOnChange = jest.fn();

render(
<Component
value="pizza"
onChange={ mockOnChange }
label="Test Toggle Group Control"
>
{ optionsWithDisabledOption }
</Component>
);

await sleep();
await press.Tab();

expect(
screen.getByRole( 'radio', { name: 'Pizza' } )
).toBeChecked();
expect(
screen.getByRole( 'radio', { name: 'Rice' } )
).toBeDisabled();
ciampo marked this conversation as resolved.
Show resolved Hide resolved

// Arrow navigation skips the disabled option
await press.ArrowRight();
expect(
screen.getByRole( 'radio', { name: 'Pasta' } )
).toBeChecked();
expect( mockOnChange ).toHaveBeenCalledTimes( 1 );
expect( mockOnChange ).toHaveBeenLastCalledWith( 'pasta' );

// Arrow navigation skips the disabled option
await press.ArrowLeft();
expect(
screen.getByRole( 'radio', { name: 'Pizza' } )
).toBeChecked();
expect( mockOnChange ).toHaveBeenCalledTimes( 2 );
expect( mockOnChange ).toHaveBeenLastCalledWith( 'pizza' );

// Clicks don't cause the option to be selected
await click( screen.getByRole( 'radio', { name: 'Rice' } ) );
expect(
screen.getByRole( 'radio', { name: 'Pizza' } )
).toBeChecked();
expect( mockOnChange ).toHaveBeenCalledTimes( 2 );
} );
} );

describe( 'isDeselectable = true', () => {
Expand Down Expand Up @@ -421,6 +475,66 @@ describe.each( [
} )
).toHaveFocus();
} );

it( 'should ignore disabled options', async () => {
const mockOnChange = jest.fn();

render(
<Component
value="pizza"
isDeselectable
onChange={ mockOnChange }
label="Test Toggle Group Control"
>
{ optionsWithDisabledOption }
</Component>
);

await sleep();
await press.Tab();

expect(
screen.getByRole( 'button', {
name: 'Pizza',
pressed: true,
} )
).toBeVisible();
expect(
screen.getByRole( 'button', {
name: 'Rice',
pressed: false,
} )
).toBeDisabled();
ciampo marked this conversation as resolved.
Show resolved Hide resolved

// Tab key navigation skips the disabled option
await press.Tab();
await press.Space();
expect(
screen.getByRole( 'button', {
name: 'Pasta',
pressed: true,
} )
).toHaveFocus();
expect( mockOnChange ).toHaveBeenCalledTimes( 1 );
expect( mockOnChange ).toHaveBeenLastCalledWith( 'pasta' );

// Tab key navigation skips the disabled option
await press.ShiftTab();
expect(
screen.getByRole( 'button', {
name: 'Pizza',
pressed: false,
} )
).toHaveFocus();

// Clicks don't cause the option to be selected.
await click(
screen.getByRole( 'button', {
name: 'Rice',
} )
);
expect( mockOnChange ).toHaveBeenCalledTimes( 1 );
} );
} );
} );
} );
7 changes: 6 additions & 1 deletion packages/components/src/toggle-group-control/toggle-group-control-option-base/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ function ToggleGroupControlOptionBase(
false
>,
// the element's id is generated internally
'id'
| 'id'
// due to how the component works, only the `disabled` prop should be used
| 'aria-disabled'
Comment on lines +56 to +57
Copy link
Contributor Author

@ciampo ciampo Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it makes sense to have the options accessibleWhenDisabled given how the component works cc @mirka

Copy link
Member

Choose a reason for hiding this comment

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

I actually thought this was one of the cases where we always want the option to be focusable when disabled, similar to ToolbarButton (#63102) or tabs in a tablist. It's also a composite widget where there will be a limited number of options, and the number of tab stops it potentially adds to the tab sequence is either zero or negligible.

Implementing that in the radio case for ToggleGroupControl would require some additional design work though 🤔 But at least Ariakit supports the behavior nicely (StackBlitz demo).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implementing that in the radio case for ToggleGroupControl would require some additional design work though

Yep. Currently, since disabled options are not really supported, the backdrop indicator also works as a focus indicator.
If we make the options accessibleWhenDisabled, then we'll need to add separate focus rings.

But at least Ariakit supports the behavior nicely (StackBlitz demo).

Yup. We'll probably need to add the custom logic when rendering individual buttons

>,
forwardedRef: ForwardedRef< any >
) {
Expand Down Expand Up @@ -82,6 +84,7 @@ function ToggleGroupControlOptionBase(
children,
showTooltip = false,
onFocus: onFocusProp,
disabled,
...otherButtonProps
} = buttonProps;

Expand Down Expand Up @@ -130,6 +133,7 @@ function ToggleGroupControlOptionBase(
{ isDeselectable ? (
<button
{ ...commonProps }
disabled={ disabled }
onFocus={ onFocusProp }
aria-pressed={ isPressed }
type="button"
Expand All @@ -139,6 +143,7 @@ function ToggleGroupControlOptionBase(
</button>
) : (
<Ariakit.Radio
disabled={ disabled }
render={
<button
type="button"
Expand Down
5 changes: 5 additions & 0 deletions packages/components/src/toggle-group-control/toggle-group-control-option-base/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ export const buttonView = ( {
border: 0;
}

&[disabled] {
opacity: 0.4;
cursor: default;
Comment on lines +68 to +69
Copy link
Member

Choose a reason for hiding this comment

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

We have more and more of these disabled styles that do opacity: 0.4. Perhaps this is a good opportunity to generalize at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, although this PR is not the right place for it. We should probably assess the current state of shared config in the package, and decide next steps. For example, some variables come from the base-styles package, some are internal to the components package. Some variables are defined in Sass, some as CSS custom properties, and others in a JS object.

Copy link
Member

Choose a reason for hiding this comment

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

Definitely should be a separate PR 👍

}

&:active {
background: ${ CONFIG.toggleGroupControlBackgroundColor };
}
Expand Down
Loading