fix(Checkbox): prevent `onClick` from being called twice #3351

Fabianopb · 2018-12-24T14:33:02Z

Alright, I have to say that this was not trivial at all, but now the handlers seem to work for all cases. After plenty of investigation and trying different approaches, the solution was to leave the call of onClick to the click handler.

Here are gifs showing the component working for three cases (in all of them I'm checking and unchecking with clicks and then with the spacebar):

Simple uncontrolled component
Controlled component with setState as an object
Controlled component with setState as a function (maintaining the functionality implemented for Checkbox: doesn't toggle its state, when using only the keyboard #2587 fixed in fix(Checkbox): match DOM checkbox behavior #2748)

Cheers and Merry Xmas! 🎄

(Closes #3348)

#3348)

codecov-io · 2018-12-24T14:36:29Z

Codecov Report

Merging #3351 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3351      +/-   ##
==========================================
- Coverage   99.89%   99.89%   -0.01%     
==========================================
  Files         170      170              
  Lines        2812     2809       -3     
==========================================
- Hits         2809     2806       -3     
  Misses          3        3

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 cda2a2f...a5a76ef. Read the comment docs.

layershifter · 2018-12-24T15:32:56Z

Please check the DOM Comparison example.

Case 1 `mouse click`: FAILED

SUIR checkbox

Click      { checked: false }
Change     { checked: true }

DOM checkbox

DOM Change { checked: true }
DOM Click  { checked: true }

Case 2 `space key click`: PASSED

SUIR checkbox

Change     { checked: true }
Click      { checked: true }

DOM checkbox

DOM Change { checked: true }
DOM Click  { checked: true }

Case 3 `ID: mouse click on checkbox`: FAILED

SUIR checkbox with ID

Change     { checked: true }

DOM checkbox with ID

DOM Change { checked: true }
DOM Click  { checked: true }

Case 4 `ID: mouse click on label`: FAILED

SUIR checkbox with ID

Change     { checked: true }

DOM checkbox with ID

DOM Change { checked: true }
DOM Click  { checked: true }

@Fabianopb I briefly checked the changes, they shown that we need more strict tests there and changes to match DOM checkbox.

Something like this:

wrapper.find('label').simulate('click')

onClick.should.have.been.calledOnce()
onClick.should.have.been.calledWith(e, { checked: true })
onChange.should.have.been.calledOnce()
onChange.should.have.been.calledWith(e, { checked: true })

onChange.should.have.been.calledAfter(onClick)

https://github.com/domenic/sinon-chai

Fabianopb · 2018-12-24T16:04:20Z

Sorry @layershifter where can I find these DOM Comparison examples?

layershifter · 2018-12-24T16:06:12Z

They are in the bottom in Checkbox docs:
https://react.semantic-ui.com/modules/checkbox/#usage-dom-comparison
http://localhost:3000/modules/checkbox/#usage-dom-comparison

Fabianopb · 2018-12-24T16:08:51Z

Alright thanks, I'll take a look!

Fabianopb · 2018-12-24T16:53:10Z

@layershifter I've found a quite curious solution, which was to remove the onClick call completely from handleClick (in other words, completely removing the click handler from Checkbox).

By doing that, the interactions still work as I've shown in all those gifs and the DOM Comparisons are all valid (you can already check from my latest commit)!

However the test handles events transparently fails as it can't find onClick being fired from handleClick. As everything works as expected, should we treat this case as an exception?

Fabianopb · 2018-12-26T19:11:22Z

@layershifter I've created four tests for DOM Comparisons, simulating the change event on:

input without id
label without id
input with id
label with id

The change event is used because onClick is invoked within handleChange (as already done previously). By doing that we also cover spacebar key "click" as it fires the same events as a click in this component.

However, I had to disable the common test isConformant due to the empty handleClick handler.

My proposition is that we merge this PR because the bug is fixed and the DOM behavior is conformant, however, we should open a new issue to discuss and tackle the empty handler and disabled test (I could open that and start the discussion). What do you say? :)

layershifter · 2018-12-31T11:24:55Z

@Fabianopb sorry for delay, I will check it after the New Year 🎄 Have happy holidays!

Fabianopb · 2019-01-01T20:09:49Z

Thanks @layershifter and Happy New Year! 🎉

layershifter · 2019-01-03T15:39:25Z

@Fabianopb I updated tests to work with DOM deeply. It will be awesome if you will be able to take a look and check them 👍

…nction (#3348).

Fabianopb · 2019-01-03T16:35:02Z

It looks good to me @layershifter, nice set up with the assertMatrix and domEvent.fire!

I've added two more tests for onChange and onClick on controlled components with setState as function, which cover the functionality of this bug fix and it's also a kind of confirmation test for #2748. From me that should be it, I think this is ready to merge! :)

Fabianopb · 2019-01-03T18:15:04Z

src/modules/Checkbox/Checkbox.js

+    // The browser calls onClick AFTER onChange since onClick is on the root element.
+    // Because of this, we call onClick in handleChange to preserve the correct order.
+    // In that case, we skip calls to onClick here to avoid duplicate calls.
+    if (onChange) return


Wouldn't this cause issues in case the user has both onClick and onChange defined?

Digging a bit deeper, this change actually breaks the use of onClick like described in #3348 , I've updated the tests on controlled components to warn us accordingly. You will notice that if you revert this commit, the tests will pass.

Thanks for the double checking. I have a couple more updates I'll push, then I think we can get a final on this and merge it.

Thinking again, it's a bit misleading to have an empty handler like I had done before, I'll push a new commit removing it completely and adding the explanation close to the onClick invocation.

…e the real behavior (#3348).

Fabianopb · 2019-01-15T11:22:13Z

Some 10 days since the last discussions, just checking, what's the status? :)

layershifter · 2019-01-15T20:31:26Z

I can't promise anything, but I'm going to check this PR again tomorrow and merge it. I know that @levithomason worked on another solution (may be cleaner), but for now, it's better to fix the issue.

…React into checkbox-double-onclick

layershifter · 2019-01-16T19:23:33Z

Merged, @Fabianopb thanks for working on it 👍

Fabianopb · 2019-01-17T11:03:04Z

Awesome @layershifter thanks!

levithomason · 2019-01-18T02:01:41Z

test/specs/commonTests/isConformant.js

@@ -13,6 +13,7 @@ import hasValidTypings from './hasValidTypings'
 * Assert Component conforms to guidelines that are applicable to all components.
 * @param {React.Component|Function} Component A component that should conform.
 * @param {Object} [options={}]
+ * @param {String[]} [options.disabledHandlers=[]] An array of listeners that are disabled.


Heads up @layershifter we'll need to undo this sometime soon. Ignoring events means we aren't testing that all events work correctly for the end user. I don't think it is high priority, but something to keep on our tech debt list.

levithomason · 2019-02-04T17:44:13Z

Released in [email protected].

Fabianopb added 2 commits December 24, 2018 12:10

fix(Checkbox): Let click handler call onClick to avoid duplicate calls (

b7360c3

#3348)

fix(Checkbox): Fix test to call onClick from a click event (#3348)

4161c41

Fabianopb changed the title ~~Checkbox double onclick~~ Checkbox: Prevent onClick from being called twice Dec 24, 2018

Fabianopb mentioned this pull request Dec 24, 2018

Checkbox: double call of onClick when event fired by mouse #3348

Closed

layershifter requested a review from levithomason December 24, 2018 15:01

layershifter added the ready for review label Dec 24, 2018

layershifter changed the title ~~Checkbox: Prevent onClick from being called twice~~ fix(Checkbox): prevent onClick from being called twice Dec 24, 2018

layershifter added needs author feedback and removed ready for review labels Dec 24, 2018

fix(Checkbox): Move onClick call completely to handleChange (#3348).

2d88857

fix(Checkbox): Create tests for DOM Comparisons (#3348).

c9f0d1b

layershifter added ready for review and removed needs author feedback labels Jan 3, 2019

layershifter added 5 commits January 3, 2019 17:18

Merge branch 'master' into checkbox-double-onclick

7efe420

revert change

5919bfe

Update Checkbox-test.js

0ef94c1

Update Checkbox-test.js

4683ade

Update isConformant.js

a028ed7

Fabianopb added 2 commits January 3, 2019 14:29

fix(Checkbox): Fix typo in handleClick comment (#3348).

6cf3eb1

fix(Checkbox): Add tests for controlled component with setState as fu…

37be6cd

…nction (#3348).

fix(Checkbox): ensure onClick is called

91f259d

Fabianopb commented Jan 3, 2019

View reviewed changes

Fabianopb added 2 commits January 3, 2019 16:35

fix(Checkbox): Fire DOM event on controlled component tests to emulat…

6f80c21

…e the real behavior (#3348).

fix(Checkbox): Completely remove click handler (#3348).

9e5a7aa

layershifter added 2 commits January 16, 2019 20:08

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

f4aac04

…React into checkbox-double-onclick

small cleanup

a5a76ef

layershifter merged commit 9ca7bc3 into Semantic-Org:master Jan 16, 2019

levithomason reviewed Jan 18, 2019

View reviewed changes

DavidLozzi mentioned this pull request Feb 12, 2019

Checkbox: onClick fires parent onClick twice #3433

Closed

hasen011 mentioned this pull request Feb 14, 2019

Checkbox: no longer prevents default after updating to version 0.85.0 #3440

Closed

carkod mentioned this pull request Sep 27, 2022

Radio: onChange not triggered for radio toggle with id #3737

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Checkbox): prevent `onClick` from being called twice #3351

fix(Checkbox): prevent `onClick` from being called twice #3351

Fabianopb commented Dec 24, 2018

codecov-io commented Dec 24, 2018 •

edited

Loading

layershifter commented Dec 24, 2018 •

edited

Loading

Fabianopb commented Dec 24, 2018

layershifter commented Dec 24, 2018

Fabianopb commented Dec 24, 2018

Fabianopb commented Dec 24, 2018

Fabianopb commented Dec 26, 2018 •

edited

Loading

layershifter commented Dec 31, 2018

Fabianopb commented Jan 1, 2019

layershifter commented Jan 3, 2019

Fabianopb commented Jan 3, 2019

Fabianopb Jan 3, 2019

Fabianopb Jan 3, 2019

levithomason Jan 3, 2019

Fabianopb Jan 4, 2019

Fabianopb commented Jan 15, 2019

layershifter commented Jan 15, 2019

layershifter commented Jan 16, 2019

Fabianopb commented Jan 17, 2019

levithomason Jan 18, 2019

levithomason commented Feb 4, 2019

fix(Checkbox): prevent onClick from being called twice #3351

fix(Checkbox): prevent onClick from being called twice #3351

Conversation

Fabianopb commented Dec 24, 2018

codecov-io commented Dec 24, 2018 • edited Loading

Codecov Report

layershifter commented Dec 24, 2018 • edited Loading

Case 1 mouse click: FAILED

Case 2 space key click: PASSED

Case 3 ID: mouse click on checkbox: FAILED

Case 4 ID: mouse click on label: FAILED

Fabianopb commented Dec 24, 2018

layershifter commented Dec 24, 2018

Fabianopb commented Dec 24, 2018

Fabianopb commented Dec 24, 2018

Fabianopb commented Dec 26, 2018 • edited Loading

layershifter commented Dec 31, 2018

Fabianopb commented Jan 1, 2019

layershifter commented Jan 3, 2019

Fabianopb commented Jan 3, 2019

Fabianopb Jan 3, 2019

Choose a reason for hiding this comment

Fabianopb Jan 3, 2019

Choose a reason for hiding this comment

levithomason Jan 3, 2019

Choose a reason for hiding this comment

Fabianopb Jan 4, 2019

Choose a reason for hiding this comment

Fabianopb commented Jan 15, 2019

layershifter commented Jan 15, 2019

layershifter commented Jan 16, 2019

Fabianopb commented Jan 17, 2019

levithomason Jan 18, 2019

Choose a reason for hiding this comment

levithomason commented Feb 4, 2019

fix(Checkbox): prevent `onClick` from being called twice #3351

fix(Checkbox): prevent `onClick` from being called twice #3351

codecov-io commented Dec 24, 2018 •

edited

Loading

layershifter commented Dec 24, 2018 •

edited

Loading

Case 1 `mouse click`: FAILED

Case 2 `space key click`: PASSED

Case 3 `ID: mouse click on checkbox`: FAILED

Case 4 `ID: mouse click on label`: FAILED

Fabianopb commented Dec 26, 2018 •

edited

Loading