Tweak history panel header color for better contrast with the tab strip background

VERIFIED FIXED in Firefox 34

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

unspecified
Firefox 36
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34 verified, firefox35 verified, firefox36 verified)

Details

Attachments

(9 attachments)

Assignee

Description

5 years ago
We changed the tab strip background color recently.
Assignee

Updated

5 years ago
No longer blocks: new-toolbar-v1
Assignee

Updated

5 years ago
Attaching current state screen shot
I've got 2 options here that I could use some feedback on.. tell me what you think :)

I also wanted to ask what size and color the text for these dividers are "Last Week", "Yesterday", etc...
Flags: needinfo?(lucasr.at.mozilla)
Assignee

Comment 3

5 years ago
(In reply to Anthony Lam (:antlam) from comment #2)
> Created attachment 8479171 [details]
> prev_dividermatte_mock1.png
> 
> I've got 2 options here that I could use some feedback on.. tell me what you
> think :)
> 
> I also wanted to ask what size and color the text for these dividers are
> "Last Week", "Yesterday", etc...

I kinda prefer the visual simplicity of option 1. Looks a little less noisy. Is it the same color than the title strip on top?
Flags: needinfo?(lucasr.at.mozilla)
Yes! It's completely dependent on the divider color to separate the two. 

The matte will be #F5F5F5 (matching the panel's header background) while the divider is #D7D9DB.

We might also want to change the text color to #777777 to match the header color. :)

#D7D9DB is also the color of the shadow that I have in my design but I just realized we haven't really explicitly talked about that. Is that magically the color we're using right now for the shadow too? :P
Flags: needinfo?(lucasr.at.mozilla)
Assignee

Comment 5

5 years ago
Is the divider below the title strip always there, on all panels? Or does it only apply to the history panel?
Flags: needinfo?(lucasr.at.mozilla) → needinfo?(alam)
I just realized that these dividers are probably part of the element usually above it and so the first element would not have a divider above. Since it's the same color as the Panel label background, they would clash when displayed next to each other.

Could we just apply the divider specifically to situations in panels where the first item is one of these "sub headers"? Like History and Recent Tabs..

Thoughts?
Flags: needinfo?(alam)
this was really bothering me lately hah ^ :)
Flags: needinfo?(lucasr.at.mozilla)
Assignee

Comment 12

5 years ago
Comment on attachment 8503191 [details] [diff] [review]
Draw divider at the bottom of about:home's tab strip (r=margaret)

I realize that this is not what antlam proposes in comment #6 but I'm confident that this is what he actually meant ;-)
Attachment #8503191 - Flags: review?(margaret.leibovic)
Flags: needinfo?(lucasr.at.mozilla)
Assignee

Comment 13

5 years ago
Comment on attachment 8503192 [details] [diff] [review]
Use consistent height in about:home's tab strip (r=margaret)

We're using 40dp on phones, it makes sense to use the same on tabets. Bigger tap areas and all.
Attachment #8503192 - Flags: review?(margaret.leibovic)
Assignee

Comment 14

5 years ago
Comment on attachment 8503193 [details] [diff] [review]
Use consistent bg color in about:home's tab strip (r=margaret)

Same color than the toolbar.
Attachment #8503193 - Flags: review?(margaret.leibovic)
Assignee

Comment 15

5 years ago
Comment on attachment 8503194 [details] [diff] [review]
Don't use top divider on about:home lists (r=margaret)

Now that we draw the divider at the bottom of about:home's tabs strip, there's no need to use the top divider in the list views.
Attachment #8503194 - Flags: review?(margaret.leibovic)
Assignee

Comment 16

5 years ago
Posted image Screenshot
This is how it looks.

Updated

5 years ago
Attachment #8503191 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8503192 [details] [diff] [review]
Use consistent height in about:home's tab strip (r=margaret)

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

::: mobile/android/base/resources/layout-large-v11/home_pager.xml
@@ +13,5 @@
>                                    android:layout_height="match_parent"
>                                    android:background="@android:color/white">
>  
>      <org.mozilla.gecko.home.TabMenuStrip android:layout_width="match_parent"
> +                                         android:layout_height="40dip"

Can you pull this value out into a dimen so that it's more obvious that it's consistent with where it's used for HomePagerTabStrip?

Also, the naming of HomePagerTabStrip and TabMenuStrip always confuses me! Maybe we can rename those at some point :)
Attachment #8503192 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8503193 [details] [diff] [review]
Use consistent bg color in about:home's tab strip (r=margaret)

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

Nice. Consistency, one style at a time! :)
Attachment #8503193 - Flags: review?(margaret.leibovic) → review+

Updated

5 years ago
Attachment #8503194 - Flags: review?(margaret.leibovic) → review+
If these patches cause no problems on Nightly, I think they would be a nice win to uplift all the way to beta to be part of the new toolbar UI release.
Assignee

Comment 20

5 years ago
(In reply to :Margaret Leibovic from comment #17)
> Comment on attachment 8503192 [details] [diff] [review]
> Use consistent height in about:home's tab strip (r=margaret)
> 
> Review of attachment 8503192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/layout-large-v11/home_pager.xml
> @@ +13,5 @@
> >                                    android:layout_height="match_parent"
> >                                    android:background="@android:color/white">
> >  
> >      <org.mozilla.gecko.home.TabMenuStrip android:layout_width="match_parent"
> > +                                         android:layout_height="40dip"
> 
> Can you pull this value out into a dimen so that it's more obvious that it's
> consistent with where it's used for HomePagerTabStrip?

Good idea, done.

> Also, the naming of HomePagerTabStrip and TabMenuStrip always confuses me!
> Maybe we can rename those at some point :)

