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

Why is the value of a checkbox using useCheckbox '' instead of 'on'? #3519

Closed
snowystinger opened this issue Sep 15, 2022 Discussed in #3516 · 8 comments · Fixed by #3554
Closed

Why is the value of a checkbox using useCheckbox '' instead of 'on'? #3519

snowystinger opened this issue Sep 15, 2022 Discussed in #3516 · 8 comments · Fixed by #3554
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@snowystinger
Copy link
Member

Discussed in #3516

Originally posted by brandonpittman September 14, 2022
Ran into this today and was wondering why is the value of a checkbox using useCheckbox '' instead of 'on'?

I'm not passing it a value, but it's getting assigned to an empty string.

A checkbox's default behavior when it doesn't have an explicit value is to send 'on' when submitted in a form.

From MDN:

If the value attribute was omitted, the default value for the checkbox is on, so the submitted data in that case would be subscribe=on

Also, this example you shared creates a checkbox with a value attribute that has no value.

The useCheckbox examples in the docs also present with this problem.

So the server pulls '' out of the FormData. The expected behavior is <input type="checkbox" > shouldn't have a value attribute in the DOM.

In the following example, you'll see that useCheckbox applies an empty value attribute. The default checkbox doesn't have that. When this usually occurs is when you apply value={null || undefined}which I now believe useToggle is doing here.

We don't come up with a value for you because that usually needs to be a string that matches something on your server should you want to submit a form. So it's empty until you supply one. The value doesn't matter if you are submitting the data via js because you'd store the data in some js object.

If you expect the browser's default behavior and submit a form normally, this is a problem.

@LFDanLu LFDanLu added the bug Something isn't working label Sep 21, 2022
@LFDanLu LFDanLu moved this to ✏️ To Groom in RSP Component Milestones Sep 21, 2022
@LFDanLu LFDanLu added the good first issue Good for newcomers label Sep 21, 2022
@sonithch
Copy link

Hi, I would like to contribute here. Can I start working on this?

@snowystinger
Copy link
Member Author

Thanks for volunteering, go for it!

@sonithch
Copy link

@snowystinger I did confirm the issue is caused by the value=undefined returned by the useToggle

The idea is to return the value attribute from the useToggle if it's only passed via props to it, else do not return it.

So, instead of value, return the expression ...(Object.prototype.hasOwnProperty.call(props, 'value') && {value}) in useToggle which checks the if the value is passed in props and then returns it. This makes sure the checkbox value is on if the value is not passed.

Let me know if this is the right way to go.

@brandonpittman
Copy link
Contributor

...(Object.prototype.hasOwnProperty.call(props, 'value') && {value})

I was kinda thinking it would have to be something like this.

@sonithch
Copy link

sonithch commented Sep 25, 2022

yeah, I'm not sure if there is a better way of doing it.

@snowystinger
Copy link
Member Author

Hey! Thanks for starting to look into it, this is such a strange bug. Undefined should be omitted from the dom. With your observations, I found both that the React Team doesn't intend to fix this, and that MUI also works around this.
facebook/react#17590
mui/material-ui#28423

I think we can do it for all of useToggle, we don't need to special case it to checkboxes. I see this affects our React Spectrum Switch component as well https://react-spectrum.adobe.com/react-spectrum/Switch.html

@sonithch
Copy link

@snowystinger Hi, thanks for the references.

Passing null in the value attribute logs a warning in the console by default. So, we omit only the undefined value in useToggle in the same way as MUI even if it's passed intentionally?

Something like this
...(value !== undefined && {value})

Let me know if this is the right way, so that I can raise a PR for the changes in useToggle.

@brandonpittman
Copy link
Contributor

Since I originally reported this, I went ahead and created a PR that fixes it over here.

Repository owner moved this from ✏️ To Groom to ✅ Done in RSP Component Milestones Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
Archived in project
4 participants