Fix radio button toggle behavior for keyboard users

[patrickhlauke]

as this breaks keyboard navigation for radio button toggles - fixes #16223

[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: f6531aa
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/57098788

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

[cvrebert]

This needs a unit test.

[cvrebert]

Also, is there a reason the condition isn't .is('input[type="radio"]') ?

[patrickhlauke]

Not sure how to unit test "if a radio button has been chosen via the keyboard", and not quite sure what to actually test for...thoughts?

As for .is('input[type="radio"]') ... well yes, that's a bit more terse (and it shows that I don't usually jQuery but do vanilla JS)

[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: 3c8977f
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/57108389

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

[patrickhlauke]

@cvrebert any idea/pointer for how this could be unit-tested? simulate actual keyboard interactions somehow?

[cvrebert]

This doesn't actually seem to fix anything: Demo: http://jsfiddle.net/oxnksxaj/

[cvrebert]

Pretty sure your check needs to happen before this line; otherwise, $btn will always be the label.btn.

[patrickhlauke]

yup, absolutely right. amended below by referencing e.target instead

[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: 613eedd
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/57940169

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

[patrickhlauke]

@cvrebert not sure what the heck i did there...i could have sworn it worked (but then i did mess around with the JS/test a bit too much and probably lost track at some point of what i was doing).

Fixed, i think...see http://jsfiddle.net/vjjmj4da/1/

[cvrebert]

@patrickhlauke Okay, but now it suffers from the "double change events" problem which is the reason that the preventDefault was added in the first place (in 4b40ee6).
Open http://jsfiddle.net/vjjmj4da/2/ , change the selected option, and watch the console output.

[cvrebert]

Regarding testing this, FWICT, the relevant difference between mouse and keyboard is that keyboard usage results in click events fired at the invisible <input>s whereas mouse usage results in click events fired at the <label>s.

[patrickhlauke]

for the double change event, i may have found a way around it. just posting here as a fiddle for now http://jsfiddle.net/203pxaej/ - basically, had to mess around with the actual toggle function as well

if ($input.prop('type') == 'radio') {
  if ($input.prop('checked')) changed = false
  if (!$input.prop('checked') || !this.$element.hasClass('active')) $parent.find('.active').removeClass('active')
}

from cursory testing, this now seems to correctly address mouse and keyboard usage, unless i've missed something.

thanks for the info on testing. will have a go at making some tests later too.

[cvrebert]

This is indented one level too much

[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: 66d7113
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/58089184

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

[patrickhlauke]

Regarding testing this, FWICT, the relevant difference between mouse and keyboard is that keyboard usage results in click events fired at the invisible <input>s whereas mouse usage results in click events fired at the <label>s.

Unfortunately, this doesn't seem to be the case - even testing the case that is currently broken for keyboard users http://jsfiddle.net/oxnksxaj/ and explictly doing something like

$("#discover").trigger('click')

in the console does check the radio button correctly, which is not the case for actual keyboard use. I might be wrong, but I can't see any immediate way to test/simulate keyboard interaction in this case?

[patrickhlauke]

@cvrebert @mdo good to merge, seeing that making an actual unit test appears to be elusive?

[patrickhlauke]

casting the net further... @hnrch02 @XhmikosR any thoughts?

[patrickhlauke]

As I haven't heard anything to the contrary, merged. If the absence of a unit test (which seems elusive, as the behavior seems to depend on actual browser interaction, which can't be tested in a straightforward way in a headless engine-only setup it seems) is a problem, I'd say let's revisit that aspect separately.

[alexw23]

👍 for this issue I've managed to replicate the issue here http://codepen.io/anon/pen/MwwyRW

[alexw23]

I don't think this actually fixes it as now we have the issue that the focus still displays which is now even more confusing for the user.

[alexw23]

From what I can see the one line the other changes break the active class. Also adding keydown.bs.button.data-api allows for the user to hit the enter key on the first input to select.

See the working fixes here: http://codepen.io/anon/pen/vOOKNj

Here's a quick diff: https://www.diffchecker.com/ogg8uuer

[cvrebert]

Your 1st codepen uses v3.3.4's JS; the fix is in master (which will be v3.3.5)

[alexw23]

@cvrebert Is there a link to a master? As I don't believe this will actually fix it.

[cvrebert]

@alexw23 https://github.com/twbs/bootstrap/blob/master/dist/js/bootstrap.js

[alexw23]

@cvrebert As I mentioned these changes have broken it even further http://codepen.io/anon/pen/waaWGZ

I can't even get it to work onClick now. Note: this is running on master.

[patrickhlauke]

@alexw23 your pen isn't working because of rawgithub

From the Chrome DevTools console:

"Refused to execute script from 'https://raw.githubusercontent.com/twbs/bootstrap/master/dist/js/bootstrap.js' because its MIME type ('text/plain') is not executable, and strict MIME type checking is enabled.
index.html:3 Resource interpreted as Script but transferred with MIME type text/plain: "https://raw.githubusercontent.com/twbs/bootstrap/master/dist/js/bootstrap.js"."

[patrickhlauke]

However, I can see what you mean now about the class not being set once moving focus off the radio buttons (just brutally forked you pen and slammed bootstrap.js into the JS part) http://codepen.io/patrickhlauke/pen/zGGKwJ

[hnrch02]

(FYI: rawgit.com serves the raw GitHub files with the proper headers.)

[alexw23]

That seemed to be the issue I've updated the codepen. http://codepen.io/anon/pen/waaWGZ

However this doesn't work for enter key (for the first tab)

[patrickhlauke]

Note that, by default (talking native standard browser behavior here, without any Bootstrap) radio buttons don't really react to ENTER. Instead, if focusing on the first of a group of unselected radio buttons, you'd use SPACE to toggle it.

[alexw23]

Okay well I guess it's fixed then sorry for the confusion.

On Fri, May 1, 2015 at 8:02 AM, Patrick H. Lauke [email protected]
wrote:

Note that, by default (talking native standard browser behavior here,
without any Bootstrap) radio buttons don't really react to ENTER. Instead,
if focusing on the first of a group of unselected radio buttons, you'd use
SPACE to toggle it.

—
Reply to this email directly or view it on GitHub
#16226 (comment).

[patrickhlauke]

No need to apologize, and nope not quite fixed yet...seems the .active class doesn't stick correctly after selecting a radio button with the keyboard. The radio itself is correctly set, but .active doesn't seem to get actually set with my original fix (which i didn't notice because .focus c;ass still looks the same as .active). I'll work on a fix for that separately though.

[alexw23]

My diff fixes this however http://codepen.io/anon/pen/vOOKNj I guess you could remove the keydown and commit this.

[patrickhlauke]

But your diff seems to suffer from the same problem I bumped againsts: change event firing twice when using the keyboard, which is undesirable.

I think I managed to simplify the existing code and avoid any major pitfalls... http://codepen.io/patrickhlauke/pen/zGGEEY

[yuan3y]

@patrickhlauke seems like it fixes, finally! tried on chrome & firefox on linux.

[alexw23]

Just to be a pain we should look at replicating checkbox functionality also... I've added non-bootstrap radio/checkboxes for reference as to the normal usability. http://codepen.io/anon/pen/eNNGjO

Edit: it appears all that is missing is the ability to select checkbox by using space bar.

[patrickhlauke]

More hacking around, seems to work now for checkboxes too (but the code is slowly getting a bit fugly, will need some cleaning up/reworking I think) http://codepen.io/patrickhlauke/pen/OVVOGL