fix: checkbox validation reset not working

[nwidynski]

Closes https://github.com/adobe/react-spectrum/issues/5693

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

✅ Pull Request Checklist:

• [x] 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 https://github.com/adobe/react-spectrum/issues/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 https://github.com/adobe/react-spectrum/issues/5693#issuecomment-1901141961, "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 https://github.com/adobe/react-spectrum/issues/5693#issuecomment-1901141961 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