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

[fdaugan]

#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 :

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

3.3.6+PR status :

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

[twbs-savage]

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]

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]

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]

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]

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]

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]

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.)

[fdaugan]

@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]

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

[fdaugan]

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

[patrickhlauke]

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?

[fdaugan]

@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]

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]

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

[patrickhlauke]

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]

@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?

[fdaugan]

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.

[twbs-savage]

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.)

[twbs-savage]

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.)

[fdaugan]

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]

@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]

@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

bootstrap/js/tests/index.html

Lines 11 to 48 in ad5e656

	// 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]

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.)

[cvrebert]

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

[cvrebert]

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

[cvrebert]

@fabdouglas Merged. Thank you!

[patrickhlauke]

Yup, great stuff, merci @fabdouglas