Closed Bug 862761 Opened 10 years ago Closed 10 years ago

The history tab is dark gray on Froyo and Gingerbread devices

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox21 unaffected, firefox22 unaffected, firefox23 verified, fennec23+)

VERIFIED FIXED
Firefox 23
Tracking Status
firefox21 --- unaffected
firefox22 --- unaffected
firefox23 --- verified
fennec 23+ ---

People

(Reporter: AdrianT, Assigned: bnicholson)

References

Details

(Keywords: regression, Whiteboard: ui-hackathon)

Attachments

(2 files)

Attached image history tab screenshot
Nightly 23.0a1 2013-04-17
LG Optimus 2X (Android 2.2) / Samsung Galaxy R (Android 2.3.4)

Steps to reproduce:
1) Have/make at least one history entry
2) Open the Awesomebar and go to the History Tab

Expected results:
The history tab background is light gray and is the same like the Bookmarks tab and Top Sites

Actual results:
The background is dark gray
Whiteboard: regression
Component: Awesomescreen → Theme and Visual Design
Flags: needinfo?(sriram)
Whiteboard: regression
The regression window for this issue is:
1. mozilla central
good build: 12.04.2013 
bad build:  13.04.2013 

pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7b8ed29c6bc0&tochange=24a6b5ed51e3

2. inbound
good build: 1365703887
bad build:  1365704428

pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=270ad24960d3&tochange=f85629692c59
Lot's of Sriram patches in that range
Assignee: nobody → sriram
Status: NEW → ASSIGNED
Blocks: 823644
Flags: needinfo?(sriram)
tracking-fennec: ? → 23+
Assignee: sriram → bnicholson
Whiteboard: ui-hackathon
After spending a long time playing around with different styles, I found that setting the childDivider to a color actually sets the background of the ExpandableListView to that color. I'm convinced that this is a bug on Froyo/GB phones, and using a drawable divider seems to work as a workaround.
Attachment #742081 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 742081 [details] [diff] [review]
Use divider drawable for ExpandableListView childDivider

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

r- to know if you tried setting dividerHeight in the style.

::: mobile/android/base/resources/values/styles.xml
@@ -53,4 @@
>  
>      <style name="Widget.ExpandableListView" parent="Widget.ListView">
>          <item name="android:groupIndicator">@android:color/transparent</item>
> -        <item name="android:childDivider">#D1D5DA</item>

Have you tried simply setting dividerHeight to 1dp? The underlying issue here is that ColorDrawables don't have intrinsic dimensions. This is why you had to define a shape for the list selector in bug 865923 for example. However, ListView (and ExpandableListView by extension) have special code to force boundaries on divider drawables. But it will only work with a ColorDrawable if you set dividerHeight explicitly. Otherwise it will try to use the (non-existing) intrinsic dimensions.
Attachment #742081 - Flags: review?(lucasr.at.mozilla) → review-
Comment on attachment 742081 [details] [diff] [review]
Use divider drawable for ExpandableListView childDivider

(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 742081 [details] [diff] [review]
> Use divider drawable for ExpandableListView childDivider
> 
> Review of attachment 742081 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- to know if you tried setting dividerHeight in the style.
> 
> ::: mobile/android/base/resources/values/styles.xml
> @@ -53,4 @@
> >  
> >      <style name="Widget.ExpandableListView" parent="Widget.ListView">
> >          <item name="android:groupIndicator">@android:color/transparent</item>
> > -        <item name="android:childDivider">#D1D5DA</item>
> 
> Have you tried simply setting dividerHeight to 1dp? The underlying issue
> here is that ColorDrawables don't have intrinsic dimensions. This is why you
> had to define a shape for the list selector in bug 865923 for example.
> However, ListView (and ExpandableListView by extension) have special code to
> force boundaries on divider drawables. But it will only work with a
> ColorDrawable if you set dividerHeight explicitly. Otherwise it will try to
> use the (non-existing) intrinsic dimensions.

Yeah, this style has the Widget.ListView style as its parent, and Widget.ListView already has a dividerHeight of 1dp set. I also tried setting the dividerHeight explicitly in this rule and it didn't make a difference.
Attachment #742081 - Flags: review- → review?(lucasr.at.mozilla)
Comment on attachment 742081 [details] [diff] [review]
Use divider drawable for ExpandableListView childDivider

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

Ok, ExpandableListView is busted on Froyo/GB then. Sigh...

::: mobile/android/base/resources/values/styles.xml
@@ -53,4 @@
>  
>      <style name="Widget.ExpandableListView" parent="Widget.ListView">
>          <item name="android:groupIndicator">@android:color/transparent</item>
> -        <item name="android:childDivider">#D1D5DA</item>

Have you tried simply setting dividerHeight to 1dp? The underlying issue here is that ColorDrawables don't have intrinsic dimensions. This is why you had to define a shape for the list selector in bug 865923 for example. However, ListView (and ExpandableListView by extension) have special code to force boundaries on divider drawables. But it will only work with a ColorDrawable if you set dividerHeight explicitly. Otherwise it will try to use the (non-existing) intrinsic dimensions.
Attachment #742081 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/c6443d8a1b13
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Verified fixed on:
-build: Firefox for Android 23.0a1(2013-05-13)
-device: Samsung Galaxy R 
-OS: Android 2.3.4
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.