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

* fix(Checkbox): Prevents click propagation from the input (#3433)

* fix(Checkbox) Create tests for click on input and label with and without id (#3433)

* fix(Checkbox): Create handler for input click (#3433)

* fix(Checkbox): Improve tests readability (#3433)

* updates

* updates

* improve coverage

* remove target

Author: Fabianopb

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
+    _.set(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/commonTests/isConformant.js

@@ -13,7 +13,6 @@ 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?
@@ -23,7 +22,6 @@ import hasValidTypings from './hasValidTypings'
  */
 export default (Component, options = {}) => {
   const {
-    disabledHandlers = [],
     eventTargets = {},
     nestingLevel = 0,
     requiredProps = {},
@@ -220,7 +218,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(_.without(listeners, ...disabledHandlers), (listenerName) => {
+        _.each(listeners, (listenerName) => {
           // onKeyDown => keyDown
           const eventName = _.camelCase(listenerName.replace('on', ''))