810278 - Background does not match location bar

Status: VERIFIED FIXED

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

Description

[Morrison Cole]

Attachment: Background and toolbar don't match

The page background texture loads a resource that does not match the toolbar colour. 

Also, a shadow is always drawn at the top of the page regardless of the level of vertical overscroll.

Comment 1

[Morrison Cole]

Attachment: Used the correct resource for page backgrounds & added automatic toggle of the toolbar shadow on overscroll

Comment 2

[Morrison Cole]

Attachment: Modified background resource WITHOUT shadow toggling

Comment 3

[Morrison Cole]

Attachment: Modified background resource WITH shadow toggling

Comment 4

[Chris Lord [:cwiiis]]

Assigning this to Morrison, but leaving as unconfirmed pending UX comment.

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 680062 [details] [diff] [review]
Used the correct resource for page backgrounds & added automatic toggle of the toolbar shadow on overscroll

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

We've spoken and you've already fixed the thread issue (which removes the need for invalidating), so with the comments below addressed, this looks good to me. Either sriram, lucasr or wesj should review the final patch.

::: mobile/android/base/BrowserToolbar.java
@@ +658,5 @@
>          }
>      }
>  
>      public void setShadowVisibility(boolean visible) {
> +        if ((mShadow.getVisibility() == View.VISIBLE && !visible) || (mShadow.getVisibility() == View.GONE && visible)) {

As this is the only place we change the visibility on this, we can rely on it being either VISIBLE or GONE (and not INVISIBLE) - with this in mind, this if statement could be more simply:

if ((mShadow.getVisibility() == View.VISIBLE) == visible)

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +694,5 @@
> +
> +    private void setShadowVisibility() {
> +        String url = Tabs.getInstance().getSelectedTab().getURL();
> +        
> +        if (!(url == null) || !url.startsWith("about:")) {

Let's do this url check as part of BrowserToolbar.setShadowVisibility itself and have BrowserToolbar.refresh just call setShadowVisibility(true) (this is ok, as a page won't be in overscroll on refresh, I think).

This will save us having the code in two places.

@@ +699,5 @@
> +            if (mPanZoomController.getOverscroll() == Axis.Overscroll.PLUS || mPanZoomController.getOverscroll() == Axis.Overscroll.NONE) {
> +                BrowserApp.mBrowserToolbar.setShadowVisibility(true);
> +            }
> +            else {
> +                BrowserApp.mBrowserToolbar.setShadowVisibility(false);

This could be simplified to:

Axis.Overscroll overscroll = mPanZoomController.getOverscroll();
BrowserApp.mBrowserToolbar.setShadowVisibility(overscroll == Axis.Overscroll.PLUS || overscroll == Axis.Overscroll.NONE);

::: mobile/android/base/ui/PanZoomController.java
@@ +1070,5 @@
>      public int getOverScrollMode() {
>          return mX.getOverScrollMode();
>      }
> +
> +    public Axis.Overscroll getOverscroll() {

Let's call this GetOverscrollY, to clarify that it's the y-axis we're dealing with. No need to add a GetOverscrollX, we can add that if it becomes necessary.

Comment 6

[Morrison Cole]

Attachment: Improved background/toolbar visual cohesion.

Modified the overscroll background resource to match the toolbar.
Also added a toggle to hide the toolbar shadow when the page is overscrolled.

Comment 7

[Morrison Cole]

Note: attachment 680084 [details] [diff] [review] also fixes Bug 810081

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 680084 [details] [diff] [review]
Improved background/toolbar visual cohesion.

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

::: mobile/android/base/BrowserToolbar.java
@@ +661,5 @@
>      public void setShadowVisibility(boolean visible) {
> +        String url = Tabs.getInstance().getSelectedTab().getURL();
> +
> +        // Only set shadow to visible when not on about screens.
> +        visible &= !(url == null || !url.startsWith("about:"));

Remove the ! before url.startsWith("about:"). i.e.:

visible &= !(url == null || url.startsWith("about:"));

This is doing a string comparison on every viewport change. Ideally I think we could just cache that result on the tab and remove it when the url changes. Can you do that here too (in a separate patch if you want). i.e. Tabs.getInstance().getSelectedTab().isAboutPage().

@@ +663,5 @@
> +
> +        // Only set shadow to visible when not on about screens.
> +        visible &= !(url == null || !url.startsWith("about:"));
> +
> +        if ((mShadow.getVisibility() == View.VISIBLE) == visible) {

This should be "!= visible". i.e.:

if ((mShadow.getVisibility() == View.VISIBLE) != visible) {

Comment 9

[Chris Lord [:cwiiis]]

There's also an intermittent start-up crash with this patch:

11-13 13:02:18.758  7198  7198 E GeckoAppShell: >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
11-13 13:02:18.758  7198  7198 E GeckoAppShell: java.lang.NullPointerException
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at org.mozilla.gecko.BrowserToolbar.setShadowVisibility(BrowserToolbar.java:665)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at org.mozilla.gecko.BrowserApp.hideAboutHome(BrowserApp.java:648)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at org.mozilla.gecko.BrowserApp.initializeChrome(BrowserApp.java:273)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at org.mozilla.gecko.GeckoApp.initialize(GeckoApp.java:1698)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at org.mozilla.gecko.GeckoApp.onWindowFocusChanged(GeckoApp.java:2099)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at com.android.internal.policy.impl.PhoneWindow$DecorView.onWindowFocusChanged(PhoneWindow.java:2410)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at android.view.View.dispatchWindowFocusChanged(View.java:5755)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at android.view.ViewGroup.dispatchWindowFocusChanged(ViewGroup.java:851)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at android.view.ViewRootImpl.handleMessage(ViewRootImpl.java:2691)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at android.os.Handler.dispatchMessage(Handler.java:99)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at android.os.Looper.loop(Looper.java:137)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at android.app.ActivityThread.main(ActivityThread.java:4699)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at java.lang.reflect.Method.invokeNative(Native Method)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at java.lang.reflect.Method.invoke(Method.java:511)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:787)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:554)
11-13 13:02:18.758  7198  7198 E GeckoAppShell: 	at dalvik.system.NativeStart.main(Native Method)

Looks like there just needs to be a NULL check somewhere.

Comment 10

[Morrison Cole]

Attachment: Improved background/toolbar visual cohesion

Made changes suggested by :wesj and added a null check to prevent the intermittent start-up crash :cwiiis found. 

I'll add the cached comparison in a separate patch.

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 682494 [details] [diff] [review]
Improved background/toolbar visual cohesion

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

Awesome patch :) Been running with it, and it's so much better... Couple of nits:

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +693,5 @@
> +        setShadowVisibility();
> +    }
> +
> +    private void setShadowVisibility() {
> +        GeckoApp.mAppContext.mMainHandler.post(new Runnable() { 

There's a trailing space on this line.

@@ +696,5 @@
> +    private void setShadowVisibility() {
> +        GeckoApp.mAppContext.mMainHandler.post(new Runnable() { 
> +            public void run() {
> +                if (BrowserApp.mBrowserToolbar != null)
> +                {

This brace should hug the if statement on the line above (with a space)

::: mobile/android/base/ui/PanZoomController.java
@@ +413,5 @@
>          mLastEventTime = time;
>      }
>  
>      private void startPanning(float x, float y, long time) {
> +

There's an extra line added here unnecessarily.

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 682494 [details] [diff] [review]
Improved background/toolbar visual cohesion

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

Two tiny nits. But looks good!

I can make these little changes and push this for you, or you can throw up a new patch for checkin. This one has a commit message, but needs your name added to it (hg qref -u "My name <[e-mail]>"). If you want me to checkin, tell me what username you'd like listed. Thanks!

::: mobile/android/base/BrowserToolbar.java
@@ +658,5 @@
>          }
>      }
>  
>      public void setShadowVisibility(boolean visible) {
> +        String url = Tabs.getInstance().getSelectedTab().getURL();

We should probably also check that selectedTab isn't null. I'm not sure when exactly that happens, but we do check for it in several places and I'd rather be safe than sorry.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +695,5 @@
> +
> +    private void setShadowVisibility() {
> +        GeckoApp.mAppContext.mMainHandler.post(new Runnable() { 
> +            public void run() {
> +                if (BrowserApp.mBrowserToolbar != null)

We generally prefer early returns. i.e. 

if (BrowserApp.mBrowserToolbar == null) {
  return;
}

Comment 13

[Wesley Johnston (:wesj)]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Kartikaya Gupta (email:[e-mail])]

*sigh* All my hard work to make Axis.java non-public undone... and completely avoidable too!

Comment 16

[Wesley Johnston (:wesj)]

I don't think its worth backing this out for that. Please file a follow up kats.

Comment 17

[Cristian Nicolae (:xti)]

Now, the background is matching the location bar. Closing bug as verified fixed on:

Firefox 20.0a1 (2012-11-20)
Device: Galaxy S2
OS: Android 4.0.3