fix(Checkbox): fix event handling with id prop

[ghost]

• onChange triggered twice when passing id attribute (Related to Checkbox3 in CodeSandbox reproduction)

Details: The Checkbox Component was rendering a label htmlFor attribute which will bind between
the label and the input tag when being clicked. The problem is that the click (change) handler
is NOT attached on the input tag but it's attached on the parent <ElementType ... /> component.

Below this ElementType we've both the input tag and the label tag (as element children), and once we click on the label
the ElementType element will get the click (change) event thanks to Event Bubbling in JavaScript,
which therefor will trigger the checkbox state change, but if htmlFor={id} is supplied then clicking on the
label tag will trigger the input tag and the ElementType, and input tag click will be bubbled up again
into ElementType which will make it trigger the onChange event twice.

Since we've the correct behavior of the label being linked to the input because of attaching the event on
the parent element then the usage of htmlFor is buggy and already implemented.

[ghost]

Related to: #2370

[codecov-io]

Codecov Report

Merging #2392 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2392      +/-   ##
==========================================
+ Coverage   99.73%   99.73%   +<.01%     
==========================================
  Files         152      152              
  Lines        2664     2667       +3     
==========================================
+ Hits         2657     2660       +3     
  Misses          7        7

Impacted Files	Coverage Δ
src/modules/Checkbox/Checkbox.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f2f3fc...ebd50a5. Read the comment docs.

[layershifter]

This PR reverts removes changes, but it's not a solution. Please see #2274 for content of id prop.

I'm sure that we will not merge this PR in current condition.

[ghost]

Hi @layershifter ,
Thanks for the response.

I don't understand what the htmlFor unless to link between label clicks to inputs? The situation you have which the click in even attached to the input parent make the htmlFor useless.

Can you explain to me why htmlFor should be added as a prop in this situation? The goal of htmlFor is to do one thing and your implementation already doing it correctly

[layershifter]

Linked issue has a description, we need id and htmlFor for accessibility purposes. SO answer has more details about this.

In fact, we need a different event handlers for container element and input.

[layershifter]

@levithomason I've pushed some changes there. Can you you take attention to them?

[ghost]

@layershifter Since you're attaching a 2 event handlers, why not keep just one handler at the parent (as it was before) but to extend it using event.target to identify where the click event came from?

Also, binding the event on just the input (not the parent container) may solve the issue since the click on the label will also trigger the input thanks to "for" html attribute

[layershifter]

why not keep just one handler at the parent

There is different behaviour of label and input in cases when we have id, I also don't like multiple event handlers, possible @levithomason has a better idea.

[levithomason]

For clarification, the input is never directly clicked by the user as it is hidden. The box next to the label is actuall a psuedo label::before element. That said, the htmlFor attribute is passing the click event along to the hidden input when there is an id present. Thus, we're getting two clicks when there is an id.

The other event coming through here is due to the onChange prop on the root element. We actually don't need that at all. Both clicks and spacebar will trigger the onClick prop. Our click handler already calls back props for onClick and onChange, since, the only way to change a checkbox is to click it.

EDIT The solution previously posted was incorrect, will merge the PR as is. Thanks!

[levithomason]

Released in [email protected]

[NullVoxPopuli]

onClick seems to be called multiple times on the checkbox.
I'm using version 0.84.0