Skip to main content

Search and Top Navigation

#5518 closed bug (fixed)

Opened April 14, 2010 11:15PM UTC

Closed November 27, 2012 01:16AM UTC

Last modified November 27, 2012 01:19AM UTC

Button: Incorrect state after double click in Firefox

Reported by: eason Owned by: eason
Priority: minor Milestone: 1.10.0
Component: ui.button Version: 1.8
Keywords: Cc:
Blocked by: Blocking:
Description

Normally a check box is supposed to be single clicked only. When double clicking on a check box, its checked status changes as if single clicking on it. But When double clicking on a check box rendered as a jquery-ui toggle button, the checked status changes, but the look of the widget does not change, i.e., it does not "toggle".

Attachments (0)
Change History (18)

Changed March 28, 2011 07:53PM UTC by danbhfive comment:1

owner: → eason
status: newpending

Hey there. I'm very sorry but I had trouble recreating your issue. Here is my work: http://jsbin.com/axube5/6/

Please provide a valid test case and reopen. Thanks!

Changed April 12, 2011 07:46AM UTC by trac-o-bot comment:2

resolution: → invalid
status: pendingclosed

Because we get so many tickets, we often need to return them to the initial reporter for more information. If that person does not reply within 14 days, the ticket will automatically be closed, and that has happened in this case. If you still are interested in pursuing this issue, feel free to add a comment with the requested information and we will be happy to reopen the ticket if it is still valid. Thanks!

Changed July 22, 2011 04:05PM UTC by mmartin comment:3

Not sure, why the author of this thicket dissapeared.. unfortunately I can't reopen the issue.

Here is a valid test case: http://jsbin.com/axube5/7/

Try double-clicking the button, so the ui-state- goes out of sync with checkbox.

(Note: make several seperate double-clicks, instead of many clicks in a row)

Here is my pull request with a fix for this: https://github.com/jquery/jquery-ui/pull/402/

Changed July 22, 2011 04:59PM UTC by mmartin comment:4

Forgot to mention that Opera and Chrome are not affected by this bug.

I can reproduce this only in Firefox.

Changed August 01, 2011 02:16PM UTC by scottgonzalez comment:5

milestone: TBD1.9
resolution: invalid
status: closedreopened

Changed August 01, 2011 02:16PM UTC by scottgonzalez comment:6

status: reopenedopen
summary: ui.button double clicking issueButton: Incorrect state after double click in Firefox

Changed November 21, 2011 01:30PM UTC by scottgonzalez comment:7

#7804 is a duplicate of this ticket.

Changed November 21, 2011 04:44PM UTC by scottgonzalez comment:8

#5960 is a duplicate of this ticket.

Changed October 11, 2012 02:43PM UTC by scottgonzalez comment:9

milestone: 1.9.01.11.0

Changed November 24, 2012 07:03PM UTC by Ult Combo comment:10

_comment0: Why is this necessary inside a click handler for checkboxes' buttons: (lines #150-151 of ui.button) \ \ {{{ \ $( this ).toggleClass( "ui-state-active" ); \ self.buttonElement.attr( "aria-pressed", self.element[0].checked ); \ }}} \ \ When `that.refresh()` is called in the checkbox's `change` handler? The `change` handler alone will take care of refreshing the display of the button to the appropriate state as well as setting its `aria-pressed`. Correct me if I'm wrong, but the 2 lines above seem to serve for nothing but messing up the button display state on Firefox. \ \ \ #7804 1353783847501043
_comment1: Why is this necessary inside a click handler for checkboxes' buttons: (lines #150-151 of ui.button) \ \ {{{ \ $( this ).toggleClass( "ui-state-active" ); \ self.buttonElement.attr( "aria-pressed", self.element[0].checked ); \ }}} \ \ When `that.refresh()` is called in the checkbox's `change` handler? The `change` handler alone will take care of refreshing the display of the button to the appropriate state as well as setting its `aria-pressed`. Correct me if I'm wrong, but the 2 lines above seem to serve for nothing but messing up the button display state on Firefox. \ \ #7804 Also suggests removing the line that causes the display bug.1353783877943795
_comment2: Why is this necessary inside a click handler for checkboxes' buttons: (lines 150-151 of ui.button) \ \ {{{ \ $( this ).toggleClass( "ui-state-active" ); \ self.buttonElement.attr( "aria-pressed", self.element[0].checked ); \ }}} \ \ When `that.refresh()` is called in the checkbox's `change` handler? The `change` handler alone will take care of refreshing the display of the button to the appropriate state as well as setting its `aria-pressed`. Correct me if I'm wrong, but the 2 lines above seem to serve for nothing but messing up the button display state on Firefox. \ \ #7804 Also suggests removing the line that causes the display bug.1353783980434457

