Closed Bug 685197 Opened 13 years ago Closed 13 years ago

Multiple select options become unselected when tapping on multi-select

Categories

(Firefox for Android Graveyard :: General, defect)

Firefox 9
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 10

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

References

Details

(Keywords: regression, testcase)

Attachments

(3 files, 1 obsolete file)

Attached file testcase
See testcase, when tapping on the multi-select, all the selected options become unselected, except the one that was tapped upon.

This is a regression from bug 681640.
This could be fixed by suppressing mouse events for select/option elements.  We could either do this in content.js, or perhaps we should pass the mouse coordinates to formAssistant.open and let it generate its own events depending on the element type.

Assigning this to myself because I can fix this when some of my current bugs are done, but please feel free to steal it (hint, hint).
Assignee: nobody → mbrubeck
OS: Windows 7 → All
Hardware: x86 → All
Version: Trunk → Firefox 9
Attached file testcase2
I thought that setting css of pointer-events: none; to options optgroups for mulit-select lists might be a good solution, but it turns out it doesn't work in empty areas and on optgroups in multi-selects.
Attached patch patch (obsolete) — Splinter Review
Otoh, it might be good enough. Empty areas of mulit-select (listcontrols) and optgroups of multi-select are not that common.
Is this really a regression?
Yes.
Attached patch patch2Splinter Review
Ok, this disables click events on selects, which is probably the best thing to do. We did it before bug 681640 was fixed (and on more form controls).

I think with this patch that bug 684723 could be fixed safely. Because I now use:
+  ContentTouchHandler.tapDown(rect.left + 1, rect.top + 1);
   ContentTouchHandler.tapSingle(rect.left + 1, rect.top + 1);
+  ContentTouchHandler.tapUp(rect.left + 1, rect.top + 1);
instead of the single tapSingle method that was used before.
This allows the mouse events to work. (the reason is that _highlightElement member isn't set with only a tapSingle method)
That's why I removed this comment:
-  // Sending a synthesized event to the combo is not working

A part of the browser_select.js test is commented out, that is because of bug 689403. Once that bug is fixed, that part can be uncommented.

One weird thing I noticed is that I tried to use:
let option = new_tab.browser.contentDocument.getElementById("listbox-multiselect").options[0];
But that caused test failures, because for some reason the click was fired at a completely different spot than the option element that I chose.

I'm seeing various failures in my browser-chrome test run on Windows, but I see them also without the patch.
However, I would appreciate it if somebody else could verify this doesn't cause test failures.
Attachment #559182 - Attachment is obsolete: true
Attachment #562789 - Flags: review?(mbrubeck)
Comment on attachment 562789 [details] [diff] [review]
patch2

Please push the patch to Try before landing.
Attachment #562789 - Flags: review?(mbrubeck) → review+
I did:
- hg qnew 685197.diff
- hg qref --message "try: -b do -p linux-android,linux-maemo5-gtk,linux-maemo5-
qt,linux-mobile,win32-mobile,macosx-mobile -u all -t none"
- hg push -f try

My .hgrc file is this:
[extensions]
mq =

[defaults]
qnew = -U

[mq]
plain = True

[ui]
username = Martijn Wargers <mwargers@mozilla.com>

[diff]
git = 1

[paths]
try = ssh://<mwargers@mozilla.com>@hg.mozilla.org/try

At this point:
$ hg push -f try
I get:
remote: Permission denied (publickey,gssapi-with-mic).
abort: no suitable response from remote hg!

What am I doing wrong here?
Where is my public key stored? In .ssh I guess? It's the id_rsa.pub file?
Assignee: mbrubeck → martijn.martijn
Comment on attachment 562789 [details] [diff] [review]
patch2

Sorry, tests/browser_select.js is failing for me on Linux desktop with "JavaScript error: chrome://mochitests/content/browser/mobile/chrome/tests/browser_select.js, line 64: option is null".  Let's fix that before checking this in.
Attachment #562789 - Flags: review+ → review-
Weird error. I have just rerun the browser-chrome tests with this patch, but this test passes just fine.
That js error indicates that new_tab.browser.contentDocument.getElementById("listbox-multiselect") is null, which seems to indicate that page has closed accidentally for some reason? Or maybe some other page has loaded?

I guess this code:
EventUtils.synthesizeKey("VK_ESCAPE", {}, window);
might have closed the page also, instead of just closing the select helper popup.
Comment on attachment 562789 [details] [diff] [review]
patch2

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #11)
> I guess this code:
> EventUtils.synthesizeKey("VK_ESCAPE", {}, window);
> might have closed the page also, instead of just closing the select helper
> popup.

Yes, it looks that was the problem.  This is probably caused by bug 684558 and related bugs.  Changing this to EventUtils.sendKey fixes the problem and the test passes for me on desktop.  r=mbrubeck with that change.  I pushed the revised test to Try for testing on Android:

https://tbpl.mozilla.org/?tree=Try&rev=9f3e9a3d43e9
Attachment #562789 - Flags: review- → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8866ebfa3c9
Status: NEW → ASSIGNED
Flags: in-testsuite+
Target Milestone: --- → Firefox 10
https://hg.mozilla.org/mozilla-central/rev/c8866ebfa3c9
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
This issue is not reproducible on build:
Mozilla/5.0 (Android;Linux armv7l;rv:10.0a1)Gecko/20111009 Firefox/10.0a1 Fennec/10.0a1
Device: HTC Desire
OS: Android 2.2
Status: RESOLVED → VERIFIED
Thanks for your help, Matt!
Depends on: 695081
No longer depends on: 695081
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: