Button: Fixed #7665 - Radio button & checkboxes ignore mouseclicks for minor mouse movements

[UltCombo]

Changes:

• Removed some duplicated code from the radios' code - let the change handler update the group's display and aria;
• Removed #6970 fix's code, which was actually caused by the now-fixed #5518. #6970's code is not only completely unnecessary now, it was also the main cause of #7665 which this pull request fixes.
• Added a synthetic click to the checkbox/radio inputs when the click event is not fired after a mousedown-mouseup sequence in the same label button.
• Passes all current unit tests (tested in FF, Chrome, IE9, Opera). The button: core Radio group tests weren't passing on Safari, seems relative to calling .click() on a label not triggering the input's change handler in Safari - see fiddle. I've replaced the .click() calls by .simulate( "click" ) and it passes all unit tests now.
• Added unit test for dragged click. Works as expected in all modern browsers but seems to fail in old IE, even though the fix does work in old IE as seen in the fiddle below. Not sure whether this needs fixing or a conditional block.
• As a side effect, the synthetic click also works around bugzilla ticket #608180, toggling the checkboxes every time you click the label button, no matter how fast you constantly click.

Fiddle preview

[mikesherov]

Thanks again, @UltCombo! For future reference, can you please use topic branches when submitting pull requests? It'll be easier to rebase if necessary.

As far as this PR goes, please use the conditional for oldIE in the unit tests, but please ensure it all still works as expected. It also has indentation issues, in that the comments need to be intended to the same level as the code block. Furthermore, it seems like more testing is in order for this. What do you think? it seems like one test for such a change is inadequate.

Thoughts?

[UltCombo]

Oh thanks, I'll start branching for future PRs.

I'll fix the indentation ASAP.

As for the tests, I'm not entirely sure what kind of tests are the most necessary. I was about to make a full-fledged radio buttons behavior test, then I noticed the button_core already has one for radio groups. Guess I'll expand it a bit and add some checkbox tests.

I just noticed that some of my code removal may bug in old IE when calling .click() on a label - it will toggle the checkbox/radio's state but does not fire the input's change event. I'll re-add the click handler as well.

[mikesherov]

@UltCombo, since you noticed the bug, add a test for that as well. That's the approach here. Try to attack the changes you made to make sure they don't introduce any regressions, and so that future changes won't introduce any either.

[UltCombo]

@mikesherov Understood.

There's some other cross-browser inconsistencies in the tests as well. More namely, when calling .click() on a label element:

• In FF, Chrome, Opera and IE9, it will toggle the checkbox and trigger the input's change handler.
• In IE 8 and below, it will toggle the checkbox's checked state but not call its change handler.
• In latest Safari for Windows, it will not toggle the checkbox nor trigger its change handler.

Fiddle

I find it unlikely for developers to call .click() on a label element expecting it to toggle the input's checked state and fire its change handler, even though that's what happens in modern browsers. The issues are:

1. It causes inconsistent display in Safari - no longer happens since PR #841, but can be seen in the fiddle above with a stable release, and will be back if I re-add the manual toggle code.
2. Without the manual toggle code, it fails the radio group tests in old IE due to IE not triggering the input's change handler when triggering the label's .click(). Should we ignore this eccentric behavior and rewrite some of the tests/use IE conditionals, or is this worth of a ticket in the jQuery core?

I'm considering another workaround, using a delayed refresh to ensure that old IE has the correct display after calling .click() on the label element, however I'd have to make some core tests into async ones then. IMO this is unnecessary, as any inconsistency in the behavior of triggering .click() on a label should at the maximum be a minor bug in the jQuery core itself. I'll try to workaround this with IE conditional in the unit tests.

[mikesherov]

I wouldn't want to land any changes that weren't first fixed in core if there's a core bug at the heart of this. The guy who really knows the horror that is browser events is @dmethvin. Perhaps we can leverage his expertise here if there's a workaround to be had?

[UltCombo]

Well, this core bug seems very minor and only affects unit tests in old IE. Of course, if @dmethvin will have free time to lend us a hand, it'd be great.

The only problem is with $('label').click() - I hardly doubt anyone would put such a thing in a live scenario and expect a perfectly cross-browser behavior to automatically toggle the checkbox and fire its change event. Of course, if this is ever fixed in the core to fire the input's change handler when the click event is triggered on a label element for older versions of IE and maybe Safari, then my PR's code would remain unchanged.

Meanwhile, I'll update the unit tests to reflect that.

[UltCombo]

Added a couple tests, all tests now pass in all supported browsers including IE6-8 and Safari. Only issue is that programmatically calling a label's .click() does not trigger the related input's change handler in old IE/Safari nor toggles it in Safari, so I've adapted the tests to take that in consideration.

IMO triggering a click event on a label and expecting the input to toggle itself and fire the change event including old browsers is asking too much and would be set to invalid if reported on the bug tracker, this is mostly a test-only scenario. A sturdy cross-browser way to do this in a production site is to set the checked property followed by triggering the change handlers:

