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: Don't autoselect option on first group focus #65892

Merged
merged 3 commits into from
Oct 9, 2024
Merged

ToggleGroupControl: Don't autoselect option on first group focus #65892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f78454
ToggleGroupControl: Don't autoselect option on first group focus
mirka Oct 5, 2024
d554d1f
Fix test
mirka Oct 7, 2024
eeaf0f6
Add changelog
mirka Oct 7, 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 @@ -7,6 +7,7 @@
- `PaletteEdit`: dedupe palette element slugs ([#65772](https://github.com/WordPress/gutenberg/pull/65772)).
- `RangeControl`: do not tooltip contents to the DOM when not shown ([#65875](https://github.com/WordPress/gutenberg/pull/65875)).
- `Tabs`: fix skipping indication animation glitch ([#65878](https://github.com/WordPress/gutenberg/pull/65878)).
- `ToggleGroupControl`: Don't autoselect option on first group focus ([#65892](https://github.com/WordPress/gutenberg/pull/65892)).

## 28.9.0 (2024-10-03)

Expand Down
12 changes: 6 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 @@ -283,7 +283,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-11"
id="toggle-group-control-as-radio-group-12"
role="radiogroup"
>
<div
Expand All @@ -297,7 +297,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-11-30"
id="toggle-group-control-as-radio-group-12-32"
role="radio"
type="button"
value="uppercase"
Expand Down Expand Up @@ -330,7 +330,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-11-31"
id="toggle-group-control-as-radio-group-12-33"
role="radio"
type="button"
value="lowercase"
Expand Down Expand Up @@ -575,7 +575,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-10"
id="toggle-group-control-as-radio-group-11"
role="radiogroup"
>
<div
Expand All @@ -588,7 +588,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-10-28"
id="toggle-group-control-as-radio-group-11-30"
role="radio"
type="button"
value="rigas"
Expand All @@ -610,7 +610,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-10-29"
id="toggle-group-control-as-radio-group-11-31"
role="radio"
type="button"
value="jack"
Expand Down
13 changes: 13 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 @@ -162,6 +162,19 @@ describe.each( [
expect( mockOnChange ).toHaveBeenCalledWith( 'rigas' );
} );

it( 'should not set a value on focus', async () => {
render(
<Component label="Test Toggle Group Control">{ options }</Component>
);

const radio = screen.getByRole( 'radio', { name: 'R' } );
expect( radio ).not.toBeChecked();

await press.Tab();
expect( radio ).toHaveFocus();
expect( radio ).not.toBeChecked();
} );

it( 'should render tooltip where `showTooltip` === `true`', async () => {
render(
<Component label="Test Toggle Group Control">
Expand Down
26 changes: 11 additions & 15 deletions 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 @@ -75,7 +75,6 @@ function ToggleGroupControlOptionBase(
value,
children,
showTooltip = false,
onFocus: onFocusProp,
disabled,
...otherButtonProps
} = buttonProps;
Expand Down Expand Up @@ -132,7 +131,6 @@ function ToggleGroupControlOptionBase(
<button
{ ...commonProps }
disabled={ disabled }
onFocus={ onFocusProp }
aria-pressed={ isPressed }
type="button"
onClick={ buttonOnClick }
Expand All @@ -142,19 +140,17 @@ function ToggleGroupControlOptionBase(
) : (
<Ariakit.Radio
disabled={ disabled }
render={
<button
type="button"
{ ...commonProps }
onFocus={ ( event ) => {
onFocusProp?.( event );
if ( event.defaultPrevented ) {
return;
}
Comment on lines -151 to -153
Copy link
Member

Choose a reason for hiding this comment

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

We used to check if the event was defaultPrevented or not - is this something we want to keep, and if not, why?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't see why we would need it, especially now that we're using onFocusVisible instead of onFocus. @ciampo Do you remember why you initially prevented the default (#50278)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find a specific reason, I believe it was just to respect a consumer wanting to preventDefault. I don't think we need it, as you say, since we're not using onFocus anymore

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for confirming, folks 👍

toggleGroupControlContext.setValue( value );
} }
/>
}
onFocusVisible={ () => {
// Conditions ensure that the first visible focus to a radio group
// without a selected option will not automatically select the option.
if (
toggleGroupControlContext.value !== null ||
toggleGroupControlContext.activeItemIsNotFirstItem?.()
Copy link
Member Author

Choose a reason for hiding this comment

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

This activeItemIsNotFirstItem() check will only trigger once in the toggle group's lifecycle, as it will have a non-null value in subsequent renders.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the logic here — if the current value is null, why would we call setValue if the active item is not the first item?

Copy link
Member Author

Choose a reason for hiding this comment

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

The logic in trunk was that, whenever an option is focused (i.e. becomes the active item), set that value in the Ariakit store explicitly (because we're managing this component in controlled mode). We want to prevent this behavior when there is no value selected yet and the active item is the first item. Once the active item is the second item, that means the user not only "focused on the ToggleGroupControl" but also actively selected an option, so we are safe to start setting values. This is how native radios behave.

Hope that made better sense, but also I'd like your input on how to improve the code comment so it is easier to understand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the toggleGroupControlContext.activeItemIsNotFirstItem check necessary, though? Does it matter which item is the active item?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. See for example the default example for Ariakit Radio. When the focus first enters the radio group, the active item is the first item ("apple"). It's only when the active item becomes something else ("orange") that we want to start setting values.

CleanShot.2024-10-08.at.05.04.46.mp4

Once we start setting values (i.e. the value is non-null), then it no longer matters which is the active item.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. I wish we could rely on the built-in behavior of the ariakit Radio component, without the need for us to re-implement the logic (I guess we can't since we're not using native input type="radio" elements?)

Also, if we even want to tweak the initial position of the active element before a selection is made, this solution won't cut it - but it's unlikely, so that doesn't seem like a big issue

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I experimented a bit, and we can't rely on the built-in behavior if we use button. I think it is ultimately possible to rewrite it so we use input type="radio", but that would be a larger refactor with possibly breaking changes (not sure).

Copy link
Contributor

@ciampo ciampo Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good — we may want to consider a v2 with such changes when we have the new design spec

) {
toggleGroupControlContext.setValue( value );
}
} }
render={ <button type="button" { ...commonProps } /> }
value={ value }
>
<ButtonContentView>{ children }</ButtonContentView>
Expand Down
3 changes: 3 additions & 0 deletions packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ function UnforwardedToggleGroupControlAsRadioGroup(

const groupContextValue = useMemo(
(): ToggleGroupControlContextProps => ( {
activeItemIsNotFirstItem: () =>
radio.getState().activeId !== radio.first(),
baseId,
isBlock: ! isAdaptiveWidth,
size,
Expand All @@ -87,6 +89,7 @@ function UnforwardedToggleGroupControlAsRadioGroup(
[
baseId,
isAdaptiveWidth,
radio,
selectedValue,
setSelectedElement,
setValue,
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/toggle-group-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ export type ToggleGroupControlProps = Pick<
};

export type ToggleGroupControlContextProps = {
activeItemIsNotFirstItem?: () => boolean;
isDeselectable?: boolean;
baseId: string;
isBlock: ToggleGroupControlProps[ 'isBlock' ];
Expand Down
Loading