Opened 10 years ago

Closed 7 years ago

Last modified 7 years ago

#5518 closed bug (fixed)

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

Change History (18)

comment:1 Changed 9 years ago by danbhfive

Owner: set to 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!

comment:2 Changed 9 years ago by trac-o-bot

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!

comment:3 Changed 8 years ago by mmartin

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/

comment:4 Changed 8 years ago by mmartin

Forgot to mention that Opera and Chrome are not affected by this bug. I can reproduce this only in Firefox.

comment:5 Changed 8 years ago by Scott González

Milestone: TBD1.9
Resolution: invalid
Status: closedreopened

comment:6 Changed 8 years ago by Scott González

Status: reopenedopen
Summary: ui.button double clicking issueButton: Incorrect state after double click in Firefox

comment:7 Changed 8 years ago by Scott González

#7804 is a duplicate of this ticket.

comment:8 Changed 8 years ago by Scott González

#5960 is a duplicate of this ticket.

comment:9 Changed 7 years ago by Scott González

Milestone: 1.9.01.11.0

comment:10 Changed 7 years ago by Ult Combo

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.

Last edited 7 years ago by Ult Combo (previous) (diff)

comment:11 Changed 7 years ago by mikesherov

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

comment:12 Changed 7 years ago by Ult Combo

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.

comment:13 Changed 7 years ago by mikesherov

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!

comment:14 Changed 7 years ago by Ult Combo

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.

Last edited 7 years ago by Ult Combo (previous) (diff)

comment:15 Changed 7 years ago by mikesherov

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!

comment:16 Changed 7 years ago by Ult Combo

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.

Last edited 7 years ago by Ult Combo (previous) (diff)

comment:17 Changed 7 years ago by Fabrício Matté

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

comment:18 Changed 7 years ago by mikesherov

Milestone: 1.11.01.10.0
Note: See TracTickets for help on using tickets.