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?

At GDC this week, but I'll work on this tonight/this afternoon.

bug 984619 has a test case

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

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...

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?

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.

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?

Sorry, I meant to link to this above: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/prompts/Prompt.java#350

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.

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);

Verified as fixed in builds: - 30.0a2 (2014-04-03); Device: Acer Iconia (Android 3.2.1)