Skip to content
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

Fix keyboard navigation for toggle buttons (checkbox, radio, single toggle) following a mouse click #19192

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
5 participants
@fabdouglas
Copy link

fabdouglas commented Feb 14, 2016

#16226 has fixed #16223 but also has broken the keyboard navigation.

This PR, still fixes #16226, but also restores the keyboard navigation broken by #16223.

3.3.6 status :
image
jsfiddle (look the console) : http://jsfiddle.net/vjjmj4da/9/

3.3.6+PR status :
image

jsfiddle (look the console) : http://jsfiddle.net/vjjmj4da/13/

Enhanced keyboard navigation with toggle buttons
#16226 has fixed #16223 but also has broken the keyboard navigation.
This PR, still fixes #16226 (invalid state) by keeping the work #16223, but also restore the keyboard navigation broken by #16223.
@twbs-savage

This comment has been minimized.

Copy link

twbs-savage commented Feb 14, 2016

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: f51ac9a
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/109179976

(Please note that this is a fully automated comment.)

@twbs-savage

This comment has been minimized.

Copy link

twbs-savage commented Feb 14, 2016

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 585e0f2
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/109180670

(Please note that this is a fully automated comment.)

@twbs-savage

This comment has been minimized.

Copy link

twbs-savage commented Feb 14, 2016

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: d0d8bf0
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/109181807

(Please note that this is a fully automated comment.)

@twbs-savage

This comment has been minimized.

Copy link

twbs-savage commented Feb 14, 2016

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 70d33f7
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/109183083

(Please note that this is a fully automated comment.)

@twbs-savage

This comment has been minimized.

Copy link

twbs-savage commented Feb 14, 2016

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 81e4bf3
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/109184143

(Please note that this is a fully automated comment.)

@patrickhlauke

This comment has been minimized.

Copy link
Member

patrickhlauke commented Feb 14, 2016

maybe i'm misunderstanding your compatibility table there, but even for http://jsfiddle.net/vjjmj4da/9/ (just tested in chrome and firefox) i can submit on enter for checkboxes and radio buttons, select radio buttons with left/right, toggle checkboxes with space.

@twbs-lmvtfy

This comment has been minimized.

Copy link

twbs-lmvtfy commented Feb 14, 2016

Hi @patrickhlauke!

You appear to have posted a live example (http://fiddle.jshell.net/vjjmj4da/9/show/light/), which is always a good first step. However, according to the HTML5 validator, your example has some validation errors, which might potentially be causing your issue:

  • line 203, column 7 thru column 89: Any input descendant of a label element with a for attribute must have an ID value that matches that for attribute.
  • line 202, column 5 thru column 53: The for attribute of the label element must refer to a non-hidden form control.

You'll need to fix these errors and post a revised example before we can proceed further.
Thanks!

(Please note that this is a fully automated comment.)

@fabdouglas

This comment has been minimized.

Copy link
Author

fabdouglas commented Feb 14, 2016

@patrickhlauke
Well I don't understand, you are telling me that, at this page, with Chrome or Firefox, when you select any input (radio or checkbox) then press ENTER, you see a "SUBMIT" message in the console ?
I've tested this fiddle from a Windows and a Mac machines, and I can't see this behavior.

@patrickhlauke

This comment has been minimized.

Copy link
Member

patrickhlauke commented Feb 14, 2016

well, works for me (just tested in Windows for now in Chrome/Firefox) https://www.youtube.com/watch?v=7aQych3ueX0

@fabdouglas

This comment has been minimized.

Copy link
Author

fabdouglas commented Feb 14, 2016

I confirm the keyboard issues for Mac OSX (Chrome, Safari and Firefox) and Windows 8 (IE11, Chrome, Firefox), vanilla browsers, no enabled extensions

image

@patrickhlauke

This comment has been minimized.

Copy link
Member

patrickhlauke commented Feb 14, 2016

Ok, looking at your actual code change, I think I'm understanding what you're saying - this wasn't clear in your actual description. Do you mean that after doing an initial selection with the MOUSE (e.g. clicking on a radio button), then keyboard doesn't work (and hence you explicitly set focus to it? In that case yes, I can see the issue and how your patch addresses that - and the other part is a simplification of closest as that would match the current element anyway without the need for checking if it has class already?

@fabdouglas

This comment has been minimized.

Copy link
Author

fabdouglas commented Feb 14, 2016

@patrickhlauke
I've just seen your screencast, and indeed I was really confused, because when I click I don't see any focus on inputs. Then I've realised you used the TAB and indeed the focus appears and events are well managed... until I click on an input.
The $.focuscall makes sure the input keeps the focus, so the ENTERkeypress event will bubble to the form.

Concerning the failling travis tests, I'm a bit lost :(. I don't know how to fix them.

TypeError: 'undefined' is not a function (evaluating '$btn.find('input').focus()')

The $.closest function checks the $(this) then the parents. Source : closest

Description: For each element in the set, get the first element that matches the selector by testing the element itself and traversing up through its ancestors in the DOM tree.

@patrickhlauke patrickhlauke changed the title Enhanced keyboard navigation with toggle buttons Fix keyboard navigation for toggle buttons (checkbox, radio, single toggle) following a mouse click Feb 14, 2016

@patrickhlauke

This comment has been minimized.

Copy link
Member

patrickhlauke commented Feb 14, 2016

Yeah, now I'm with you. The fix looks good to me. Will double-check in a bit and merge. I think travis and savage are ... having a moment ther, as I don't think the tiny change in your code should cause those issues. So just ignore for now.

@cvrebert

This comment has been minimized.

Copy link
Member

cvrebert commented Feb 14, 2016

@patrickhlauke Both build failures are legitimate. These changes are causing some button.js unit tests to fail.

@patrickhlauke

This comment has been minimized.

Copy link
Member

patrickhlauke commented Feb 14, 2016

Yeah sorry just seen that as I look deeper into it. The issue appears when it's a <button> that acts as toggle, as the $btn.find('input') then clearly comes back empty, so can't fire $btn.find('input').focus()

so, you'll need some extra check there to make sure this only triggers focus when there IS an actual input.

@patrickhlauke

This comment has been minimized.

Copy link
Member

patrickhlauke commented Feb 14, 2016

@cvrebert shouldn't the code as is be acceptable though? when $btn.find('input') is empty, no focus() will be fired...and that's ok? it shouldn't cause any actual error in real use, no?

@fabdouglas

This comment has been minimized.

Copy link
Author

fabdouglas commented Feb 14, 2016

What I don't understand is :

  • $btn cannot be undefined
  • $btn.find('input') cannot be undefined, but empty, why not.
  • $btn.find('input').focus() will use an empty jquery context, so no fired events.

In fact, to be really nice I should use $btn.find('input:visible').first().focus() to manage a specific case where there are several hidden/visible inputs in the .btn container, but the ci tests will fail too.

Update button.js
Manage inputs and buttons
@twbs-savage

This comment has been minimized.

Copy link

twbs-savage commented Feb 14, 2016

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 88a2761
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/109232350

(Please note that this is a fully automated comment.)

Update button.js
Manage mixed button/input scénarii
@twbs-savage

This comment has been minimized.

Copy link

twbs-savage commented Feb 14, 2016

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: ecb3496
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/109234308

(Please note that this is a fully automated comment.)

@fabdouglas

This comment has been minimized.

Copy link
Author

fabdouglas commented Feb 14, 2016

I've updated the test to manage some serious cases for the tests, but Travis give this error :

TypeError: 'undefined' is not a function (evaluating '$btn.focus()')

How is it possible?

@patrickhlauke

This comment has been minimized.

Copy link
Member

patrickhlauke commented Feb 15, 2016

@fabdouglas i'm also not quite sure why/how those would fail in qunit. digging in deeper (but i definitely don't think it's anything that you're necessarily doing wrong at your end...hang in there ;) )

