Button: Fixed #5518 - Incorrect state after double click in Firefox

[UltCombo]

Fiddle

Passes all current unit tests for ui.button.

Tested on all major browsers: Chrome 23 Stable - 25 Canary, Firefox 17 Stable - 20 Nightly, Opera 12.11, Safari 5.1.7, IE6-9 on Windows 7 Ultimate 64 bits (using IE Tester for IE6-8; could use a VM but IE Tester hasn't failed me for such basic JS interaction so far).

As commented on #5518, there's apparently no reason to have the toggleClass inside the checkboxes' click handler, as its change handler will call that.refresh() updating the label button's display and aria to the correct state.

And if anyone is wondering why the checkbox doesn't toggle twice when you double click the label button on Firefox, it is Firefox's default behavior - see this minimalist fiddle.

[mikesherov]

@UltCombo, for future reference, browserstack.com gives you IE virtualization in the cloud. It's my best friend. Also, if you can turn your fiddle into a unit test, that's all that's needed for this to land. Thanks again!

[UltCombo]

Wasn't aware of browserstack, thanks for the heads up!

I have close to no experience with qUnit, but I'll give it a try. =]

[UltCombo]

Tried making a test with pure JS but so far, triggering .click() twice on a label will toggle the checkbox twice, as well as dispatching a native synthetic event to it twice will also toggle the checkbox twice. Both $.simulate and $.fireEvent plugin didn't help either.

The bug only happens when manually (through Firefox UI) double clicking a checkbox's label on Firefox, in this case a Visual Test would be more fitting, correct?

[UltCombo]

I've added the Visual Test in a new file tickets.html in the button category. As there is already a tickets group in the unit tests, I thought it could fit in the Visual tests as well. However, maybe this one could go under a new "cross-browser" category as it affects mainly Firefox? Well, that's not up to me.

Feel free to post feedback or if I have to add/fix anything. =]

[UltCombo]

Also, this is a 2 years old Firefox bug, see https://bugzilla.mozilla.org/show_bug.cgi?id=608180

Hence it seems nearly impossible to reproduce in an unit test.

[mikesherov]

Thanks for looking into it. Can you sign our CLA if you haven't already: http://jquery.github.com/cla.html

[mikesherov]

@UltCombo, sorry for being so daft, but it doesn't look like there is any tests that covers aria_pressed and checked for a single click case. Can we at least cover that with a unit test? If I'm wrong, I apologize.

[UltCombo]

I did sign it about 12 hours ago with my real name "Fabrício Matté" (which is public on my profile). It's past my sleep time, but I should be able to check it and add the corresponding test tomorrow. In fact, it shouldn't take more than a couple minutes to add such a test.

ps. There's nothing daft about that, even though I do not use qUnit on an often basis, I understand perfectly fine the importance of unit tests.

[UltCombo]

Some sleep procrastination as usual and committed the requested unit test. Not entirely sure whether that was the most appropriate file and test naming, feel free to comment/edit it as necessary.

[mikesherov]

@UltCombo, I'm going to land this without the visual tests. Thanks again for your hard work and effort here!

[mikesherov]

I'm also going to remove the code instead of leaving it commented out.

[scottgonzalez]

The test should go in button_core.js, as button_events.js is for events triggered by the widget.

Also, I wonder if radios have a similar issue since we run the same code (plus some more).

[mikesherov]

Thanks, landed in caacf8f

[UltCombo]

@mikesherov yes, I also don't think such a small bug would require visual tests. There's still the fiddle for testing in case a regression ever happens. You're welcome. =]

@scottgonzalez I wasn't sure what button_events.js was for, thanks for the clarification. As this is basic behavior, it makes sense for the core to be the more appropriate test file.

About the radios, what I can say is that the bug is not present. The problem with the checkboxes in Firefox was the duplicated code, that is, the label button's click handler was toggling its class right before the input's change handler updated it to the correct state. When you double click the label button in Firefox, the click handler for the button fires two times, but the input's change event does not fire the second time you click (because it keeps the same checked state, as linked to bugzilla ticket #608180). Check out this fiddle for a more practical explanation.

There may be duplicated code in the radio button's code though, I may investigate that later this week while I try to fix some other button-related bugs. Even if there's duplicated code, it doesn't generate a bug as, when radios are clicked, the clicked one receives the active state while others from the same group have it removed. Even if it's done twice, the final outcome will be the same. It might need a clean-up though.

[scottgonzalez]

That makes sense. Thanks for looking into it.