Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Checkbox Group Validation #5693

Open
dbani-dev-shell opened this issue Jan 13, 2024 · 9 comments · May be fixed by #6079
Open

Checkbox Group Validation #5693

dbani-dev-shell opened this issue Jan 13, 2024 · 9 comments · May be fixed by #6079
Labels
bug Something isn't working help wanted Extra attention is needed RAC

Comments

@dbani-dev-shell
Copy link

Provide a general summary of the issue here

It seems the Checkbox Group validation example does not revalidate after the checkbox is pressed.

To note: Fantastic work on react-aria-components, keep up the great work!

🤔 Expected Behavior?

Checkbox Group should clear validation when at least one checkbox is selected.

😯 Current Behavior

Checkbox Group does not clear validation.

💁 Possible Solution

No response

🔦 Context

No response

🖥️ Steps to Reproduce

React Aria's Documentation Example for Checkbox Group Validation

  1. Press Submit
  2. Press a Checkbox
  3. Validation is still displayed

Version

react-aria-components@1.0.0

What browsers are you seeing the problem on?

Chrome

If other, please specify.

No response

What operating system are you using?

MacOS

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

@LFDanLu
Copy link
Member

LFDanLu commented Jan 19, 2024

Thanks for filing this issue. I noticed this actually doesn't happen in our React Spectrum CheckboxGroup equivalent, seems to be due to our usage of a visually hidden input for the RAC Checkbox vs using an actual input. Typically onChange would fire here as a result of the checkbox state changing, updating the form's state but RAC's visually hidden input doesn't actually get pressed when you click on it hence no onChange firing. A potential solution would be to fire state.commitValidation here instead for RAC Checkbox group items, will need to test if this has any unforseen behavior changes though. cc @devongovett in case he has any further insight into this form behavior

@LFDanLu LFDanLu added bug Something isn't working RAC labels Jan 19, 2024
@LFDanLu LFDanLu added the help wanted Extra attention is needed label Feb 2, 2024
@smozely
Copy link

smozely commented Feb 19, 2024

#5836 appears to be a duplicate, but in that case isolating it to also happening on just Checkbox rather than a group. And that the behavior is different when using a mouse vs keyboard which is the same in the example

@nwidynski
Copy link

nwidynski commented Mar 18, 2024

@LFDanLu I took some time to take a look at this.

This issue seems pretty isolated to RAC's implementation of a visually hidden input, so can't we fix this inside RAC?
Maybe i am missing something related to how event dispatching is handled across different browsers/devices.

A slightly hacky, but working solution could look something like this:

/**
 * Setup a stable callback to propagate changes in the state.
 */
let onChange = props.onChange;

if (!groupState || groupState.isInvalid) {
  const event = new Event('change');
  const dispatch = () => inputRef.current?.dispatchEvent(event);

  onChange = chain(dispatch, onChange);
}

props.onChange = useEffectEvent(onChange);

@LFDanLu
Copy link
Member

LFDanLu commented Mar 18, 2024

@nwidynski While that may certainly fix it for RAC, my concern is that someone may build their own checkbox using our checkbox hooks instead of opting to use RAC and use a visually hidden input in a similar fashion like we suggest here. They would then run into original problem as well then and need to create a similar fix local to their implementation.

@nwidynski
Copy link

nwidynski commented Mar 18, 2024

@LFDanLu Got it.

Since a fix here won't do the trick for the isolated scenario, how about something like this in useCheckbox:

/**
 * Hook the label `onClick` handler to clear the validation state. 
 * This is useful when implementing `<Checkbox />` with a hidden input for styling.
 *
 * See: https://github.com/adobe/react-spectrum/issues/5693
 */
const { [privateValidationStateProp]: groupValidationState } = props;

const dispatch = () => {
  const { realtimeValidation, commitValidation } = groupValidationState
    ? groupValidationState
    : validationState;
  
  if (realtimeValidation.isInvalid) {
    commitValidation();
  }
};

labelProps.onClick = chain(dispatch, labelProps.onClick);

@LFDanLu
Copy link
Member

LFDanLu commented Mar 19, 2024

@nwidynski Yep, that seems promising! Do you want to open a PR for that change?

@aulonm
Copy link

aulonm commented Apr 26, 2024

Hi,

We've come across the same bug, but it seems that it works with tab + space to check the checkbox, but it does not work with mouse clicks. Would #6079 fix that as well?

@LFDanLu
Copy link
Member

LFDanLu commented May 2, 2024

Yes I believe so, is the behavior in these built docs essentially what you are looking for?

@aulonm
Copy link

aulonm commented May 3, 2024

Yes I believe so, is the behavior in these built docs essentially what you are looking for?

Yes! Both the Group Validation and the Individual Validation behaviour is what we have been trying to do, and the examples on the individual are spot on for our use case as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed RAC
Projects
Status: ✏️ To Groom
Development

Successfully merging a pull request may close this issue.

5 participants