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

src/modules/Checkbox/Checkbox.js

@@ -1,7 +1,7 @@
 import cx from 'classnames'
 import _ from 'lodash'
 import PropTypes from 'prop-types'
-import React from 'react'
+import React, { createRef } from 'react'
 
 import {
   AutoControlledComponent as Component,
@@ -117,6 +117,9 @@ export default class Checkbox extends Component {
 
   static autoControlledProps = ['checked', 'indeterminate']
 
+  inputRef = createRef()
+  labelRef = createRef()
+
   componentDidMount() {
     this.setIndeterminate()
   }
@@ -139,29 +142,50 @@ export default class Checkbox extends Component {
     return disabled ? -1 : 0
   }
 
-  handleInputRef = c => (this.inputRef = c)
-
-  handleChange = (e, fromMouseUp) => {
-    debug('handleChange()')
+  handleClick = (e) => {
+    debug('handleClick()', _.get(e, 'target.tagName'))
     const { id } = this.props
     const { checked, indeterminate } = this.state
 
+    const hasId = !_.isNil(id)
+    const isLabelClick = e.target === this.labelRef.current
+    const isLabelClickAndForwardedToInput = isLabelClick && hasId
+
+    // https://github.com/Semantic-Org/Semantic-UI-React/pull/3351
+    if (!isLabelClickAndForwardedToInput) {
+      _.invoke(this.props, 'onClick', e, {
+        ...this.props,
+        checked: !checked,
+        indeterminate: !!indeterminate,
+      })
+    }
+
+    if (this.isClickFromMouse) {
+      this.isClickFromMouse = false
+
+      if (isLabelClick && !hasId) {
+        this.handleChange(e)
+      }
+
+      if (hasId) {
+        // To prevent two clicks from being fired from the component we have to stop the propagation
+        // from the "input" click: https://github.com/Semantic-Org/Semantic-UI-React/issues/3433
+        e.stopPropagation()
+      }
+    }
+  }
+
+  handleChange = (e) => {
+    const { checked } = this.state
+
     if (!this.canToggle()) return
-    if (fromMouseUp && !_.isNil(id)) return
+    debug('handleChange()', _.get(e, 'target.tagName'))
 
-    // 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,
     })
-
     this.trySetState({ checked: !checked, indeterminate: false })
   }
 
@@ -174,24 +198,23 @@ export default class Checkbox extends Component {
       checked: !!checked,
       indeterminate: !!indeterminate,
     })
-    _.invoke(this.inputRef, 'focus')
 
+    _.invoke(this.inputRef.current, 'focus')
+    // Heads up!
+    // We need to call "preventDefault" to keep element focused.
     e.preventDefault()
   }
 
   handleMouseUp = (e) => {
     debug('handleMouseUp()')
     const { checked, indeterminate } = this.state
 
+    this.isClickFromMouse = true
     _.invoke(this.props, 'onMouseUp', e, {
       ...this.props,
       checked: !!checked,
       indeterminate: !!indeterminate,
     })
-
-    // Handle mouseUp only on the left mouse button.
-    // https://github.com/Semantic-Org/Semantic-UI-React/issues/3419
-    if (e.button === 0) this.handleChange(e, true)
   }
 
   // Note: You can't directly set the indeterminate prop on the input, so we
