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 comment:1
owner: | → eason |
---|---|
status: | new → pending |
Changed April 12, 2011 07:46AM UTC by comment:2
resolution: | → invalid |
---|---|
status: | pending → closed |
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 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 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 comment:5
milestone: | TBD → 1.9 |
---|---|
resolution: | invalid |
status: | closed → reopened |
Changed August 01, 2011 02:16PM UTC by comment:6
status: | reopened → open |
---|---|
summary: | ui.button double clicking issue → Button: Incorrect state after double click in Firefox |
Changed October 11, 2012 02:43PM UTC by comment:9
milestone: | 1.9.0 → 1.11.0 |
---|
Changed November 24, 2012 07:03PM UTC by 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 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 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 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 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 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 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 comment:17
resolution: | → fixed |
---|---|
status: | open → closed |
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 comment:18
milestone: | 1.11.0 → 1.10.0 |
---|
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!