Attached is a new visual style for in-content pages on private browsing windows. The style is consistent with in-content page styles, as specified by Project Chameleon (http://people.mozilla.org/~jgruen/chameleon/#nav4 and about:preferences in Fx29+). The copy is also simplified removing redundancies in referring to this window. I’ll open another bug for implementation after closing this one. The title and text paragraph, as a unit, are centered vertically and horizontally. This is the followup implementation work for bug 986644.

Adding diamond bug status. Boriss, any suggestions for a mentor here?

Is the mask image available somewhere? Also, what about the copy/image for the normal about:privatebrowsing page? To access the page, you can type in about:privatebrowsing in a non-private window.

(In reply to Mike Hoye [:mhoye] from comment #1) > Adding diamond bug status. Boriss, any suggestions for a mentor here? Unfocused might be a good one, as he's mentored an intern in the area previously (sorry Blair!) (In reply to Birunthan Mohanathas [:poiru] from comment #2) > Is the mask image available somewhere? It's currently in mxr as a toolbar item: http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/Toolbar@2x.png. I can put it in another format if that's easier.

Correcting color for new style (#424E5A)

Correcting color for new style (#424E5A)

This removes unnecessary id attributes and divs.

Comment on attachment 8467085 [details] [diff] [review] Clean-up aboutPrivateBrowsing.xhtml Review of attachment 8467085 [details] [diff] [review]: ----------------------------------------------------------------- Woo, it was nice to see this patch too - nice cleanup! ::: browser/base/content/aboutprivatebrowsing/aboutPrivateBrowsing.xhtml @@ +58,5 @@ > > // Set up the help link > + let learnMoreURL = Cc["@mozilla.org/toolkit/URLFormatterService;1"] > + .getService(Ci.nsIURLFormatter) > + .formatURLPref("app.support.baseURL"); You could import Services.jsm at the top, and use Services.urlFormatter here

Philipp: Mind looking at the strings used in this patch? Attached is a screenshot of the patch applied, when not in private browsing mode. These strings weren't in any mockup.

Comment on attachment 8467084 [details] [diff] [review] Implement new visual style for about:privatebrowsing Review of attachment 8467084 [details] [diff] [review]: ----------------------------------------------------------------- I don't think these files should be moved under /browser/base/content/aboutprivatebrowsing/. It does put the about page in the same location as some other about pages, but I don't think it makes sense to have private browsing things split between two directories under /browser/. The /browser/ parts of private browsing are more than just the about page. So lets keep everything in browser/components/privatebrowsing ::: browser/base/content/aboutprivatebrowsing/aboutPrivateBrowsing.css @@ +20,5 @@ > + max-width: 512px; > +} > + > +.titleText { > + background: url("mask.png") left 0 no-repeat; This should be a chrome:// URI. ::: browser/base/jar.mn @@ +69,5 @@ > > +* content/browser/aboutPrivateBrowsing.css (content/aboutprivatebrowsing/aboutPrivateBrowsing.css) > +* content/browser/aboutPrivateBrowsing.xhtml (content/aboutprivatebrowsing/aboutPrivateBrowsing.xhtml) > + content/browser/aboutprivatebrowsing/mask.png (content/aboutprivatebrowsing/mask.png) > + content/browser/aboutprivatebrowsing/mask@2x.png (content/aboutprivatebrowsing/mask@2x.png) These images should go in browser/themes/shared/ ::: browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd @@ +1,5 @@ > <!-- This Source Code Form is subject to the terms of the Mozilla Public > - License, v. 2.0. If a copy of the MPL was not distributed with this > - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > > +<!ENTITY privatebrowsingpage.title "You're browsing privately"> Unfortunately, we can't re-use string IDs like this. If you're changing the content or the string (or the context it's used in), you need to use a new ID. (Otherwise it's too easy for the change to be missed by localizers.)

Addressed comment 12.

(In reply to Blair McBride [:Unfocused] from comment #11) > Created attachment 8469030 [details] > Screenshot - normal window > > Philipp: Mind looking at the strings used in this patch? Attached is a > screenshot of the patch applied, when not in private browsing mode. These > strings weren't in any mockup. Why not use the current strings ?

Comment on attachment 8469061 [details] [diff] [review] Implement new visual style for about:privatebrowsing Review of attachment 8469061 [details] [diff] [review]: ----------------------------------------------------------------- One last thing, though I still want Philipp to look at this. ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css @@ +20,5 @@ > + max-width: 512px; > +} > + > +.titleText { > + background: url("chrome://browser/skin/mask.png") left 0 no-repeat; Just noticed you're not using the HiDPI version of this (mask@2x.png). To test this without using a HiDPI display, set the layout.css.devPixelsPerPx pref to 2 (default should be -1, which is autodetect).

(In reply to Blair McBride [:Unfocused] from comment #11) > Created attachment 8469030 [details] > Screenshot - normal window > > Philipp: Mind looking at the strings used in this patch? Attached is a > screenshot of the patch applied, when not in private browsing mode. These > strings weren't in any mockup. Looks good! Let's re-add the line »You are currently not in a private window« between the last paragraph and the button.

Comment on attachment 8469030 [details] Screenshot - normal window ui-r- just for that missing line of text mentioned in the last comment. Everything else is fine.

(In reply to Guillaume C. [:ge3k0s] from comment #14) > Why not use the current strings ? See mockup attachment 8421484 [details] - simplifying language/removing redundancies.

(In reply to Blair McBride [:Unfocused] from comment #15) > Just noticed you're not using the HiDPI version of this (mask@2x.png). Fixed. (In reply to Philipp Sackl [:phlsa] from comment #17) > Comment on attachment 8469030 [details] > Screenshot - normal window > > ui-r- just for that missing line of text mentioned in the last comment. > Everything else is fine. Added the line so marking ui-r+.

Comment on attachment 8469259 [details] [diff] [review] Implement new visual style for about:privatebrowsing Review of attachment 8469259 [details] [diff] [review]: ----------------------------------------------------------------- Code looks good - but patch now longer applies cleanly due to bug 1000625 :\ That bug changed a lot with the in-content styles - I'm pretty sure you'll just need to merge the link style with the new rules for .text-link etc, but I'd like to see the updated patch.

Comment on attachment 8474342 [details] [diff] [review] Part 1: Move inline-link style from preferences.css to common.inc.css Review of attachment 8474342 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/themes/shared/in-content/common.inc.css @@ +385,5 @@ > } > > /* Links */ > > +*.text-link, For the in-content CSS framework, since we're liberally mixing XUL and HTML namespaces, it's best to always explicitly specify which namespace a selector applies to (ie, the part before the |). So re-add the xul namespace to this selector. @@ +405,5 @@ > text-decoration: none; > } > > +html|a.inline-link:hover { > + text-decoration: underline; I think we should keep all types of links consistent. So just make html|.textlink work exactly the same as the other link classes we have in terms of color and text-decoration (but not line-height and font-size, as this class should inherit those since it's inline). ::: toolkit/themes/osx/global/in-content/common.css @@ +58,5 @@ > font-size: 1.25rem; > line-height: 22px; > } > + > +html|a.inline-link:-moz-focusring { This should apply to the other link classes we have too. And have similar rules for Windows: http://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/global.css#289 And Linux: http://dxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/global.css#279

Comment on attachment 8474343 [details] [diff] [review] Part 2: Implement new visual style for about:privatebrowsing Review of attachment 8474343 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml @@ +120,3 @@ > </p> > <p id="moreInfoLinkContainer"> > + <a id="moreInfoLink" class="text-link" target="_blank">&aboutPrivateBrowsing.learnMore;</a> Ah, now I see why you removed the namespace in the CSS for .text-link. Should keep that selector as just applying to XUL, and instead add a general selector for html|a. Then you won't need the text-link class here.

Comment on attachment 8478087 [details] [diff] [review] Part 1: Move inline-link style from preferences.css to common.inc.css Review of attachment 8478087 [details] [diff] [review]: ----------------------------------------------------------------- Ship it!

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed82256c4a29 https://hg.mozilla.org/integration/mozilla-inbound/rev/8f5ad2b5a775 https://hg.mozilla.org/integration/mozilla-inbound/rev/27e503753e00 Try push: https://tbpl.mozilla.org/?tree=Try&rev=f2368d308f28

https://hg.mozilla.org/mozilla-central/rev/ed82256c4a29 https://hg.mozilla.org/mozilla-central/rev/8f5ad2b5a775 https://hg.mozilla.org/mozilla-central/rev/27e503753e00

Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.4 using latest Nightly 34.0a1 (buildID: 20140828030205) and one mention should be done here: - on Windows and Ubuntu, the tab/newtab name is "You're browsing privately" - on Mac, the tab/newtab name is "New Tab". Please see screenshot "privatebrowsing.png" Is this intended behaviour?

(In reply to Camelia Badau, QA [:cbadau] from comment #29) > Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.4 using > latest Nightly 34.0a1 (buildID: 20140828030205) and one mention should be > done here: > - on Windows and Ubuntu, the tab/newtab name is "You're browsing privately" > - on Mac, the tab/newtab name is "New Tab". Please see screenshot > "privatebrowsing.png" > Is this intended behaviour? That was how it was in the past. I'm not sure there is any valid reason for it, though. phlsa, should we use a common window title for all platforms? If so, should it be "New Tab" or something else?

IIRC the previous string was "new private tab" and not "you're browsing privately".

Let's use »Private Browsing« as the tab title (on all platforms). Reasons: - »You're browsing privately« is too long and gets cut off - »New private tab« could imply that just this one tab is private, which is not the case

(In a new bug, please)

I filled bug 1062264.