Semantic-ui-react: Checkbox: focus not properly activated on click

[skleeschulte]

Steps

1. Add a checkbox: <Checkbox label="Label"/>
2. Click the checkbox or the label.

Expected Result

The checkbox should get focus. onFocus should be triggered.

Actual Result

On Windows 7 64-bit:
Firefox 51.0.1 (32-bit): The checkbox does not get focus. onFocus is not triggered.
Chromium 58.0.3009.0 (64-bit): The checkbox does not get focus. onFocus is not triggered.
Internet Explorer 11.0.9600.18537: onFocus is triggered (as well as onBlur), but the focus is not visible.

In all three browsers the focus works as expected when using the tab key.

Checkboxes in core Semantic UI work as expected.

Version

0.64.8

Testcase

http://codepen.io/anon/pen/apMgvO

[levithomason]

Superb report, thank you. I've marked this issue for help, PRs welcome.

[tarang9211]

@levithomason this should be as straightforward as adding an onFocus callback here right?

I'll be tackling this issue.

[layershifter]

@tarang9211 Yes, we need onFocus and onBlur handlers and new tests for this behaviours.

[levithomason]

Rather than adding callbacks, we just need to set focus on click. Here is the SUI core code that does this in their checkbox.js plugin:

https://github.com/Semantic-Org/Semantic-UI/blob/master/dist/components/checkbox.js#L187

We simply need to call .focus() on the checkbox input ref in the click handler. Note, we should call focus before we call the click/change callbacks since that is the order of these events in a vanilla DOM implementation:

  handleClick = (e) => {
    debug('handleClick()')
    const { onChange, onClick } = this.props
    const { checked, indeterminate } = this.state

+    _.invoke('focus', this.checkboxRef)

    if (this.canToggle()) {
      if (onClick) onClick(e, { ...this.props, checked: !!checked, indeterminate: !!indeterminate })
      if (onChange) onChange(e, { ...this.props, checked: !checked, indeterminate: false })

      this.trySetState({ checked: !checked, indeterminate: false })
    }
  }

No other changes are necessary, though, we should add a test that asserts onFocus is called when the label is clicked and assert the input is the activeElement. Also, a test that asserts onFocus is called when the input is clicked and assert the input is the activeElement.

[tarang9211]

@levithomason is _.invoke a lodash utility?

[levithomason]

Try googling it ;)

[tarang9211]

Yea, that was my first step, but I found this which doesn't quite match the function signature we have above.

[levithomason]

I see, in this case we are importing lodash/fp up top, which is the "functional programming" variant. One of the major differences is that data is provided as the last argument rather than the first, to facilitate function composition:

https://github.com/lodash/lodash/wiki/FP-Guide

[levithomason]

FWIW, I hope someday to convert the entire project to lodash FP usage only. We currently have a mix.

[tarang9211]

got it. and the function to invoke is passed in as the first arg (as a string) ?

[levithomason]

Indeed, these both invoke the focus method if present:

require('lodash').invoke(this.checkboxRef, 'focus')
require('lodash/fp').invoke('focus', this.checkboxRef)

[skleeschulte]

We simply need to call .focus() on the checkbox input ref in the click handler. Note, we should call focus before we call the click/change callbacks since that is the order of these events in a vanilla DOM implementation:

Note that a "vanilla DOM checkbox" can receive focus without the click or change event being triggered: click-and-hold the checkbox, move the mouse-pointer away from the checkbox, release the mouse button.

You can try it out here.

[levithomason]

Ah, you are very correct. I've neglected the fact that focus should be called from the onMouseDown callback, not the onClick callback, since, onClick is called only on mouse up.

@tarang9211 ^ We need to add an onMouseDown and call focus in there instead.

[tarang9211]

so something on the lines of

<ElementType onMouseDown={this.handleFocus}/>

handleFocus = (e) => { _.invoke('focus', this.checkboxRef) }

?

[levithomason]

Yes, however, we should keep the handlers named after the events they are listening to: handleFocus => handleMouseDown. It is OK that within this handler we are setting focus.

[tarang9211]

Correct, sorry. Got my names messed up. @levithomason so once we invoke focus with _.invoke('focus', this.checkboxRef) , the DOM takes over to call the focus event right?

[skleeschulte]

I just had a quick look at https://www.w3.org/TR/html5/editing.html#focus. If I understand it correctly, the behaviour when exactly an element receives focus might be platform dependent and determined by the user agent. But probably it is not possible to rely on the user agent's "native" onFocus because the actual checkbox element is hidden?

Not sure how checkboxes/input elements act on other platforms, being mostly a Windows user I'm used to input elements receiving focus on mouse-down.

[levithomason]

@tarang9211 yes. update the component and play with a checkbox example to see how it works.

[tarang9211]

@levithomason got it. we should also be calling the blur event right?

[levithomason]

@tarang9211 no, the browser should handle that automatically when the element is actually blurred. We only need to set focus manually because the input is actually hidden so the user can never click on it to focus it. They can only click on the label and pseudo element provided, hence, we then need to manually focus the hidden input for them.

[skleeschulte]

Sorry to be the bearer of bad news, but for me this is still not fixed. Look at the old testcase, adjusted to use the latest version of Semantic-UI css and Semantic-UI-React js:
https://codepen.io/anon/pen/BZKVoM

Results on Windows 7 64-bit:
Firefox 53.0.3 (32-bit): The checkbox does not get focus. onFocus is not triggered.
Chromium 61.0.3126.0 (64-bit): The checkbox does not get focus. onFocus is not triggered.
Internet Explorer 11.0.9600.18665: onFocus is triggered (as well as onBlur), but the focus is only visible when the checkbox is checked.

Strangely, IE 11 behaves exactly the same for old and new testcase. I might have overlooked that the focus in IE is visible when the checkbox is checked. Or something changed between version 11.0.9600.18537 and 11.0.9600.18665.

[layershifter]

@skleeschulte Thanks for feedback, seems we really overlooked something.

Expected behaviour