-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Checkbox: focus not properly activated on click #1335
Comments
Superb report, thank you. I've marked this issue for help, PRs welcome. |
@levithomason this should be as straightforward as adding an I'll be tackling this issue. |
@tarang9211 Yes, we need |
Rather than adding callbacks, we just need to set focus on click. Here is the SUI core code that does this in their https://github.com/Semantic-Org/Semantic-UI/blob/master/dist/components/checkbox.js#L187 We simply need to call 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 |
@levithomason is |
Try googling it ;) |
Yea, that was my first step, but I found this which doesn't quite match the function signature we have above. |
I see, in this case we are importing |
FWIW, I hope someday to convert the entire project to lodash FP usage only. We currently have a mix. |
got it. and the function to invoke is passed in as the first arg (as a string) ? |
Indeed, these both invoke the focus method if present: require('lodash').invoke(this.checkboxRef, 'focus')
require('lodash/fp').invoke('focus', this.checkboxRef) |
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. |
Ah, you are very correct. I've neglected the fact that focus should be called from the @tarang9211 ^ We need to add an |
so something on the lines of
? |
Yes, however, we should keep the handlers named after the events they are listening to: |
Correct, sorry. Got my names messed up. @levithomason so once we invoke focus with |
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. |
@tarang9211 yes. update the component and play with a checkbox example to see how it works. |
@levithomason got it. we should also be calling the |
@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. |
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: Results on Windows 7 64-bit: 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. |
@skleeschulte Thanks for feedback, seems we really overlooked something. Expected behaviour |
Steps
<Checkbox label="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 asonBlur
), 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
The text was updated successfully, but these errors were encountered: