The default bug view has changed. See this FAQ.

Background does not match location bar

VERIFIED FIXED in Firefox 19

Status

()

Firefox for Android
Theme and Visual Design
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Morrison Cole, Assigned: Morrison Cole)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 19
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 680062 [details] [diff] [review]
Used the correct resource for page backgrounds & added automatic toggle of the toolbar shadow on overscroll
Attachment #680062 - Flags: feedback?(chrislord.net)
(Assignee)

Comment 2

4 years ago
Created attachment 680063 [details]
Modified background resource WITHOUT shadow toggling
(Assignee)

Comment 3

4 years ago
Created attachment 680064 [details]
Modified background resource WITH shadow toggling

Comment 4

4 years ago
Assigning this to Morrison, but leaving as unconfirmed pending UX comment.
Assignee: nobody → admin

Comment 5

4 years ago
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+
(Assignee)

Comment 6

4 years ago
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.
Attachment #680062 - Attachment is obsolete: true
Attachment #680084 - Flags: review?(wjohnston)
(Assignee)

Comment 7

4 years ago
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+

Comment 9

4 years ago
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.
(Assignee)

Comment 10

4 years ago
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.
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/integration/mozilla-inbound/rev/eeda949e9f4a
https://hg.mozilla.org/mozilla-central/rev/eeda949e9f4a
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Blocks: 813236
No longer blocks: 813236
Depends on: 813236

Updated

4 years ago
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: 813311
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
You need to log in before you can comment on or make changes to this bug.