704864 - Completely rework tab menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Ian Barlow (:ibarlow)]

Our new tab menu feels a bit... lackluster. 

It would be great to work in thumbnails, similar to what we're doing on Honeycomb, and what we were doing on previous versions of fennec. 

I am still iterating on the design, but it will likely resemble this sketch http://cl.ly/1z353R0z1P3J1D3F1W2H
* 2 columns, big thumbnails
* leave some space at the bottom of the menu to still show web content, scroll content if it grows beyond menu height. 

More to come soon.

Comment 1

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

How important is this for the initial release? The portrait mode tab list on tablets uses favicons too.

Comment 2

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

Sriram - unassigned

Comment 3

[Ian Barlow (:ibarlow)]

It would be a really nice to have, but not a show stopper either if we don't make it in. So, we should probably focus on current priorities and look at this if we have time.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: screen shot

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 577038 [details]
patch

Patch is a PNG. :(

What is the plan here wrt UX?  Land first (the big pieces) then figure out how it should actually look?

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 9

[Lucas Rocha (:lucasr)]

(In reply to Brad Lassey [:blassey] from comment #5)
> Created attachment 577035 [details]
> screen shot

This probably needs to be reviewed by the design team. Personally, I don't think the UI is looking good enough to land. There's simply too much visual noise for each tab (thumbnail + favicon + title + url). Also, thumbnails on this size are not very helpful...

Comment 10

[Doug Turner (:dougt)]

the native ui has the bytes for the screenshot already.  It would be kinda nice to be able to use that.  We could also use this for the "last saved screenshot".  I am not sure if it will work, bit if it does, we can remove the JNI stuff and the screeshot event.

Take a look at birch/mobile/android/base/gfx/GeckoSoftwareLayerClient.java.  It is mBuffer.

Comment 11

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

Kudos to Brad for getting a patch, but I don't think we want this feature, now or ever on phones. The screenshot looks way too busy and noisy. I really don't see how it is a benefit.

Comment 12

[Ian Barlow (:ibarlow)]

Attachment: rough tab menu

Thanks for taking a look at this, Brad!

I think thumbnails is something we should push for, but we will need to rework the menu layout a bit to make it less cluttered and more useful. I've attached a rough wireframe to give you a sense of how that could look. 

- similar treatment to our tab column on honeycomb at the moment
- bigger thumbnails that are easier to recognize
- small page titles
- x button on the thumbnail
- no favicons

---

As an aside, I've been playing with the stock browser on my new Galaxy Nexus for the last couple days, and I must say, their tab menu is pretty slick and kind of blows us out of the water at the moment. It would be great to get a more interesting looking tab menu in for our first release.

Comment 13

[Sriram Ramasubramanian [:sriram]]

I had a patch last week and thought the bug was assigned to me :(

Comment 14

[Lucas Rocha (:lucasr)]

(In reply to Ian Barlow (:ibarlow) from comment #12)
> As an aside, I've been playing with the stock browser on my new Galaxy Nexus
> for the last couple days, and I must say, their tab menu is pretty slick and
> kind of blows us out of the water at the moment. It would be great to get a
> more interesting looking tab menu in for our first release.

Although I agree that the tab UI on ICS's stock browser is slick, I find it a bit over-the-top and causes a big context switch every time you change tabs. It follows the roughly same approach than iOS's browser. We should not simply follow that trend IMO.

I still think there's space for a simpler, straight-to-the-point yet beautiful tab UI. The perfect balance between simplicity and beauty might not be in our current favicon+title+url menu yet but I still think we should keep things simple for 1.0 and iterate on top of that.

Comment 15

[Ian Barlow (:ibarlow)]

Lucas, the slickness I was referring to was more to do with the polish of their design. 

I fully agree that we shouldn't switch someone out of their current context, which is why the mockup I showed uses the same kind of pulldown menu we currently use, just with a bit of a different layout inside :)

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: screen shot

some visual clean up

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

this uses mBuffer to generate the screenshots

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

needed a qref

Comment 20

[Doug Turner (:dougt)]

Comment on attachment 577458 [details] [diff] [review]
patch

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

I didn't really review the *.xml files.  I am not sure if we want to go with this look.  Still looks pretty busy to me.

::: mobile/android/base/Tab.java
@@ +137,5 @@
> +    public void updateThumbnail(Bitmap b) {
> +        if (sMinDim == 0) {
> +            DisplayMetrics metrics = new DisplayMetrics();
> +            GeckoApp.mAppContext.getWindowManager().getDefaultDisplay().getMetrics(metrics);
> +            sMinDim = metrics.widthPixels < metrics.heightPixels ? metrics.widthPixels : metrics.heightPixels;

sMinDim = Math.min(metrics.widthPixels, metrics.heightPixels)

@@ +139,5 @@
> +            DisplayMetrics metrics = new DisplayMetrics();
> +            GeckoApp.mAppContext.getWindowManager().getDefaultDisplay().getMetrics(metrics);
> +            sMinDim = metrics.widthPixels < metrics.heightPixels ? metrics.widthPixels : metrics.heightPixels;
> +        }
> +        mThumbnail = new BitmapDrawable(Bitmap.createBitmap(b, 0, 0, sMinDim, sMinDim));

Should this happen off the main thread?

::: mobile/android/base/gfx/CairoUtils.java
@@ +77,5 @@
> +        case CairoImage.FORMAT_A8:   return Bitmap.Config.ALPHA_8;
> +        case CairoImage.FORMAT_ARGB32: return Bitmap.Config.ARGB_8888;
> +        case CairoImage.FORMAT_RGB16_565:   return Bitmap.Config.RGB_565;
> +        default:        throw new RuntimeException("Unknown Skia bitmap config");
> +        }

The spacing is all random in this method.  Lets fix it before one of the style guide authors mock us! :D

::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java
@@ +186,5 @@
>          mNewContent = true;
>      }
>  
> +    public Bitmap getBitmap() {
> +        Bitmap b = Bitmap.createBitmap(mWidth, mHeight, CairoUtils.cairoFormatTobitmapConfig(mFormat));

Put cairoFormatTobitmapConfig on a new line.

Comment 21

[Doug Turner (:dougt)]

> Should this happen off the main thread?

Discussed.  We can do this in a followup if it because a problem

With the other nits addressed, r+.

Comment 22

[Sriram Ramasubramanian [:sriram]]

https://bugzilla.mozilla.org/attachment.cgi?id=577458&action=diff#a/mobile/android/base/TabsTray.java_sec2

This should have been,

| if (thumbnailImage != null) |

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

mfinkle wants to morph this bug, moved the patch to bug 706321

Comment 24

[Sriram Ramasubramanian [:sriram]]

I was playing with showing thumbnails for sometime (thanks to Brad Lassey for the plumbing :) and thanks to Adapters .. one good concept in Android). Changing to GridView worked like a charm.

However, the activity takes quite sometime to display: 
I/ActivityManager(  115): Displayed org.mozilla.fennec_sriramramasubramanian/org.mozilla.gecko.TabsTray: +716ms

Also, the point of taking screenshot sounds to be not right. handleSelectTab() is called when someone selects a tab, event goes to Gecko, Gecko asks to switch to a tab. Instead, the screenshot should be taken when we are about to show TabsTray to the user.

This approach works fine, as long as we don't want to show live update of the thumbnail to the user, while TabsTray is shown. If we need live updating in TabsTray, onTabsChanged() should take a screenshot. When I tried doing it, I got a OutOfMemoryError. :(

The last time I worked on doing live updates of thumbnail, it worked without this issue.

Comment 25

[Sriram Ramasubramanian [:sriram]]

+
+    public Drawable getScreenshot() {
+        mTileLayer.beginTransaction();
+
+        Bitmap bitmap = Bitmap.createBitmap(mWidth, mHeight, Bitmap.Config.RGB_565);
+        bitmap.copyPixelsFromBuffer(mBuffer);
+
+        Drawable screenshot = (Drawable) (new BitmapDrawable(Bitmap.createBitmap(bitmap, 0, 0, 400, 300)));
+
+        mTileLayer.endTransaction();
+        return screenshot;
+    }

This was my code to get a thumbnail, as per Patrick's suggestion.

Comment 26

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

This patch cleans up screenshots a bit, and moves the place for taking screenshots to onTabsChanged(). This ensures the latest screenshot being available for live updating.

The TabsTray loads around the same time as it was earlier:

I/ActivityManager(  115): Displayed org.mozilla.fennec_sriramramasubramanian/org.mozilla.gecko.TabsTray: +261ms

This doesn't remove the resources that were added earlier, like the border for the TabsTray container. Once there is a spec and finalized design, they can be cleaned up.

This is an issue with scaling of the thumbnail. I accidentally deleted Brad's patch to get the minWidth of the device. I can add it back later to ensure a better cropped thumbnail.

Comment 27

[Sriram Ramasubramanian [:sriram]]

Attachment: Screenshot: Thumbnails in Portrait

Comment 28

[Sriram Ramasubramanian [:sriram]]

Attachment: Screenshot: Thumbnails in Landscape

Comment 29

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 578188 [details] [diff] [review]
WIP

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

I'd prefer if you rename tab_row.xml because its no longer a row

::: mobile/android/base/GeckoApp.java
@@ +593,2 @@
>          ByteArrayOutputStream bos = new ByteArrayOutputStream();
> +        final Bitmap bitmap = ((BitmapDrawable) screenshot).getBitmap();

this will cause us to crash. The splash screen image needs to be 1024x2048

::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java
@@ -180,5 @@
>              return new ViewportMetrics(mGeckoViewport);
>          return null;
>      }
>  
> -    public Bitmap getBitmap() {

this is used for the splash screen, you can't really remove it.

http://mxr.mozilla.org/projects-central/source/birch/mobile/android/base/GeckoApp.java#592

@@ +187,5 @@
> +        mTileLayer.beginTransaction();
> +
> +        Bitmap bitmap = Bitmap.createBitmap(mWidth, mHeight, Bitmap.Config.RGB_565);
> +        bitmap.copyPixelsFromBuffer(mBuffer);
> +        Drawable screenshot = (Drawable) (new BitmapDrawable(Bitmap.createBitmap(bitmap, 0, 0, 400, 300)));

I prefer returning the whole bitmap and letting the caller crop and/or scale it as needed

Comment 31

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

Comment on attachment 578188 [details] [diff] [review]
WIP

I think Ian might want the 'X' overlayed on the thumbnail too. So we don't have empty space on the right side of the thumbnail..

I agree with Brad's suggestions, but otherwise looks good.

f+, but with Brad's f- you're at zero :)

Comment 32

[Ian Barlow (:ibarlow)]

Attachment: Tab menu mockup

Sriram and I have been exploring the use of thumbnails in two columns in the menu, and while it is an improvement over the text-only menu, it is a bit too visually dense and can be somewhat cumbersome to browse.

This mockup addresses that issue by switching to a single column list of thumbnails, with more legible titles next to it. 

This can be considered the design of record, and I will post specs and assets shortly.

Comment 33

[Ian Barlow (:ibarlow)]

Attachment: Tab menu specs

Comment 34

[Ian Barlow (:ibarlow)]

Attachment: Graphic assets for tab menu

Comment 35

[Ian Barlow (:ibarlow)]

Attachment: Assets for tab shadows/borders, and active tab shape

Comment 36

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch implements the design as expected.

Known issues:
1. There is a 1px variation in the "tab-selected" image when we show a placeholder. It seems not to fit inside the thumbnail. This is because of the placeholder has a shadow in its own. I have requested Ian to give an image without shadow. That will fix the issue.
2. Having a height of 2/3 for the menu getting harder. I tried using layout_weight, but in vain. I would like to take it as a follow up bug.

Comment 37

[Ian Barlow (:ibarlow)]

Attachment: updated "borderless" placeholder assets

Sriram, this should fix your first issue :)

Comment 38

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch adds a file that the previous patch did.
Few images have been modified for better clarity.
Also, a density parameter is used to get almost same screenshot in different devices (like hdpi and xhdpi).

Comment 39

[Sriram Ramasubramanian [:sriram]]

The last patch might need clobbering a bit. As the background has changed and moved into proper folders. Gingerbread might refer to old resource inside drawable folder.

Comment 40

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

Comment on attachment 581794 [details] [diff] [review]
Patch

>                 if (bitmap != null) {
>+                    ByteArrayOutputStream bos = new ByteArrayOutputStream();
>+                    bitmap.compress(Bitmap.CompressFormat.PNG, 0, bos);
>+                    mLastScreen = bos.toByteArray();
>+
>                     // Make a thumbnail for the given tab, if it's still selected
>                     if (tab == mThumbnailTab)
>                         mThumbnailTab.updateThumbnail(bitmap);
>-
>-                    ByteArrayOutputStream bos = new ByteArrayOutputStream();
>-                    bitmap.compress(Bitmap.CompressFormat.PNG, 0, bos);
>-                    mLastScreen = bos.toByteArray();

Add a bitmap.recycle() call here. We do not want it in updateThumbnail(...)

>     void showTabs() {
>+        Tab tab = Tabs.getInstance().getSelectedTab();
>+        if (tab != null) {
>+            if (tab.getURL().equals("about:home"))
>+                tab.updateThumbnail(null);

I am not 100% sure why we need the about:home check right here. I would hope it is handled in some other place like handleDocumentStop of handleAddTab

>+            else
>+                tab.updateThumbnail(mSoftwareLayerClient.getBitmap());
>+        }

Remove this part. We do not update thumbnails dynamically. Only when the tab loads.

>diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java

>+    static float sDensity = 1;

I do not like this. I kinda want thumbnails to be save at the same size all the time. Let's just use a density of 1 and see how it looks. We can change it later if we have too.

>                         Bitmap cropped = Bitmap.createBitmap(b, 0, 0, sMinDim, sMinDim);
>                         Bitmap bitmap = Bitmap.createScaledBitmap(cropped, kThumbnailSize, kThumbnailSize, false);
>+                        saveThumbnailToDB(new BitmapDrawable(bitmap));
>+                        b.recycle();

This is too dangerous since the bitmap is passed in as a param. We need to move the recycle call to wherever the bitmap itself is created. Like in the SessionSnapshotRunnable

>+
>+                        bitmap = Bitmap.createBitmap(cropped, 0, 0, (int) (138 * sDensity), (int) (78 * sDensity));

I do not like this fixed aspect ratio. Since we could be saving to the system DB, we need to see how bad this looks in the Stock browser too.


r- for the recycle and showTabs changes. I want a followup bug filed for the thumbnail aspect ratio. The rest looks OK

Comment 41

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #40)
> Comment on attachment 581794 [details] [diff] [review]
> Patch
> 
> >                 if (bitmap != null) {
> >+                    ByteArrayOutputStream bos = new ByteArrayOutputStream();
> >+                    bitmap.compress(Bitmap.CompressFormat.PNG, 0, bos);
> >+                    mLastScreen = bos.toByteArray();
> >+
> >                     // Make a thumbnail for the given tab, if it's still selected
> >                     if (tab == mThumbnailTab)
> >                         mThumbnailTab.updateThumbnail(bitmap);
> >-
> >-                    ByteArrayOutputStream bos = new ByteArrayOutputStream();
> >-                    bitmap.compress(Bitmap.CompressFormat.PNG, 0, bos);
> >-                    mLastScreen = bos.toByteArray();
> 
> Add a bitmap.recycle() call here. We do not want it in updateThumbnail(...)

Adding "bitmap.recyle()" is a problem. updateThumbnail() "posts" a message to a thread. The execution times can differ and hence the bitmap might have been recycled before updateThumbnail consumes it. That's the reason I dropped bitmap.recycle() here and did it there.

> 
> >     void showTabs() {
> >+        Tab tab = Tabs.getInstance().getSelectedTab();
> >+        if (tab != null) {
> >+            if (tab.getURL().equals("about:home"))
> >+                tab.updateThumbnail(null);
> 
> I am not 100% sure why we need the about:home check right here. I would hope
> it is handled in some other place like handleDocumentStop of handleAddTab

onSavedInstance() tend to get the current screenshot and save it. So for the first time, about:home will have a blank white screenshot. This is never reset anywhere, other than here.

> 
> >+            else
> >+                tab.updateThumbnail(mSoftwareLayerClient.getBitmap());
> >+        }

This is to get a latest screenshot while loading the tab. The document might have been loading -- and I don't do a live screenshot when TabsTray is shown. This is the most recent screenshot that can be taken for the active tab. Removing this will fail to update a screenshot for a document that had loaded some time back (as we can't update the screenshot for a background tab).

> 
> Remove this part. We do not update thumbnails dynamically. Only when the tab
> loads.
> 
> >diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java
> 
> >+    static float sDensity = 1;
> 
> I do not like this. I kinda want thumbnails to be save at the same size all
> the time. Let's just use a density of 1 and see how it looks. We can change
> it later if we have too.

The density parameter is used only for "showing" the thumbnails. The thumbnails are saved at 96x96 pixels on all devices. The thumbnails shown on tabs-menu need different cropping based on the screen densities.

> 
> >                         Bitmap cropped = Bitmap.createBitmap(b, 0, 0, sMinDim, sMinDim);
> >                         Bitmap bitmap = Bitmap.createScaledBitmap(cropped, kThumbnailSize, kThumbnailSize, false);
> >+                        saveThumbnailToDB(new BitmapDrawable(bitmap));
> >+                        b.recycle();
> 
> This is too dangerous since the bitmap is passed in as a param. We need to
> move the recycle call to wherever the bitmap itself is created. Like in the
> SessionSnapshotRunnable

The execution is not sequential. Each one operates in different threads (or in same thread at different times). Hence moving back to SessionSnapshotRunnable is a problem.
One option is to make a copy there and send it here (which is kind of costly) -- which makes sure, each function recycles the bitmap after it has consumed it.

> 
> >+
> >+                        bitmap = Bitmap.createBitmap(cropped, 0, 0, (int) (138 * sDensity), (int) (78 * sDensity));
> 
> I do not like this fixed aspect ratio. Since we could be saving to the
> system DB, we need to see how bad this looks in the Stock browser too.

This doesn't affect the system db. This is just thumnails in tabs menu.

> 
> 
> r- for the recycle and showTabs changes. I want a followup bug filed for the
> thumbnail aspect ratio. The rest looks OK

Comment 42

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

Comment on attachment 581794 [details] [diff] [review]
Patch

Given Sriram's answers, I'll r+ if we remove taking a thumbnail in showTabs, move the "about:home" check to SessionSnapshotRunnable and add a comment to SessionSnapshotRunnable about the bitmap getting recycled in updateThumbnail.

Comment 43

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/53d6808502fa

Comment 44

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/53d6808502fa

Comment 45

[Carla Nadastean]

Verified with:
Aurora 11.0a2 (2012-01-08) HTC Desire Z (Android 2.3)
Nightly 12.0a1 (2012-01-08) HTc Desire Z (Android 2.3)

Tab menu is displayed according to specs from comment33.

Tabs thumbnail images are not displayed in tabs menu, but this issue is tracked by another bug. (Bug 711543 - Get thumbnail images of tabs loaded in the background, and display them in the tab menu)

Marking this bug as verified.

Comment 46

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/53d6808502fa

Comment 47

[Camelia Urian]

Test case created in litmus: https://litmus.mozilla.org/show_test.cgi?id=33815