diff --git a/src/modules/Checkbox/Checkbox.js b/src/modules/Checkbox/Checkbox.js index 8464c47aa6..61379eff13 100644 --- a/src/modules/Checkbox/Checkbox.js +++ b/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,8 +198,10 @@ 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() } @@ -183,15 +209,12 @@ export default class Checkbox extends Component { 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 { - )} + {createHTMLLabel(label, { + defaultProps: { htmlFor: id, ref: this.labelRef }, + autoGenerateKey: false, + }) || ) } diff --git a/test/specs/commonTests/isConformant.js b/test/specs/commonTests/isConformant.js index 386dcea032..2218e4c622 100644 --- a/test/specs/commonTests/isConformant.js +++ b/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', '')) diff --git a/test/specs/modules/Checkbox/Checkbox-test.js b/test/specs/modules/Checkbox/Checkbox-test.js index b0fee1926e..c0bf26390e 100644 --- a/test/specs/modules/Checkbox/Checkbox-test.js +++ b/test/specs/modules/Checkbox/Checkbox-test.js @@ -6,8 +6,6 @@ import Checkbox from 'src/modules/Checkbox/Checkbox' import * as common from 'test/specs/commonTests' import { domEvent, sandbox } from 'test/utils' -const mockedMouseEvent = { button: 0 } - // ---------------------------------------- // Wrapper // ---------------------------------------- @@ -25,9 +23,7 @@ const wrapperMount = (element, opts) => { const wrapperShallow = (...args) => (wrapper = shallow(...args)) describe('Checkbox', () => { - common.isConformant(Checkbox, { - disabledHandlers: ['onClick'], - }) + common.isConformant(Checkbox) common.hasUIClassName(Checkbox) common.propKeyOnlyToClassName(Checkbox, 'checked') @@ -68,25 +64,30 @@ describe('Checkbox', () => { describe('checking', () => { it('can be checked and unchecked', () => { - wrapperShallow() + wrapperMount() 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() + wrapperMount() 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() }) }) @@ -146,14 +147,16 @@ describe('Checkbox', () => { it('cannot be checked', () => { wrapperShallow() - wrapper.simulate('mouseup', mockedMouseEvent) + wrapper.find('label').simulate('mouseup') + wrapper.find('label').simulate('click') wrapper.find('input').should.not.be.checked() }) it('cannot be unchecked', () => { wrapperShallow() - wrapper.simulate('mouseup', mockedMouseEvent) + wrapper.find('label').simulate('mouseup') + wrapper.find('label').simulate('click') wrapper.find('input').should.be.checked() }) @@ -221,7 +224,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().simulate('mouseup', mockedMouseEvent) + + wrapperMount() + + wrapper.find('label').simulate('mouseup') + wrapper.find('label').simulate('click') onChange.should.have.been.calledOnce() onChange.should.have.been.calledWithMatch( @@ -236,24 +243,19 @@ describe('Checkbox', () => { it('is not called when on change when "id" is passed', () => { const onChange = sandbox.spy() - mount().simulate('mouseup', mockedMouseEvent) - - onChange.should.have.not.been.called() - }) - - it('is not called with other button click in mouse up', () => { - const onChange = sandbox.spy() - mount().simulate('mouseup', { button: 2 }) + wrapperMount() + wrapper.find('label').simulate('mouseup') + wrapper.find('label').simulate('click') onChange.should.have.not.been.called() }) }) 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().simulate('mouseup', mockedMouseEvent) + mount().simulate('click') onClick.should.have.been.calledOnce() onClick.should.have.been.calledWithMatch( @@ -267,16 +269,10 @@ describe('Checkbox', () => { it('is not called when "id" is passed', () => { const onClick = sandbox.spy() - mount().simulate('mouseup', mockedMouseEvent) - - onClick.should.have.not.been.called() - }) - - it('is not called with left button click in mouse up', () => { - const onClick = sandbox.spy() - const mockedMouseRightClickEvent = { button: 2 } - mount().simulate('mouseup', mockedMouseRightClickEvent) + wrapperMount() + wrapper.find('label').simulate('mouseup') + wrapper.find('label').simulate('click') onClick.should.have.not.been.called() }) }) @@ -290,13 +286,7 @@ describe('Checkbox', () => { onMousedDown.should.have.been.calledOnce() onMousedDown.should.have.been.calledWithMatch({}, props) }) - it('prevents default event', () => { - const preventDefault = sandbox.spy() - wrapperShallow() - wrapper.simulate('mousedown', { preventDefault }) - preventDefault.should.have.been.calledOnce() - }) it('sets focus to container', () => { wrapperMount() const input = document.querySelector('.ui.checkbox input') @@ -310,7 +300,7 @@ describe('Checkbox', () => { it('is called with (event, data) on mouse up', () => { const onMouseUp = sandbox.spy() const props = { name: 'foo', value: 'bar', checked: false, indeterminate: true } - mount().simulate('mouseup', mockedMouseEvent) + mount().simulate('mouseup') onMouseUp.should.have.been.calledOnce() onMouseUp.should.have.been.calledWithMatch({}, props) @@ -326,15 +316,17 @@ describe('Checkbox', () => { describe('readOnly', () => { it('cannot be checked', () => { - wrapperShallow() + wrapperMount() - 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() + wrapperMount() - wrapper.simulate('click') + wrapper.find('label').simulate('mouseup') + wrapper.find('label').simulate('click') wrapper.find('input').should.be.checked() }) }) @@ -382,40 +374,74 @@ describe('Checkbox', () => { describe('comparisons with native DOM', () => { const assertMatrix = [ { - description: 'click on label: fires on mouse up', - event: 'mouseup', - target: 'label', + description: 'click on label: fires on mouse click', + events: { + label: ['mouseup', 'click'], + input: ['click'], + }, + }, + { + description: 'click on input: fires on mouse click', + events: { + label: ['mouseup', 'click'], + input: ['click'], + }, }, { description: 'key on input: fires on space key', - event: 'click', - target: 'input', + events: { + label: ['mouseup', 'click'], + input: ['click'], + }, }, { - 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( - , +
+ +
, { 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 +453,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 diff --git a/test/utils/domEvent.js b/test/utils/domEvent.js index cc58cd7814..9fdfc87202 100644 --- a/test/utils/domEvent.js +++ b/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,