Last Comment Bug 715925 - Options inside optgroup should show up as indented in select helper popup
: Options inside optgroup should show up as indented in select helper popup
: testcase
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
P4 normal (vote)
: Firefox 12
Assigned To: Wesley Johnston (:wesj)
: Sebastian Kaspari (:sebastian)
Depends on:
  Show dependency treegraph
Reported: 2012-01-06 08:59 PST by Martijn Wargers [:mwargers]
Modified: 2012-03-02 16:09 PST (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Screenshot (77.02 KB, image/png)
2012-01-18 09:02 PST, Wesley Johnston (:wesj)
no flags Details
Patch (1.18 KB, patch)
2012-01-24 11:33 PST, Wesley Johnston (:wesj)
sriram.mozilla: review-
Details | Diff | Splinter Review
Patch v2 (3.83 KB, patch)
2012-01-24 12:27 PST, Wesley Johnston (:wesj)
sriram.mozilla: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Martijn Wargers [:mwargers] 2012-01-06 08:59:50 PST
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 User image Wesley Johnston (:wesj) 2012-01-18 09:02:43 PST
Created attachment 589535 [details]

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 User image Wesley Johnston (:wesj) 2012-01-24 11:33:48 PST
Created attachment 591196 [details] [diff] [review]

Simple solution. We could do a separate layout or something if we think its needed?
Comment 3 User image Sriram Ramasubramanian [:sriram] 2012-01-24 11:47:29 PST
Comment on attachment 591196 [details] [diff] [review]

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 User image Wesley Johnston (:wesj) 2012-01-24 12:27:03 PST
Created attachment 591219 [details] [diff] [review]
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 User image Sriram Ramasubramanian [:sriram] 2012-01-24 12:36:10 PST
Comment on attachment 591219 [details] [diff] [review]
Patch v2

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

This looks good to me.
Comment 7 User image :Ms2ger (⌚ UTC+1/+2) 2012-01-25 07:15:32 PST
Comment 8 User image Wesley Johnston (:wesj) 2012-01-25 09:52:53 PST
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 User image Martijn Wargers [:mwargers] 2012-01-25 12:38:59 PST
Verified fixed in current Native trunk build.
Comment 10 User image Alex Keybl [:akeybl] 2012-01-25 17:03:31 PST
Comment on attachment 591219 [details] [diff] [review]
Patch v2

[Triage Comment]
Mobile only - approved for Aurora.
Comment 11 User image Matt Brubeck (:mbrubeck) 2012-01-26 17:34:52 PST

Note You need to log in before you can comment on or make changes to this bug.