@cvrebert

This comment has been minimized.

Copy link
Member

cvrebert commented Feb 16, 2016

@fabdouglas You need to use .trigger('focus') instead of .focus(). jQuery allows folks to create customized jQuery builds which exclude various modules of jQuery, in particular the "event aliases" module that contains convenience methods for events such as .focus(), and we currently aim to support such folks, so Bootstrap's JS can't utilize those methods.
(Which is why we kill those alias methods when running the test suite via

// Disable jQuery event aliases to ensure we don't accidentally use any of them
(function () {
var eventAliases = [
'blur',
'focus',
'focusin',
'focusout',
'load',
'resize',
'scroll',
'unload',
'click',
'dblclick',
'mousedown',
'mouseup',
'mousemove',
'mouseover',
'mouseout',
'mouseenter',
'mouseleave',
'change',
'select',
'submit',
'keydown',
'keypress',
'keyup',
'error',
'contextmenu',
'hover',
'bind',
'unbind',
'delegate',
'undelegate'
]
for (var i = 0; i < eventAliases.length; i++) {
$.fn[eventAliases[i]] = undefined
}
})()
)

@twbs-savage

This comment has been minimized.

Copy link

twbs-savage commented Feb 16, 2016

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 314bcb8
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/109587221

(Please note that this is a fully automated comment.)

@patrickhlauke patrickhlauke added this to the v4.0.0-alpha.3 milestone Feb 16, 2016

@cvrebert

This comment has been minimized.

Copy link
Member

cvrebert commented Feb 16, 2016

Looks good now. We'll squash the commits on our end when merging.

@cvrebert

This comment has been minimized.

Copy link
Member

cvrebert commented Feb 16, 2016

And we should mention this in the visual test of the buttons plugin to help prevent future regressions.

@cvrebert cvrebert closed this in ad1e98d Feb 16, 2016

@cvrebert

This comment has been minimized.

Copy link
Member

cvrebert commented Feb 16, 2016

@fabdouglas Merged. Thank you!

@patrickhlauke

This comment has been minimized.

Copy link
Member

patrickhlauke commented Feb 16, 2016

Yup, great stuff, merci @fabdouglas

@patrickhlauke patrickhlauke modified the milestones: v3.3.7, v4.0.0-alpha.3 Feb 16, 2016

@mdo mdo referenced this pull request Feb 16, 2016

Closed

v3.3.7 ship list #18331

cvrebert added a commit that referenced this pull request Feb 16, 2016

Port #19219 to v3
CONTRIBUTING: Document restriction regarding jQuery event alias methods

Refs #19192
[ci skip]

@fabdouglas fabdouglas deleted the fabdouglas:patch-1 branch Mar 7, 2016

@fabdouglas fabdouglas restored the fabdouglas:patch-1 branch Mar 7, 2016

@fabdouglas fabdouglas deleted the fabdouglas:patch-1 branch Mar 7, 2016

@fabdouglas fabdouglas restored the fabdouglas:patch-1 branch Mar 7, 2016

chiraggmodi pushed a commit to chiraggmodi/bootstrap that referenced this pull request Apr 8, 2019

button.js: Fix keyboard navigation
This PR fixes the keyboard navigation again while still keeping twbs#16223 fixed.

Closes twbs#19192
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.