1076260 - Fix visual dividers of tabs on top

Status: RESOLVED FIXED

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

Description

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

Noticed these dividers were different to what we have in the design. I want to see if we can fit this in V1? I know it's a little different to what we have on desktop but wanted to give this a try. Thoughts?

Not sure how we can go about changing these but I feel like the gradient that connects to the toolbar is kind of distracting.

See bug 1060413 for the design.

Comment 1

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

I think the design in https://bug1060413.bugzilla.mozilla.org/attachment.cgi?id=8494678 is doable.

What color is the divider? And do you have specs on the size?

Comment 2

[Lucas Rocha (:lucasr)]

(In reply to Anthony Lam (:antlam) from comment #0)
> Noticed these dividers were different to what we have in the design. I want
> to see if we can fit this in V1? I know it's a little different to what we
> have on desktop but wanted to give this a try. Thoughts?
> 
> Not sure how we can go about changing these but I feel like the gradient
> that connects to the toolbar is kind of distracting.
> 
> See bug 1060413 for the design.

Yeah, it should be pretty simple to change it.

Comment 3

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

Attachment: spec_tabdiv1.png

Attaching specs!

The div is actually #555555 (the only place we use this grey but the darkness of the background called for a new special use case color). 

The dimensions of it itself in my design is 30dp x 1 dp. Hope this is enough!

Comment 4

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

My bad! The blue is more about the width (distance from the left and right elements) and not the height. 

Not sure why I used that huge block to show the width...

Comment 5

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

Attachment: Adjust new tablet divider size and color

Lucas, the blue in the spec is the width between elements (12dp) but I'm not sure how to adjust that - to save time, do you mind taking that part (if it's not already correct)?

Comment 6

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

Attachment: spec_tabdiv2.png

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8499004 [details] [diff] [review]
Adjust new tablet divider size and color

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

Looks good with suggested changes.

::: mobile/android/base/resources/drawable-large-v11/new_tablet_tab_strip_divider.xml
@@ +11,5 @@
>      <size android:width="1dp"
> +          android:height="30dp"/>
> +
> +    <!-- We draw this ourselves in TabStripView.draw() and to avoid implementing more
> +         than we have to, only bottom padding is taken into account. -->

Is this comment really necessary?

::: mobile/android/base/tabs/TabStripView.java
@@ +26,5 @@
>      private static final String LOGTAG = "GeckoTabStrip";
>  
>      private final TabStripAdapter adapter;
>      private final Drawable divider;
> +    // Filled by calls to ShapeDrawable.getPadding(); saved to prevent allocation in draw().

nit: add empty line before comment, break lines maybe?

@@ +27,5 @@
>  
>      private final TabStripAdapter adapter;
>      private final Drawable divider;
> +    // Filled by calls to ShapeDrawable.getPadding(); saved to prevent allocation in draw().
> +    private final Rect dividerPadding;

initialize directly here.

@@ +144,5 @@
>      @Override
>      public void draw(Canvas canvas) {
>          super.draw(canvas);
>  
> +        divider.getPadding(dividerPadding);

Do you actually need to fetch padding on every draw call? The divider drawable never changes anyway. Move this to the constructor?

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #5)
> Created attachment 8499004 [details] [diff] [review]
> Adjust new tablet divider size and color
> 
> Lucas, the blue in the spec is the width between elements (12dp) but I'm not
> sure how to adjust that - to save time, do you mind taking that part (if
> it's not already correct)?

Please file a follow-up and assign to me.

Comment 9

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

(In reply to Lucas Rocha (:lucasr) from comment #7)
> > +    <!-- We draw this ourselves in TabStripView.draw() and to avoid implementing more
> > +         than we have to, only bottom padding is taken into account. -->
> 
> Is this comment really necessary?

I like it - if you're not sure how this view is used, you'd assume the Android system takes care of it (like I did) and wonder why the padding is not working - this comment short-circuits the time wasted when you realize you're wrong and is not very invasive.

Comment 10

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

Attachment: Adjust new tablet divider size and color

Address nits.

Comment 11

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

https://hg.mozilla.org/integration/fx-team/rev/15d93e79679e

Comment 12

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/mozilla-central/rev/15d93e79679e