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

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

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

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

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

* revert change

* Update Checkbox-test.js

* Update Checkbox-test.js

* Update isConformant.js

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

* fix(Checkbox): Add tests for controlled component with setState as function (#3348).

* fix(Checkbox): ensure onClick is called

* fix(Checkbox): Fire DOM event on controlled component tests to emulate the real behavior (#3348).

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

* small cleanup

Author: Fabianopb

src/modules/Checkbox/Checkbox.js

@@ -149,26 +149,22 @@ export default class Checkbox extends Component {
     if (!this.canToggle()) return
     if (fromMouseUp && !_.isNil(id)) return
 
+    // We don't have a separate click handler as it's already called in here,
+    // and also to avoid duplicate calls, matching all DOM Checkbox comparisons.
     _.invoke(this.props, 'onClick', e, {
       ...this.props,
       checked: !checked,
       indeterminate: !!indeterminate,
     })
-    _.invoke(this.props, 'onChange', e, { ...this.props, checked: !checked, indeterminate: false })
+    _.invoke(this.props, 'onChange', e, {
+      ...this.props,
+      checked: !checked,
+      indeterminate: false,
+    })
 
     this.trySetState({ checked: !checked, indeterminate: false })
   }
 
-  handleClick = (e) => {
-    // We handle onClick in onChange if it is provided, to preserve proper call order.
-    // Don't call onClick twice if their is already an onChange handler, it calls onClick.
-    // https://github.com/Semantic-Org/Semantic-UI-React/pull/2748
-    const { onChange, onClick } = this.props
-    if (onChange || !onClick) return
-
-    onClick(e, this.props)
-  }
-
   handleMouseDown = (e) => {
     debug('handleMouseDown()')
     const { checked, indeterminate } = this.state
@@ -244,7 +240,6 @@ export default class Checkbox extends Component {
         {...rest}
         className={classes}
         onChange={this.handleChange}
-        onClick={this.handleClick}
         onMouseDown={this.handleMouseDown}
         onMouseUp={this.handleMouseUp}
       >

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.
  * @param {Object} [options.eventTargets={}] Map of events and the child component to target.
  * @param {Number} [options.nestingLevel=0] The nesting level of the component.
  * @param {boolean} [options.rendersChildren=false] Does this component render any children?
@@ -22,6 +23,7 @@ import hasValidTypings from './hasValidTypings'
  */
 export default (Component, options = {}) => {
   const {
+    disabledHandlers = [],
     eventTargets = {},
     nestingLevel = 0,
     requiredProps = {},
@@ -218,7 +220,7 @@ export default (Component, options = {}) => {
       // This test catches the case where a developer forgot to call the event prop
       // after handling it internally. It also catch cases where the synthetic event was not passed back.
       _.each(syntheticEvent.types, ({ eventShape, listeners }) => {
-        _.each(listeners, (listenerName) => {
+        _.each(_.without(listeners, ...disabledHandlers), (listenerName) => {
           // onKeyDown => keyDown
           const eventName = _.camelCase(listenerName.replace('on', ''))
 

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

@@ -23,7 +23,9 @@ const wrapperMount = (element, opts) => {
 const wrapperShallow = (...args) => (wrapper = shallow(...args))
 
 describe('Checkbox', () => {
-  common.isConformant(Checkbox)
+  common.isConformant(Checkbox, {
+    disabledHandlers: ['onClick'],
+  })
   common.hasUIClassName(Checkbox)
 
   common.propKeyOnlyToClassName(Checkbox, 'checked')
@@ -239,7 +241,7 @@ describe('Checkbox', () => {
   })
 
   describe('onClick', () => {
-    it('is called with (event, data) on mouse up', () => {
+    it('is called with (event, data) on mouseup', () => {
       const onClick = sandbox.spy()
       const props = { name: 'foo', value: 'bar', checked: false, indeterminate: true }
       mount(<Checkbox onClick={onClick} {...props} />).simulate('mouseup')
@@ -254,7 +256,7 @@ describe('Checkbox', () => {
       )
     })
 
-    it('is not called when on change when "id" is passed', () => {
+    it('is not called when "id" is passed', () => {
       const onClick = sandbox.spy()
       mount(<Checkbox id='foo' onClick={onClick} />).simulate('mouseup')
 
@@ -352,4 +354,73 @@ describe('Checkbox', () => {
         .should.have.prop('type', 'radio')
     })
   })
+
+  describe('comparisons with native DOM', () => {
+    const assertMatrix = [
+      {
+        description: 'click on label: fires on mouse up',
+        event: 'mouseup',
+        target: 'label',
+      },
+      {
+        description: 'key on input: fires on space key',
+        event: 'click',
+        target: 'input',
+      },
+
+      {
+        description: 'click on label: fires on mouse click',
+        event: 'click',
+        target: 'label',
+        id: 'foo',
+      },
+    ]
+
+    assertMatrix.forEach(({ description, event, target, ...props }) => {
+      it(description, () => {
+        const dataId = _.uniqueId('checkbox')
+        const selector = `[data-id=${dataId}] ${target}`
+
+        const onClick = sandbox.spy()
+        const onChange = sandbox.spy()
+
+        wrapperMount(
+          <Checkbox {...props} data-id={dataId} onClick={onClick} onChange={onChange} />,
+          { attachTo },
+        )
+        domEvent.fire(selector, event)
+
+        onClick.should.have.been.calledOnce()
+        onChange.should.have.been.calledOnce()
+
+        onChange.should.have.been.calledAfter(onClick)
+      })
+    })
+  })
+
+  describe('Controlled component', () => {
+    const getControlledCheckbox = isOnClick =>
+      class ControlledCheckbox extends React.Component {
+        state = { checked: false }
+        toggle = () => this.setState(prevState => ({ checked: !prevState.checked }))
+        render() {
+          const handler = isOnClick ? { onClick: this.toggle } : { onChange: this.toggle }
+          return <Checkbox label='Check this box' checked={this.state.checked} {...handler} />
+        }
+      }
+
+    it('toggles state on "change" with "setState" as function', () => {
+      const ControlledCheckbox = getControlledCheckbox(false)
+      wrapperMount(<ControlledCheckbox />)
+      domEvent.fire(document.querySelector('input'), 'click')
+      wrapper.state().should.eql({ checked: true })
+    })
+
+    it('toggles state on "click" with "setState" as function', () => {
+      const ControlledCheckbox = getControlledCheckbox(true)
+      wrapperMount(<ControlledCheckbox />)
+      domEvent.fire(document.querySelector('input'), 'click')
+      wrapper.state().should.eql({ checked: true })
+    })
+  })
 })