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

Status: VERIFIED FIXED

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

Description

[Lucas Rocha (:lucasr)]

We changed the tab strip background color recently.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: Screenshot_2014-08-26-09-36-45.png

Attaching current state screen shot

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: 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...

Comment 3

[Lucas Rocha (:lucasr)]

(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?

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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

Comment 5

[Lucas Rocha (:lucasr)]

Is the divider below the title strip always there, on all panels? Or does it only apply to the history panel?

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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?

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

this was really bothering me lately hah ^ :)

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Draw divider at the bottom of about:home's tab strip (r=margaret)

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: Use consistent height in about:home's tab strip (r=margaret)

Comment 10

[Lucas Rocha (:lucasr)]

Attachment: Use consistent bg color in about:home's tab strip (r=margaret)

Comment 11

[Lucas Rocha (:lucasr)]

Attachment: Don't use top divider on about:home lists (r=margaret)

Comment 12

[Lucas Rocha (:lucasr)]

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 ;-)

Comment 13

[Lucas Rocha (:lucasr)]

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.

Comment 14

[Lucas Rocha (:lucasr)]

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

Same color than the toolbar.

Comment 15

[Lucas Rocha (:lucasr)]

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.

Comment 16

[Lucas Rocha (:lucasr)]

Attachment: Screenshot

This is how it looks.

Comment 17

[:Margaret Leibovic]

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 :)

Comment 18

[:Margaret Leibovic]

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! :)

Comment 19

[:Margaret Leibovic]

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.

Comment 20

[Lucas Rocha (:lucasr)]

(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 ;-)

Comment 21

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/e2ae96557b36
https://hg.mozilla.org/integration/fx-team/rev/5e0c38703a20
https://hg.mozilla.org/integration/fx-team/rev/d303c71eb541
https://hg.mozilla.org/integration/fx-team/rev/4f831092e958

Comment 22

[Lucas Rocha (:lucasr)]

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

Comment 23

[Lucas Rocha (:lucasr)]

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

Comment 24

[Lucas Rocha (:lucasr)]

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

Comment 25

[Lucas Rocha (:lucasr)]

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

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/e2ae96557b36
https://hg.mozilla.org/mozilla-central/rev/5e0c38703a20
https://hg.mozilla.org/mozilla-central/rev/d303c71eb541
https://hg.mozilla.org/mozilla-central/rev/4f831092e958

Comment 27

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cf046fae6299
https://hg.mozilla.org/releases/mozilla-aurora/rev/09328e5e4932
https://hg.mozilla.org/releases/mozilla-aurora/rev/df199b7b7196
https://hg.mozilla.org/releases/mozilla-aurora/rev/6cba3d56ecea

Patch 4 needs rebasing for beta uplift.

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

Mike - If Lucas can't get to this soon (he's on stay-at-home-PTO), can you take a shot?

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Nightly, w/o patch 4

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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").

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Also, the code patch 4 removes does not appear anywhere in the beta tree so I figure it was never added.

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7d2f3db4567d
https://hg.mozilla.org/releases/mozilla-beta/rev/a73c379cfa5f
https://hg.mozilla.org/releases/mozilla-beta/rev/47ef137f046f

Comment 35

[Lucas Rocha (:lucasr)]

Not sure there's anything else to be done here. Canceling needinfo.

Comment 36

[Cristina Madaras, QA [:CristinaM]]

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)