Why is this necessary inside a click handler for checkboxes' buttons: (lines 150-151 of ui.button)

    $( this ).toggleClass( "ui-state-active" );
    self.buttonElement.attr( "aria-pressed", self.element[0].checked );

When that.refresh() is called in the checkbox's change handler? The change handler alone will take care of refreshing the display of the button to the appropriate state as well as setting its aria-pressed. Correct me if I'm wrong, but the 2 lines above seem to serve for nothing but messing up the button display state on Firefox.

#7804 also suggests removing the line that causes the display bug.

Changed November 24, 2012 07:22PM UTC by mikesherov comment:11

Ult Combo, thanks for the detective work. Can you provide a pull request that comes with unit tests proving it's superfluous and that it fixes this bug? I'm also not sure why that code is there myself either. I was playing with this same bug the other day and also couldn't see what it was supposed to be doing while reviewing this PR: https://github.com/jquery/jquery-ui/pull/402

Changed November 25, 2012 12:48AM UTC by Ult Combo comment:12

After commenting out these 2 lines, the code passes all current unit tests for ui.button and the initial field test seems to work nicely.

Of course, as that click handler would serve for nothing then, to not leave an empty block it is better to remove the if ( this.type === "checkbox" ) { block completely and change the last else by else if ( this.type !== "checkbox" ) {.

Not sure what kind of unit tests I should add, plus I'm having a busy weekend, but should be able to submit a pull request by tomorrow.

Changed November 25, 2012 03:32AM UTC by mikesherov comment:13

Ult Combo, perfect. A unit test showing the fiddle above doesn't fail should suffice, and it couldn't hurt to add checks for aria-pressed to that test. No rush, enjoy the weekend!

Changed November 25, 2012 05:21PM UTC by Ult Combo comment:14

_comment0: Just noticed that we can't comment out the entire click handler for checkboxes because the `return false;` inside of the handler will cause jQuery to trigger `event.preventDefault()` and `event.stopPropagation()`, without these the current fix for #6970 wouldn't work properly, therefore it is better to just comment out those 2 lines to fix this issue. \ \ The fix for #6970 should be revamped, as how it currently stands, it blocks #7665 which I'm also working on. As different bugs should have different commits, I'll prepare a fiddle for this bug fix and do some more testing.1353864183221310

Just noticed that we can't comment out the entire click handler for checkboxes because the return false; inside of the handler will cause jQuery to trigger event.preventDefault() and event.stopPropagation(), without these the current fix for #6970 wouldn't work properly, therefore it is better to just comment out those 2 lines to fix this issue.

The fix for #6970 should be revamped, as how it currently stands, it blocks #7665 which I'm also working on. As different bugs should have different commits, I'll make a fiddle for this bug fix and test it a little more.

Changed November 25, 2012 06:35PM UTC by mikesherov comment:15

Ult Combo, it's fine if one commit fixes two bugs as long as the commit message reflects that. Thanks again for the work here!

Changed November 25, 2012 06:41PM UTC by Ult Combo comment:16

_comment0: Thanks for the clarification. `=]` \ \ Here's my [PR](https://github.com/jquery/jquery-ui/pull/841) with some more detail and a fiddle. Review it when you have time and we can make the necessary changes if any.1353868934235178

Thanks for the clarification. =]

Here's my PR with some more detail and a fiddle. Review it when you have time and we can make the necessary changes if any.

Changed November 27, 2012 01:16AM UTC by Fabrício Matté comment:17

resolution: → fixed
status: openclosed

Button: Let change handler handle display and aria update. Fixed #5518 - Button: Incorrect state after double click in Firefox

Changeset: caacf8f5041ad434b8e0dd1069936eb91e4c3394

Changed November 27, 2012 01:19AM UTC by mikesherov comment:18

milestone: 1.11.01.10.0