730775 - Grey bar underneath the awesomebar on tablets

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Hommey [:glandium]]

Attachment: Screenshot

See attached screenshot. The bottom part of the background image looks weird, and the right part, for the menu, is disconnected.

This is on linaro android based on ICS.

Comment 1

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #0)
> See attached screenshot. The bottom part of the background image looks
> weird

In fact it looks like it's repeated vertically.

FWIW I expect the same to happen on the TF700 (the next ASUS Transformer, which is supposed to have a Full HD display) due in june.

Comment 2

[Sriram Ramasubramanian [:sriram]]

We don't have resources ready for tablets yet. So waiting for the tablet UI designs and resources to fix this issue.

Comment 3

[Mike Hommey [:glandium]]

So, it turns out this also happens at 1280x800 on the asus transformer. So all it would take is a higher density on phones for that to happen.

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

This is tablet work. Out of scope for the initial release.

Comment 6

[Mike Hommey [:glandium]]

"Grey bar underneath the awesomebar on tablets" is not the whole story, there's also the gap on the tab button.

I'm curious also, doesn't it happen on the Google Nexus in landscape mode (which has the same horizontal resolution as the ASUS Transformer, which does it)

Comment 7

[Matt Brubeck (:mbrubeck)]

(In reply to Mike Hommey [:glandium] from comment #6)
> "Grey bar underneath the awesomebar on tablets" is not the whole story,
> there's also the gap on the tab button.

This is bug 737479.

> I'm curious also, doesn't it happen on the Google Nexus in landscape mode
> (which has the same horizontal resolution as the ASUS Transformer, which
> does it)

No, because the Galaxy Nexus has a higher pixel density so it uses different layout and assets.  The various layout bugs on tablets should be fixed when we start working on the layout and assets for the tablet UI.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

The action-bar in tablets use a 56dip height for both portrait and landscape. Our current resources are 40dip (landscape) and 48dip (portrait). This brings the grey bar in tablets.

This patch resolves it by adding few more folders that qualify for tablets. The resources are just scaled up as place holders until we get the actual resources from UX team.

drawable-sw600dp-*/ is for supporting 7" tablets. We can drop that folder, if we don't want to support 7" tablets. However the action-bar in 7" tablets will have 56dip, which will cause this issue there again.

Comment 11

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

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

feedback+

Waiting for the real PNGs before r+. 

The size of address_bar_texture_tablet.png seems quite large. Is there anyway to reduce the filesize a bit? I'd like to keep the APK size as small as we can, since we are adding other things, like locales.

Comment 12

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch replaces the resources from the previous patch. This is a bug in honeycomb that causes the background of action-bar not to repeat. This has to be done in code manually. This patch fixes that too.

(For a brief second, the urlbar is corrupted. This is for the time when android loads it from xml, and we do initialize(). We cannot do anything about it.
However, this will be fixed as a part of bug 739355, which replaces the entire browser's url-bar for tablets).

Comment 13

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

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

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

>+    public boolean isTablet() {
>+        DisplayMetrics metrics = new DisplayMetrics();
>+        getWindowManager().getDefaultDisplay().getMetrics(metrics);
>+
>+        // Checking for sw600dp for 7" tablet.
>+        if (((metrics.widthPixels / metrics.density) >= 600) &&
>+            ((metrics.heightPixels / metrics.density) >= 600))
>+            return true;
>+        else
>+            return false;
>+    }

I am not a big fan of needing to do this in Java code. It seems brittle and easily broken. We already have an isTablet method in GeckoAppShell too.

>     public void refreshActionBar() {

>+            // Version 11 and 12 doesn't support tiling of bitmap in action bar.
>+            if (Build.VERSION.SDK_INT == 11 || Build.VERSION.SDK_INT == 12) {
>+                BitmapDrawable bitmap = new BitmapDrawable(BitmapFactory.decodeResource(getResources(), R.drawable.address_bar_texture_tablet));
>+                bitmap.setTileModeX(android.graphics.Shader.TileMode.REPEAT);
>+                background = bitmap;

Is this code removed in later patches (in bug 739355) ?

>+            } else if (isTablet()) {
>+                background = getResources().getDrawable(R.drawable.address_bar_bg);
>+            } else {
>+                background = getResources().getDrawable(R.drawable.gecko_actionbar_bg);
>+            }

We can't do this in layouts XML ?


>+        // Version 11 & 12 doesn't support tiling of bitmaps in action bar.
>+        // Refresh it to avoid corruption.
>+        if (Build.VERSION.SDK_INT == 11 || Build.VERSION.SDK_INT == 12)
>+            refreshActionBar();

Is this code removed in bug 739355 ?

>diff --git a/mobile/android/base/Makefile.in b/mobile/android/base/Makefile.in
>+RES_DRAWABLE_SW600DP_MDPI = \
>+  res/drawable-sw600dp-mdpi/address_bar_bg.xml \
>+  res/drawable-sw600dp-mdpi/address_bar_texture_tablet.png \
>+  $(NULL)
>+
>+RES_DRAWABLE_SW600DP_HDPI = \
>+  res/drawable-sw600dp-hdpi/address_bar_texture_tablet.png \
>+  $(NULL)
>+
>+RES_DRAWABLE_SW600DP_XHDPI = \
>+  res/drawable-sw600dp-xhdpi/address_bar_texture_tablet.png \
>+  $(NULL)

I wonder if we should add a comment about these being for 7" tablets

r+, but the big images and the Java code for tablets worries me. Also, I want to make sure we remove all the code we can when bug 739355 lands.

Comment 14

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #13)
> Comment on attachment 626674 [details] [diff] [review]
> Patch
> 
> >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> 
> >+    public boolean isTablet() {
> >+        DisplayMetrics metrics = new DisplayMetrics();
> >+        getWindowManager().getDefaultDisplay().getMetrics(metrics);
> >+
> >+        // Checking for sw600dp for 7" tablet.
> >+        if (((metrics.widthPixels / metrics.density) >= 600) &&
> >+            ((metrics.heightPixels / metrics.density) >= 600))
> >+            return true;
> >+        else
> >+            return false;
> >+    }
> 
> I am not a big fan of needing to do this in Java code. It seems brittle and
> easily broken. We already have an isTablet method in GeckoAppShell too.

Probably we should consolidate both and move to GeckoApplication. Since it's not going to change between activities, we can probably have it there. Or, we can have it in GeckoApp as its the base activity. Basically, it's better to have it UI thread, so all other UI calling it won't have "wrong thread exceptions". I would like to take this as a followup.

> 
> >     public void refreshActionBar() {
> 
> >+            // Version 11 and 12 doesn't support tiling of bitmap in action bar.
> >+            if (Build.VERSION.SDK_INT == 11 || Build.VERSION.SDK_INT == 12) {
> >+                BitmapDrawable bitmap = new BitmapDrawable(BitmapFactory.decodeResource(getResources(), R.drawable.address_bar_texture_tablet));
> >+                bitmap.setTileModeX(android.graphics.Shader.TileMode.REPEAT);
> >+                background = bitmap;
> 
> Is this code removed in later patches (in bug 739355) ?

This can be removed only after we add custom menu. Otherwise, the default menu button will have corrupted background. We can have a background for menu button given by android, but not for the overflow items like refresh and bookmark.

> 
> >+            } else if (isTablet()) {
> >+                background = getResources().getDrawable(R.drawable.address_bar_bg);
> >+            } else {
> >+                background = getResources().getDrawable(R.drawable.gecko_actionbar_bg);
> >+            }
> 
> We can't do this in layouts XML ?

This needs to be set, reset everytime we rotate the phone in ICS. The action-bar background holds the tail for Nexus. This can also be removed with custom menu.

> 
> 
> >+        // Version 11 & 12 doesn't support tiling of bitmaps in action bar.
> >+        // Refresh it to avoid corruption.
> >+        if (Build.VERSION.SDK_INT == 11 || Build.VERSION.SDK_INT == 12)
> >+            refreshActionBar();
> 
> Is this code removed in bug 739355 ?

This can be removed with tablet UI, if there is no corruption found. I'll try it and remove there.

> 
> >diff --git a/mobile/android/base/Makefile.in b/mobile/android/base/Makefile.in
> >+RES_DRAWABLE_SW600DP_MDPI = \
> >+  res/drawable-sw600dp-mdpi/address_bar_bg.xml \
> >+  res/drawable-sw600dp-mdpi/address_bar_texture_tablet.png \
> >+  $(NULL)
> >+
> >+RES_DRAWABLE_SW600DP_HDPI = \
> >+  res/drawable-sw600dp-hdpi/address_bar_texture_tablet.png \
> >+  $(NULL)
> >+
> >+RES_DRAWABLE_SW600DP_XHDPI = \
> >+  res/drawable-sw600dp-xhdpi/address_bar_texture_tablet.png \
> >+  $(NULL)
> 
> I wonder if we should add a comment about these being for 7" tablets
> 
> r+, but the big images and the Java code for tablets worries me. Also, I
> want to make sure we remove all the code we can when bug 739355 lands.

Comment 15

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b46ce2cea55b

Comment 16

[Ed Morley [:emorley]]

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