ToggleGroupControl: Don't autoselect option on first group focus

[mirka]

Fixes #62981

What?

Fixes a bug in ToggleGroupControl where the first Tab focus to a toggle group with an unselected option automatically selected the first option.

Why?

This isn't how a radio group should work. When controlled by keyboard, only an arrow key, space, or enter key should select an option when the radio group doesn't yet have an option selected.

Testing Instructions

See the ToggleGroupControl story in Storybook. It should work as expected via keyboard and via mouse. (The mechanics should basically mirror how a native radio group works — compare with an MDN example.)

Screenshots or screencast

CleanShot.2024-10-06.at.08.51.12.mp4

[mirka]

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

[ciampo]

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?

[mirka]

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.

[ciampo]

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

[mirka]

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.

[ciampo]

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

[mirka]

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).

[ciampo]

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

[github-actions]

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 <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: jeryj <[email protected]>

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

[ciampo]

I wonder if we should show a visual hint for focused but not selected option items — ie. what happens when moving focus to the ToggleGroupControl while there's no selected option item

[tyxla]

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

[mirka]

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)?

[ciampo]

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

[tyxla]

Thanks for confirming, folks 👍

[mirka]

I wonder if we should show a visual hint for focused but not selected option items — ie. what happens when moving focus to the ToggleGroupControl while there's no selected option item

We definitely should, but this is reliant on us aligning on a new design (#64439), which is going to take a while. As discussed, I'd like to fix the functional part of this bug right now and get it into 6.7 without waiting for new designs.

[mirka]

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).

[mirka]

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)?

[ciampo]

LGTM 🚀

There are a few rough edges (like the lack of focus styles for a focused but not active item), but as discussed above, they are not in scope for this PR. We will iterate in the future, likely when the new design spec is ready for development.

[github-actions]

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 e86152bc8b45d5fe213ed688f145e82f73bb2fd4
# 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.