Closed Bug 810342 Opened 7 years ago Closed 7 years ago

Toolbar shadow is not drawn on about:blank

Categories

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

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 21

People

(Reporter: admin, Assigned: kshriram18)

Details

(Whiteboard: [mentor=lucasr][lang=java])

Attachments

(2 files, 3 obsolete files)

Attached image Screenshot #1
The shadow that is drawn between the toolbar and the page on web pages is not drawn on about:blank.
This has become more noticeable of recent as we appear to set the URL to about:blank now while bringing up the browser(?)
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.
Following the comment 2 I'm confirming this Bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [mentor=lucasr][lang=java]
Attachment #695452 - Flags: review?(lucasr.at.mozilla)
Uses the right String class method, and adds relevant information to the corresponding comment
Attachment #695452 - Attachment is obsolete: true
Attachment #695452 - Flags: review?(lucasr.at.mozilla)
Assignee: nobody → kshriram18
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 :)
Attachment #695467 - Flags: review?(lucasr.at.mozilla)
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).
Attachment #695467 - Flags: review?(lucasr.at.mozilla) → review-
Status: NEW → ASSIGNED
Replaces compareTo with equals as per last comment.
Attachment #695467 - Attachment is obsolete: true
Attachment #699906 - Flags: review?(lucasr.at.mozilla)
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.
Fixes mistake from previous patch as per last comment. 
Thank you Kats. Sorry about that. Done in haste
Attachment #699906 - Attachment is obsolete: true
Attachment #699906 - Flags: review?(lucasr.at.mozilla)
Attachment #699992 - Flags: review?(lucasr.at.mozilla)
Attachment #699992 - Flags: review?(lucasr.at.mozilla) → review+
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 ;-)
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
https://hg.mozilla.org/mozilla-central/rev/d70c9eb65c75
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
You need to log in before you can comment on or make changes to this bug.