AGREE ;-)
Assignee

Comment 22

5 years ago
Comment on attachment 8503191 [details] [diff] [review]
Draw divider at the bottom of about:home's tab strip (r=margaret)

Approval Request Comment
[Feature/regressing bug #]: bug 1010740
[User impact if declined]: Unpolished tab strip in about:home. Make us look amateur.
[Describe test coverage new/current, TBPL]: Local tests only. Let's bake this in Nightly for a couple days and then uplift.
[Risks and why]: Very low. Only visual tweaks.
[String/UUID change made/needed]: n/a
Attachment #8503191 - Flags: approval-mozilla-beta?
Attachment #8503191 - Flags: approval-mozilla-aurora?
Assignee

Comment 23

5 years ago
Comment on attachment 8503192 [details] [diff] [review]
Use consistent height in about:home's tab strip (r=margaret)

Approval Request Comment
[Feature/regressing bug #]: bug 1010740
[User impact if declined]: Unpolished tab strip in about:home. Make us look amateur.
[Describe test coverage new/current, TBPL]: Local tests only. Let's bake this in Nightly for a couple days and then uplift.
[Risks and why]: Very low. Only visual tweaks.
[String/UUID change made/needed]: n/a
Attachment #8503192 - Flags: approval-mozilla-beta?
Attachment #8503192 - Flags: approval-mozilla-aurora?
Assignee

Comment 24

5 years ago
Comment on attachment 8503193 [details] [diff] [review]
Use consistent bg color in about:home's tab strip (r=margaret)

Approval Request Comment
[Feature/regressing bug #]: bug 1010740
[User impact if declined]: Unpolished tab strip in about:home. Make us look amateur.
[Describe test coverage new/current, TBPL]: Local tests only. Let's bake this in Nightly for a couple days and then uplift.
[Risks and why]: Very low. Only visual tweaks.
[String/UUID change made/needed]: n/a
Attachment #8503193 - Flags: approval-mozilla-beta?
Attachment #8503193 - Flags: approval-mozilla-aurora?
Assignee

Comment 25

5 years ago
Comment on attachment 8503194 [details] [diff] [review]
Don't use top divider on about:home lists (r=margaret)

Approval Request Comment
[Feature/regressing bug #]: bug 1010740
[User impact if declined]: Unpolished tab strip in about:home. Make us look amateur.
[Describe test coverage new/current, TBPL]: Local tests only. Let's bake this in Nightly for a couple days and then uplift.
[Risks and why]: Very low. Only visual tweaks.
[String/UUID change made/needed]: n/a
Attachment #8503194 - Flags: approval-mozilla-beta?
Attachment #8503194 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Target Milestone: Firefox 35 → Firefox 36
Comment on attachment 8503191 [details] [diff] [review]
Draw divider at the bottom of about:home's tab strip (r=margaret)

Although I don't think this is a major issue for Firefox 34, this looks safe enough to take in beta2. Beta+

Lucas - FYI, in future, to keep things simple, just fill out the approval questionnaire once and flag the rest of the patches that you want to uplift.
Attachment #8503191 - Flags: approval-mozilla-beta?
Attachment #8503191 - Flags: approval-mozilla-beta+
Attachment #8503191 - Flags: approval-mozilla-aurora?
Attachment #8503191 - Flags: approval-mozilla-aurora+
Attachment #8503192 - Flags: approval-mozilla-beta?
Attachment #8503192 - Flags: approval-mozilla-beta+
Attachment #8503192 - Flags: approval-mozilla-aurora?
Attachment #8503192 - Flags: approval-mozilla-aurora+
Attachment #8503193 - Flags: approval-mozilla-beta?
Attachment #8503193 - Flags: approval-mozilla-beta+
Attachment #8503193 - Flags: approval-mozilla-aurora?
Attachment #8503193 - Flags: approval-mozilla-aurora+
Attachment #8503194 - Flags: approval-mozilla-beta?
Attachment #8503194 - Flags: approval-mozilla-beta+
Attachment #8503194 - Flags: approval-mozilla-aurora?
Attachment #8503194 - Flags: approval-mozilla-aurora+
Mike - If Lucas can't get to this soon (he's on stay-at-home-PTO), can you take a shot?
Flags: needinfo?(michael.l.comella)
Posted image Beta, w/o patch 4
I don't think we need patch 4 - this screenshot does not have the issue that the screenshot in comment 30 has (a gap below the page selection bar and the history section title, "Today").
Flags: needinfo?(michael.l.comella)
Also, the code patch 4 removes does not appear anywhere in the beta tree so I figure it was never added.
Ryan, can you land these patches on beta without patch 4 (don't use top divider...)?

Lucas, if this was incorrect of me, we can backout/fix these issues later.
Flags: needinfo?(ryanvm)
Assignee

Comment 35

5 years ago
Not sure there's anything else to be done here. Canceling needinfo.
Flags: needinfo?(lucasr.at.mozilla)
History panel looks like in the attachment from comment 16, so I will mark this as verified fixed.
Builds:
Firefox for Android 34.0.1
Firefox for Android 35 Beta 10
Firefox for Android 36.0a2 (2015-01-07)

Device: Samsung Galaxy S4 (Android 4.4.2)
You need to log in before you can comment on or make changes to this bug.