Closed apollo79 closed 7 months ago
This might be fixable by using the technique in the hidden-select-base
component here:
https://github.com/kobaltedev/kobalte/blob/2f5802af077f0bb66d507ee6f38163d33a1b4f84/packages/core/src/select/hidden-select-base.tsx#L90
Dispatching the events to the inputs of the Checkbox
and RadioGroupItem
should allow for the onChange
and onInput
events to be called to fix issues with form libraries.
But how is it that only clicking the indicator doesn't work and clicking the label does (and changes the state)? 🤔
Labels increase the hit area of the input and pass the click event onto the input which triggers the onChange. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label#:~:text=touches/taps%20a-,label,-%2C%20the%20browser%20passes
Ah yes, I forgot that they are connected that way. So in which part of the component should the dispatching happen? Also in the hidden inputs probably?
Most likely. I'm not too sure if there any other issues with doing this or not, but seems to be fine.
I tried adding something like this with pnpm patch
to the checkbox input component, but it doesn't seem to change the behavior, the indicator is still not clickable.
createEffect(on(() => context.checked(), () => {
setIsInternalChangeEvent(true);
ref?.dispatchEvent(new Event("input", { bubbles: true, cancelable: true }));
ref?.dispatchEvent(
new Event("change", { bubbles: true, cancelable: true }),
);
}, { defer: true }));
Just for clarification, do you mean the Checkbox.Indicator
or Checkbox.Input
? Indicator
is just a visual thing, it doesn't have any interactive abilities. The Input
is also by default 1px
in size.
If you meant the Checkbox.Control
, I'm not too sure why it wouldn't be clickable.
Anyways, I seem to have gotten it to work if you want to check and let me know if that is the expected behavior: https://stackblitz.com/edit/solidjs-templates-hnhkch?file=src%2FApp.tsx
Yeah, I meant the Control
… I confuse it with Indicator
sometimes.
Yes, your solution meets my expected behavior. So the solution is setting the checked
state on the Input
.
And for RadioGroup
i would do
<RadioGroup.ItemInput
checked={radioGroupValue() == valueOfThisItem}
onChange={(event) =>
setRadioGroupValue(event.currentTarget.value)
}
/>
Is it expected, that it doesn't work if it is set on the Root
?
Oh, no I think it doesn't meet the expected behavior.
It visually represents the right state now, but doesn't change it in modular-forms
when clicking on the Control
and the initial state isn't reflected.
true
, but the Indicator
is not shown while when I submit the form, the state is true
Label
and submit again, the visual state changes to true
and the internal state remains true
as it was beforeLabel
again, it changes visually and internally to false
Control
, it changes visually only and the internal state remains false
. Simplified example with only one checkbox, but otherwise the same: https://stackblitz.com/edit/solidjs-templates-sxdm3p?file=src%2FApp.tsx
I didn't realize that stackblitz wouldn't save the changes to node_modules
. I made changes to the checkbox-input
to make it work.
Here are the changes:
const [isInternalChangeEvent, setIsInternalChangeEvent] = createSignal(false);
const onChange = (e) => {
callHandler(e, local.onChange);
e.stopPropagation();
if (!isInternalChangeEvent()) {
const target = e.target;
context.setIsChecked(target.checked);
// Unlike in React, inputs `checked` state can be out of sync with our toggle state.
// for example a readonly `<input type="checkbox" />` is always "checkable".
//
// Also, even if an input is controlled (ex: `<input type="checkbox" checked={isChecked} />`,
// clicking on the input will change its internal `checked` state.
//
// To prevent this, we need to force the input `checked` state to be in sync with the toggle state.
target.checked = context.checked();
}
setIsInternalChangeEvent(false);
};
createEffect(
on(
[() => context.checked(), () => context.value()],
() => {
setIsInternalChangeEvent(true);
ref?.dispatchEvent(
new Event('input', { bubbles: true, cancelable: true })
);
ref?.dispatchEvent(
new Event('change', { bubbles: true, cancelable: true })
);
},
{
defer: true,
}
)
);
For the initial state value, you can set defaultChecked
on the Root
.
If this works, I can make a PR for the changes and test it on RadioGroup
as well.
I tried this solution locally and on stackblitz, but the behavior is still the same. Clicking on the Control
now changes the state visually, but not for modular-forms
while clicking the Label
does both. Is that not the same for you?
It does work on my end. Here's a new stackblitz with the applied changes to check
https://stackblitz.com/github/dev-rb/kobalte-checkbox-input-fix?file=src%2FApp.tsx
Oh awesome, that works! I have no idea what I did wrong, but that's amazing.
I would love if you could make a PR for that and fix it for RadioGroup
too.
Awesome, I updated the stackblitz with the RadioGroup
fix.
Going to work on that PR now 😄
Thank you so much for the fast fix!!
@dev-rb Sorry to bother again, but I just realized, that controlling the value of a radiogroup / the checked
state of a checkbox (e.g. using an array of checkboxes) still breaks the somehow.
https://stackblitz.com/edit/github-napdmm?file=src%2FApp.tsx
In this repro, the state is value
of the radiogroup is set to what we get from modular-forms, so we can set the value with its setValue
function. That works perfectly fine, but again, clicking on the control part of a radiogroup item does not change the state.
Maybe I am just not seeing something important here ... if so, I apologize in advance.
No worries @apollo79.
If you look at the stackblitz I sent previously, I used defaultValue/defaultChecked
on the root of RadioGroup/Checkbox instead of using value
. It should help with fixing your issues.
Let me know if that works or not.
Here's a link to a fork of your stackblitz with the changes made https://stackblitz.com/edit/github-napdmm-k8yh3k?file=src%2FApp.tsx
So it isn't possible to have a controlled radiogroup / checkbox?
Because using defaultValue
of course only sets the default value, but does not reflect possible changes.
So using setValue
from modular-forms does for example not work anymore. In the example I sent, I am not able to change the value manually, but in the example with defaultValue
it is not possible to change the value programatically, so the code I placed in the onMount
does not affect the state.
I can explore some more options later today and get back to you. There might be something I missed with the original solution.
Could you just make use of setValue
from modular forms in the onChange
that is on the RadioGroup
Root component?
Here's what I mean: https://stackblitz.com/edit/github-napdmm-tk3mcg?file=src%2FApp.tsx
When you provide a value
, it becomes controlled by you and changes are passed to the onChange
function. You can't provide a value
prop and have it be controlled by Kobalte (uncontrolled).
Or is there something else?
Oh, okay, I thought, this would be somehow handled by passing the onChange
to the RadioGroup.ItemInput
, which connects to modular-forms, and sync somehow. I have to wrap my head around that :-)
Okay... this makes it kinda hard to abstract then, but it is doable I guess.
Thanks again!
Thank you for your job, it works for radio button but still doesn't for checkboxes. So, If I click on checkboxes label it works fine but it fails if I click on control(if I use it within modular forms Field component . So the problem is in place I suppose. Please look for my implementation
@Tur8008 It seems like you are using the Checkbox incorrectly in your example. The onChange
prop for Checkbox
passes in a boolean it's parameter not a string value.
Here is the updated example of your implementation. Take a look at the setValue
prop you are passing to the Checkbox
component in the Home.jsx file.
https://stackblitz.com/edit/solidjs-templates-puxgnu?file=src%2Fviews%2Fhome%2FHome.jsx
@dev-rb Thank you for the aswear! Modular form's component gets the value and it can be submitted now but there are the problems:
[1, 3, 4]
where number are checkbox values -- in my cases weekday numbers.@Tur8008 Please check my stackblitz now. I have updated it to handle check and uncheck and multiple checkboxes.
@dev-rb yeah it work's, great! The only nasty thing left is duplication of values within array. Look at the screenshoot
@Tur8008 This does seem like a bug that I'll have to fix. The duplication is happening because of the recent change I made to address the above issue that @apollo79
brought up. The CheckboxInput
is also setting the field value using the modular-forms Field
's onInput function from props.
Current solution that fixes the duplication issue, but may result in other issues, is to remove checked
and setValue
. Meaning just let the CheckboxInput
set the values. One of the issues is this: https://github.com/kobaltedev/kobalte/issues/291#issuecomment-1893862295
I'll look into how we can fix this.
Another solution is to not pass onInput
or onChange
to CheckboxInput
if you want to have it be controlled. In your case, you could add empty onInput
and onChange
functions to override the ones passed in by Field
props.
@dev-rb Ok, thank you! You do run the brilliant lib! I enjoy using it! Looking forward to addressing this problem. But in the meantime I can just remove duplicating elements of array before submitting form to a server.
Another solution is to not pass
onInput
oronChange
toCheckboxInput
if you want to have it be controlled. In your case, you could add emptyonInput
andonChange
functions to override the ones passed in byField
props.
It works! I just added onInput={()=>null}
and it worked out. Good job, thanks a lot again!
I still don't fully understand why I have to do these workarounds when the other inputs (TextField
etc.) "just work". Could you explain that to me again @dev-rb ?
Sorry for the late reply @apollo79 . If you could make a reproduction in a stackblitz with TextField and Checkbox how you're using them, I can look into it some more. It could be possible that there may have been a different bug that I overlooked and I'll have to revisit this issue then.
As for why you need these workarounds, it's primarily the way you might be using it and how modular forms describes the integration. Either you control the component yourself and set the form values manually or you use modular-forms's method of letting the Input
sub-component make the changes without control.
Again, it could be something I overlooked that makes the functionality different from other components like TextField
. But I am happy to look into it some more.
No problem @dev-rb , I really appreciate the help!
Here you go: https://stackblitz.com/edit/solidjs-templates-oerhak?file=src%2FApp.tsx
So still the problem is for the checkbox to be controlled from kobalte and modular-forms at the same time. For a text-field it works, meaning I can input and have it controlled. When I use a checkbox though, I can have it controlled by modular-forms by setting checked={field.value}
, but then for some reason I can change the state, but only by clicking on the Checkbox.Control
, but not on the Checkbox.Input
. When I remove the checked
state, it works normally, but I can not set a state from modular-forms.
One thing I also tried out was adding the checked
state to the Checkbox.Input
instead of Checkbox.Root
. Now I can use the input to click on, but changing the state from modular-forms is not represented visually but recognized, meaning in the reproduction I sent the following will happen when you remove the checked
from rootProps
and add it to inputProps
in the Checkbox.tsx
file and reload the site and wait for a second: The state of the TextField
will change and the state of the Checkbox
also, but it is not visible, so when you click, it changes to false
and it is still visually unchecked. When you click again, then it will visually show the indicator and be checked.
So I am wondering why in the first case when using it with checked
normally I can't change the state by clicking on the Input
but by clicking on the Control
.
The state should be controlled by Kobalte, not Modular Forms nor by both (although this works in some cases, it's not optimal).
The best would be to call setValue(form, fieldName, value)
in the onChange of the checkbox/radio.
A better integration with Modular Forms is planned in the future.
So this is not considered a bug, right? I am just confused by the "clicking on one part works but the other one doesn't"-stuff.
Describe the bug When making a controlled checkbox, it works fine with this
but this breaks the ability to change the value of the checkbox by clicking on the
<Checkbox.Indicator />
The same accounts for `RadioGroup
This works
while this doesn't
This breaks especially compatibility with modular-forms (meaning the issue will almost always happen when using kobalte with modular-forms), which passes down event-handlers to be used with native input elements and events. Although I think it was mentioned that this will become different in the next months, I still think this is a bug that should be fixed.
To Reproduce Steps to reproduce the behavior:
Go to https://stackblitz.com/edit/solidjs-templates-f3yzu3?file=src%2FApp.tsx For both examples (
Checkbox
andRadioGroup
) the upper one works with clicking on the indicator, the one on the bottom doesn't.Expected behavior I expect it to work with clicking on the indicator in both examples.
Desktop (please complete the following information):