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
Status: VERIFIED FIXED
: 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)
:
Mentors:
https://bug685197.bugzilla.mozilla.or...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-06 08:59 PST by Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
Modified: 2012-03-02 16:09 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed
+
fixed


Attachments
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 | 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 | Review

Description Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 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 Wesley Johnston (:wesj) 2012-01-18 09:02:43 PST
Created attachment 589535 [details]
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) 2012-01-24 11:33:48 PST
Created attachment 591196 [details] [diff] [review]
Patch

Simple solution. We could do a separate layout or something if we think its needed?
Comment 3 Sriram Ramasubramanian [:sriram] 2012-01-24 11:47:29 PST
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) 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 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 8 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 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2012-01-25 12:38:59 PST
Verified fixed in current Native trunk build.
Comment 10 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 Matt Brubeck (:mbrubeck) 2012-01-26 17:34:52 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/d8a62bf6787c

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