The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla24

Status

()

Core
Layout: Form Controls
--
minor
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: riageek+bugzilla, Assigned: silverwind)

Tracking

({regression, testcase})

Trunk
mozilla24
x86
All
regression, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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.
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
Component: General → General
Keywords: regression, regressionwindow-wanted, testcase
Product: Firefox → Core
QA Contact: general → general
Whiteboard: dupeme
Version: unspecified → 1.9.1 Branch
Created attachment 487173 [details]
Reporter's Testcase

Comment 3

7 years ago
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>
Blocks: 382369
Status: UNCONFIRMED → NEW
Component: General → Layout: Form Controls
Ever confirmed: true
Keywords: regressionwindow-wanted
QA Contact: general → layout.form-controls
Whiteboard: dupeme
Version: 1.9.1 Branch → Trunk
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
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.
Attachment #760021 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

4 years ago
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)
(Assignee)

Comment 9

4 years ago
It's my first patch, so I'd need someone to check in for me.
Flags: needinfo?(silv3rwind)
Keywords: checkin-needed
(Assignee)

Comment 10

4 years ago
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
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Attachment #760021 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
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.
Attachment #760131 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

4 years ago
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.
(Assignee)

Comment 14

4 years ago
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)
(Assignee)

Comment 16

4 years ago
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.
(Assignee)

Comment 18

4 years ago
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?
(Assignee)

Updated

4 years ago
Attachment #760131 - Attachment is obsolete: true
Attachment #760131 - Flags: review?(bzbarsky)
Mochitest and mochitest-chrome, probably... and then probably asking Neil Deakin to review.
(Assignee)

Comment 20

4 years ago
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.
(Assignee)

Comment 21

4 years ago
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 22

4 years ago
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+
(Assignee)

Comment 23

4 years ago
Created attachment 762704 [details] [diff] [review]
Third revision of the patch (whitespace corrected)

review+ carried over.
Attachment #762297 - Attachment is obsolete: true
Attachment #762704 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d61d5cf467ab
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d61d5cf467ab
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Updated

4 years ago
Flags: needinfo?(enndeakin)
You need to log in before you can comment on or make changes to this bug.