fix(Checkbox): prevent click propagation from the input element #3435

Fabianopb · 2019-02-13T09:29:00Z

Initially the click was being fired from both label and input

Now it only comes from label

(Closes #3433)
(Closes #3440)

codecov-io · 2019-02-13T09:32:42Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3435      +/-   ##
==========================================
+ Coverage   99.89%   99.89%   +<.01%     
==========================================
  Files         172      172              
  Lines        2820     2830      +10     
==========================================
+ Hits         2817     2827      +10     
  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 5c7766f...96211aa. Read the comment docs.

layershifter · 2019-02-13T09:40:19Z

Actually we should check:

only one click was propagated with id on label
only one click was propagated with id on input
only one click was propagated without id on input
only one click was propagated without id on label

Fabianopb · 2019-02-13T09:54:38Z

I can check that later, but just to confirm, the id goes directly on the Checkbox, right? And then it's passed to the input and label within the component.

So if we're testing

mount(
  <div role='presentation' onClick={onClick}>
    <Checkbox id='foo' />
  </div>,
)

We could also test

mount(
  <div role='presentation' onClick={onClick}>
    <Checkbox />
  </div>,
)

Then it's all or nothing with the id, right?

layershifter · 2019-02-13T09:58:50Z

the id goes directly on the Checkbox, right?

Absolutely

const wrapperWithId = mount(
  <div role='presentation' onClick={onClick}>
    <Checkbox id='foo' />
  </div>,
)
const wrapper = mount(
  <div role='presentation' onClick={onClick}>
    <Checkbox />
  </div>,
)

wrapperWithId.find('input').click() // 1
wrapperWithId.find('label').click() // 2
wrapper.find('input').click() // 3
wrapper.find('label').click() // 4

Fabianopb · 2019-02-13T10:02:09Z

Oh that's right, we set the id in the Checkbox, but then we can simulate the click both on the input and on the label. I'll check that later!

…out id (Semantic-Org#3433)

Fabianopb · 2019-02-13T14:40:52Z

Tests updated @layershifter !
inputs don't propagate click because there's always a label on top of them, so the click event is always propagated from label.

layershifter · 2019-02-14T10:36:07Z

test/specs/modules/Checkbox/Checkbox-test.js

+          </div>,
+        )
+        const target = document.querySelector(`.ui.checkbox ${assertion.target}`)
+        domEvent.click(target)


-const target = document.querySelector(`.ui.checkbox ${assertion.target}`) -domEvent.click(target) +domEvent.click(`.ui.checkbox ${assertion.target}`) This should be a valid change 😺

Good to know!

layershifter · 2019-02-14T10:37:50Z

test/specs/modules/Checkbox/Checkbox-test.js

+        const target = document.querySelector(`.ui.checkbox ${assertion.target}`)
+        domEvent.click(target)
+        if (assertion.propagates) onClick.should.have.been.calledOnce()
+        else onClick.should.not.have.been.called()


-if (assertion.propagates) onClick.should.have.been.calledOnce() -else onClick.should.not.have.been.called() +onClick.should.have.callCount(Number(assertion.propagates))

We can test the call count directly, https://github.com/domenic/sinon-chai

layershifter · 2019-02-14T10:38:10Z

test/specs/modules/Checkbox/Checkbox-test.js

@@ -201,6 +201,50 @@ describe('Checkbox', () => {
    })
  })

