Last Comment Bug 810278 - Background does not match location bar
: Background does not match location bar
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Theme and Visual Design (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: Firefox 19
Assigned To: Morrison Cole
:
: Anthony Lam (:antlam)
Mentors:
Depends on: 813315 813311
Blocks: 813236
  Show dependency treegraph
 
Reported: 2012-11-09 05:49 PST by Morrison Cole
Modified: 2012-11-20 06:27 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Background and toolbar don't match (129.33 KB, image/png)
2012-11-09 05:49 PST, Morrison Cole
no flags Details
Used the correct resource for page backgrounds & added automatic toggle of the toolbar shadow on overscroll (7.75 KB, patch)
2012-11-09 05:54 PST, Morrison Cole
chrislord.net: feedback+
Details | Diff | Splinter Review
Modified background resource WITHOUT shadow toggling (126.58 KB, image/png)
2012-11-09 05:55 PST, Morrison Cole
no flags Details
Modified background resource WITH shadow toggling (127.74 KB, image/png)
2012-11-09 05:55 PST, Morrison Cole
no flags Details
Improved background/toolbar visual cohesion. (8.84 KB, patch)
2012-11-09 08:24 PST, Morrison Cole
wjohnston2000: review+
Details | Diff | Splinter Review
Improved background/toolbar visual cohesion (8.95 KB, patch)
2012-11-16 08:56 PST, Morrison Cole
wjohnston2000: review+
chrislord.net: feedback+
Details | Diff | Splinter Review

Description Morrison Cole 2012-11-09 05:49:02 PST
Created attachment 680061 [details]
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 2012-11-09 05:54:00 PST
Created attachment 680062 [details] [diff] [review]
Used the correct resource for page backgrounds & added automatic toggle of the toolbar shadow on overscroll
Comment 2 Morrison Cole 2012-11-09 05:55:32 PST
Created attachment 680063 [details]
Modified background resource WITHOUT shadow toggling
Comment 3 Morrison Cole 2012-11-09 05:55:59 PST
Created attachment 680064 [details]
Modified background resource WITH shadow toggling
Comment 4 Chris Lord [:cwiiis] 2012-11-09 05:59:06 PST
Assigning this to Morrison, but leaving as unconfirmed pending UX comment.
Comment 5 Chris Lord [:cwiiis] 2012-11-09 06:36:59 PST
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 2012-11-09 08:24:11 PST
Created attachment 680084 [details] [diff] [review]
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 2012-11-09 08:38:07 PST
Note: attachment 680084 [details] [diff] [review] also fixes Bug 810081
Comment 8 Wesley Johnston (:wesj) 2012-11-13 09:50:46 PST
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] 2012-11-13 13:04:07 PST
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 2012-11-16 08:56:24 PST
Created attachment 682494 [details] [diff] [review]
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] 2012-11-16 09:46:14 PST
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) 2012-11-16 12:44:41 PST
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 <name@email.com>"). 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 14 Ryan VanderMeulen [:RyanVM] 2012-11-16 19:05:24 PST
https://hg.mozilla.org/mozilla-central/rev/eeda949e9f4a
Comment 15 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-11-19 11:33:43 PST
*sigh* All my hard work to make Axis.java non-public undone... and completely avoidable too!
Comment 16 Wesley Johnston (:wesj) 2012-11-19 13:43:35 PST
I don't think its worth backing this out for that. Please file a follow up kats.
Comment 17 Cristian Nicolae (:xti) 2012-11-20 06:27:21 PST
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

Note You need to log in before you can comment on or make changes to this bug.