810342 - Toolbar shadow is not drawn on about:blank

Status: RESOLVED FIXED

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

Description

[Morrison Cole]

Attachment: Screenshot #1

The shadow that is drawn between the toolbar and the page on web pages is not drawn on about:blank.

Comment 1

[Chris Lord [:cwiiis]]

This has become more noticeable of recent as we appear to set the URL to about:blank now while bringing up the browser(?)

Comment 2

[Lucas Rocha (:lucasr)]

Morrison, this is probably because of this line here:

  http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserToolbar.java#770

We don't show shadow on "about:" pages so that there's no visual distinction between the page background and the toolbar in about:home, about:firefox, and about:addons. Seems like the same shouldn't apply to about:blank.

Comment 3

[Paul Feher]

Following the comment 2 I'm confirming this Bug.

Comment 4

[Shriram (irc: Mavericks)]

Attachment: Patch that fixes Toolbar shadow issue for about:blank

Comment 5

[Shriram (irc: Mavericks)]

Attachment: Patch that fixes Toolbar shadow issue for about:blank

Uses the right String class method, and adds relevant information to the corresponding comment

Comment 6

[:Margaret Leibovic]

Comment on attachment 695467 [details] [diff] [review]
Patch that fixes Toolbar shadow issue for about:blank

Shriram, I'm just assuming you forgot to ask lucasr for review on this new patch :)

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 695467 [details] [diff] [review]
Patch that fixes Toolbar shadow issue for about:blank

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

Thanks for the Patch Shriram. Looks almost right. Use equals instead of compareTo.

::: mobile/android/base/BrowserToolbar.java
@@ +928,5 @@
>          String url = tab.getURL();
>  
> +        // Only set shadow to visible when not on about screens except about:blank.
> +        visible &= !(url == null || (url.startsWith("about:") && 
> +                     url.compareTo("about:blank")));

Doesn't String.compareTo() return an integer? I think you can simply use equals() here (which returns a boolean).

Comment 8

[Shriram (irc: Mavericks)]

Attachment: Patch that fixes Toolbar shadow issue for about:blank

Replaces compareTo with equals as per last comment.

Comment 9

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

Comment on attachment 699906 [details] [diff] [review]
Patch that fixes Toolbar shadow issue for about:blank

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

::: mobile/android/base/BrowserToolbar.java
@@ +928,5 @@
>          String url = tab.getURL();
>  
> +        // Only set shadow to visible when not on about screens except about:blank.
> +        visible &= !(url == null || (url.startsWith("about:") && 
> +                     url.equals("about:blank")));

drive-by comment: i think this should be !url.equals instead of url.equals.

Comment 10

[Shriram (irc: Mavericks)]

Attachment: Patch that fixes Toolbar shadow issue for about:blank

Fixes mistake from previous patch as per last comment. 
Thank you Kats. Sorry about that. Done in haste

Comment 11

[Lucas Rocha (:lucasr)]

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d70c9eb65c75

Thanks for your patch Shriram! Let me know if you're looking for your next bug to fix ;-)

Comment 12

[Shriram (irc: Mavericks)]

Definitely. Thanks again Lucas for the review. I've a few bugs in mind.
I've also noted down couple of bugs you mentioned on IRC #mobile. will stay in touch

Comment 13

[Ed Morley [:emorley]]

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