792410 - thumbnails are currently cut off in the tabs tray, update thumbnail styling in tabs menu

Status: VERIFIED FIXED

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

Description

[Ian Barlow (:ibarlow)]

Attachment: tab thumbnails

Building on the work Wes has done in bug 787765 to fix our thumbnail styling on about:home, we should apply these changes in the tab menu as well. Specifically, 

1. Removing the "recessed" border
2. Adding a drop shadow -- 2dp, no blur, black with 25% opacity
3. Bonus points for 2dp rounded corners

A mockup is attached here. Note that tab thumbnails have a shorter aspect ratio than the ones on about:home, since they don't have a title overlay along the bottom.

Comment 1

[:Margaret Leibovic]

Attachment: patch

I removed the border, added the shadow, and made the thumbnails bigger.

For the shadow height, I chose 88dp = 108dp (local_tab_row_height) - 2*10dp (margin). Then for the thumbnail height I chose 86dp to make the 2dp shadow, and then scaled the width to keep it close to the same width/height ratio as before.

I didn't know how to add the rounded corners, but it looks like we don't have those on about:home right now anyway.

Comment 2

[:Margaret Leibovic]

Attachment: screenshot

Ian, how's this?

(PS, I'm doing this for the maple cookies ;)

Comment 3

[:Margaret Leibovic]

Attachment: patch v2

Actually, I realized we can get rid of that second ImageView.

Comment 4

[Ian Barlow (:ibarlow)]

(In reply to Margaret Leibovic [:margaret] from comment #2)
> Created attachment 677972 [details]
> screenshot
> 
> Ian, how's this?
> 
> (PS, I'm doing this for the maple cookies ;)

This is looking awesome! You might just want to check your shadow opacity and size compared to what Wes did on about:home. These look a little on the heavy side to me. 

Other than that, very cookie worthy :)

Comment 5

[Wesley Johnston (:wesj)]

I had to remove the shadows on about:home for performance reasons.

Comment 6

[:Margaret Leibovic]

(In reply to Ian Barlow (:ibarlow) from comment #4)

> This is looking awesome! You might just want to check your shadow opacity
> and size compared to what Wes did on about:home. These look a little on the
> heavy side to me. 

After implementing this, I was actually wondering if I did the right thing. Basically my patch just puts a 2dp gray shadow below the thumbnail image, but given that we're talking about opacity, should this shadow be overlapping the bottom 2dp of the thumbnail image?

(In reply to Wesley Johnston (:wesj) from comment #5)
> I had to remove the shadows on about:home for performance reasons.

Uh oh, what were the problems? Were you doing the shadow differently than I'm doing in this patch?

Comment 7

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 677978 [details] [diff] [review]
patch v2

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

This looks good. But..

1. R.drawable.tab_thumbnail_shadow in different folders must be removed if they aren't used anywhere else.
2. layout-xlarge-v11/tabs_row.xml isn't modified. 10" tablets will suffer.

Doubts:
1. How would 150dp width work on smaller phones? Isn't the text super truncated?

Suggestions:
If we are creating a similar thumbnail for both about:home and tabs-tray, we could merge them both to a custom view, and use it in XML.
Thereby when "rounded corners" or "shadows" are added, they would work in all places the same way.

Comment 8

[Sriram Ramasubramanian [:sriram]]

The way "selected tab" is shown has changed in newer mockups. It's better to fix them all in one-go (as they all are layout changes in the same file).

Comment 9

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #7)

> 1. R.drawable.tab_thumbnail_shadow in different folders must be removed if
> they aren't used anywhere else.
> 2. layout-xlarge-v11/tabs_row.xml isn't modified. 10" tablets will suffer.

Oops, good catches. I'll fix those. :)

> Doubts:
> 1. How would 150dp width work on smaller phones? Isn't the text super
> truncated?

I'll can test that on a Galaxy S, but I think that's the smallest screen phone I have.

> Suggestions:
> If we are creating a similar thumbnail for both about:home and tabs-tray, we
> could merge them both to a custom view, and use it in XML.
> Thereby when "rounded corners" or "shadows" are added, they would work in
> all places the same way.

The about:home thumbnails are slightly different because they have the title in them at the bottom. I'm not sure if that will make sharing them more complicated.

(In reply to Sriram Ramasubramanian [:sriram] from comment #8)
> The way "selected tab" is shown has changed in newer mockups. It's better to
> fix them all in one-go (as they all are layout changes in the same file).

Are these mockups in a bug somewhere?

I'll try to find some time to update my patch this weekend. Contributing to Fennec is like my hobby right now :)

Comment 11

[Brian Nicholson (:bnicholson)]

Without these changes, thumbnails are currently cut off in the tabs tray (see screenshot in bug 823297).

Comment 12

[:Margaret Leibovic]

Oh no, pressure's on! I can try to get back into this later this week, but anyone, feel free to take this from me if you have the time sooner.

Comment 13

[Alex Keybl [:akeybl]]

Sriram - Margaret is understandably slammed with other B2G work. Any chance you can jump on this early in the FF19 beta cycle?

Comment 14

[Sriram Ramasubramanian [:sriram]]

Margaret is coming back! :)
But ya, I'm working on tabs menu changes and can take care of this.

Comment 15

[:Margaret Leibovic]

Yeah, I'm sorry, I won't have time for this before the 24th, which is a long time from now.

Comment 16

[Alex Keybl [:akeybl]]

Can we get a fix finished this week? We'd really like to get this into our third beta (going to build Tuesday), since it'll be hard to justify this fix much later than that.

Comment 17

[Sriram Ramasubramanian [:sriram]]

If this is regarding the styling, it'll done as a part of private browsing. https://bug817675.bugzilla.mozilla.org/attachment.cgi?id=697581
I don't know why it is being tracked for 19, and what's expected for that.

Comment 18

[Wesley Johnston (:wesj)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #7)
> Suggestions:
> If we are creating a similar thumbnail for both about:home and tabs-tray, we
> could merge them both to a custom view, and use it in XML.

I have a WIP for this, but since this will need to be uplifted at this point, it seems better to do something simple like what margaret has here. I'll unbitrot and clean it up a bit.

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This is more complex than it looks. Basically, we need a scale type that will maintain the thumbnail's aspect ratio, but force it to take up the entire view size. CENTER_CROP does that, but doesn't keep the view's top left corner in place, leading to the thumbnail looking clipped. This does it using a custom GeckoImageView class and keeps the top left in place. I anticipate we can extend it later if we need support for other things.

We should also use this on about:home, but wanted to minimize the risk for beta 3 a bit.

Comment 20

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 703586 [details] [diff] [review]
Patch v1

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

Looks good. Please test it in GB phones with the canvas changes.
r+ with the changes.

::: mobile/android/base/GeckoViewsFactory.java
@@ +111,5 @@
>                  return new GeckoTextSwitcher(context, attrs);
>              else if (TextUtils.equals(viewName, "TextView"))
>                  return new GeckoTextView(context, attrs);
> +            else if (TextUtils.equals(viewName, "widget.GeckoImageView"))
> +                return new GeckoImageView(context, attrs);

The identifier could be "Gecko.ThumbnailView" and you could compare only "ThumbnailView".
The class name could be "ThumbnailView".

GeckoImageView is so generic in that it doesn't convey the meaning.

::: mobile/android/base/resources/layout-xlarge-v11/tabs_row.xml
@@ +21,5 @@
>                          android:layout_height="wrap_content"
>                          android:layout_weight="1.0"
>                          android:layout_margin="10dip">
>  
> +            <org.mozilla.gecko.widget.GeckoImageView android:id="@+id/thumbnail"

<Gecko.ThumbnailView/> is better.

::: mobile/android/base/widget/GeckoImageView.java
@@ +15,5 @@
> +
> +/* Special version of ImageView for thumbnails. Scales a thumbnail so that it maintains its aspect
> + * ratio and so that the images width and height are the same size or greater than the view size
> + */
> +public class GeckoImageView extends ImageView {

GeckoImageView is so generic in that it doesn't convey the meaning.
ThumbnailView could be a better one.

@@ +26,5 @@
> +        super(context, attrs);
> +    }
> +
> +    @Override
> +    public void onDraw(Canvas canvas) {

This requires saving the canvas, doing the draw, and restoring the canvas.

@@ +33,5 @@
> +            int w1 = d.getIntrinsicWidth();
> +            int h1 = d.getIntrinsicHeight();
> +            int w2 = getWidth();
> +            int h2 = getHeight();
> +    

Extra space.

@@ +39,5 @@
> +            if (w2/h2 < w1/h1) {
> +                scale = (float)h2/h1;
> +            } else {
> +                scale = (float)w2/w1;
> +            }

This could be shortened using a ternary operator.

@@ +41,5 @@
> +            } else {
> +                scale = (float)w2/w1;
> +            }
> +
> +            mMatrix = new Matrix();

Android forbids creating new Matrix inside onDraw(). This will show error with lint.
Could you initialize just once, and change the scale when needed?

@@ +50,5 @@
> +        d.draw(canvas);
> +    }
> +
> +    @Override
> +    protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {

I don't think we need to do this on onMeasure().
onSizeChanged() could work I guess. And, we could make the scale of the matrix to 1.0f instead of setting it to null.
There by, only one object is used.

Comment 21

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f81c93ea0ea

Comment 22

[Wesley Johnston (:wesj)]

Comment on attachment 703586 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 787765 - retheme about:home thumbnails
User impact if declined: lack of polish on tabs tray thumbnails
Testing completed (on m-c, etc.): landed on mc today
Risk to taking this patch (and alternatives if risky): Low. While there's a lot of code here to extend the Android ImageView, most of its just java stuff. This does the exact same thing as the normal imageView with a slightly different scale matrix.
String or UUID changes made by this patch: none.

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 703586 [details] [diff] [review]
Patch v1

[Triage Comment]
We'll all need to be on the lookout for tab/UX regressions, but this is good to go into FF19b3 (going to build Tuesday).

Comment 24

[Wesley Johnston (:wesj)]

Grr... Playing with the onSizeChange/onLayout fix i forgot to add a curly brace back in the final patch. Landed a fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/40badd9d545a

Comment 25

[Wesley Johnston (:wesj)]

Heh. I missed a bunch of review nits and broke the tree, so I just yanked this and will put it back in right.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe60d3c7d0a0

Comment 26

[Wesley Johnston (:wesj)]

Back in. Tested and everything builds fine here. Sorry 'bout that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba89f8698668

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0340843194ea

This is also in my queue to land on Aurora once it reopens.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/20752324e6e6

Comment 30

[Catalin Suciu [:csuciu]]

Unable to reproduce on Nightly 21.0a1 (2012-01-28), Aurora 20.0a2 (2012-01-28), Beta 19.0b3.

Verifying.