715925 - Options inside optgroup should show up as indented in select helper popup

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P4)

Description

[Martijn Wargers (dead)]

See testcase from url, which comes from bug 685197.

As you can see, the options "Three", "Four", "Five" are indented, because they are inside the "test" optgroup.
However, they don't show that way in the select helper popup (the popup that appears when tapping on the select).
The stock browser does show them as indented.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Ahh. Yeah, I don't really like how the stock browser presents these, so I wasn't trying to match their style. But we do need to do something so that users can tell which items are in the optgroup, and which aren't.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch

Simple solution. We could do a separate layout or something if we think its needed?

Comment 3

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 591196 [details] [diff] [review]
Patch

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

setPadding's unit is pixels and not in dpi. Using pixels and not taking dpi into account makes me scary.
Please see if an expandable list view can do the trick. If not, try using "density" from "DeviceManager" metrics.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Ahh. Good catch. So this does the conversion from dip to pixels for us. I looked at using an ExpandableListView instead. That would give us the ability to open and close groups of items, but wouldn't automatically fix this.

We'd have to create a special layout for these items, and to do it right we'd have to implement it for single select lists, multiselect lists, and context menu like lists. I don't want to have to manage separate layouts for every permutation possible here I think.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 591219 [details] [diff] [review]
Patch v2

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

This looks good to me.

Comment 6

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17f835ac21d3

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/17f835ac21d3

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 591219 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Regression caused by (bug #): bug 695485
User impact if declined: ui can not make sense at times
Testing completed (on m-c, etc.): landed jan 24
Risk to taking this patch (and alternatives if risky): very low. this is odd to run into anyway

Comment 9

[Martijn Wargers (dead)]

Verified fixed in current Native trunk build.

Comment 10

[Alex Keybl [:akeybl]]

Comment on attachment 591219 [details] [diff] [review]
Patch v2

[Triage Comment]
Mobile only - approved for Aurora.

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d8a62bf6787c