fix: checkbox validation reset not working

[nwidynski]

Closes #5693

I've signed the CLA and looking forward for feedback!

✅ 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 Could you take a look why the Storybook 17 build is failing here?

[snowystinger]

GET_BUILD

[rspbot]

Build successful! 🎉

• View the storybook
• View the storybook-17
• View the storybook-16
• View the documentation

[rspbot]

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

[snowystinger]

Thanks for the PR!
Maybe I'm following the steps wrong, but I still see the validation displayed on the built docs from our CI in the comment above.
Were you able to verify it with the docs? or how did you verify?
Would you mind including tests if they aren't too hard? See https://react-spectrum.adobe.com/contribute.html#pull-requests

[sookmax]

I might be missing something, but is the if-statement below necessary?

// Reset validation state on label click for checkbox with a hidden input.
let dispatch = () => {
  // @ts-expect-error
  let {[privateValidationStateProp]: groupValidationState} = props;

  let {realtimeValidation, commitValidation} = groupValidationState
  ? groupValidationState
  : validationState;

-  if (realtimeValidation.isInvalid) {
    commitValidation();
-  }
};

I checked both of the docs examples

• http://localhost:1234/react-aria/CheckboxGroup.html#group-validation
• http://localhost:1234/react-aria/CheckboxGroup.html#individual-checkbox-validation

work properly when there's no if-condition above.

[nwidynski]

@sookmax This is required because the component will otherwise display a validation result obtained through the validate() prop in realtime. In any case this should be displayValidation.isInvalid instead, which fixes @snowystinger's issue with the isRequired scenario.

Speaking of validate(), i have also filed #6096 which takes effect on <Checkbox /> as well. There appears to be a more deeply rooted issue with the timings of commitValidation() which cause both the validate() prop and isRequired to be applied in realtime.

I don't know whether this should also be taken care of inside this PR?

[sookmax]

I might be wrong, but I thought realtime validation is what's desired?

Per #5693 (comment), "Thanks for filing this issue. I noticed this actually doesn't happen in our React Spectrum CheckboxGroup equivalent..."

And the "React Spectrum equivalent" does realtime validation. You can check from group-validation and individual-checkbox-validation

[nwidynski]

@sookmax It is unclear to me whether isRequired should display a validation error in realtime or not. This could either be a bug related to #6096 or intended behavior.

The docs specifically mention errors produced by the validate() prop to not be displayed in realtime. Just from a changeset POV it might be less impactful to align the docs of validate() with the current behavior to close #6096.

From how I understood #5693 (comment) and #5693 in general, just the reset of an actively displayed validation error should be in realtime, not its display in the first place. The latest change should accomplish this.

[LFDanLu]

At this point I'm not quite sure what the behavior should be either haha. My original understanding of the desired behavior for isRequired was that it should mimic native in how the error is displayed upon submission and then be cleared upon filling/toggling out the field/checkbox/etc (aka what @nwidynski stated above). However, I didn't realize that the React Spectrum CheckboxGroup will also show the error if you check -> uncheck a checkbox as @sookmax has discovered, even despite focus not leaving the CheckboxGroup itself.

IMO it should reset the validation error in realtime and re-validate on blur or submit for consistency with TextField/etc but I'm not entirely sure if there was prior history to warrant the current behavior of React Spectrum CheckboxGroup. Maybe simplest would be to align with the behavior React Spectrum CheckboxGroup has right now. I'll see if the team has any other opinions

[LFDanLu]

Discussed with the team, we actually want it to match with the React Spectrum version of CheckboxGroup aka it gets re-invalidated if the user deselects the checkbox when the checkbox is required. This is a bit of an exception to the general "native" validation that is mentioned in the docs.

That would mean you should be able just call validationState.commitValidation() here I believe. @devongovett I recall you mentioned something about the keyboard and mouse behavior not matching up as well but I forget the specifics, do you recall? If it was along the lines of having "Enter" properly commitValidation as well, then I think we'll need to pass that down to useToggle as well.

Additionally would you mind adding a test for this?

[nwidynski]

@LFDanLu Will surely do, I was waiting on general guidance where this should head.

Would you mind expanding on the reasoning why this behavior is desired in RAC? As you already recognized it will create inconsistencies with the docs as well as other RACs or custom components utilizing form validation. I would be interested in finding out more here as its relevant to our component library and I would like to make a call whether to patch this if not relevant to us.

Also do you have guidance on what to do with #6096 ? Its very closely related and will remain open if this is an exception for CheckboxGroup only.

[LFDanLu]

From the discussion with the team, it was mainly that an attempt was made to make CheckboxGroup's validation 100% consistent with the other components when validate was provided but it proved to be difficult hence the current behavior in RSP CheckboxGroup.

As for the "realtime" behavior, it follows how the native change event works hence why the textfield will only validate on blur but the checkbox/radiogroup validates on selection/deselection.