Closed Bug 810278 Opened 12 years ago Closed 12 years ago

Background does not match location bar

Categories

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

All
Android
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 19

People

(Reporter: admin, Assigned: admin)

References

Details

Attachments

(4 files, 2 obsolete files)

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.
Assigning this to Morrison, but leaving as unconfirmed pending UX comment.
Assignee: nobody → admin
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.
Attachment #680062 - Flags: feedback?(chrislord.net) → feedback+
Modified the overscroll background resource to match the toolbar.
Also added a toggle to hide the toolbar shadow when the page is overscrolled.
Attachment #680062 - Attachment is obsolete: true
Attachment #680084 - Flags: review?(wjohnston)
Note: attachment 680084 [details] [diff] [review] also fixes Bug 810081
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) {
Attachment #680084 - Flags: review?(wjohnston) → review+
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.
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.
Attachment #680084 - Attachment is obsolete: true
Attachment #682494 - Flags: review?(wjohnston)
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.
Attachment #682494 - Flags: feedback+
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;
}
Attachment #682494 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/eeda949e9f4a
Status: UNCONFIRMED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
No longer blocks: 813236
Depends on: 813236
Blocks: 813236
No longer depends on: 813236
*sigh* All my hard work to make Axis.java non-public undone... and completely avoidable too!
I don't think its worth backing this out for that. Please file a follow up kats.
Depends on: 813315
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
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: