Last Comment Bug 608180 - Double/rapid clicking a checkbox label does not work as expected
: Double/rapid clicking a checkbox label does not work as expected
Status: RESOLVED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: x86 All
: -- minor with 2 votes (vote)
: mozilla24
Assigned To: silverwind
:
Mentors:
Depends on:
Blocks: 382369
  Show dependency treegraph
 
Reported: 2010-10-28 20:16 PDT by riageek+bugzilla
Modified: 2013-06-18 10:39 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reporter's Testcase (148 bytes, text/html)
2010-10-30 08:36 PDT, XtC4UaLL [:xtc4uall]
no flags Details
Allow rapid clicks on label elements to go through while still allowing text selection. (2.63 KB, patch)
2013-06-07 17:16 PDT, silverwind
bzbarsky: review+
Details | Diff | Review
Allow rapid clicks on label elements to go through while still allowing text selection. (2.87 KB, patch)
2013-06-08 11:31 PDT, silverwind
no flags Details | Diff | Review
Third revision of the patch (3.55 KB, patch)
2013-06-13 13:52 PDT, silverwind
enndeakin: review+
Details | Diff | Review
Third revision of the patch (whitespace corrected) (3.54 KB, patch)
2013-06-14 08:06 PDT, silverwind
me: review+
Details | Diff | Review

Description riageek+bugzilla 2010-10-28 20:16:26 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.12) 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.
Comment 1 XtC4UaLL [:xtc4uall] 2010-10-30 08:36:02 PDT
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.
Comment 2 XtC4UaLL [:xtc4uall] 2010-10-30 08:36:20 PDT
Created attachment 487173 [details]
Reporter's Testcase
Comment 3 Alice0775 White 2010-11-01 11:53:47 PDT
Regression window for m-c build:
Works:
http://hg.mozilla.org/mozilla-central/rev/d74e551f46a3
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090323 Namoroka/3.6a1pre ID:20090323044646
Fails:
http://hg.mozilla.org/mozilla-central/rev/6cfe70091cfc
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090324 Namoroka/3.6a1pre ID:20090324044213
Pushlog;
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d74e551f46a3&tochange=6cfe70091cfc

Regression window for 1.9.1 branch;
Works:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0ac64f9870d4
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b4pre) Gecko/20090415 Shiretoko/3.5b4pre ID:20090415044329
Fails:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c0fb52b14fd8
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b4pre) Gecko/20090416 Shiretoko/3.5b4pre ID:20090416044319
Pushlog:
http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=0ac64f9870d4&tochange=c0fb52b14fd8

Most suspected bug:
Bug 382369 - Cannot select text in <label> associated with a <select>
Comment 4 silverwind 2013-06-07 06:51:15 PDT
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.
Comment 5 silverwind 2013-06-07 17:16:35 PDT
Created attachment 760021 [details] [diff] [review]
Allow rapid clicks on label elements to go through while still allowing text selection.

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.
Comment 6 silverwind 2013-06-07 17:25:46 PDT
BTW, line 31 is just a trailing whitespace my editor picked up.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-06-07 21:41:26 PDT
Comment on attachment 760021 [details] [diff] [review]
Allow rapid clicks on label elements to go through while still allowing text selection.

Seems reasonable.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-06-07 21:41:53 PDT
silverwind, can you check this in yourself, or do you need someone to do it for you?
Comment 9 silverwind 2013-06-08 03:58:50 PDT
It's my first patch, so I'd need someone to check in for me.
Comment 10 silverwind 2013-06-08 06:17:23 PDT
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
Comment 11 silverwind 2013-06-08 11:31:44 PDT
Created attachment 760131 [details] [diff] [review]
Allow rapid clicks on label elements to go through while still allowing text selection.

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.
Comment 12 silverwind 2013-06-11 04:48:36 PDT
review ping.
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-06-11 07:32:35 PDT
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.
Comment 14 silverwind 2013-06-11 07:57:31 PDT
No problem. Also, feel free to land it for me, if you like.
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-06-11 20:18:11 PDT
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?
Comment 16 silverwind 2013-06-12 09:04:18 PDT
That certainly wasn't the intention of the patch - I'll have another look at it.
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-06-12 09:46:32 PDT
That behavior change arises from the FLAG_BYMOVEFOCUS to FLAG_BYMOUSE change, for what it's worth.
Comment 18 silverwind 2013-06-12 12:07:22 PDT
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?
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-06-12 12:19:13 PDT
Mochitest and mochitest-chrome, probably... and then probably asking Neil Deakin to review.
Comment 20 silverwind 2013-06-13 13:52:56 PDT
Created attachment 762297 [details] [diff] [review]
Third revision of the patch

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 21 silverwind 2013-06-14 07:05:14 PDT
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.
Comment 22 Neil Deakin 2013-06-14 07:40:49 PDT
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.
Comment 23 silverwind 2013-06-14 08:06:24 PDT
Created attachment 762704 [details] [diff] [review]
Third revision of the patch (whitespace corrected)

review+ carried over.
Comment 24 Ryan VanderMeulen [:RyanVM] 2013-06-14 19:54:25 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d61d5cf467ab
Comment 25 Ryan VanderMeulen [:RyanVM] 2013-06-15 18:45:57 PDT
https://hg.mozilla.org/mozilla-central/rev/d61d5cf467ab

Note You need to log in before you can comment on or make changes to this bug.