1009370 - Implement new visual style for private browsing mode in-content page

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: New private browsing mode in-content page

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.

Comment 1

[Mike Hoye [:mhoye]]

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

Comment 2

[Biru [:poiru]]

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.

Comment 3

[Jennifer Morrow [:Boriss] (UX)]

(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/[e-mail].  I can put it in another format if that's easier.

Comment 4

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Private_browsing_mask.png (small version, 20px width)

Comment 5

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Private_browsing_mask_2x.png Private_browsing_mask.png (large version, 40px width)

Comment 6

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mask icon: 90x90 px

Correcting color for new style (#424E5A)

Comment 7

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mask icon: 45x45 px

Correcting color for new style (#424E5A)

Comment 8

[Biru [:poiru]]

Attachment: Implement new visual style for about:privatebrowsing

Comment 9

[Biru [:poiru]]

Attachment: Clean-up aboutPrivateBrowsing.xhtml

This removes unnecessary id attributes and divs.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: 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.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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/[e-mail] (content/aboutprivatebrowsing/[e-mail])

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.)

Comment 13

[Biru [:poiru]]

Attachment: Implement new visual style for about:privatebrowsing

Addressed comment 12.

Comment 14

[u428464]

(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 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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 ([e-mail]).

To test this without using a HiDPI display, set the layout.css.devPixelsPerPx pref to 2 (default should be -1, which is autodetect).

Comment 16

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(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 17

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Guillaume C. [:ge3k0s] from comment #14)
> Why not use the current strings ?

See mockup attachment 8421484 [details] - simplifying language/removing redundancies.

Comment 19

[Biru [:poiru]]

Attachment: Implement new visual style for about:privatebrowsing

(In reply to Blair McBride [:Unfocused] from comment #15)
> Just noticed you're not using the HiDPI version of this ([e-mail]).

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 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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 21

[Biru [:poiru]]

Attachment: Part 1: Move inline-link style from preferences.css to common.inc.css

Comment 22

[Biru [:poiru]]

Attachment: Part 2: Implement new visual style for about:privatebrowsing

Comment 23

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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 25

[Biru [:poiru]]

Attachment: Part 1: Move inline-link style from preferences.css to common.inc.css

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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!

Comment 27

[Biru [:poiru]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Camelia Badau [:cbadau], Desktop QA]

Attachment: privatebrowsing.png

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?

Comment 30

[Biru [:poiru]]

(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?

Comment 31

[u428464]

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

Comment 32

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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

Comment 33

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In a new bug, please)

Comment 34

[Camelia Badau [:cbadau], Desktop QA]

I filled bug 1062264.