ToggleGroupControl: Don't set value on focus after a reset #66151

mirka · 2024-10-16T00:03:50Z

What?

Fixes an incomplete bug fix in #65892 where ToggleGroupControl could still autoselect an option on focus after the selected value was reset by the consumer.

Testing Instructions for Keyboard

Modify the Storybook story so it has a reset button, like this:

diff --git a/packages/components/src/toggle-group-control/stories/index.story.tsx b/packages/components/src/toggle-group-control/stories/index.story.tsx
index 92f1e60762..01e5fc4eae 100644
--- a/packages/components/src/toggle-group-control/stories/index.story.tsx
+++ b/packages/components/src/toggle-group-control/stories/index.story.tsx
@@ -48,15 +48,18 @@ const Template: StoryFn< typeof ToggleGroupControl > = ( {
 		useState< ToggleGroupControlProps[ 'value' ] >();
 
 	return (
-		<ToggleGroupControl
-			__nextHasNoMarginBottom
-			{ ...props }
-			onChange={ ( ...changeArgs ) => {
-				setValue( ...changeArgs );
-				onChange?.( ...changeArgs );
-			} }
-			value={ value }
-		/>
+		<>
+			<ToggleGroupControl
+				__nextHasNoMarginBottom
+				{ ...props }
+				onChange={ ( ...changeArgs ) => {
+					setValue( ...changeArgs );
+					onChange?.( ...changeArgs );
+				} }
+				value={ value }
+			/>
+			<button onClick={ () => setValue( undefined ) }>Reset</button>
+		</>
 	);
 };

1. Select a value.
2. Reset the value using the reset button.
3. Using the keyboard, focus back onto the component. A value should not be selected automatically.

github-actions · 2024-10-16T00:04:09Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

mirka · 2024-10-16T00:09:14Z

packages/components/src/toggle-group-control/toggle-group-control-option-base/component.tsx

+								toggleGroupControlContext.value === null ||
+								toggleGroupControlContext.value === '';


Due to how we add a layer of value conversion in controlled mode, the Ariakit store will hold a null value on first render with no value provided, but then an empty string after a consumer resets the value to undefined.

mirka · 2024-10-16T00:10:01Z

packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx

+	// Ensures that the active id is also reset after the value is "reset" by the consumer.
+	useEffect( () => {
+		if ( selectedValue === '' ) {
+			radio.setActiveId( undefined );
+		}
+	}, [ radio, selectedValue ] );


I can feel the tech debt catching up to us 😅

Food for thought: I wonder if this can be improved with a centralized controlled component layer that all controlled components would use. Probably a too idealistic idea, since some components use Ariakit and others do not, and this layer would be quite complicated to cover all cases.

I think that the most practical fix here is to change the APIs to follow the standard convention for controlled / uncontrolled component. Since that would be a breaking change, we could just release a V2 of the component that would give us also a chance to:

implement / test the new design spec alongside the legacy one

review the current API strategy around "deselectability" and potentially multiple selection (should they be separate components? Or different subcomponents?)

For this case specifically, I think there might be room to argue that resetting the active id on empty-ish (or invalid) values should be handled by Ariakit out of the box. I'll try and follow up.

github-actions · 2024-10-16T00:32:19Z

Flaky tests detected in 1c8b689.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11356382384
📝 Reported issues:

[Flaky Test] Entering zoomed out mode zooms the canvas #66051 in /test/e2e/specs/site-editor/zoom-out.spec.js

tyxla

LGTM, thanks for the fix 👍 🚀

tyxla · 2024-10-16T06:50:43Z

packages/components/src/toggle-group-control/toggle-group-control-option-base/component.tsx

@@ -141,10 +141,14 @@ function ToggleGroupControlOptionBase(
 					<Ariakit.Radio
 						disabled={ disabled }
 						onFocusVisible={ () => {
+							const selectedValueIsNullish =


Naming nit: nullish often refers to null or undefined, so if someone isn't reading closely, they might be confused and expect this checks for undefined and not an empty string.

Maybe selectedValueIsEmpty ? Or even inlining the check?

I'd be fine with inlining it 👍

Good point, I'll go with selectedValueIsEmpty here. (It started as an inline condition, but it was hard to parse/understand so I prefer it named.)

tyxla · 2024-10-16T06:54:21Z

packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx

+	// Ensures that the active id is also reset after the value is "reset" by the consumer.
+	useEffect( () => {
+		if ( selectedValue === '' ) {
+			radio.setActiveId( undefined );
+		}
+	}, [ radio, selectedValue ] );


Food for thought: I wonder if this can be improved with a centralized controlled component layer that all controlled components would use. Probably a too idealistic idea, since some components use Ariakit and others do not, and this layer would be quite complicated to cover all cases.

tyxla · 2024-10-16T06:55:27Z

packages/components/src/toggle-group-control/test/index.tsx

@@ -175,6 +175,32 @@ describe.each( [
 		expect( radio ).not.toBeChecked();
 	} );

+	if ( mode === 'controlled' ) {


Should we move it next to the other controlled-only tests?

No strong opinion, but I think I prefer it here because it's closely related to the test right above it.

github-actions · 2024-10-16T16:27:48Z

There was a conflict while trying to cherry-pick the commit to the wp/6.7 branch. Please resolve the conflict manually and create a PR to the wp/6.7 branch.

PRs to wp/6.7 are similar to PRs to trunk, but you should base your PR on the wp/6.7 branch instead of trunk.

# Checkout the wp/6.7 branch instead of trunk.
git checkout wp/6.7
# Create a new branch for your PR.
git checkout -b my-branch
# Cherry-pick the commit.
git cherry-pick b9b633059bb448fa4281c43d8fa8d696ad6881cd
# Check which files have conflicts.
git status
# Resolve the conflict...
# Add the resolved files to the staging area.
git status
git add .
git cherry-pick --continue
# Push the branch to the repository
git push origin my-branch
# Create a PR and set the base to the wp/6.7 branch.
# See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request.

ciampo · 2024-10-16T21:58:22Z

@mirka just a ping in case you didn't notice the failed backport

* Add test * ToggleGroupControl: Don't set value on focus after a reset * Add changelog * Rename to `selectedValueIsEmpty` Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> # Conflicts: # packages/components/CHANGELOG.md

mirka · 2024-10-17T09:56:07Z

Backported at 4a7de72

…#66151) * Add test * ToggleGroupControl: Don't set value on focus after a reset * Add changelog * Rename to `selectedValueIsEmpty` Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org>

mirka added 2 commits October 16, 2024 08:51

Add test

9a85cd8

ToggleGroupControl: Don't set value on focus after a reset

65e2a9b

mirka added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 16, 2024

mirka self-assigned this Oct 16, 2024

mirka requested a review from ajitbohra as a code owner October 16, 2024 00:03

mirka commented Oct 16, 2024

View reviewed changes

Add changelog

1c8b689

mirka requested a review from a team October 16, 2024 00:11

mirka mentioned this pull request Oct 16, 2024

ImageSizeControls: Replace ButtonGroup with ToggleGroupControl #65386

Merged

tyxla approved these changes Oct 16, 2024

View reviewed changes

Rename to selectedValueIsEmpty

c550dec

mirka enabled auto-merge (squash) October 16, 2024 15:55

mirka merged commit b9b6330 into trunk Oct 16, 2024
65 of 66 checks passed

mirka deleted the fix/toggle-group-control-focus branch October 16, 2024 16:26

github-actions bot added this to the Gutenberg 19.5 milestone Oct 16, 2024

mirka added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 17, 2024

getdave mentioned this pull request Oct 21, 2024

Packages update for WordPress 6.7 RC1 WordPress/wordpress-develop#7600

Closed

sirreal mentioned this pull request Oct 25, 2024

test/pr automation workflow #66457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ToggleGroupControl: Don't set value on focus after a reset #66151

ToggleGroupControl: Don't set value on focus after a reset #66151

mirka commented Oct 16, 2024

github-actions bot commented Oct 16, 2024 •

edited

Loading

mirka Oct 16, 2024

mirka Oct 16, 2024

tyxla Oct 16, 2024

ciampo Oct 16, 2024 •

edited

Loading

mirka Oct 16, 2024

github-actions bot commented Oct 16, 2024

tyxla left a comment

tyxla Oct 16, 2024

ciampo Oct 16, 2024

tyxla Oct 16, 2024

mirka Oct 16, 2024

tyxla Oct 16, 2024

tyxla Oct 16, 2024

mirka Oct 16, 2024

github-actions bot commented Oct 16, 2024

ciampo commented Oct 16, 2024

mirka commented Oct 17, 2024

		toggleGroupControlContext.value === null \|\|
		toggleGroupControlContext.value === '';

ToggleGroupControl: Don't set value on focus after a reset #66151

ToggleGroupControl: Don't set value on focus after a reset #66151

Conversation

mirka commented Oct 16, 2024

What?

Testing Instructions for Keyboard

github-actions bot commented Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 16, 2024

tyxla 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

github-actions bot commented Oct 16, 2024

ciampo commented Oct 16, 2024

mirka commented Oct 17, 2024

github-actions bot commented Oct 16, 2024 •

edited

Loading

ciampo Oct 16, 2024 •

edited

Loading