1083263 - Sort out aspect ratio of thumbnails for new tablet UI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martyn Haigh (:mhaigh)]

The thumbnails in the grid view are at the wrong aspect ratio in the new tablet UI.

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: Sort out aspect ratio of thumbnails for new tablet UI

This patch fixes the size of the thumbnail in the new gridview, introduces a new asset (xhdpi & hdpi sizes) and sorts out the aspect ratio for the thumbnail, whilst retaining existing functionality for non new tablet UI.

Comment 2

[Martyn Haigh (:mhaigh)]

Attachment: Screenshot of grid view with new assets and styles. The new tab asset is blocky.

I'm probably going to need a new empty tab asset in xhdpi (202 x 180) and hdpi (152 x 135) as the one I've done looks a bit blocky

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8510337 [details] [diff] [review]
Sort out aspect ratio of thumbnails for new tablet UI

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

Looks good. Please test this on phones and old tablet UI to make sure it doesn't cause regressions. Also, look out for weird thumbnails on top sites grid in the new tablet UI.

::: mobile/android/base/home/TopSitesGridItemView.java
@@ +231,5 @@
>          Favicons.cancelFaviconLoad(mLoadId);
>          ImageLoader.with(getContext()).cancelRequest(mThumbnailView);
>  
> +        if(NewTabletUI.isEnabled(getContext())) {
> +            mThumbnailView.setScaleType(NEW_TABLET_SCALE_TYPE_THUMBNAIL);

How does this look on thumbnail images? Does it crop the top/bottom of the image? Shouldn't it be FIT_START or something?

Comment 4

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

Do you need the whole image (globe + background color as one image) or just the globe? I remember Ian went through some of this last time we were in London back in May... But, I can't recall how we implemented these.

Comment 5

[Martyn Haigh (:mhaigh)]

Anthony: We've currently using an asset which is the entire image.

Comment 6

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

Ok, I'll just make those for you to get this bug moving! :) 

But, keep in mind that we wanted to look at more dynamic solutions for things like Private browsing tabs in the near future. E.g. blurring these screenshots just in PB (can't find the bug number now).

NI-ing Lucas to get his take on this^ too.

Comment 7

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

Attachment: img_tabtab_default.zip

^ check these out! :)

Comment 8

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

In case there are questions why the aspect ratio is a bit different to our current tabs in Android...

I've noticed that with newer devices and websites/ browsing in general, the content is actually getting "taller". What this means from a practical standpoint for us is that when we design something like these "website previews", we need to be aware of trying to show more meaningful content so the user can make those mental connections easier ("Oh! that WAS exactly what I was looking for!").

tl;dr : Between orientations, browsing habits, and general market trends, it'd be useful to show more content in the previews so I've made them taller too.

On an encouraging note: 
"The Nexus 9 is a premium 8.9” tablet with a screen size of 2048 x 1536 pixels (288 ppi), which translates to 1024 x 768 dip. This is a 4:3 aspect ratio, which is unique compared to earlier tablets. The Nexus 9 falls into the xhdpi density bucket, and you should already have assets in the drawable-xhdpi folder. " :)

Comment 9

[Martyn Haigh (:mhaigh)]

Attachment: Sort out aspect ratio of thumbnails for new tablet UI

Current working patch - this replaces the old patch completely as the top sites weren't working properly in that patch.  This one fixes the aspect ratio in the tabs tray and top sites (this is the matrix related code), but the issues left remaining is that the globe icon is being scaled despite following the Android guidelines of how to now get an image to scale in a LayerList.  

Can you spot what I'm doing wrong?

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8514286 [details] [diff] [review]
Sort out aspect ratio of thumbnails for new tablet UI

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

Looks good overall. I still don't understand why the image gets scaled up. Digging.

This new_tablet_tab_thumbnail_default is not used anywhere, remove it.

::: mobile/android/base/ThumbnailHelper.java
@@ +33,5 @@
>  
>      public static final float THUMBNAIL_ASPECT_RATIO = 0.571f;  // this is a 4:7 ratio (as per UX decision)
>  
> +    // Should actually be more like 0.83 (140/168) but various roundings mean that 0.9 works better
> +    public static final float NEW_TABLET_THUMBNAIL_ASPECT_RATIO = 0.9f;

Changes to ThumbnailHelper look good.

