Closed Bug 608180 Opened 10 years ago Closed 7 years ago

Double/rapid clicking a checkbox label does not work as expected


(Core :: Layout: Form Controls, defect, minor)

Not set





(Reporter: riageek+bugzilla, Assigned: me)



(Keywords: regression, testcase)


(2 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv: Gecko/20101026 Firefox/3.6.12
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv: Gecko/20101026 Firefox/3.6.12

When using a checkbox element with a label the behavior of double clicking the label vs. the checkbox is different.

When double clicking the checkbox itself the state correctly returns to its original checked state. If the checkbox was checked before any action it is checked after the double click which essentially toggles the state twice.

When double clicking the label associated with the checkbox however the state is only toggled once which results in an incorrect state.

The text of the label is also selected which is not incorrect but may have something to do with the problem.

While this problem may seem minor it is causing a lot of problems with Javascript libraries which check the value of a hidden checkbox. The hidden checkbox can become out of sync with a visual representation of the checked value. 

Reproducible: Always

Steps to Reproduce:
1. Create a very simple HTML document with the following HTML:

<label style="border: 1px black solid" for="cbTest"><span>This is a label</span></label>
<input id="cbTest" name="cbTestInput" type="checkbox">

2. Load the document in Firefox.
3. Rapidly click the checkbox itself. Notice the correct behavior.
4. Rapidly click the checkbox label and notice the click lag and incorrect checkbox state.
Actual Results:  
The checkbox does not toggle correctly when rapidly clicking the label.

Expected Results:  
The behavior when clicking the checkbox and the checkbox label should be the same.
I can reproduce this with 1.9.1 (and later), but not with 1.9.0 (on WinXP).
Thus this is a Regression against 1.9.0.
Severity: major → minor
Product: Firefox → Core
QA Contact: general → general
Whiteboard: dupeme
Version: unspecified → 1.9.1 Branch
Regression window for m-c build:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090323 Namoroka/3.6a1pre ID:20090323044646
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090324 Namoroka/3.6a1pre ID:20090324044213

Regression window for 1.9.1 branch;
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b4pre) Gecko/20090415 Shiretoko/3.5b4pre ID:20090415044329
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b4pre) Gecko/20090416 Shiretoko/3.5b4pre ID:20090416044319

Most suspected bug:
Bug 382369 - Cannot select text in <label> associated with a <select>
Blocks: 382369
Component: General → Layout: Form Controls
Ever confirmed: true
QA Contact: general → layout.form-controls
Whiteboard: dupeme
Version: 1.9.1 Branch → Trunk
This bug still applies in the current version, and is rather annoying when you have checkboxes styled through their label (with the real checkbox hidden) and quickly want to toggle them on/off.

I'm pretty sure the bug has to do with a double-click timeout preventing the checkbox from getting triggered.
This patch allows rapid toggling of checkboxes through their label. Double clicks on the label will still select the text fine, while toggling the checkbox on each click. This is the same behavior Chrome shows.
Attachment #760021 - Flags: review?(bzbarsky)
BTW, line 31 is just a trailing whitespace my editor picked up.
Comment on attachment 760021 [details] [diff] [review]
Allow rapid clicks on label elements to go through while still allowing text selection.

Seems reasonable.
Attachment #760021 - Flags: review?(bzbarsky) → review+
silverwind, can you check this in yourself, or do you need someone to do it for you?
Assignee: nobody → silv3rwind
Flags: needinfo?(silv3rwind)
It's my first patch, so I'd need someone to check in for me.
Flags: needinfo?(silv3rwind)
Keywords: checkin-needed
I've ran the mochitests, and encountered failures in content/events/test/test_bug409604.html. Investigating right now.

(focus) unexpected element: test for legend expected: test for label
(focus) unexpected element: test text expected: test for legend
(focus) unexpected element: p expected: m
(focus) unexpected element: undefined expected: test for label
(focus) unexpected element: y expected: p
(focus) unhandled elements remaining: x,y
Keywords: checkin-needed
Attachment #760021 - Attachment is obsolete: true
Reworked patch because the previous version wasn't passing mochitests. Switched the flags for SetFocus from FLAG_BYMOVEFOCUS to FLAG_BYMOUSE as the docs seem to indicate that the element is scrolled to unless FLAG_NOSCROLL is given.

The patch passes mochitests now and I've tested a build locally to confirm the element is still scrolled to.
Attachment #760131 - Flags: review?(bzbarsky)
review ping.
Sorry for the lag here.  I've been dealing with other reviews...  I'll try to get to this in the next day or so.
No problem. Also, feel free to land it for me, if you like.
This patch seems to change behavior in this testcase:

  <label for="xyz">Click me</label><br>
  <input id="xyz" value="Do I get selected?">

the old behavior is to select the text in the input, but the new one is to not select it...  Do we want that change?
Flags: needinfo?(enndeakin)
That certainly wasn't the intention of the patch - I'll have another look at it.
That behavior change arises from the FLAG_BYMOVEFOCUS to FLAG_BYMOUSE change, for what it's worth.
The problem with FLAG_BYMOVEFOCUS for the original test case was, that on doubleclick, the text gets deselected immediately by the second click event, which isn't trapped any more by event->clickCount > 1.

From what I've tried so far, supplying both flags looks promising, but I still have to run it through the test. Are there any other tests beside mochitest that you would recommend?
Attachment #760131 - Attachment is obsolete: true
Attachment #760131 - Flags: review?(bzbarsky)
Mochitest and mochitest-chrome, probably... and then probably asking Neil Deakin to review.
Attached patch Third revision of the patch (obsolete) — Splinter Review
Third revisions of the patch attached. This keeps FLAG_BYMOVEFOCUS again and passes both testcases mentioned in this bug, as well as these (which were problematic before):

mochitest-plain content/events/test/test_bug409604.html
mochitest-chrome dom/tests/mochitest/chrome/test_focus.xul

Once I've ran the full mochitest-plain and mochitest-chrome, I'll put this up for review.
Comment on attachment 762297 [details] [diff] [review]
Third revision of the patch

Alright, the patch passes all tests in mochitest-plain and mochitest-chrome, so I think its ready for review.
Attachment #762297 - Flags: review?(enndeakin)
Comment on attachment 762297 [details] [diff] [review]
Third revision of the patch

>+              nsCOMPtr<nsIDOMElement> elem = do_QueryInterface(content);
>+              fm->SetFocus(elem,nsIFocusManager::FLAG_BYMOVEFOCUS);

Don't remove the space after the comma.
Attachment #762297 - Flags: review?(enndeakin) → review+
review+ carried over.
Attachment #762297 - Attachment is obsolete: true
Attachment #762704 - Flags: review+
Keywords: checkin-needed
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Flags: needinfo?(enndeakin)
You need to log in before you can comment on or make changes to this bug.