fix: checkbox validation reset not working

[nwidynski]

Replaces #6079
Closes #5693

✅ Pull Request Checklist:

• Included link to corresponding React Spectrum GitHub Issue.
• Added/updated unit tests and storybook for this change (for new code or code which already has tests).
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
• Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

[nwidynski]

@LFDanLu Pipeline seems to be stuck, can someone restart? Otherwise i think this is good to go.

[nwidynski]

@LFDanLu I think what @devongovett was referencing here was using onPress to commit the validation.

Is there a specific reason for useToggle() not accepting onPress handlers? Alternatively, we could piggyback the state.toggle().

[LFDanLu]

@nwidynski I don't quite remember if there was a specific reason for not accepting onPress handlers, but I wanna say that it was mainly because a toggle didn't really feel like it needed to handle user specific press handling since users could hook into onChange instead. As a gut feeling, piggybacking off of state.toggle like you've done feels sound, but I'll need to poke around and test it some more. Thanks for the updates, will definite see about taking a deeper look soon!

[LFDanLu]

LGTM, verified the proper behavior on both the RAC/RSP docs and in our storybook examples. Thank you so much for coming back and picking this one back up!

[LFDanLu]

A coworker pointed out that this is mutating the state object and thus if the child that called useCheckbox is rerendering more than the state is being recreated then multiple state.toggles could be chained here. What do you think about just calling usePress in here and duplicating what useToggle does for labelProps? A bit gross that we'd be duplicating the code but maybe good enough for now until we decide whether or not useToggle should accept press handlers.

[nwidynski]

Good catch 👍

I don't think it matters as useToggleState isn't memoized anyways, but we can usePress here if you want to make sure. Alternatively, if you don't like a duplicated usePress, we could memoize instead.

state.toggle = chain(useCallback(state.toggle, [isSelected]), () => {
  ...
});

[snowystinger]

I still don't like mutating the state object, it breaks some assumptions and makes it harder to debug.
I'd prefer to just duplicate the code.

[LFDanLu]

Thanks for the updates!