fix(Checkbox): focus should be obtained on mouseDown

[layershifter]

Finally fixes #1335.

We added focus capture in #1361, but we added it into the onMouseDown handler while it should be inside onClick handler.

SUI behavior

[layershifter]

@levithomason I removed usage of lodash/fp because I don't understand how I can make this in FP style:

_.invoke(this.props, 'onMouseDown', e, { ...this.props, checked: !!checked, indeterminate: !!indeterminate })

[levithomason]

No worries, this works 👍

[codecov-io]

Codecov Report

Merging #1762 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1762      +/-   ##
==========================================
+ Coverage   99.75%   99.75%   +<.01%     
==========================================
  Files         142      142              
  Lines        2459     2466       +7     
==========================================
+ Hits         2453     2460       +7     
  Misses          6        6

Impacted Files	Coverage Δ
src/modules/Checkbox/Checkbox.js	100% <100%> (ø)	⬆️
src/elements/Button/Button.js	100% <0%> (ø)	⬆️
src/modules/Dropdown/Dropdown.js	100% <0%> (ø)	⬆️
src/elements/Input/Input.js	100% <0%> (ø)	⬆️
src/modules/Search/Search.js	100% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 044c68d...10532de. Read the comment docs.

[levithomason]

Order of events

The order of events as fired by the browser are: mouseDown, focus, mouseUp, click. I've added the following handlers to this comment textarea on github and here's the result:

new_comment_field.addEventListener('click', () => console.log('click'))
new_comment_field.addEventListener('mousedown', () => console.log('mousedown'))
new_comment_field.addEventListener('mouseup', () => console.log('mouseup'))
new_comment_field.addEventListener('focus', () => console.log('focus'))

A click, long press, and release logs this:

onBlur

That said, our bug is that the onBlur callback is called immediately after the onFocus callback. So, the reported codepen ends up setting state true in onFocus and setting it back to false in onBlur. The onFocus event is working correctly, it is just that there is an extra onBlur event firing first. You can see this easily by removing the onBlur handler and the state is updated correctly via onFocus.

This extra blur event is a side effect of calling inputRef.focus() in the onMouseDown handler for what is actually the label element. We can circumvent this issue by simply preventing the default event in onMouseDown. That way, we call onMouseDown then inputRef.focus() without causing the extra unnecessary blur:

Update to this PR

// handleClick() {
// ..snip
    _.invoke(this.props, 'onChange', e, { ...this.props, checked: !checked, indeterminate: false })

    this.trySetState({ checked: !checked, indeterminate: false })
-   _.invoke(this.inputRef, 'focus')
  }

  handleMouseDown = e => {
    debug('handleMouseDown()')
    const { checked, indeterminate } = this.state

    _.invoke(this.props, 'onMouseDown', e, { ...this.props, checked: !!checked, indeterminate: !!indeterminate })
+   _.invoke(this.inputRef, 'focus')
+   e.preventDefault()
  }

The test

This results in the correct behavior as well as the correct order of events fired:

---

Give that a try and let me know your thoughts.

[layershifter]

@levithomason Thanks for a detailed reply. You're right there, seems that SUI implements focus incorrectly, it triggers focus after click. The behaviour that you described is more native. I will perform changes in this PR.

[levithomason]

Thanks for the confirmation and second round.

[levithomason]

Released in [email protected]