Closed Bug 985875 Opened 6 years ago Closed 6 years ago

Regression: Unable to select a different value in a <select>

Categories

(Firefox for Android :: General, defect, major)

ARM
Android
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox28 --- unaffected
firefox29 --- unaffected
firefox30 + verified
firefox31 + verified
fennec 30+ ---

People

(Reporter: cos_flaviu, Assigned: wesj)

References

()

Details

(Keywords: regression, reproducible)

Attachments

(1 file, 1 obsolete file)

Environment: 
Device: Acer Iconia Tab (Android 3.2.1);
Build: Nightly 31.0a1 (2014-03-20).

Steps to reproduce:
1. Go to http://bit.ly/1kFnodJ;
2. Tap on the drop down;
3. Select different option from the drop down list.

Expected result:
Any options from the drop down list can be selected.

Actual result:
The default selected option can not be changed.

Notes:
The bug is also reproducible on LG Nexus 4 (Android 4.4.2) and Samsung Galaxy Tab 2 7.0 (Android 4.2.2);
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=395177ab859b&tochange=852fa926deae

bug 973013?

Can we get tests for basic forms?
Severity: normal → major
tracking-fennec: --- → ?
Flags: needinfo?(wjohnston)
Flags: in-testsuite?
Summary: Can not select different option from a drop down list → Regression: Unable to select a different value in a <select>
At GDC this week, but I'll work on this tonight/this afternoon.
Flags: needinfo?(wjohnston)
Duplicate of this bug: 984619
Assignee: nobody → wjohnston
tracking-fennec: ? → 30+
Hmm... The problem is we recently moved to trusting the adapter to tell us what's selected. Unfortunately, in this case it hasn't updated yet, so it gives us the wrong number:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/prompts/Prompt.java#160
Attached patch Patch v1 (obsolete) — Splinter Review
So this makes us throw away the adapter in the "No buttons" case, which means we'll just use this "which" parameter to determine what was selected.  In all other cases, the which parameter is the button. The adapter isn't providing valid data for this case, so throwing it away seems fine.

This feels confusing to me, and I'm sure to anyone else. The only reason that "which" is ever a button is because I previously had us return the selected item as "button". We need to maintain that for backward compat with add-ons, so maybe we're just stuck in a bit of a jam.

Trying to think of better ways through it, but was hoping for some feedback at least...
Attachment #8394520 - Flags: review?(bnicholson)
Status: NEW → ASSIGNED
Just trying to understand the problem a bit better -- where does the adapter get updated, and why hasn't it been updated yet in this case? Could we fix this by ensuring the adapter *does* have the correct selections somehow?
Attached patch Patch v2Splinter Review
Heh. I completely forgot this selection is controlled by us actually. We can set it ourselves. The adapter doesn't really know anything about single/multi selection, so we have to do some flipping manually here. We could do that inside the adapter somehow. Either make it aware, or pass something to the method. I don't have strong opinions, except that I'd prefer to fix this and worry about the underlying data somewhere else.
Attachment #8396667 - Flags: review?(bnicholson)
Attachment #8394520 - Attachment is obsolete: true
Attachment #8394520 - Flags: review?(bnicholson)
Comment on attachment 8396667 [details] [diff] [review]
Patch v2

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

Nice, this seems much easier to follow.

::: mobile/android/base/prompts/Prompt.java
@@ +272,5 @@
> +                // clear any other selected items first.
> +                ArrayList<Integer> selected = mAdapter.getSelected();
> +                for (Integer sel : selected) {
> +                    mAdapter.toggleSelected(sel);
> +                }

I don't understand the need for this part. The prompt is either a multiselect or single select list, but not both. Since we're in this listener, we know it's a single select prompt. So why do we need to check for existing selected items to revert them? The only other place that touches the adapter is the onClick listener for the multiselect list, but that won't be called since this isn't a multiselect prompt. Aren't all adapter entries guaranteed to be unselected at this point?
Comment on attachment 8396667 [details] [diff] [review]
Patch v2

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

::: mobile/android/base/prompts/Prompt.java
@@ +280,1 @@
>                  closeIfNoButtons(which);

Oh, perhaps this is the reason -- the prompt may not be closed after toggling the selected item, so selecting another single item needs to clear the old one. Is that correct?
No. Something is passed up as selected when the list is shown (for instance, item 2). Its shown with a checkmark next to it. If you then tap item 1, we'll return to javascript a selected array of [1,2].

With the remove loop in place, we only return [1].
Comment on attachment 8396667 [details] [diff] [review]
Patch v2

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

OK, this works for me. Semi-related suggestion: looking through Prompt.java, it's confusing that onClick, which is used only for multi-select lists, is a class-level method (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/prompts/Prompt.java#208). And then you have this onClick listener you're using here, which is defined anonymously in addSingleSelectList. Can you change `listView.setOnItemClickListener(this);` to also use an anonymous listener instead of implementing it in Prompt?
Attachment #8396667 - Flags: review?(bnicholson) → review+
Keywords: verifyme
Comment on attachment 8396667 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 974286
User impact if declined: Can't select anything earlier than the currently selected item in lists.
Testing completed (on m-c, etc.): Landed on mc this week
Risk to taking this patch (and alternatives if risky): This is pretty low risk. We weren't using this before. We are now, but I forgot to update things in one place. There's an alternative, but worse, patch here. We could backout, but we're planning to uplift more of this work (quickshare bug 942270), so that's not really an option.
String or IDL/UUID changes made by this patch: None.
Attachment #8396667 - Flags: approval-mozilla-aurora?
Blocks: 983424
https://hg.mozilla.org/mozilla-central/rev/73edfcfeb6e2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Flags: needinfo?(flaviu.cos)
Verified as fixed in build 31.0a1 (2014-03-28);
Device: Acer Iconia Tab (Android 3.2.1);

Bug still reproducible on the latest aurora build: 30.0a2 (2014-03-28);
Flags: needinfo?(flaviu.cos)
Attachment #8396667 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in builds:
- 30.0a2 (2014-04-03);
Device: Acer Iconia (Android 3.2.1)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.