Closed Bug 1009370 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.3

People

(Reporter: jboriss, Assigned: poiru, Mentored)

References

()

Details

(Whiteboard: [diamond])

Attachments

(8 files, 6 obsolete files)

91.41 KB, image/png
Details
2.77 KB, image/png
Details
1.85 KB, image/png
Details
5.98 KB, patch
Unfocused
: review+
Details | Diff | Splinter Review
165.20 KB, image/png
phlsa
: ui-review-
Details
33.08 KB, patch
Unfocused
: review+
Details | Diff | Splinter Review
4.96 KB, patch
Unfocused
: review+
Details | Diff | Splinter Review
91.81 KB, image/png
Details
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.
Flags: firefox-backlog+
Whiteboard: p=0 [qa?]
Adding diamond bug status. Boriss, any suggestions for a mentor here?
Flags: needinfo?(jboriss)
Whiteboard: p=0 [qa?] → p=0 [qa?] [diamond]
Points: --- → 5
Whiteboard: p=0 [qa?] [diamond] → [diamond]
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.
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
Flags: needinfo?(jboriss)
Hardware: x86 → All
Flags: needinfo?(jboriss)
QA Whiteboard: [qa?]
Iteration: --- → 34.1
(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.
Flags: needinfo?(jboriss)
QA Whiteboard: [qa?] → [qa+]
QA Contact: camelia.badau
Attached image Mask icon: 90x90 px
Correcting color for new style (#424E5A)
Attachment #8463769 - Attachment is obsolete: true
Attached image Mask icon: 45x45 px
Correcting color for new style (#424E5A)
Attachment #8463768 - Attachment is obsolete: true
Attachment #8467084 - Flags: review?(bmcbride)
This removes unnecessary id attributes and divs.
Attachment #8467085 - Flags: review?(bmcbride)
Iteration: 34.1 → 34.2
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
Attachment #8467085 - Flags: review?(bmcbride) → review+
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.
Attachment #8469030 - Flags: ui-review?(philipp)
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.)
Attachment #8467084 - Flags: review?(bmcbride) → review-
Mentor: bmcbride
Addressed comment 12.
Attachment #8467084 - Attachment is obsolete: true
Attachment #8469061 - Flags: review?(bmcbride)
(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).
Attachment #8469061 - Flags: review?(bmcbride) → review-
(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.
Attachment #8469030 - Flags: ui-review?(philipp) → ui-review-
(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+.
Attachment #8469061 - Attachment is obsolete: true
Attachment #8469259 - Flags: ui-review+
Attachment #8469259 - Flags: review?(bmcbride)
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.
Attachment #8469259 - Flags: review?(bmcbride) → review-
Iteration: 34.2 → ---
Attachment #8469259 - Attachment is obsolete: true
Attachment #8474343 - Flags: review?(bmcbride)
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
Attachment #8474342 - Flags: review?(bmcbride) → review-
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.
Attachment #8474343 - Flags: review?(bmcbride) → review+
Attachment #8474342 - Attachment is obsolete: true
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!
Attachment #8478087 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/ed82256c4a29
https://hg.mozilla.org/mozilla-central/rev/8f5ad2b5a775
https://hg.mozilla.org/mozilla-central/rev/27e503753e00
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
QA Whiteboard: [qa+]
Flags: qe-verify+
Depends on: 1059243
Iteration: --- → 34.3
Attached image 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?
Flags: needinfo?(birunthan)
(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?
Flags: needinfo?(birunthan) → needinfo?(philipp)
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
Flags: needinfo?(philipp)
(In a new bug, please)
I filled bug 1062264.
Status: RESOLVED → VERIFIED
Depends on: 1062843
Depends on: 1106792
You need to log in before you can comment on or make changes to this bug.