::: mobile/android/base/home/TopSitesGridItemView.java
@@ +280,4 @@
>          }
>  
>          mThumbnailView.setScaleType(SCALE_TYPE_FAVICON);
> +        mThumbnailView.setImageBitmap(favicon, false);

Why isn't scaletype enough here?

::: mobile/android/base/home/TopSitesThumbnailView.java
@@ +39,4 @@
>      // Paint for drawing the border.
>      private final Paint mBorderPaint;
>  
> +    private boolean mResize = false;

This is looking a bit smelly. Can you clarify why a simple scaletype is not working?

@@ +97,5 @@
>          final int height = (int) (width * ThumbnailHelper.THUMBNAIL_ASPECT_RATIO);
>          setMeasuredDimension(width, height);
> +
> +
> +        if(NewTabletUI.isEnabled(getContext()) && mResize) {

fitStart doesn't work in this case?

::: mobile/android/base/resources/values/dimens.xml
@@ +128,5 @@
>      <dimen name="history_tab_indicator_height">50dp</dimen>
>  
>  
> +    <dimen name="new_tablet_tab_thumbnail_height">140dp</dimen>
> +    <dimen name="new_tablet_tab_thumbnail_width">168dp</dimen>

This is just because you don't have the patch from bug 1064415, right?

::: mobile/android/base/tabs/TabsLayoutItemView.java
@@ +100,5 @@
>          } else {
> +            if(NewTabletUI.isEnabled(getContext())) {
> +                mThumbnail.setImageResource(R.drawable.new_tablet_tab_background);
> +            } else {
> +                mThumbnail.setImageResource(R.drawable.tab_thumbnail_default);

The layer drawable should be applicable in all UIs, no? You can even remove tab_thumbnail_default from the tree.

Comment 11

[Martyn Haigh (:mhaigh)]

Attachment: Sort out aspect ratio of thumbnails for new tablet UI

So the issue is that currently the new globe drawable asset isn't scaling properly - the globe is now the correct size, but the surrounding background only covers behind the globe.

Comment 12

[Martyn Haigh (:mhaigh)]

Attachment: Sort out aspect ratio of thumbnails for new tablet UI and implement new empty tab globes

Moved the globes and associated files to the normal res folder to add compatibility with non tablet devices.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8514445 [details] [diff] [review]
Sort out aspect ratio of thumbnails for new  tablet UI and implement new empty tab globes

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

*Awesome*. I assume you can remove tab_thumbnail_default now? Giving r+, please apply suggested changes before landing and test this patch with phones and old tablet UI.

::: mobile/android/base/home/TopSitesThumbnailView.java
@@ +102,5 @@
> +            setScaleType(ScaleType.MATRIX);
> +            RectF rect = new RectF(0, 0, width, height);
> +            Matrix matrix = new Matrix();
> +            matrix.setRectToRect(rect, rect, Matrix.ScaleToFit.CENTER);
> +            setImageMatrix(matrix);

You should be doing this in onLayout(), as onMeasure() is often called multiple times in each layout traversal.

::: mobile/android/base/resources/values/dimens.xml
@@ +128,5 @@
>      <dimen name="history_tab_indicator_height">50dp</dimen>
>  
>  
> +    <dimen name="new_tablet_tab_thumbnail_height">140dp</dimen>
> +    <dimen name="new_tablet_tab_thumbnail_width">168dp</dimen>

This is not necessary once you rebase on top of bug 1064415, right?

::: mobile/android/base/widget/ThumbnailView.java
@@ +34,4 @@
>      @Override
>      public void onDraw(Canvas canvas) {
>          Drawable d = getDrawable();
> +        if (mLayoutChanged && mScale) {

It's more like:

public void onDraw(Canvas canvas) {
     if (!mScale) {
          super.onDraw(canvas);
          return;
     }

     Drawable d = getDrawable();
     ...
}

@@ +72,5 @@
> +            mScale = false;
> +        } else {
> +            mScale = true;
> +            setScaleType(ScaleType.FIT_CENTER);
> +        }

nit: add empty line here.

Comment 14

[Martyn Haigh (:mhaigh)]

Attachment: Sort out aspect ratio of thumbnails for new tablet UI and implement new empty tab globes

Fixed nits

Comment 16

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/integration/fx-team/rev/52abcdd555d1

Comment 17

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/mozilla-central/rev/52abcdd555d1