$('input[type=checkbox]').prop('checked', true).change();

That's the correct approach that will never fail in any browser and should be adopted over expecting a checkbox to magically toggle itself and trigger its change event by dispatching a synthetic event to a label element. The latter would be just encouraging bad practices IMHO.

[UltCombo]

I can standardize triggered click on label elements cross-browser by either using another timeout and a change check or propose it to the core by adding 2 flags to $.support - labelClickTogglesInput (for Safari) and labelClickFiresChange (for IE<9 and Safari) - and adding special label click treatment to .trigger() .

IMO it is unnecessary and mostly just encourages bad practices, but I can add it in order to don't generate back-compat issues if needed.

[mikesherov]

@UltCombo, thanks again for the detective work here. I'll be reviewing this a bit more fully this weekend.

[UltCombo]

Alright, I'll add a couple more tests later today.

[UltCombo]

@mikesherov Just did another makeover on the Button code. Calling .click() on a label button now has normalized cross-browser behavior, meaning it can again be used in the unit tests without requiring workarounds. The code looks much cleaner as well.

[UltCombo]

Concluded standardizing the button labels' click handler removing all setTimeouts. Passes all tests in all supported browsers still.

[mikesherov]

Please remove all commented out code lines. They don't need to be there :-)

[UltCombo]

My bad, kept them while testing and ended up leaving it there before committing. Removed now.

[UltCombo]

The only issue that I see with my fix's code above is that click handlers for the labels may not fire for long drags while still toggling the checkbox's checked property.

Maybe utilizing a real button inside the label as in http://jsfiddle.net/2nLqb/1/ could be a better solution as buttons allow for long drags naturally, discarding the mousedown and mouseup handlers. Problem is that IE<=8 and Opera won't change the checkbox input's checked state when clicking on a button inside of the label, so the click behavior normalization code would have to stay. Also having a label element would be redundant then.

[mikesherov]

@UltCombo, thanks again for the work here. Seeing as this got a lot larger, I'm going to wait till after 1.10 is out to land this, as it needs heavy testing. Thanks again!

[UltCombo]

No problem, you're welcome. =] I haven't had much free time lately, I may take one more look to try to make this fix simpler in the future but the code is already very simple relative to what it does.

The only issue that still stands is the label's click handlers not firing for long drag clicks while the change handler does fire for the checkbox/radio input. That can be fixed with some extra handlers, but I find it highly unlikely for any page to have user-bound click handlers to label elements used by the button widget and it's a very minor thing compared to the drag+click bug which currently affects all pages using the button widget to style checkbox/radio inputs.

[IainMNorman]

I'm using your http://ultcombo.github.com/UltButtons/ plugin UltCombo.

I'm also using Knockout.js to bind to a simple radio button array using the checked binding. http://knockoutjs.com/documentation/checked-binding.html

When I click normally without moving at all then the datamodel is updated. If I click and drag then it's not.

[scottgonzalez]

@teknohippy This in not an appropriate place to ask for help with 3rd party plugins.

[UltCombo]

@teknohippy open an issue in the appropriate repo please, and while you're at it provide a test case there so we can solve it.

[UltCombo]

@scottgonzalez The issue reported by tekno may be related to this PR as well though.

Although jQuery has no compromise to work with other libraries, with this PR's code the Button widget will no longer trigger natively attached listeners, meaning some people may be hesitant to upgrade afterwards.

There's also one question left, shouldn't the button widget call $.fn.disableSelection on checkbox/radio labels and anchor elements? As I've previously said, users usually don't expect to be able to select a button's text, but then it may raise accessibility issues as you've commented in the bugtracker ticket. I guess this may deserve a separate ticket/PR.

Also, if this needs rebasing before landing, just comment and I'll update the code.

[scottgonzalez]

.disableSelection() is deprecated and cannot be used in any new code.

[UltCombo]

I wasn't aware of that, thanks for the info. My next plugins will keep using their own version of disableSelection when necessary then.

[mikesherov]

@UltCombo, thanks for being so patient. I'm ready to review this code again. Can you rebase this pull request?

[UltCombo]

Of course. Just when I was trying to decide what to do code for this weekend. =]

[UltCombo]

@mikesherov Ok, I believe I did the rebasing correctly. =] Will use the proper branching for next fixes.

If there's an issue in how I did the merge, just @ me and I'll start a new pull from a branch.

[jzaefferer]

That looks like you did git merge origin/master (or just master) and used "rebase" as the commit message. Rebasing is something else: http://git-scm.com/book/en/Git-Branching-Rebasing

@mikesherov probably more sane to just throw away that merge commit and do the rebase yourself?

[mikesherov]

@jzaefferer There'll be more commits from review. I'd like to start from a rebased position.

@UltCombo, please open a fresh PR from a branch other than master. If you need guidance, the link @jzaefferer provided has all the info you'll need. For now, I'm going to close this one.

[UltCombo]

@mikesherov Yes, I've read that before but had some issues with git rebease at the time. I'll do this right and submit a clean PR at later then.