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

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[riageek+bugzilla]

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

[(mostly gone) XtC4UaLL [:xtc4uall]]

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

[(mostly gone) XtC4UaLL [:xtc4uall]]

Attachment: Reporter's Testcase

Comment 3

[Alice0775 White]

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]

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]

Attachment: 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]

BTW, line 31 is just a trailing whitespace my editor picked up.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

silverwind, can you check this in yourself, or do you need someone to do it for you?

Comment 9

[silverwind]

It's my first patch, so I'd need someone to check in for me.

Comment 10

[silverwind]

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]

Attachment: 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]

review ping.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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]

No problem. Also, feel free to land it for me, if you like.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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]

That certainly wasn't the intention of the patch - I'll have another look at it.

Comment 17

[Boris Zbarsky [:bzbarsky]]

That behavior change arises from the FLAG_BYMOVEFOCUS to FLAG_BYMOUSE change, for what it's worth.

Comment 18

[silverwind]

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 [:bzbarsky]]

Mochitest and mochitest-chrome, probably... and then probably asking Neil Deakin to review.

Comment 20

[silverwind]

Attachment: 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]

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]

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]

Attachment: Third revision of the patch (whitespace corrected)

review+ carried over.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d61d5cf467ab

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d61d5cf467ab