diff --git a/src/modules/Checkbox/Checkbox.js b/src/modules/Checkbox/Checkbox.js index 778dee9c12..468db39df8 100644 --- a/src/modules/Checkbox/Checkbox.js +++ b/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} > diff --git a/test/specs/commonTests/isConformant.js b/test/specs/commonTests/isConformant.js index 2218e4c622..386dcea032 100644 --- a/test/specs/commonTests/isConformant.js +++ b/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', '')) diff --git a/test/specs/modules/Checkbox/Checkbox-test.js b/test/specs/modules/Checkbox/Checkbox-test.js index 32981670e7..075a3acc3f 100644 --- a/test/specs/modules/Checkbox/Checkbox-test.js +++ b/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().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().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( + , + { 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 + } + } + + it('toggles state on "change" with "setState" as function', () => { + const ControlledCheckbox = getControlledCheckbox(false) + wrapperMount() + 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() + domEvent.fire(document.querySelector('input'), 'click') + wrapper.state().should.eql({ checked: true }) + }) + }) })