Closed Bug 818065 Opened 9 years ago Closed 9 years ago

PBM - Make an about:privatebrowsing page

Categories

(Firefox for Android Graveyard :: General, defect, P1)

x86
macOS
defect

Tracking

(firefox20+ verified, firefox21 verified, fennec20+)

VERIFIED FIXED
Firefox 21
Tracking Status
firefox20 + verified
firefox21 --- verified
fennec 20+ ---

People

(Reporter: ibarlow, Assigned: bnicholson)

References

Details

Attachments

(6 files, 4 obsolete files)

When a user creates a new tab, they should land on a page that explains what private browsing is, and what it does / doesn't protect against. 

Designs to follow shortly. 

--

Note: this should be implemented before we consider Android Private Browsing v1 to be complete.
Assignee: nobody → bnicholson
tracking-fennec: --- → 20+
Priority: -- → P1
Attached patch Implement about:privatebrowsing (obsolete) — Splinter Review
Here's a preliminary patch that uses a heavily modified version of desktop's about:privatebrowsing page.
Attached image screenshot (obsolete) —
Attachment #690007 - Flags: feedback?(ibarlow)
Attachment #690007 - Attachment is patch: false
Attachment #690007 - Attachment mime type: text/plain → image/png
Duplicate of this bug: 826273
We'll need this to ship private browsing.
(In reply to Brian Nicholson (:bnicholson) from comment #4)
> We'll need this to ship private browsing.

This really needs to land no later than early next week (given l10n impact). I suggest we land what we've got before the end of this week, and iterate on the UX afterwards. The string seems to be a sure thing, since it's been hardly changed from desktop's about:privatebrowsing.
Blocks: pb
Hi Brian, here is a mockup you can follow for this page. I'm not sure how you are building this, so can you let me know if you need mdpi/hdpi/xhdpi assets for the mask image?
Attached image screenshot (non-private) (obsolete) —
Screenshot of current patch if user goes to about:privatebrowsing in a non-private tab.
Attached image mockup
Let's at least style it to look like a light version of the real about:privatebrowsing. 

- Text colour is #222222, sizes are the same as the mockups above
- No mask image
- Use light blue in-content UI background
- Fix tab image.
(In reply to Ian Barlow (:ibarlow) from comment #8)
> - Fix tab image.

That's happening because of bug 817828.
There's still a bit of work to do with the about:privatebrowsing page to make it compatible with different device resolutions, and the assets aren't available yet. Can we just go with a strings-only patch and worry about the rest later? These are the strings shown in Ian's mockups.
Attachment #698314 - Flags: review?(mark.finkle)
Attachment #690007 - Attachment is obsolete: true
Attachment #690007 - Flags: feedback?(ibarlow)
Attachment #690006 - Attachment is obsolete: true
Attachment #698021 - Attachment is obsolete: true
Comment on attachment 698314 [details] [diff] [review]
Part 1: Add about:privatebrowsing strings

Can we get a simple placeholder page ready to go soon? We want the l10n folks to be able to see the page in action to assist in the translation.
Attachment #698314 - Flags: review?(mark.finkle) → review+
Ian's mockups show "You have opened a new private tab." in the same block as the private browsing description. I'm no l10n expert, but I think we'll need to make another entry that glues the sentences together, allowing RTL languages to reverse the order. I did this by adding the "privatebrowsingpage.description.private" entity.
Attachment #698314 - Attachment is obsolete: true
Attachment #698321 - Flags: review?(mark.finkle)
Comment on attachment 698321 [details] [diff] [review]
Part 1: Add about:privatebrowsing strings, v2

The "private" mockup and the "not private" mockup treat the intro strings differently, which is a little odd.:

* "You have opened a new private tab." is part of the opening paragraph
* "You are not in a private tab." is a header, separate from the opening paragraph

The way you have the entities should work, but I think you could prepend the privatebrowsingpage.issueDesc.normal text directly to the privatebrowsingpage.description.private text and still make it work. Not a big deal I think.
Attachment #698321 - Flags: review?(mark.finkle) → review+
Hi Brian, here's the mask asset you can drop in - it's XHDPI, let me know if you need others.
(In reply to Mark Finkle (:mfinkle) from comment #11)
> Comment on attachment 698314 [details] [diff] [review]
> Part 1: Add about:privatebrowsing strings
> 
> Can we get a simple placeholder page ready to go soon? We want the l10n
> folks to be able to see the page in action to assist in the translation.

Yeah, I'll try to get something up ASAP.

http://hg.mozilla.org/integration/mozilla-inbound/rev/fee24a580b1c
Whiteboard: [leave open]
(In reply to Ian Barlow (:ibarlow) from comment #14)
> Created attachment 698327 [details]
> private browsing mask image
> 
> Hi Brian, here's the mask asset you can drop in - it's XHDPI, let me know if
> you need others.

Thanks - do you also have the asset for the dark gray background image?
Here you go Brian

Just note that this might not exactly match up in colour to the title bar background right now, but that it will shortly -- Sriram and I are still tweaking the title bar colour, so we just need to make a couple of changes to match it up to this background.
Follow-up patch for missing angled bracket in DTD:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6d11daac3fb
Here's a build that should be pretty close to the mockups: http://dl.dropbox.com/u/35559547/fennec-about-pb.apk

Ian, can you take a look? Even if it's not perfect yet, it'd be good to land something now to help localizers (see comment 11).
Flags: needinfo?(ibarlow)
This looks pretty awesome Brian. 

Weirdly, Roboto Light renders considerably lighter in weight on my phone than it does in Photoshop. We might want to try Roboto Regular in its place. I'm happy to move that into a different bug though if you need to land this.
Flags: needinfo?(ibarlow)
(In reply to Ian Barlow (:ibarlow) from comment #21)
> This looks pretty awesome Brian. 
> 
> Weirdly, Roboto Light renders considerably lighter in weight on my phone
> than it does in Photoshop. We might want to try Roboto Regular in its place.
> I'm happy to move that into a different bug though if you need to land this.

Yeah, I'm not sure what's happening there. On my Nexus, the font is rendered pixel-thin; on most of my other devices, though, it's actually quite a bit thicker than your mockup. Perhaps it's DPI related...
I was originally going to define a bunch of media queries to make this look correct on different devices, but I found that setting the "width=device-width, initial-scale=1;" has the effect of making px measurements into dip ones, simplifying the CSS. But without a fixed height for the content, there's no way that I know of to vertically center the page, so I'm still using media queries to set top margins for various resolutions.
Attachment #698830 - Flags: review?(mark.finkle)
Comment on attachment 698830 [details] [diff] [review]
Part 2: Implement about:privatebrowsing

>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java

>-        if (tab != null && "about:home".equals(tab.getURL()))
>+        if (tab != null && ("about:home".equals(tab.getURL()) ||
>+                            "about:privatebrowsing".equals(tab.getURL())))

I smell a whitelist in the future. Any more "about:xxx" and this will be too unwieldy
 
>diff --git a/mobile/android/components/AboutRedirector.js b/mobile/android/components/AboutRedirector.js

>+  privatebrowsing: {
>+    uri: "chrome://browser/content/aboutPrivateBrowsing.xhtml",
>+    privileged: true

Might be nice to try to get this unprivileged someday

>diff --git a/mobile/android/themes/core/images/privatebrowsing-mask.png b/mobile/android/themes/core/images/privatebrowsing-mask.png

This is a big file, I think. How big on disk? I wonder if Ian "crunched" it.

r+, looks good for now
Attachment #698830 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #24)
> >diff --git a/mobile/android/themes/core/images/privatebrowsing-mask.png b/mobile/android/themes/core/images/privatebrowsing-mask.png
> 
> This is a big file, I think. How big on disk?
> 

Looks like it's 35.6kB.
https://hg.mozilla.org/mozilla-central/rev/960e37ed0a59
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Flags: in-moztrap?(fennec)
Comment on attachment 698830 [details] [diff] [review]
Part 2: Implement about:privatebrowsing

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature
User impact if declined: creating a new private tab will just show about:home
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk (mostly just adds a new html page)
String or UUID changes made by this patch: none (strings were landed separately in part 1)
Attachment #698830 - Flags: approval-mozilla-aurora?
Attachment #698830 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on:
-build: Firefox for Android 20.0a2 (2013-01-14), Firefox for Android 21.0a1 (2013-01-14)
-device: Samsung Galaxy Nexus 
-OS: Android 4.1.2
Status: RESOLVED → VERIFIED
Test case added: https://moztrap.mozilla.org/manage/case/6090/
Flags: in-moztrap?(fennec) → in-moztrap+
Depends on: 830540
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.