Closed Bug 608180 Opened 10 years ago Closed 7 years ago
Double/rapid clicking a checkbox label does not work as expected
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.
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>
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
It's my first patch, so I'd need someone to check in for me.
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
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.
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?
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?
Mochitest and mochitest-chrome, probably... and then probably asking Neil Deakin to 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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.