811905 - Make bookmarks and history buttons in awesomescreen look more clickable

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

I got some feedback yesterday (when showing off my awesome ViewPager work) that the bookmarks and history tabs in the awesomescreen don't look like tabs. I think we should add a subtle line between the items in the list to indicate that they're more like "buttons".

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

Ok, I'm no graphics artist, but I took a shot at this one ... I might should have asked if this is what you had in mind before I did all this (?) Hopes for the best !

I also might need help testing all 6 screen resolution sets / images ... the one shown (attached) is from my GS3.

Comment 2

[Mark Capella [:capella]]

Attachment: Clickable Tabs

Comment 3

[Wesley Johnston (:wesj)]

Hmm... I was picturing something more subtle, like a line between the items (not quite sure how that works with the fat tab background, but I imagined it would hide behind it) sorta like what we have in the tabs tray now. But lets see if ibarlow has any ideas. I know he has mockups where we change this design entirely to look more like native android tabs. I wonder if he'd be willing to put that in a bit early? or if he'd take some in between state for a bit here?

Comment 4

[Ian Barlow (:ibarlow)]

Can we first try it with just a simple line divider, like this? 

http://cl.ly/image/3I3E3F0C0N0A

Comment 5

[Mark Capella [:capella]]

Attachment: Patch (v2)

well ... I like the other way personally, though I understand the minimalist version is more consistent :P

Something like this?

Comment 6

[Mark Capella [:capella]]

Attachment: Thin Divider

Comment 7

[Ian Barlow (:ibarlow)]

(In reply to Mark Capella [:capella] from comment #5)

> Something like this?

Almost there. Please change the line to a lighter grey - #7E7E7E, and shorten it by 3dp so it doesn't come so close to touching the bottom of the tab bar.

Comment 8

[Mark Capella [:capella]]

Attachment: Patch (v3)

Omi ... next bug I'm going to outsource the graphics ... coding is a lot easier :D

Comment 9

[Mark Capella [:capella]]

Attachment: Thinner Shorter Dividers

Comment 10

[Mark Capella [:capella]]

Attachment: Shortest Dividers

Comment 11

[Mark Capella [:capella]]

Attachment: Patch (v4)

Shorter still by same difference as last time ...

Comment 12

[Ian Barlow (:ibarlow)]

ship it!

Comment 13

[Wesley Johnston (:wesj)]

Comment on attachment 703656 [details] [diff] [review]
Patch (v4)

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

A couple questions about the nine patches that I want answered first. Other than that seems fine.

::: mobile/android/base/AwesomeBarTabs.java
@@ +226,5 @@
>                  else
>                      view.resetTheme();
>              }
>  
> +            if (i == (selIndex - 2))

Just in case we someday have 100 tabs, this should be >=

@@ +233,4 @@
>                  view.getBackground().setLevel(1);
>              else if (i == (selIndex + 1))
>                  view.getBackground().setLevel(2);
> +            else if (i == (selIndex + 2))

Same (<=)

::: mobile/android/base/Makefile.in
@@ +515,2 @@
>    res/drawable/awesomebar_tab_right.9.png \
> +  res/drawable/awesomebar_sep_right.9.png \

Is there any reason to use a 9 patch? I guess we could use it so that we don't need to recreate these at different dpi's, but we're still doing that here? Even at that, these images have a lot of empty space. Is there a reason for that?

::: mobile/android/base/resources/drawable/awesomebar_tab_unselected.xml
@@ +51,5 @@
> +
> +            <item android:bottom="2dip"
> +                  android:drawable="@drawable/awesomebar_sep_right"/>
> +        </layer-list>
> +    </item>

I don't like having level 0 sitting around doing nothing. Can we hijack it for one of these?

Comment 14

[Mark Capella [:capella]]

Attachment: Patch (v5)

Yah, I never got back to considering more than a hardcoded 3-tab awesomebar situation. I've simplified the code as attached to (with hope) transparently account for that.

|level 0 sitting around doing nothing|
Confused ... Level 0 is used for edge cases left and right of screen where the selected tab is not at that position, and simple default styling is required.

|Is there any reason to use a 9 patch?|
I can't answer (not a graphics artist). Maybe ibarlow can help me here. I had cloned the existing tab .png files and use them as sizing templates to create the vertical dividers. This may not be any better than using a non 9-patch version as I'm not sure the 9-patch sizing capabilites are actually being used here. (How to create a vertical seperator line that sizes / floats always to the right ?)

re: |images have a lot of empty space| ... before deflating / compressing they average ~500b each for a combined total of 5,381b ... after deflating / compressing they consume ~180b each in the .apk file for a combined total of 2,211b.

Comment 15

[Mark Capella [:capella]]

Ping reviewer ...

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Sep image

This is sorta what I imagined for these separator images. Seems to work here. This basically draws a line on the lower left, stretches some blank pixels in the upper right as the stretchy area, and allows text anywhere starting 1px to the right of the line. It kills off some padding on the right and left, but because our text is centered, I don't think it matters.

Comment 17

[Mark Capella [:capella]]

Attachment: Patch (v6)

Hmmm ... modifying the left separator images in that fashion is easy enough, (see attached) and does provide space savings. 

Are we wondering about further changes to use the same separators for left and right ?

Comment 18

[Mark Capella [:capella]]

Attachment: Patch (v7)

Ok .. I reversed the polarity of the left side tabs originally provided by you (haha... "horizontally flipped them") and created the associated right side tab versions.

Dang things seem to work!

Comment 19

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 719465 [details] [diff] [review]
Patch (v7)

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

Overall, the patch looks good to me. However, we might be removing the tabs from the awesomebar soon :)
Also, why do we need two images, the left and the right. Aren't they both the same?

Comment 20

[Sriram Ramasubramanian [:sriram]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #19)
> Comment on attachment 719465 [details] [diff] [review]
> Patch (v7)
> 
> Review of attachment 719465 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall, the patch looks good to me. However, we might be removing the tabs
> from the awesomebar soon :)
> Also, why do we need two images, the left and the right. Aren't they both
> the same?

Oh! how do these work with private tabs? And themes?

Comment 21

[Wesley Johnston (:wesj)]

Yeah. They look (mostly) fine. If you had a theme that happened to match these exactly, they'd disappear, but that's fairly rare. A better separator should probably have two colors.

Comment 22

[Mark Capella [:capella]]

Push to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=a85c0f64063f

Comment 23

[Mark Capella [:capella]]

and on to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee20a1ffe205

Comment 24

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/ee20a1ffe205