1121515 - Tapping under the last option in a combo box will select the last option and deselect all the others

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Flaviu Cos]

Environment: 
Device: Samsung Galaxy S4 (Android 4.4.2);
Build: Nightly 38.0a1 (2015-01-14);

Steps to reproduce:
1. Go to bugzilla.mozilla.org;
2. Tap on Search button;
3. Tap on Advanced Search;
4. Tap on the first combo box;
5. Select some of the available options from the combo box;
6. Tap on Done button;
7. Tap again on the combo box but this time on the empty space under the last option.

Expected result:
The combo box is opened and the options selected at step 5 are selected.

Actual result:
The combo box is opened and only the last option is selected. 

Notes:
Please check the video: http://youtu.be/ViCQ8ZP7YC0

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

It would be good to test this on combo boxes that are on other sites. Maybe it requires a scrollable list?

Comment 2

[Flaviu Cos]

(In reply to Kevin Brosnan [:kbrosnan] from comment #1)
> It would be good to test this on combo boxes that are on other sites. Maybe
> it requires a scrollable list?

The issue is reproducible also on http://www.gpo.gov/fdsys/search/advanced/advsearchpage.action

Comment 3

[swaroop.rao]

I can take this up, if it's available. I'm not yet sure how to go about fixing it, but I can start looking at it. If you can provide some pointers, that would probably save me some time.

Comment 4

[Mark Capella [:capella]]

This is interesting ...

   Mobile behaves this way, because that's how core Gecko is designed. Using [0] as a testcase on desktop or Mobile, you can see that the code at [1] returns the last item in the list on mouseclick into empty space.

   This seemingly sets |element.selectedIndex|, before firing the element.onClick() event where we notice it via listener on the Mobile side [2].

   Chrome desktop however handles this differently, and selects / returns nothing. Complicating things somewhat, it finesses the situation on Mobile by changing the basic element rendering [3].

   Sooooooooo, not sure if this is evangelism / something we can change in core so mobile "just works", something that is truly a Mobile only fix, or a combination of both sides.

   I'm poking roc for ni? as I see his name as previous reviewer of |nsListControlFrame.cpp| ...

[0] https://www.dropbox.com/s/ukvqqq7mq4mglyw/test_bug1121515.html?dl=0
[1] http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp?rev=79800010be78&mark=1775-1776,1783-1783#1725
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=057f0bd4075c&mark=5114-5114#5099
[3] https://www.dropbox.com/s/ejbgq6hr8w1dus4/test_bug1121515.mp4?dl=0

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know the mobile combobox code. I'm surprised that that line of code is hit at all on mobile. Maybe we should remove it on mobile on desktop; I don't think it's actually helpful on desktop.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

on mobile "and" desktop

Comment 7

[Mark Capella [:capella]]

Attachment: bug1121515.diff

That works for me!

This provides Chrome desktop compatibility, and solves the issue on the Android side.

If you can signoff on desktop, I'll get wesj to approve Mobile next.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8632437 [details] [diff] [review]
bug1121515.diff

Review of attachment 8632437 [details] [diff] [review]:
-----------------------------------------------------------------

For consistency we should probably eliminate the "is the event above the first option" code too.

Comment 9

[Mark Capella [:capella]]

Attachment: bug1121515.diff

(wondered)

Ok, re-tested both builds locally. I'll get an insurance TRY push after all review?'s.

Comment 10

[Mark Capella [:capella]]

Attachment: bug1121515_testUpdate.diff

Update existing test to correct expectations, push all to TRY.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aca0f3886e57

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d0590592a75b
https://hg.mozilla.org/integration/fx-team/rev/51c6347fe448

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0590592a75b
https://hg.mozilla.org/mozilla-central/rev/51c6347fe448