@@ -200,7 +223,7 @@ export default class Checkbox extends Component {
   setIndeterminate = () => {
     const { indeterminate } = this.state
 
-    if (this.inputRef) this.inputRef.indeterminate = !!indeterminate
+    if (this.inputRef.current) this.inputRef.current.indeterminate = !!indeterminate
   }
 
   render() {
@@ -242,6 +265,7 @@ export default class Checkbox extends Component {
       <ElementType
         {...rest}
         className={classes}
+        onClick={this.handleClick}
         onChange={this.handleChange}
         onMouseDown={this.handleMouseDown}
         onMouseUp={this.handleMouseUp}
@@ -254,7 +278,7 @@ export default class Checkbox extends Component {
           id={id}
           name={name}
           readOnly
-          ref={this.handleInputRef}
+          ref={this.inputRef}
           tabIndex={this.computeTabIndex()}
           type={type}
           value={value}
@@ -263,9 +287,10 @@ export default class Checkbox extends Component {
          Heads Up!
          Do not remove empty labels, they are required by SUI CSS
          */}
-        {createHTMLLabel(label, { defaultProps: { htmlFor: id }, autoGenerateKey: false }) || (
-          <label htmlFor={id} />
-        )}
+        {createHTMLLabel(label, {
+          defaultProps: { htmlFor: id, ref: this.labelRef },
+          autoGenerateKey: false,
+        }) || <label htmlFor={id} ref={this.labelRef} />}
       </ElementType>
     )
   }

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

@@ -68,25 +68,30 @@ describe('Checkbox', () => {
 
   describe('checking', () => {
     it('can be checked and unchecked', () => {
-      wrapperShallow(<Checkbox />)
+      wrapperMount(<Checkbox />)
 
       wrapper.find('input').should.not.be.checked()
 
-      wrapper.simulate('mouseup', mockedMouseEvent)
+      wrapper.find('label').simulate('mouseup')
+      wrapper.find('label').simulate('click')
       wrapper.find('input').should.be.checked()
 
-      wrapper.simulate('mouseup', mockedMouseEvent)
+      wrapper.find('label').simulate('mouseup')
+      wrapper.find('label').simulate('click')
       wrapper.find('input').should.not.be.checked()
     })
+
     it('can be checked but not unchecked when radio', () => {
-      wrapperShallow(<Checkbox radio />)
+      wrapperMount(<Checkbox radio />)
 
       wrapper.find('input').should.not.be.checked()
 
-      wrapper.simulate('mouseup', mockedMouseEvent)
+      wrapper.find('label').simulate('mouseup')
+      wrapper.find('label').simulate('click')
       wrapper.find('input').should.be.checked()
 
-      wrapper.simulate('mouseup', mockedMouseEvent)
+      wrapper.find('label').simulate('mouseup')
+      wrapper.find('label').simulate('click')
       wrapper.find('input').should.be.checked()
     })
   })
@@ -221,7 +226,11 @@ describe('Checkbox', () => {
     it('is called with (e, data) on mouse up', () => {
       const onChange = sandbox.spy()
       const props = { name: 'foo', value: 'bar', checked: false, indeterminate: true }
-      mount(<Checkbox onChange={onChange} {...props} />).simulate('mouseup', mockedMouseEvent)
+
+      wrapperMount(<Checkbox onChange={onChange} {...props} />)
+
+      wrapper.find('label').simulate('mouseup')
+      wrapper.find('label').simulate('click')
 
       onChange.should.have.been.calledOnce()
       onChange.should.have.been.calledWithMatch(
@@ -250,10 +259,10 @@ describe('Checkbox', () => {
   })
 
   describe('onClick', () => {
-    it('is called with (event, data) on mouseup', () => {
+    it('is called with (event, data) on click', () => {
       const onClick = sandbox.spy()
       const props = { name: 'foo', value: 'bar', checked: false, indeterminate: true }
-      mount(<Checkbox onClick={onClick} {...props} />).simulate('mouseup', mockedMouseEvent)
+      mount(<Checkbox onClick={onClick} {...props} />).simulate('click')
 
       onClick.should.have.been.calledOnce()
       onClick.should.have.been.calledWithMatch(
@@ -290,13 +299,7 @@ describe('Checkbox', () => {
       onMousedDown.should.have.been.calledOnce()
       onMousedDown.should.have.been.calledWithMatch({}, props)
     })
-    it('prevents default event', () => {
-      const preventDefault = sandbox.spy()
-      wrapperShallow(<Checkbox />)
 
-      wrapper.simulate('mousedown', { preventDefault })
-      preventDefault.should.have.been.calledOnce()
-    })
     it('sets focus to container', () => {
       wrapperMount(<Checkbox />)
       const input = document.querySelector('.ui.checkbox input')
@@ -326,15 +329,17 @@ describe('Checkbox', () => {
 
   describe('readOnly', () => {
     it('cannot be checked', () => {
-      wrapperShallow(<Checkbox readOnly />)
+      wrapperMount(<Checkbox readOnly />)
 
-      wrapper.simulate('click')
+      wrapper.find('label').simulate('mouseup')
+      wrapper.find('label').simulate('click')
       wrapper.find('input').should.not.be.checked()
     })
     it('cannot be unchecked', () => {
-      wrapperShallow(<Checkbox defaultChecked readOnly />)
+      wrapperMount(<Checkbox defaultChecked readOnly />)
 
-      wrapper.simulate('click')
+      wrapper.find('label').simulate('mouseup')
+      wrapper.find('label').simulate('click')
       wrapper.find('input').should.be.checked()
     })
   })
@@ -382,40 +387,77 @@ describe('Checkbox', () => {
   describe('comparisons with native DOM', () => {
     const assertMatrix = [
       {
-        description: 'click on label: fires on mouse up',
-        event: 'mouseup',
+        description: 'click on label: fires on mouse click',
+        events: {
+          label: ['mouseup', 'click'],
+          input: ['click'],
+        },
+        target: 'label',
+      },
+      {
+        description: 'click on input: fires on mouse click',
+        events: {
+          label: ['mouseup', 'click'],
+          input: ['click'],
+        },
         target: 'label',
       },
       {
         description: 'key on input: fires on space key',
-        event: 'click',
+        events: {
+          label: ['mouseup', 'click'],
+          input: ['click'],
+        },
         target: 'input',
       },
 
       {
-        description: 'click on label: fires on mouse click',
-        event: 'click',
-        target: 'label',
+        description: 'click on label with "id": fires on mouse click',
+        events: {
+          label: ['mouseup', 'click'],
+          input: ['click'],
+        },
+        id: 'foo',
+      },
+      {
+        description: 'click on input with "id": fires on mouse click',
+        events: {
+          label: ['mouseup', 'click'],
+          input: ['click'],
+        },
+        id: 'foo',
+      },
+      {
+        description: 'key on input with "id: fires on space key',
+        events: {
+          input: ['click'],
+        },
         id: 'foo',
       },
     ]
 
-    assertMatrix.forEach(({ description, event, target, ...props }) => {
+    assertMatrix.forEach(({ description, events, ...props }) => {
       it(description, () => {
         const dataId = _.uniqueId('checkbox')
-        const selector = `[data-id=${dataId}] ${target}`
 
         const onClick = sandbox.spy()
         const onChange = sandbox.spy()
+        const onParentClick = sandbox.spy()
 
         wrapperMount(
-          <Checkbox {...props} data-id={dataId} onClick={onClick} onChange={onChange} />,
+          <div onClick={onParentClick} role='presentation'>
+            <Checkbox {...props} data-id={dataId} onClick={onClick} onChange={onChange} />
+          </div>,
           { attachTo },
         )
-        domEvent.fire(selector, event)
+
+        _.forEach(events, (event, target) => {
+          domEvent.fire(`[data-id=${dataId}] ${target}`, event)
+        })
 
         onClick.should.have.been.calledOnce()
         onChange.should.have.been.calledOnce()
+        onParentClick.should.have.been.calledOnce()
 
         onChange.should.have.been.calledAfter(onClick)
       })
@@ -427,6 +469,7 @@ describe('Checkbox', () => {
       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} />

test/utils/domEvent.js

@@ -34,6 +34,14 @@ export const click = (node, data) => fire(node, 'click', data)
  */
 export const keyDown = (node, data) => fire(node, 'keydown', data)
 
+/**
+ * Dispatch a 'mousedown' event on a DOM node.
+ * @param {String|Object} node A querySelector string or DOM node.
+ * @param {Object} [data] Additional event data.
+ * @returns {Object} The event
+ */
+export const mouseDown = (node, data) => fire(node, 'mousedown', data)
+
 /**
  * Dispatch a 'mouseenter' event on a DOM node.
  * @param {String|Object} node A querySelector string or DOM node.
@@ -89,6 +97,7 @@ export default {
   mouseEnter,
   mouseLeave,
   mouseOver,
+  mouseDown,
   mouseUp,
   resize,
   scroll,