+  describe('click propagation', () => {
+    const assertions = [


Suggested change

const assertions = [

const assertMatrix= [

Sure, I'll change that

layershifter · 2019-02-14T10:39:06Z

src/modules/Checkbox/Checkbox.js

@@ -258,6 +258,7 @@ export default class Checkbox extends Component {
          tabIndex={this.computeTabIndex()}
          type={type}
          value={value}
+          onClick={e => e.stopPropagation()}


Can we create a handleInputClick method and make a comment there with a link to the matching issue?

And sort props in alphabetic order ❤️

layershifter · 2019-02-14T10:40:30Z

@Fabianopb I made some comments, can you please address them? Nice job 👍

Fabianopb · 2019-02-14T11:20:28Z

Awesome @layershifter thanks for the comments, I'll try address them later today!

Fabianopb

@layershifter I just pushed the changes!

Fabianopb · 2019-02-14T18:41:01Z

src/modules/Checkbox/Checkbox.js

@@ -258,6 +258,7 @@ export default class Checkbox extends Component {
          tabIndex={this.computeTabIndex()}
          type={type}
          value={value}
+          onClick={e => e.stopPropagation()}


Fabianopb · 2019-02-14T18:44:25Z

test/specs/modules/Checkbox/Checkbox-test.js

@@ -201,6 +201,50 @@ describe('Checkbox', () => {
    })
  })

+  describe('click propagation', () => {
+    const assertions = [


Sure, I'll change that

Fabianopb · 2019-02-14T18:44:40Z

test/specs/modules/Checkbox/Checkbox-test.js

+          </div>,
+        )
+        const target = document.querySelector(`.ui.checkbox ${assertion.target}`)
+        domEvent.click(target)


Good to know!

Fabianopb · 2019-02-14T18:44:58Z

test/specs/modules/Checkbox/Checkbox-test.js

+        const target = document.querySelector(`.ui.checkbox ${assertion.target}`)
+        domEvent.click(target)
+        if (assertion.propagates) onClick.should.have.been.calledOnce()
+        else onClick.should.not.have.been.called()


…React into fix/checkbox-parent-click

layershifter · 2019-02-19T14:12:39Z

@Fabianopb I added changes that I picked from @levithomason branch 😸 Can you please help to test these changes? Did I miss something? I.e. double clicks, event propagation, right mouse click?

Fabianopb

@layershifter that looks good to me, just one detail I got from re-testing the behavior manually:

without "id" the parent listens for a click coming necessarily from the label
with "id" the parent listens for a click coming necessarily from the input

Is that ok/intended?

Fabianopb · 2019-02-19T17:30:56Z

test/specs/modules/Checkbox/Checkbox-test.js

+        events: {
+          label: ['mouseup', 'click'],
+          input: ['click'],
+        },
        target: 'label',


This target property doesn't seem to be used anymore, is that so?

Nice catch 👍

…React into fix/checkbox-parent-click

layershifter · 2019-02-21T17:54:45Z

Is that ok/intended?

For me it looks fine, I don't see a big issue. I've tested it again, works like a charm 💎

Thank you!

levithomason · 2019-02-23T23:18:42Z

@Fabianopb You've contributed here before and you've shown care and initiative for ensuring SUIR becomes better. We appreciate this and want to enable you to do more. I've added you as a collaborator. You're now able to manage issues and PRs as well as create branches here in the main repo. You no longer need to work from a fork. Merging to master is the only reserved permission.

We are very select about who we add and when, so know that we trust your abilities and your judgment. Please feel free to weigh in on any issues and PRs that you have an opinion about. You have a share of ownership and say here now.

Once you accept the invite (see your email), you might want to show the Semantic Org badge publicly on your profile. To do that, navigate to the "people" page and change the "private" dropdown to "public" for your user in the table. Here's a direct link to your user in the table.

/cc
@Semantic-Org/react-collaborators
@Semantic-Org/react-maintainers

Please welcome @Fabianopb to our ranks 😄 🎉

levithomason · 2019-03-13T07:53:13Z

Released in semantic-ui-react@0.86.0.

fix(Checkbox): Prevents click propagation from the input (Semantic-Or…

8b6c4a8

…g#3433)

layershifter added the ready for review label Feb 13, 2019

fix(Checkbox) Create tests for click on input and label with and with…

8f02615

…out id (Semantic-Org#3433)

layershifter reviewed Feb 14, 2019

View reviewed changes

layershifter changed the title ~~Checkbox: Prevents click propagation from the input~~ fix(Checkbox): prevent click propagation from the input element Feb 14, 2019

Fabianopb added 2 commits February 14, 2019 20:48

fix(Checkbox): Create handler for input click (Semantic-Org#3433)

eca6f52

fix(Checkbox): Improve tests readability (Semantic-Org#3433)

d73360b

Fabianopb commented Feb 14, 2019

View reviewed changes

layershifter added 4 commits February 19, 2019 13:30

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

52b8f47

…React into fix/checkbox-parent-click

updates

f4242e9

updates

5837fc3

improve coverage

58d8a93

Fabianopb commented Feb 19, 2019

View reviewed changes

layershifter added 2 commits February 21, 2019 15:47

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

a7d98a3

…React into fix/checkbox-parent-click

remove target

96211aa

layershifter merged commit aaee3b1 into Semantic-Org:master Feb 21, 2019

layershifter mentioned this pull request Feb 21, 2019

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

Closed

layershifter mentioned this pull request Mar 25, 2019

Checkbox: slider is not clickable in the entire area #3502

Closed

agriffis mentioned this pull request Jun 26, 2019

Checkbox: label only clickable when simple element #3683

Closed

layershifter mentioned this pull request Jul 7, 2019

test(Checkbox): remove incorrect event combinations #3691

Merged

carkod mentioned this pull request Sep 28, 2022

fix: toggle when id is passed #4385

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Checkbox): prevent click propagation from the input element #3435

fix(Checkbox): prevent click propagation from the input element #3435

Fabianopb commented Feb 13, 2019 •

edited by layershifter

Loading

codecov-io commented Feb 13, 2019 •

edited

Loading

layershifter commented Feb 13, 2019

Fabianopb commented Feb 13, 2019

layershifter commented Feb 13, 2019

Fabianopb commented Feb 13, 2019

Fabianopb commented Feb 13, 2019 •

edited

Loading

layershifter Feb 14, 2019

Fabianopb Feb 14, 2019

layershifter Feb 14, 2019

Fabianopb Feb 14, 2019

layershifter Feb 14, 2019

Fabianopb Feb 14, 2019

layershifter Feb 14, 2019

layershifter Feb 14, 2019

Fabianopb Feb 14, 2019

layershifter commented Feb 14, 2019

Fabianopb commented Feb 14, 2019

Fabianopb left a comment

Fabianopb Feb 14, 2019

Fabianopb Feb 14, 2019

Fabianopb Feb 14, 2019

Fabianopb Feb 14, 2019

layershifter commented Feb 19, 2019

Fabianopb left a comment

Fabianopb Feb 19, 2019

layershifter Feb 21, 2019

layershifter commented Feb 21, 2019

levithomason commented Feb 23, 2019

levithomason commented Mar 13, 2019

fix(Checkbox): prevent click propagation from the input element #3435

fix(Checkbox): prevent click propagation from the input element #3435

Conversation

Fabianopb commented Feb 13, 2019 • edited by layershifter Loading

codecov-io commented Feb 13, 2019 • edited Loading

Codecov Report

layershifter commented Feb 13, 2019

Fabianopb commented Feb 13, 2019

layershifter commented Feb 13, 2019

Fabianopb commented Feb 13, 2019

Fabianopb commented Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Feb 14, 2019

Fabianopb commented Feb 14, 2019

Fabianopb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Feb 19, 2019

Fabianopb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Feb 21, 2019

levithomason commented Feb 23, 2019

levithomason commented Mar 13, 2019

Fabianopb commented Feb 13, 2019 •

edited by layershifter

Loading

codecov-io commented Feb 13, 2019 •

edited

Loading

Fabianopb commented Feb 13, 2019 •

edited

Loading