Closed Bug 1259340 Opened 4 years ago Closed 4 years ago

New Private Browsing Window Update

Categories

(Firefox :: Private Browsing, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- fixed

People

(Reporter: bbell, Assigned: rickychien)

References

(Depends on 2 open bugs)

Details

(Whiteboard: [fxprivacy])

Attachments

(2 files, 11 obsolete files)

1.50 KB, image/svg+xml
Details
58 bytes, text/x-review-board-request
past
: review+
Gijs
: review+
rickychien
: review+
Details
The "New Private Browsing Window - Welcome Screen" needs a refreshed look and tweaks to the wording in an effort to conform to the new style guide and speak with a more human-like voice.

A mock-up of the proposed changes can be seen here:
http://people.mozilla.org/~bbell/about-page-improvements/about-privateBrowsing.html

---

The impetus for this change came from a content audit we commissioned from Michelle Heubusch. She was tasked with evaluating all of the copy we use in Firefox, so she could make recommendations on how we can speak to our user in a more consistent and human voice. 

I have a short video clip of her discussing this page in particular: http://cl.ly/3w3H463x433m

In conjunction with copy changes we are also working on standardizing the look and feel to conform with an official style guide, produced by Steven Horlander. This design is now in compliance with those efforts.

---

Improvements include a clearer method for enabling and disabling Tracking protection, More visual feedback for the user to recognize if Tracking Protection is on, and a more understandable description of what's going on in Private browsing/Tracking Protection Mode.
Hi Evelyn -

Could you help to find someone to take this bug ?

Thanks
Flags: needinfo?(ehung)
Tim, would you like to have someone in your team to take this?
Flags: needinfo?(ehung) → needinfo?(timdream)
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: needinfo?(timdream)
yay
I haven't encounter obstacles on modifying UI relevant stuffs and patch is still WIP will be submitted soon.
Comment on attachment 8736177 [details] [diff] [review]
New Private Browsing Window Update r=past

Hi Panos,

Here is a new private browsing landing page patch. My first time dig into private browsing codebase and didn't know it's good method to add this feature.

Please point out any problem if you see. thanks!
Attachment #8736177 - Flags: review?(past)
Thanks for the patch! I'll try to take a look some time this week.
I haven't looked at the patch yet, but from visual inspection I can see that you don't seem to be using the following style change:

strong {
  font-weight: normal;
  color: var(--color-grey-lightest);
}

Is this on purpose? I'll also note that this needs a minor rebase after bug 1259847.
A few more observations:

- when I shrink the window width so much that a vertical scrollbar appears, the bottom part of the page has a different background color for some reason. This doesn't happen in Bryan's mockup.

- the circular white knob in the green toggle is slightly smaller than the mockup, but it looks good enough that I dodn't notice at first. However,

- when the window width is shrunk too much so that the "Tracking Protection" heading  starts to wrap, the green background becomes too small to hold both the tick and the circle and the widget starts to disintegrate.

I'm using an empty private browsing tab and another tab with Bryan's mockup next to it to spot these things.
I noticed that the "Tracking Protection" heading starts to disappear if I shrink the window height and it is unable to scroll to see the heading even my scroll bar has reached to the top (it happened in Bryan's mockup too). However, it doesn't happen in safari and works great as expect.

Removing body's justify-content: center; will get rid of this issue but I don't understand why it happened.
Attachment #8736177 - Attachment is obsolete: true
Attachment #8736177 - Flags: review?(past)
Comment on attachment 8737083 [details] [diff] [review]
New Private Browsing Window Update

Patch was updated to address all of these issues on comment 9. 

Panos, you could try that again. :)
Attachment #8737083 - Flags: review?(past)
Sorry for the delay here, but I've been traveling and pretty busy with other stuff lately. I expect to look at the patch probably on Thursday.
Attached patch Fixed several test fails (obsolete) — Splinter Review
New Private Browsing Window Update
Attachment #8739301 - Flags: review?(past)
Attachment #8737083 - Attachment is obsolete: true
Attachment #8737083 - Flags: review?(past)
Fixed several test fails


New Private Browsing Window Update
Attachment #8739302 - Flags: review?(past)
Attachment #8739301 - Attachment is obsolete: true
Attachment #8739301 - Flags: review?(past)
Updated patch and fixed test failed
Attachment #8739313 - Flags: review?(past)
Attachment #8739302 - Attachment is obsolete: true
Attachment #8739302 - Flags: review?(past)
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #8739314 - Flags: review?(past)
Attachment #8739313 - Attachment is obsolete: true
Attachment #8739313 - Flags: review?(past)
Comment on attachment 8739314 [details] [diff] [review]
Updated patch

Review of attachment 8739314 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good from a functional perspective, apart from the toggling of TP not toggling the appearance of "with Tracking Protection". I didn't have time to look thoroughly at the CSS changes, but I have a few important comments about the string changes. The strings in .dtd and .properties files need to have their keys changed when the value is modified. This is one of the ugly quirks that our l10n system has, but don't worry, you'll get used to it after a while! If you can't come up with a better name in every case, just append a version number to the key (foo.bar, foo.bar2, foo.bar3, etc.).

Good job overall, this is really close!

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css
@@ -1,3 @@
> -/* 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/. */

We can't remove copyright notices from files like this.

@@ -4,2 @@
>  
> -@import url("chrome://global/skin/in-content/common.css");

I'm concerned by the removal of the common styles import statement. I don't have much more time to look into the changes in this file today, but can't we keep overriding common styles here? Is every new style so fundamentally different?

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js
@@ +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/. */
> +
> +var { classes: Cc, interfaces: Ci, utils: Cu } = Components;

This file appears entirely changed due to some whitespace changes (line endings?). Since you are not using MozReview, which makes it easier to ignore such changes, could you please avoid mixing significant with whitespace changes?

@@ +7,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const FAVICON_QUESTION = "chrome://global/skin/icons/question-32.png";

Why this change? I can't find where this icon is displayed, but shouldn't it be the same size as the one below?

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
@@ +26,5 @@
>  
>    <body dir="&locale.dir;" class="private">
>      <p class="showNormal">&aboutPrivateBrowsing.notPrivate;</p>
>      <button xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +            id="start-private-browsing"

I'd prefer it if you could avoid such gratuitous changes. They obscure the important ones and unnecessarily complicate the review. It's not as if this makes the file consistent anyway.

@@ +34,5 @@
> +    <div class="showPrivate about-content-container">
> +      <section class="section-main">
> +        <section class="about-header">
> +  	      <div class="icon"></div>
> +  	      <div class="content">&aboutPrivateBrowsing.title; &aboutPrivateBrowsing.tracking;</div>

There are two problems with this:
- you are concatenating two strings in the markup instead of the strings file, which would localizers the opportunity to follow unusual concatenation rules in their languages
- the two strings should actually correspond to separate page elements, so that toggling TP would toggle the "with Tracking Protection" string as in the mockup

@@ +71,5 @@
> +          <div id="tp-icon" class="icon"></div>
> +  	      <div class="content">
> +    	      <div class="slider-toggle-group">
> +              <div class="slider-toggle-label">&trackingProtection.title;</div>
> +              <input id="tp-toggle" class="toggle toggle-input" type='checkbox'/>

Nit: double quotes consistently please.

@@ +91,5 @@
> +
> +      <section class="section-main">
> +        <section class="about-info">
> +  	      <div class="gutter"></div>
> +          <div class="content">

Nit: here and elsewhere in this file you can see that indentation is off. This was pointed to me when importing your patch in my repo using git (or git-cinnabar). If you use a different setup, you can look for hard tabs manually.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_about.js
@@ +56,5 @@
>  
>    let { win, tab } = yield openAboutPrivateBrowsing();
>  
>    yield testLinkOpensTab({ win, tab,
> +    elementId: "learn-more",

Again, this is the kind of gratuitous change that I've already mentioned.

::: browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd
@@ +3,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +<!ENTITY aboutPrivateBrowsing.notPrivate                 "You are currently not in a private window.">
> +<!ENTITY privateBrowsingPage.openPrivateWindow.label     "Open a Private Window">
> +<!ENTITY privateBrowsingPage.openPrivateWindow.accesskey "P">

Gratuitous changes in the key name on the other hand will cause all locales to do more work or fall behind on translation quality, so I'd prefer it if we left the key as it is. Whenever it has to be updated, we can fix the key name at the same time.

::: browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.properties
@@ +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/.
>  
> +title=Private Browsing

You need to change the key for the l10n tools to pick up the change.
Attachment #8739314 - Flags: review?(past) → review-
New Private Browsing Window Update
Updated patch to address above comment problems
Attachment #8739855 - Flags: review?(past)
Attachment #8739314 - Attachment is obsolete: true
const FAVICON_QUESTION = "chrome://global/skin/icons/question-32.png";

I found there are other better higher dimensions images located at toolkit/themes/[osx|windows]/global/icons/

However, there are nothing for linux. Not sure how our inner mapping mechanism works but I can see my question icon become more clear after replacing with 32 x 32 png on my retina display.
Attachment #8739856 - Flags: review?(past)
Attachment #8739855 - Attachment is obsolete: true
Attachment #8739855 - Flags: review?(past)
New Private Browsing Window Update
Attachment #8739858 - Flags: review?(past)
Attachment #8739856 - Attachment is obsolete: true
Attachment #8739856 - Flags: review?(past)
I know you're going to need images. Shall I post them on this bug?
Flags: needinfo?(rchien)
Comment on attachment 8739858 [details] [diff] [review]
Attachment to Bug 1259340 - New Private Browsing Window Update

Review of attachment 8739858 [details] [diff] [review]:
-----------------------------------------------------------------

This is much better, thanks. There are still some issues with the strings and the styling that will require another round. The CSS seems to be a significant rewrite, and I'm not sure how strict I should be in this case, so I'll defer to Gijs on that and on the question below.

(In reply to Ricky Chien [:rickychien] from comment #22)
> I've addressed almost of problems on review comment but got a question on
> this one
> 
> const FAVICON_QUESTION = "chrome://global/skin/icons/question-32.png";
> 
> I found there are other better higher dimensions images located at
> toolkit/themes/[osx|windows]/global/icons/
> 
> However, there are nothing for linux. Not sure how our inner mapping
> mechanism works but I can see my question icon become more clear after
> replacing with 32 x 32 png on my retina display.

Gijs, can we go with a 32px favicon in this case?

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css
@@ +47,5 @@
> +/* -------General-------- */
> +/* ====================== */
> +
> +* {
> +  margin: 0;

This causes the bottom of the page to display a white background in large window sizes.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js
@@ +8,5 @@
> +Cu.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const FAVICON_QUESTION = "chrome://global/skin/icons/question-16.png";
> +const FAVICON_PRIVACY = "chrome://browser/skin/Privacy-16.png";

The mockup already specifies a new favicon that we can use for FAVICON_PRIVACY:

http://people.mozilla.org/~bbell/about-page-improvements/images/favicon-privateBrowsing-16.svg

::: browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd
@@ +7,5 @@
>  <!ENTITY privatebrowsingpage.openPrivateWindow.accesskey "P">
>  
> +<!ENTITY privateBrowsing.title                           "Private Browsing">
> +<!ENTITY aboutPrivateBrowsing.tracking                   "with Tracking Protection">
> +<!ENTITY aboutPrivateBrowsing.info.notsaved              "When you browse in a Private Window, Firefox <strong>does not save</strong>:">

Like I said previously, you have to change the key when the value is modified. Just use aboutPrivateBrowsing.info.notsaved2 here.

@@ +12,5 @@
> +<!ENTITY aboutPrivateBrowsing.info.visited               "visited pages">
> +<!ENTITY aboutPrivateBrowsing.info.searches              "searches">
> +<!ENTITY aboutPrivateBrowsing.info.cookies               "cookies">
> +<!ENTITY aboutPrivateBrowsing.info.temporaryFiles        "temporary files">
> +<!ENTITY aboutPrivateBrowsing.info.saved                 "Firefox <strong>will save</strong> your:">

Same here.

@@ +15,5 @@
> +<!ENTITY aboutPrivateBrowsing.info.temporaryFiles        "temporary files">
> +<!ENTITY aboutPrivateBrowsing.info.saved                 "Firefox <strong>will save</strong> your:">
> +<!ENTITY aboutPrivateBrowsing.info.downloads             "downloads">
> +<!ENTITY aboutPrivateBrowsing.info.bookmarks             "bookmarks">
> +<!ENTITY aboutPrivateBrowsing.note1                      "Private Browsing <strong>doesn’t make you anonymous</strong> on the Internet. Your employer or Internet service provider can still know what page you visit.">

Same here, make this .note2.

@@ +16,5 @@
> +<!ENTITY aboutPrivateBrowsing.info.saved                 "Firefox <strong>will save</strong> your:">
> +<!ENTITY aboutPrivateBrowsing.info.downloads             "downloads">
> +<!ENTITY aboutPrivateBrowsing.info.bookmarks             "bookmarks">
> +<!ENTITY aboutPrivateBrowsing.note1                      "Private Browsing <strong>doesn’t make you anonymous</strong> on the Internet. Your employer or Internet service provider can still know what page you visit.">
> +<!ENTITY aboutPrivateBrowsing.learnMore                  "Learn more about">

Ditto: learnMore2

@@ +19,5 @@
> +<!ENTITY aboutPrivateBrowsing.note1                      "Private Browsing <strong>doesn’t make you anonymous</strong> on the Internet. Your employer or Internet service provider can still know what page you visit.">
> +<!ENTITY aboutPrivateBrowsing.learnMore                  "Learn more about">
> +
> +<!ENTITY trackingProtection.title                        "Tracking Protection">
> +<!ENTITY trackingProtection.description1                 "Some websites use trackers that can monitor your activity across the Internet. With Tracking Protection Firefox will block content from known trackers that can collect your personal browsing information.">

Ditto: description2.
Attachment #8739858 - Flags: review?(past)
Attachment #8739858 - Flags: review?(gijskruitbosch+bugs)
Attachment #8739858 - Flags: review-
(In reply to bbell from comment #25)
> I know you're going to need images. Shall I post them on this bug?

I don't think you need to, unless they are different from what you have in the mockup in comment 0.
Comment on attachment 8739858 [details] [diff] [review]
Attachment to Bug 1259340 - New Private Browsing Window Update

Review of attachment 8739858 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd
@@ +8,5 @@
>  
> +<!ENTITY privateBrowsing.title                           "Private Browsing">
> +<!ENTITY aboutPrivateBrowsing.tracking                   "with Tracking Protection">
> +<!ENTITY aboutPrivateBrowsing.info.notsaved              "When you browse in a Private Window, Firefox <strong>does not save</strong>:">
> +<!ENTITY aboutPrivateBrowsing.info.visited               "visited pages">

I'm not sure if changing the key is necessary in the cases where only capitalization changes. Since the layout of the page is changed, I think all locales would need to go over the new strings, but let's ask the experts.

@@ +20,5 @@
> +<!ENTITY aboutPrivateBrowsing.learnMore                  "Learn more about">
> +
> +<!ENTITY trackingProtection.title                        "Tracking Protection">
> +<!ENTITY trackingProtection.description1                 "Some websites use trackers that can monitor your activity across the Internet. With Tracking Protection Firefox will block content from known trackers that can collect your personal browsing information.">
> +<!ENTITY trackingProtection.startTour1                   "See how it works">

Similar question here: changing "this" to "it" seems like just an improvement of the english locale, or should we make sure localizers revisit the string?
Attachment #8739858 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8739858 [details] [diff] [review]
Attachment to Bug 1259340 - New Private Browsing Window Update

Can you:
- address Panos' review comments
- clean up the SVG files you're adding: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines
- fix the diff for browser/components/privatebrowsing/content/aboutPrivateBrowsing.js to not suck? I'm assuming it's broken because you're switching it to either Windows or Unix line-ends. Please do that as a no-op separate commit (or, if it's already unix line-ends, please don't change that). The diff view is unusable right now. Alternatively, submit to mozreview so I don't have to filter out the whitespace changes by comparing the splinter views character-by-character.

and then re-request review? Thanks!
Attachment #8739858 - Flags: review?(gijskruitbosch+bugs)
(In reply to Panos Astithas [:past] from comment #26)
> Comment on attachment 8739858 [details] [diff] [review]
> Attachment to Bug 1259340 - New Private Browsing Window Update
> 
> Review of attachment 8739858 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is much better, thanks. There are still some issues with the strings
> and the styling that will require another round. The CSS seems to be a
> significant rewrite, and I'm not sure how strict I should be in this case,
> so I'll defer to Gijs on that and on the question below.
> 
> (In reply to Ricky Chien [:rickychien] from comment #22)
> > I've addressed almost of problems on review comment but got a question on
> > this one
> > 
> > const FAVICON_QUESTION = "chrome://global/skin/icons/question-32.png";
> > 
> > I found there are other better higher dimensions images located at
> > toolkit/themes/[osx|windows]/global/icons/
> > 
> > However, there are nothing for linux. Not sure how our inner mapping
> > mechanism works but I can see my question icon become more clear after
> > replacing with 32 x 32 png on my retina display.
> 
> Gijs, can we go with a 32px favicon in this case?

I don't really understand this question because your comment on the diff seems to suggest we should be using an SVG file?
Comment on attachment 8739858 [details] [diff] [review]
Attachment to Bug 1259340 - New Private Browsing Window Update

Review of attachment 8739858 [details] [diff] [review]:
-----------------------------------------------------------------

I don't remember the last time I had such a hard time figuring out the diff from Bugzilla :-\

::: browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd
@@ +7,4 @@
>  <!ENTITY privatebrowsingpage.openPrivateWindow.accesskey "P">
>  
> +<!ENTITY privateBrowsing.title                           "Private Browsing">
> +<!ENTITY aboutPrivateBrowsing.tracking                   "with Tracking Protection">

This makes assumptions on the language structure. Having one single string with the entire sentence (maybe a <span> for the second part) would be a lot safer and clearer; if that's technically too complicated, make sure to add a clear comment explaining what's going to happen with those two strings.

@@ +8,5 @@
>  
> +<!ENTITY privateBrowsing.title                           "Private Browsing">
> +<!ENTITY aboutPrivateBrowsing.tracking                   "with Tracking Protection">
> +<!ENTITY aboutPrivateBrowsing.info.notsaved              "When you browse in a Private Window, Firefox <strong>does not save</strong>:">
> +<!ENTITY aboutPrivateBrowsing.info.visited               "visited pages">

Normally a simple capitalization change doesn't require a new ID, but in this case I would consider it safer to change them, since this item ("visited pages", lowercase) would be added to a list of existing elements previously capitalized, and the risk of introducing inconsistencies is pretty high.

Also, given the change of layout, it might be possible to use longer translations than before for existing keys.

@@ +16,5 @@
> +<!ENTITY aboutPrivateBrowsing.info.saved                 "Firefox <strong>will save</strong> your:">
> +<!ENTITY aboutPrivateBrowsing.info.downloads             "downloads">
> +<!ENTITY aboutPrivateBrowsing.info.bookmarks             "bookmarks">
> +<!ENTITY aboutPrivateBrowsing.note1                      "Private Browsing <strong>doesn’t make you anonymous</strong> on the Internet. Your employer or Internet service provider can still know what page you visit.">
> +<!ENTITY aboutPrivateBrowsing.learnMore                  "Learn more about">

String is followed by a link that uses the title for text (privateBrowsing.title): don't do that! Never reuse strings in different contexts just because English looks identical. Other languages might need a different capitalization, or a completely different grammar case.

@@ +20,5 @@
> +<!ENTITY aboutPrivateBrowsing.learnMore                  "Learn more about">
> +
> +<!ENTITY trackingProtection.title                        "Tracking Protection">
> +<!ENTITY trackingProtection.description1                 "Some websites use trackers that can monitor your activity across the Internet. With Tracking Protection Firefox will block content from known trackers that can collect your personal browsing information.">
> +<!ENTITY trackingProtection.startTour1                   "See how it works">

It's OK to keep the existing ID.
Attachment #8739858 - Flags: feedback?(francesco.lodolo) → feedback-
Comment on attachment 8739858 [details] [diff] [review]
Attachment to Bug 1259340 - New Private Browsing Window Update

Review of attachment 8739858 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd
@@ +16,5 @@
> +<!ENTITY aboutPrivateBrowsing.info.saved                 "Firefox <strong>will save</strong> your:">
> +<!ENTITY aboutPrivateBrowsing.info.downloads             "downloads">
> +<!ENTITY aboutPrivateBrowsing.info.bookmarks             "bookmarks">
> +<!ENTITY aboutPrivateBrowsing.note1                      "Private Browsing <strong>doesn’t make you anonymous</strong> on the Internet. Your employer or Internet service provider can still know what page you visit.">
> +<!ENTITY aboutPrivateBrowsing.learnMore                  "Learn more about">

Before I forget: if you can't put the markup inside the full string, you'll need a "after" string, even if it's empty for English.

before+link+after (3 strings)

Imagine if you want to obtain in English
"Learn more about" + "<a>Private Browsing</a>" + "."

In this case "after" would be empty for en-US, it won't for other languages. And explain clearly in a comment how these strings are used.

Welcome to the beauty of localizing with DTDs.
(In reply to :Gijs Kruitbosch from comment #30)
> (In reply to Panos Astithas [:past] from comment #26)
> > (In reply to Ricky Chien [:rickychien] from comment #22)
> > > I've addressed almost of problems on review comment but got a question on
> > > this one
> > > 
> > > const FAVICON_QUESTION = "chrome://global/skin/icons/question-32.png";
> > 
> > Gijs, can we go with a 32px favicon in this case?
> 
> I don't really understand this question because your comment on the diff
> seems to suggest we should be using an SVG file?

The SVG suggestion was about FAVICON_PRIVACY, while this question is about FAVICON_QUESTION, which is used when visiting about:privatebrowsing in a normal window (i.e. not in PBM).
Duplicate of this bug: 1259004
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [fxprivacy]
Sorry for this late reply because I had an operation last week.

I'll setup my mozreview and update my patch as soon as possible.
Flags: needinfo?(rchien)
I noticed that the mockup website disappeared?

http://people.mozilla.org/~bbell/about-page-improvements/images/favicon-privateBrowsing-16.svg
Flags: needinfo?(bbell)
It seems to work for me, so I'm attaching the image for your convenience.
Flags: needinfo?(bbell)
Patch is ready for review but somehow I'm getting stuck in setting mozreview. Need to figure out why it always throw:

abort: error performing cinnabar push: authorization failed
MozReview-Commit-ID: 6uS7E6GhCMc
Attachment #8742653 - Flags: review?(past)
Attachment #8739858 - Attachment is obsolete: true
I'm still getting stuck in mozreview probably it should upload another public key? However, my moz-git-tools works well so I've no idea what's going on in authorization phase.

I can upload my patch to github to let you see pretty diff.
One thought that comes to mind (especially if you haven't used mozreview in the past) is whether you have configured reviewboard-hg.mozilla.org in your ssh config the same way you configured hg.mozilla.org? Also, did you upload your new ssh key to input.mozilla.org? If you can stop by in #fx-team we could try to sort this out.
Attachment #8742653 - Flags: review?(past)
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47451/diff/1-2/
Attachment #8742760 - Attachment description: MozReview Request: Bug 1259340 - New Private Browsing Window Update; r=past → MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch
Attachment #8742760 - Flags: review?(gijskruitbosch+bugs)
Attachment #8742653 - Attachment is obsolete: true
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

https://reviewboard.mozilla.org/r/47451/#review44131

Assuming it is displayed correctly, a 32px favicon could work. Alternatively, perhaps we can copy/reuse https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/help-glyph.svg ?

However, the patch has a number of other issues which I believe ought to be fixed before landing this.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:5
(Diff revision 2)
>  /* 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/. */
>  
>  @import url("chrome://global/skin/in-content/common.css");

If we're restyling the whole thing anyway, can we please move the theming (colors etc.) to something in browser/themes/shared instead and fix bug 1244441 while we're here? It's very strange (not to say 'wrong') to be importing from /skin/ but use it in /content/

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:43
(Diff revision 2)
> +  --color-darkTheme-adaptiveButton-bg: hsla(0, 0%, 100%, .05);
> +  --color-darkTheme-adaptiveButton-bg-hover: hsla(0, 0%, 100%, .1);
> +  --color-darkTheme-adaptiveButton-bg-active: hsla(0, 0%, 100%, .15);
> +  --color-darkTheme-adaptiveButton-border: hsla(0, 0%, 100%, .4);
> +
> +  --border-radius-osx: 4px;

This is unused.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:50
(Diff revision 2)
> +
> +/* ====================== */
> +/* -------General-------- */
> +/* ====================== */
> +
> +* {

Use of the universal selector is pretty evil. Why are we doing this?

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:60
(Diff revision 2)
>    display: flex;
>    flex-direction: column;
>    align-items: center;

Please reuse info-pages.css and get rid of the redundant styles.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:375
(Diff revision 2)
> +input.toggle + label.toggle-btn::before {
> +  float: left;
> +  left: 9px;
> +  visibility: hidden;
> +  border-style: none;
> +  background-image: url("chrome://browser/skin/privatebrowsing/check.svg") !important;

This is another reason this should live in themes: you're relying on browser/themes for the styling to work.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:91
(Diff revision 2)
>  
>  function toggleTrackingProtection() {
>    // Ask chrome to enable tracking protection
>    document.dispatchEvent(
>      new CustomEvent("AboutPrivateBrowsingToggleTrackingProtection",
> -                    {bubbles:true}));
> +                    { bubbles: true }));

Please don't touch blame on these lines where you're not actually changing anything.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:45
(Diff revision 2)
> -          <div>
> -            <p class="list-header">&aboutPrivateBrowsing.info.notsaved;</p>
> -            <ul id="forgotten">
> -              <li>&aboutPrivateBrowsing.info.history;</li>
> +            <span id="titleTracking">&privateBrowsing.title.tracking;</span>
> +          </div>
> +        </section>
> +
> +        <section class="about-message">
> +          <div class="gutter"></div>

These `gutter` divs seem to have no actual function. They do not get a background image or content. Can we eliminate them from the markup and use margin or padding to achieve the same thing?

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:86
(Diff revision 2)
> -        <p>&aboutPrivateBrowsing.note1;</p>
> -        <a id="learnMore" target="_blank">&aboutPrivateBrowsing.learnMore;</a>
> +        </section>
> +      </section>
> +
> +      <section class="section-main">
> +        <section class="about-subheader">
> +          <div id="tpIicon" class="icon"></div>

Likewise, it feels like this is decorative imaging and could be on the content div. If it isn't purely decorative and has semantic value, it should be using an `<img>` tag instead of a `<div>` with a background image. If the background-image moves to the content div, that makes that container likely useless, so we can eliminate a level of nesting - it generally feels like the file as-is has more nesting than necessary.

::: browser/themes/shared/privatebrowsing/check.svg:7
(Diff revision 2)
> +  <style>
> +    .default-style {
> +      fill: #fff;
> +      fill-rule: evenodd;
> +    }
> +  </style>
> +  <g>
> +    <path class="default-style" d="M222.057,9.752L207.9,23.909h0l-4.044,4.045-4.045-4.045h0l-6.068-6.067,4.045-4.045,6.068,6.067L218.012,5.707Z" transform="translate(-192)"/>

You can get rid of the styles and just set the fill attribute on the path. The fill-rule doesn't seem to be used.

Rather than transforming the path with the transform attribute, the path's `d` attribute should be updated so it does not need translating.

::: browser/themes/shared/privatebrowsing/tracking-protection.svg:6
(Diff revision 2)
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- 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/. -->
> +
> +<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32 32" xml:space="preserve">

Here and below, please get rid of `xml:space="preserve"`.
Attachment #8742760 - Flags: review?(gijskruitbosch+bugs)
(also, "subheaders" should likely use semantically correct elements like h1/h2/h3/... rather than divs styled with classes.)
Attachment #8742760 - Flags: review?(past) → review+
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

https://reviewboard.mozilla.org/r/47451/#review44163

r=me with the following changes and after Gijs gives it an r+ as well.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:11
(Diff revision 2)
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  const FAVICON_QUESTION = "chrome://global/skin/icons/question-16.png";

OK, let's use the 32 px image as you suggested, since Gijs agrees.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:49
(Diff revision 2)
> +              <strong>&aboutPrivateBrowsing.info.notsaved.emphasize;</strong>
> +              &aboutPrivateBrowsing.info.notsaved.after;

We want the notsaved.after string to appear immediately after the <strong> tag, in order to avoid the newline being rendered as a whitespace in the page (which looks weird).

::: browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd:30
(Diff revision 2)
> -
> -<!ENTITY aboutPrivateBrowsing.info.saved       "Saved">
> -<!ENTITY aboutPrivateBrowsing.info.downloads   "Downloads">
> -<!ENTITY aboutPrivateBrowsing.info.bookmarks   "Bookmarks">
> -
> -<!ENTITY aboutPrivateBrowsing.note1            "Please note that your employer or Internet service provider can still track the pages you visit.">
> +<!ENTITY aboutPrivateBrowsing.note.after                 " on the Internet. Your employer or Internet service provider can still know what page you visit.">
> +<!ENTITY aboutPrivateBrowsing.learnMore2                 "Learn more about">
> +<!ENTITY aboutPrivateBrowsing.learnMore2.title           "Private Browsing">
> +
> +<!ENTITY trackingProtection.title                        "Tracking Protection">
> +<!ENTITY trackingProtection.description2                 "Some websites use trackers that can monitor your activity across the Internet. With Tracking Protection Firefox will block content from known trackers that can collect your personal browsing information.">

Bryan's mockup has a slightly different second sentence here:

With Tracking Protection Firefox will block many trackers that can collect information about your browsing behavior.

::: browser/themes/linux/jar.mn
(Diff revision 2)
>  * skin/classic/browser/pageInfo.css
>    skin/classic/browser/pageInfo.png
>    skin/classic/browser/page-livemarks.png
>    skin/classic/browser/pointerLock-16.png
>    skin/classic/browser/pointerLock-64.png
> -  skin/classic/browser/Privacy-16.png

There are a couple of places where we still use this file, browser/themes/linux/{browser.css,places/places.css}:

  list-style-image: url("chrome://browser/skin/Privacy-16.png");
  
I believe you can just remove these entries however.

::: browser/themes/windows/jar.mn:245
(Diff revision 2)
>  % override chrome://browser/skin/Info.png                             chrome://browser/skin/Info-XP.png                                 os=WINNT osversion<6
>  % override chrome://browser/skin/livemark-folder.png                  chrome://browser/skin/livemark-folder-XP.png                      os=WINNT osversion<6
>  % override chrome://browser/skin/menu-back.png                        chrome://browser/skin/menu-back-XP.png                            os=WINNT osversion<6
>  % override chrome://browser/skin/menu-forward.png                     chrome://browser/skin/menu-forward-XP.png                         os=WINNT osversion<6
>  % override chrome://browser/skin/pageInfo.png                         chrome://browser/skin/pageInfo-XP.png                             os=WINNT osversion<6
>  % override chrome://browser/skin/Privacy-16.png                       chrome://browser/skin/Privacy-16-XP.png                           os=WINNT osversion<6

If you remove the images, you have to remove this, too.
(In reply to Panos Astithas [:past] from comment #46)
> ::: browser/themes/linux/jar.mn
> (Diff revision 2)
> >  * skin/classic/browser/pageInfo.css
> >    skin/classic/browser/pageInfo.png
> >    skin/classic/browser/page-livemarks.png
> >    skin/classic/browser/pointerLock-16.png
> >    skin/classic/browser/pointerLock-64.png
> > -  skin/classic/browser/Privacy-16.png
> 
> There are a couple of places where we still use this file,
> browser/themes/linux/{browser.css,places/places.css}:
> 
>   list-style-image: url("chrome://browser/skin/Privacy-16.png");
>   
> I believe you can just remove these entries however.

Marco, do you agree? Are these icons being used anywhere (and if so, why only on Linux)?
Flags: needinfo?(mak77)
https://reviewboard.mozilla.org/r/47451/#review44163

> There are a couple of places where we still use this file, browser/themes/linux/{browser.css,places/places.css}:
> 
>   list-style-image: url("chrome://browser/skin/Privacy-16.png");
>   
> I believe you can just remove these entries however.

Do you mean that I can just replace them by Privacy-32.png? or it's ok to remove them and display nothing?
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47451/diff/2-3/
Attachment #8742760 - Attachment description: MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch → MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs
Attachment #8742760 - Flags: review?(gijskruitbosch+bugs)
(In reply to Ricky Chien [:rickychien] from comment #48)
> https://reviewboard.mozilla.org/r/47451/#review44163
> 
> > There are a couple of places where we still use this file, browser/themes/linux/{browser.css,places/places.css}:
> > 
> >   list-style-image: url("chrome://browser/skin/Privacy-16.png");
> >   
> > I believe you can just remove these entries however.
> 
> Do you mean that I can just replace them by Privacy-32.png? or it's ok to
> remove them and display nothing?

I meant that you should remove them entirely, unless Marco knows that they are used somehow.
(In reply to Panos Astithas [:past] from comment #47)
> >   list-style-image: url("chrome://browser/skin/Privacy-16.png");
> >   
> > I believe you can just remove these entries however.
> 
> Marco, do you agree? Are these icons being used anywhere (and if so, why
> only on Linux)?

It is used as an icon in the contextual menu for "Open in a New Private Window"

the reason it's linux only it's because linux is the only OS we support where all the contextual menu entries use to have an icon, so for that contextual menuitem it was reused the private window icon, clearly.
Flags: needinfo?(mak77)
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47451/diff/3-4/
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47451/diff/4-5/
Attachment #8742760 - Attachment description: MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs → MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

https://reviewboard.mozilla.org/r/47451/#review44441

I tried to go through all of it, but there was a lot, so it might take a few more iterations. Note that this is all code-based; I haven't even started to apply the patch and look at the results yet.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:5
(Diff revision 3)
>  /* 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/. */
>  
> -@import url("chrome://global/skin/in-content/common.css");
> +@import url("chrome://global/skin/in-content/info-pages.css");

We should include info-pages from the theme file, not the content file.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:11
(Diff revision 3)
> -  align-items: center;
> -  justify-content: center;
> -  font-weight: 300;
> -}
> +:root {
> +  --color-background-light: #fff;
> +  --color-background-dark: #303033;
> +  --color-background-dark-purple: #1c023c;

None of these variables in the :root block are used in this file, so please remove them as well.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:71
(Diff revision 3)
> -#main {
> +section:not(.section-main) {
> -  padding: 0 2em;
> -  flex: 1;
>    display: flex;
> -  flex-flow: row wrap;
> +  flex-direction: row;
> +  justify-content: flex-start;
>    align-items: center;
> -  justify-content: center;
> +  margin-bottom: 24px;
>  }
>  
> -.sectionHeader {
> -  font-size: 1.75em;
> -}
> +section > .content {
> +  flex-grow: 1;
> +  margin-left: 88px;
> -
> -/* PRIVATE BROWSING SECTION */
> -
> -#privateBrowsingSection {
> -  margin: 1em;
> -  padding: 0 1em;
>  }

As far as I can tell none of these styles except perhaps the margins are necessary.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:90
(Diff revision 3)
>  
>  function toggleTrackingProtection() {
>    // Ask chrome to enable tracking protection
>    document.dispatchEvent(
>      new CustomEvent("AboutPrivateBrowsingToggleTrackingProtection",
> -                    {bubbles:true}));
> +                    { bubbles: true }));

You marked my issue as resolved, but you're still touching blame here.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:97
(Diff revision 3)
>  
>  function dontShowIntroPanelAgain() {
>    // Ask chrome to disable the doorhanger
>    document.dispatchEvent(
>      new CustomEvent("AboutPrivateBrowsingDontShowIntroPanelAgain",
> -                    {bubbles:true}));
> +                    { bubbles: true }));

And here.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:37
(Diff revision 3)
>              class="showNormal"
>              label="&privatebrowsingpage.openPrivateWindow.label;"
>              accesskey="&privatebrowsingpage.openPrivateWindow.accesskey;"/>
> -    <div id="bar" class="showPrivate">&privateBrowsing.title;</div>
> -    <div id="main" class="showPrivate">
> -      <div id="privateBrowsingSection"
> +    <div class="showPrivate about-content-container">
> +      <section class="section-main">
> +        <section class="about-header">

As I noted earlier, this should probably be an `<h1>` as it is the main page title.

If you reuse the classes used in info-pages.css (like "title"), maybe we can avoid more of the custom styling.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:38
(Diff revision 3)
>              label="&privatebrowsingpage.openPrivateWindow.label;"
>              accesskey="&privatebrowsingpage.openPrivateWindow.accesskey;"/>
> -    <div id="bar" class="showPrivate">&privateBrowsing.title;</div>
> -    <div id="main" class="showPrivate">
> -      <div id="privateBrowsingSection"
> -           style="width: &aboutPrivateBrowsing.width1;">
> +    <div class="showPrivate about-content-container">
> +      <section class="section-main">
> +        <section class="about-header">
> +            <img class="icon" />

As far as I can understand, the icon has no meaning that isn't already conveyed in the text next to it (the title / titleTracking). So it should be a background-image on the section header, not an <img> element.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:39
(Diff revision 3)
>              accesskey="&privatebrowsingpage.openPrivateWindow.accesskey;"/>
> -    <div id="bar" class="showPrivate">&privateBrowsing.title;</div>
> -    <div id="main" class="showPrivate">
> -      <div id="privateBrowsingSection"
> -           style="width: &aboutPrivateBrowsing.width1;">
> -        <div class="sectionHeader">&aboutPrivateBrowsing.title;</div>
> +    <div class="showPrivate about-content-container">
> +      <section class="section-main">
> +        <section class="about-header">
> +            <img class="icon" />
> +          <div class="header-content">

This div is not styled, so please get rid of the extra nesting, which doesn't accomplish anything.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:45
(Diff revision 3)
> +        <section class="about-message">
> +          <div class="content">

Both here and below, the `.content` divs are the only children of the `<section>` elements, so please get rid of them and reduce the amount of nesting.

In fact, it seems the `<section class="about-message">` also has no semantic or stylistic use, so we can eliminate it, too.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:52
(Diff revision 3)
> +              <ul>
> +                <li>&aboutPrivateBrowsing.info.visited;</li>
> -              <li>&aboutPrivateBrowsing.info.searches;</li>
> +                <li>&aboutPrivateBrowsing.info.searches;</li>
> +              </ul>
> +              <ul>
> -              <li>&aboutPrivateBrowsing.info.cookies;</li>
> +                <li>&aboutPrivateBrowsing.info.cookies;</li>
> -              <li>&aboutPrivateBrowsing.info.temporaryFiles;</li>
> +                <li>&aboutPrivateBrowsing.info.temporaryFiles;</li>
> -            </ul>
> +              </ul>

Why are these two <ul>s if it's 1 list ? It should be 1 ul, and you can use float properties in CSS to make the items align the way you want.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:67
(Diff revision 3)
> +              <ul>
> -              <li>&aboutPrivateBrowsing.info.bookmarks;</li>
> +                <li>&aboutPrivateBrowsing.info.bookmarks;</li>
> -            </ul>
> +              </ul>
> +              <ul>
> +                <li>&aboutPrivateBrowsing.info.downloads;</li>
> +              </ul>

Same here.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:84
(Diff revision 3)
> -        </div>
> +          </div>
> -        <p>&aboutPrivateBrowsing.note1;</p>
> -        <a id="learnMore" target="_blank">&aboutPrivateBrowsing.learnMore;</a>
> +        </section>
> +      </section>
> +
> +      <section class="section-main">
> +        <section class="about-subheader">

This should be an h2

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:88
(Diff revision 3)
> +      <section class="section-main">
> +        <section class="about-subheader">
> +          <img id="tpIicon" class="icon" />
> +          <div class="slider-toggle-label">&trackingProtection.title;</div>
> +          <input id="tpToggle" class="toggle toggle-input" type="checkbox"/>
> +          <label class="toggle-btn" for="tpToggle"></label>

The label associated with a control should contain useful text. If there is no label, the label element should also not be there.

Separately, if we don't want to display a useful label, we should still set a tooltip (title attribute) on the input control for accessibility reasons that explains what the toggle is for / does.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:91
(Diff revision 3)
> +        <section class="about-message">
> +          <div class="content">

Again, neither of these containers are necessary.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:102
(Diff revision 3)
> -                style="width: &trackingProtection.state.width;"
> -                class="showTpEnabled">&trackingProtection.state.enabled;</span>
> +        <section class="about-info">
> +          <div class="content">

It seems like the about-info class could be added to the parent of this section and you could eliminate both of these levels of nesting, too.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:24
(Diff revision 3)
> +  --color-blue-dark: #0670cc;
> +  --color-blue-darker: #005bab;
> +  --color-blue-darkest: #004480;
> +  --color-blue-focusRing: hsla(208, 100%, 69%, .75);
> +
> +  --color-darkTheme-grey: #303033;

This looks like it should just be `#303030`.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:24
(Diff revision 3)
> +  --color-darkTheme-grey: #303033;
> +  --color-darkTheme-grey-light: #434347;
> +  --color-darkTheme-grey-lighter: #56565c;
> +  --color-darkTheme-blueText: #1a9fff;

None of these are used.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:33
(Diff revision 3)
> +  --color-darkTheme-adaptiveButton-bg-hover: hsla(0, 0%, 100%, .1);
> +  --color-darkTheme-adaptiveButton-bg-active: hsla(0, 0%, 100%, .15);

Neither of these are used.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:46
(Diff revision 3)
> +
> +body {
> +  padding: 40px;
> +  background-color: var(--color-background-light);
> +  font: message-box;
> +  font-size: 16px;

Don't set absolute font-sizes, overriding user preferences in their OS and things like large font OS themes used for accessibility.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:63
(Diff revision 3)
> +}
> +
> +a:hover {
> +  color: var(--color-blue-dark);
> +  text-decoration: underline;
> +  cursor: pointer;

This doesn't seem like it's necessary.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:68
(Diff revision 3)
> +  cursor: pointer;
> +}
> +
> +a:hover:active {
> +  color: var(--color-blue-darker);
> +  outline: none;

This shouldn't be needed

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:73
(Diff revision 3)
> +  outline: none;
> +}
> +
> +a:visited {
> +  color: var(--color-blue-darker);
> +  outline: none;

Ditto

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:76
(Diff revision 3)
> +:focus {
> +  outline: none;
> +  border-color: #fff !important;
> +  box-shadow: 0 0 0 2px var(--color-blue-focusRing);
> +  border-radius: 2px;
> +}

I don't think this is appropriate. info-page.css should already have the relevant styles for in-content links, and we should use the same style here. If we want to change it, we should update it there so that we update it everywhere. This kind of applies to the hover/active/visited styling as well. The only thing we might want to change is the colors, in which case you should make sure that the specificity of these rules is sufficient to do that.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:83
(Diff revision 3)
> +a:link:focus:active {
> +  outline: none;
> +  box-shadow: none;
> +  outline: none;
> +}

This breaks accessibility features, and you duplicated outline. Please just get rid of this rule

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:89
(Diff revision 3)
> +.about-header {
> +  font-size: 2.25em;
> +  font-weight: lighter;
> +  line-height: 1.5em;
> +}

IMO we should reuse the styles from info-pages (probably .title) for this by setting a class in the markup; if that somehow doesn't work, we can revisit.

It's also not clear why you're custom-setting the line-height here.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:95
(Diff revision 3)
> +.about-header .icon {
> +  height: 64px;
> +  background-image: url("chrome://browser/skin/privatebrowsing/private-browsing.svg");

So this background should be on the about-header, which can have a 64px start padding to avoid the icon and the text overlapping. Make sure you position the image appropriately in RTL as well ( you can test with https://addons.mozilla.org/en-GB/firefox/addon/force-rtl/ )

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:103
(Diff revision 3)
> +section.about-subheader {
> +  font-size: 1.5em;
> +  font-weight: lighter;
> +}
> +
> +section.about-subheader + section {
> +  margin-top: -8px;
> +}
> +
> +section.about-subheader {
> +  font-size: 1.5em;
> +  font-weight: lighter;
> +}
> +
> +section.about-subheader + section {
> +  margin-top: -8px;
> +}

This is here twice.

Also, the negative margin-top is confusing. If we use the appropriate elements for the headers, I expect their margins will work out fine as-is, but if not, we should change the margin-bottom of the header, not the margin-top (0 by default) of the next element using a sibling selector.

Finally, if the class is not used by multiple element types (which it isn't here) then please don't add the tag name to the selector (this applies everywhere in this file, as this isn't the only place where you're doing this).

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:151
(Diff revision 3)
> +/* ====================== */
> +/* -------Buttons-------- */
> +/* ====================== */
> +
> +a.button {
> +  outline: 0 !important;

Don't use !important without comments as to why it's necessary. Here, I don't see any reason to do it.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:152
(Diff revision 3)
> +/* -------Buttons-------- */
> +/* ====================== */
> +
> +a.button {
> +  outline: 0 !important;
> +  width: 200px;

This will break l10n in case the text in the button is bigger than you expected. Just set padding and let the size of the text in the "button" do the rest of the work.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:153
(Diff revision 3)
> +/* ====================== */
> +
> +a.button {
> +  outline: 0 !important;
> +  width: 200px;
> +  height: 32px;

Likewise, don't do this...

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:156
(Diff revision 3)
> +  outline: 0 !important;
> +  width: 200px;
> +  height: 32px;
> +  padding: 5px;
> +  background-color: #381e59;
> +  border: 1px #43256a solid;

The canonical order here is width, style, color - please follow it.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:158
(Diff revision 3)
> +  height: 32px;
> +  padding: 5px;
> +  background-color: #381e59;
> +  border: 1px #43256a solid;
> +  font: caption;
> +  font-size: 15px;

Likewise, the explicit font-sizes should be in em instead, if we need them at all. You can then use padding to achieve the desired height.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:160
(Diff revision 3)
> +  background-color: #381e59;
> +  border: 1px #43256a solid;
> +  font: caption;
> +  font-size: 15px;
> +  border-radius: 4px;
> +  text-align: center;

If we get rid of the width, this is no longer necessary.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:161
(Diff revision 3)
> +  border: 1px #43256a solid;
> +  font: caption;
> +  font-size: 15px;
> +  border-radius: 4px;
> +  text-align: center;
> +  text-decoration: none;

Unnecessary, see https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#449

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:162
(Diff revision 3)
> +  font: caption;
> +  font-size: 15px;
> +  border-radius: 4px;
> +  text-align: center;
> +  text-decoration: none;
> +  vertical-align: middle;

And once there's no height set, just padding, this is also not necessary.

In fact, because you set display: block, I'm fairly sure this doesn't actually do anything already anyway:

http://jsbin.com/wukegacoxa/edit?html,css,output

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:167
(Diff revision 3)
> +a.button:hover,
> +a.button:visited {

Do we really want the same style for hover and visited links? That seems surprising...

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:170
(Diff revision 3)
> +}
> +
> +a.button:hover,
> +a.button:visited {
> +  background-color: #2E184A;
> +  border: 1px #3C215E solid;

Again, please use the canonical order of width, style, color.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:174
(Diff revision 3)
> +  background-color: #2E184A;
> +  border: 1px #3C215E solid;
> +  color:  var(--color-grey-light);
> +}
> +
> +button {

The design doesn't specify what this button looks like, but as it is I:

a) don't think this applies, because the button is in XUL namespace but the CSS doesn't use namespaces and the default document namespace is HTML.
b) even if it did apply, almost all of these properties would get overridden by the in-content common css styling for buttons
c) that is actually kind of fine, because I don't see any particular reason to use a different style for the button on this page compared to other in-content pages.

So I would like to get rid of this and all the other styles for `button`, but if we really need them (please explain why) then we should make sure we fix all of the above.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:209
(Diff revision 3)
> +  background-color: var(--color-blue);
> +  border-color: var(--color-blue-dark);
> +  color: #fff;
> +}
> +
> +button.default:hover {

The markup only has 1 <button> element, and I don't see us setting the 'default' class from script or in the markup, so either it is the default or it is not, but it doesn't seem useful to have two different styles when we're only going to use one.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:228
(Diff revision 3)
> +button:first-child {
> +  margin-left: 0;
> +}
> +
> +button:last-child {
> +  margin-right: 0;
> +}

There's only 1 button here, so it seems like you just set both these margin to 4px and are now setting them back to 0. Am I missing something?

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:236
(Diff revision 3)
> +.private button:not(.default) {
> +  background-color: var(--color-darkTheme-adaptiveButton-bg);
> +  border-color: var(--color-darkTheme-adaptiveButton-border);
> +  color: var(--color-darkTheme-purple-text);
> +}

What button is this used for? The "start private browsing" button is invisible when in private mode, right?

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:250
(Diff revision 3)
> +.toggle {
> +  display: none;
> +}
> +
> +.toggle + .toggle-btn {

The toggle button is currently not keyboard-focusable. Please have a look at how in-content checkboxes are currently implemented, and avoid some of the same pitfalls (it'll need opacity: 0, pointer-events: none, etc. - but not display: none).

But really, if we can't have a visible label for this, it seems like we should be using something else to implement this, like a clickable, focusable span with the right aria attributes set so it's accessible for screen-reader users.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:263
(Diff revision 3)
> +  position: relative;
> +  cursor: pointer;
> +  min-width: 60px;
> +  height: 24px;
> +  padding: 0 1px;
> +  border-radius: 16px;

24px / 2 = 12px. Why is there a 16px radius on this?

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:285
(Diff revision 3)
> +  border-style: none;
> +  background-image: url("chrome://browser/skin/privatebrowsing/check.svg") !important;
> +  background-size: 16px;
> +  background-repeat: no-repeat;
> +  background-position: 0px;
> +  background-color: initial;
> +  box-shadow: none;
> +  border: none;

you're resetting borders twice, as well as background-color, background-position, box-shadow - but AFAICT those are not set anywhere else on this pseudoelement, and it isn't inheriting them. So why do they all need setting?

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:286
(Diff revision 3)
> +input.toggle + label.toggle-btn::before {
> +  float: left;
> +  left: 9px;
> +  visibility: hidden;
> +  border-style: none;
> +  background-image: url("chrome://browser/skin/privatebrowsing/check.svg") !important;

This doesn't look like it needs important.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:299
(Diff revision 3)
> +.toggle-input + .toggle-btn {
> +  background-color: var(--color-grey);
> +  border: 1px var(--color-grey) solid;
> +  border-radius: 2em;
> +  padding: 2px;
> +  transition: all .4s ease;
> +}

If we're keeping this all, it should be in the same selector as the one a few rules up.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:307
(Diff revision 3)
> +  border-radius: 2em;
> +  padding: 2px;
> +  transition: all .4s ease;
> +}
> +
> +.toggle-input + .toggle-btn:after {

You're using 3 different selectors for the first element in this sibling selector throughout this file:

.toggle
.toggle-input
input.toggle

Please pick 1.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:310
(Diff revision 3)
> +}
> +
> +.toggle-input + .toggle-btn:after {
> +  border-radius: 50%;
> +  background: white;
> +  transition: all .2s ease;

Don't use transition: all; please list what properties you're expecting to transition. That way we won't get uncomfortable surprises if layout suddenly implements more properties.
Attachment #8742760 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

https://reviewboard.mozilla.org/r/47451/#review44467

::: browser/themes/linux/browser.css
(Diff revision 5)
> -#placesContext_open\:newprivatewindow,
> -#privateBrowsingItem {
> -  list-style-image: url("chrome://browser/skin/Privacy-16.png");
> -}

I discussed this with Marco and even though I don't see these icons on my stock Ubuntu environment, it may be that other linux distros ship with a theme/WM that displays these icons, so let's put these back along with browser/themes/linux/Privacy-16.png.
Attachment #8742760 - Flags: review+
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

https://reviewboard.mozilla.org/r/47451/#review44471

Woops, I didn't mean to clear my r+.
Attachment #8742760 - Flags: review+
https://reviewboard.mozilla.org/r/47451/#review44441

> This should be an h2

I extracted about-subheader from section-main

> The label associated with a control should contain useful text. If there is no label, the label element should also not be there.
> 
> Separately, if we don't want to display a useful label, we should still set a tooltip (title attribute) on the input control for accessibility reasons that explains what the toggle is for / does.

I just grab a customize toggle checkbox from onlnie example to achieve mockup's effect. Most of examples tried the same way to hide input and draw custom style on label.

If there is a better approach, could you point out the example? thanks

> So this background should be on the about-header, which can have a 64px start padding to avoid the icon and the text overlapping. Make sure you position the image appropriately in RTL as well ( you can test with https://addons.mozilla.org/en-GB/firefox/addon/force-rtl/ )

I only saw chrome and some content pages are reversed to rtl by this add-on but it doesn't affect other content pages (new tab, private browsing)

> The design doesn't specify what this button looks like, but as it is I:
> 
> a) don't think this applies, because the button is in XUL namespace but the CSS doesn't use namespaces and the default document namespace is HTML.
> b) even if it did apply, almost all of these properties would get overridden by the in-content common css styling for buttons
> c) that is actually kind of fine, because I don't see any particular reason to use a different style for the button on this page compared to other in-content pages.
> 
> So I would like to get rid of this and all the other styles for `button`, but if we really need them (please explain why) then we should make sure we fix all of the above.

I'll remove them all, it just copyed from bbell's mockup and didn't aware of it.

> The toggle button is currently not keyboard-focusable. Please have a look at how in-content checkboxes are currently implemented, and avoid some of the same pitfalls (it'll need opacity: 0, pointer-events: none, etc. - but not display: none).
> 
> But really, if we can't have a visible label for this, it seems like we should be using something else to implement this, like a clickable, focusable span with the right aria attributes set so it's accessible for screen-reader users.

I don't know how to make this. Does it works if we give an invisible span with the right aria attributes?

> you're resetting borders twice, as well as background-color, background-position, box-shadow - but AFAICT those are not set anywhere else on this pseudoelement, and it isn't inheriting them. So why do they all need setting?

box-shadow is necessary to override common.css

> This doesn't look like it needs important.

It needs actually. There is longer selector:
html|input[type="checkbox"]:checked + html|label::before
in common.css:528 override it.
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47451/diff/5-6/
Attachment #8742760 - Flags: review?(gijskruitbosch+bugs)
Attachment #8742760 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

https://reviewboard.mozilla.org/r/47451/#review44807

Thanks for the quick update. This is already better, but we're not quite there yet.

For the next patch you submit, can you please check carefully for unused CSS variables and duplicate CSS properties, and check if all the points from existing reviews have been addressed?

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:16
(Diff revision 6)
> -@media screen and (min-width: 1200px) and (min-height: 700px) {
> -  body.private {
> +.title-hide {
> +  display: none;
> -    font-size: 1.25em;
> -  }
>  }

Having re-read everything after this rule 5 times, I think you should move all of it into the theming stylesheet, and get rid of the comment-based header in this file (as there'll only be a "General" section, and so there isn't much point....).

You can also add the selector for this rule to the preceding rule.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:33
(Diff revision 6)
> -#main {
> +section:not(.section-main) {
> -  padding: 0 2em;
> -  flex: 1;
>    display: flex;
> -  flex-flow: row wrap;
>    align-items: center;
> -  justify-content: center;
> +  margin-bottom: 24px;
> -}
> -
> -.sectionHeader {
> -  font-size: 1.75em;
>  }

This should become part of the styling of the h2 / about-subheader instead of using :not(.section-main) as there are no other sections this applies to anyway.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:40
(Diff revision 6)
> -.sectionHeader {
> -  font-size: 1.75em;
>  }
>  
> -/* PRIVATE BROWSING SECTION */
> -
> +p {
> +  line-height: 1.5em;

The font size is in the theming stylesheet, so this should probably be there too.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:52
(Diff revision 6)
>    display: flex;
>    flex-direction: row;
> -  justify-content: flex-start;
> +  align-items: stretch;

flex-box is nice, but this is all not necessary - just use:

overflow: auto;

to make the box size with the floated items.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:57
(Diff revision 6)
>    flex-direction: row;
> -  justify-content: flex-start;
> +  align-items: stretch;
> -  align-items: flex-start;
>  }
>  
> -#list-area > div {
> +.list-row:not(:last-child) {

Please unify this rule with the same one for p:not(:last-child), and move that and the negative margin top for .list-row into the theming stylesheet - all the margin narrowing between these things is strictly appearance-related (and really, I think these 3 (or 2, after unification) rules could be removed without the page looking significantly worse, but I recognize that you're trying to follow the mock-up...

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:62
(Diff revision 6)
> -  margin-bottom: 0.4em;
> -  font-weight: bold;
> +  flex: 1 1 auto;
> +  margin-left: 2em;

The first rule here has no effect, the second is overridden by a more specific rule elsewhere, so we can just nuke this block.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:73
(Diff revision 6)
> +  width: 220px;
> +  margin-left: 1em;
>    margin-bottom: 0;
>  }
>  
> -#startTour {
> +.icon {

Nothing has this class anymore, so this rule can go.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:27
(Diff revision 6)
> -    if (prefBranch.getBoolPref("enabled")) {
> -      document.body.setAttribute("globalTpEnabled", "true");
> +    let tpSubHeader = document.getElementById("tpSubHeader");
> +    let tpToggle = document.getElementById("tpToggle");
> +    let title = document.getElementById("title");
> +    let titleTracking = document.getElementById("titleTracking");
> +
> +    if (prefBranch.getBoolPref("pbmode.enabled")) {

Now that I think about it... what happens if you've enabled tracking protection everywhere, and the private browsing mode pref you read here is turned off. Does tracking protection work in private browsing mode or not? If not (which is what the code implies) please can we have a comment for that here. If it does, this code is wrong and needs to check the other preference as well.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:28
(Diff revision 6)
>      <link rel="stylesheet" href="chrome://browser/content/aboutPrivateBrowsing.css" type="text/css" media="all"/>
> +    <link rel="stylesheet" href="chrome://browser/skin/privatebrowsing/aboutPrivateBrowsing.css" type="text/css" media="all"/>
>      <script type="application/javascript;version=1.7" src="chrome://browser/content/aboutPrivateBrowsing.js"></script>
>    </head>
>  
>    <body dir="&locale.dir;" class="private">

You'll likely need to either restart with force rtl enabled, or manually set this attribute to rtl to check for RTL issues. If you need to set it manually, it'd be nice to file a github issue against force-rtl so we can fix it.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:71
(Diff revision 6)
> +      <section id="tpSubHeader" class="about-subheader">
> +        <p class="tpTitle">&trackingProtection.title;</p>
> +        <input id="tpToggle" class="toggle toggle-input" type="checkbox"/>
> +        <label class="toggle-btn" for="tpToggle"></label>
> +      </section>

This `<section>` should still be an h2 instead, I think - it's a subheading, so h2 is appropriate. This was part of my previous feedback. Is there a reason that doesn't work?

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:74
(Diff revision 6)
> +      </section>
> +
> +      <section id="tpSubHeader" class="about-subheader">
> +        <p class="tpTitle">&trackingProtection.title;</p>
> +        <input id="tpToggle" class="toggle toggle-input" type="checkbox"/>
> +        <label class="toggle-btn" for="tpToggle"></label>

Instead of using a label, use an empty `<span>`, without the "for" attribute.

On the input element, set a title or aria-label attribute that says something like "Turn off" or "Turn on" depending on what the state of the element is.

::: browser/themes/linux/places/places.css
(Diff revision 6)
> -#placesContext_open\:newprivatewindow,
> -menuitem[command="placesCmd_open:privatewindow"] {
> -  list-style-image: url("chrome://browser/skin/Privacy-16.png");
> -}

We decided not to remove this image, so you should not remove the styling either.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:13
(Diff revision 6)
> +  --color-grey-lighter: #ebebeb;
> +  --color-grey-light: #d4d4d4;

These are now unused.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:16
(Diff revision 6)
> +  --color-grey-dark: #858585;
> +  --color-grey-darkest: #6A6A6A;
> +  --color-grey-disabled: #666666;

These are also unused.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:44
(Diff revision 6)
> +/* ====================== */
> +
> +body {
> +  padding: 40px;
> +  background-color: var(--color-background-light);
> +  margin: 0;

This is already set in common.css:

https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#55

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:45
(Diff revision 6)
> +
> +body {
> +  padding: 40px;
> +  background-color: var(--color-background-light);
> +  margin: 0;
> +  font-size: 16px;

As I said before, please stop using font sizes in pixels.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:71
(Diff revision 6)
> +
> +a:visited {
> +  color: var(--color-blue-darker);
> +}
> +
> +h1.title {

This can just be ".title", as far as I can tell.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:74
(Diff revision 6)
> +}
> +
> +h1.title {
> +  background-image: url("chrome://browser/skin/privatebrowsing/private-browsing.svg");
> +  background-size: 64px;
> +  font-size: 2.25em;

It seems this is here because otherwise this wraps inside the container with the font size set on the body. This is all unfortunate because we should be using user / OS font-sizes. Once we do that, this is all going to come down.

You're also relying on padding-start of 2.3em from info-pages.css to make this line up with the rest of the content, which has a margin-left of 88px - if we change the font-size, that will break. That means it is going to get out of sync when there are different font-sizes than the one you're testing with (AFAICT: default mac font sizes), so stuff will look misaligned on Linux and/or Windows

It's not really clear to me why the element has 4px left margin.

So here's what I suggest:
- set margin-left to 0
- create a variable for the required amount of spacing to allow for the 64x64px icon plus spacing - make sure it's in px (so based on the current design, 84px);
- set -moz-padding-start on the header using that variable;
- set -moz-margin-start on the content sections using that variable;

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:79
(Diff revision 6)
> +  height: 64px;
> +  display: flex;
> +  align-items: center;

This doesn't work well with variable font-sizing. If we're trying to make sure there's enough room for the background-image, use min-height instead.

Then, get rid of display:flex and align-items:center, which also doesn't work with variable font sizing.

Instead, use background-position: left 0, center 0; or something like this to make sure that text aligns with the image irrespective of its font-size.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:87
(Diff revision 6)
> +}
> +
> +.about-subheader {
> +  font-size: 1.5em;
> +  font-weight: lighter;
> +  height: 32px;

Same issue with the height and background image alignment here.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:91
(Diff revision 6)
> +  font-weight: lighter;
> +  height: 32px;
> +  background-image: url("chrome://browser/skin/privatebrowsing/tracking-protection.svg");
> +  background-repeat: no-repeat;
> +  background-size: 32px;
> +  margin-left: 38px;

So you can set this to -moz-margin-start: calc(var(--icon-margin) - 32px), and you should fix -moz-padding-start to be 52px or whatever so that the text lines up and the icon is far enough away from the text.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:98
(Diff revision 6)
> +.about-subheader .tpTitle {
> +  margin: 0 12px 0 50px;
> +}

With the above note about margins, this rule can just go away.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:119
(Diff revision 6)
> +
> +a.button {
> +  padding: 5px 40px;
> +  background-color: #381e59;
> +  border: 1px solid #43256a;
> +  font: caption;

This uses the title-bar font setting. I'm fairly sure it's not what you want. What do you want?

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:122
(Diff revision 6)
> +  display: block;
> +  text-decoration: none;
> +  display: inline-block;

Please settle on one... block or inline-block?

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:161
(Diff revision 6)
> +
> +input.toggle + label.toggle-btn::before {
> +  float: left;
> +  left: 9px;
> +  visibility: hidden;
> +  background-image: url("chrome://browser/skin/privatebrowsing/check.svg") !important;

If this is necessary, there should be a comment about why it's necessary (which isn't because there is a "longer selector" as you said, but because the  rule in common.css uses !important, too).

But in fact, if we switch the element we use from a `label` to something else, I'm fairly sure we won't need important anymore.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:164
(Diff revision 6)
> +  left: 9px;
> +  visibility: hidden;
> +  background-image: url("chrome://browser/skin/privatebrowsing/check.svg") !important;
> +  background-size: 16px;
> +  background-repeat: no-repeat;
> +  background-color: initial;

Please explicitly set this to transparent instead, if it needs setting after the change away from a label.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:165
(Diff revision 6)
> +  box-shadow: none;
> +  border: none;

Please add a comment above these two properties saying we're overriding the common.css styling - if we need to keep this when switching away from a label.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:169
(Diff revision 6)
> +  background-color: initial;
> +  box-shadow: none;
> +  border: none;
> +}
> +
> +.toggle:checked + .toggle-btn::after {

Nit: settle on whether you want "::" or ":" for after/before. You're switching back and forth here.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:173
(Diff revision 6)
> +
> +.toggle:checked + .toggle-btn::after {
> +  left: 35px;
> +}
> +
> +.toggle + .toggle-btn:after {

Please unify this with the other rule that has exactly the same selector.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:179
(Diff revision 6)
> +  border-radius: 50%;
> +  background: white;
> +  transition: left .2s ease;
> +}
> +
> +.toggle:checked + .toggle-btn {

Move this before the style for this element's pseudo content (so before the ".toggle:checked + .toggle-btn::after" one)

::: browser/themes/shared/privatebrowsing/check.svg:7
(Diff revision 6)
> +<!-- 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/. -->
> +
> +<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32">
> +  <g>

You can omit the <g> and </g> in this file

::: browser/themes/shared/privatebrowsing/tracking-protection-off.svg:6
(Diff revision 6)
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- 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/. -->
> +
> +<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32 32" xml:space="preserve">

This file still has the xml:space="preserve" thing
https://reviewboard.mozilla.org/r/47451/#review44807

> Please unify this rule with the same one for p:not(:last-child), and move that and the negative margin top for .list-row into the theming stylesheet - all the margin narrowing between these things is strictly appearance-related (and really, I think these 3 (or 2, after unification) rules could be removed without the page looking significantly worse, but I recognize that you're trying to follow the mock-up...

Removed.
yeah, I though it will be reviewed by UX so follow the mock-up is necessary. If it's not, it would be easier to style.

> Now that I think about it... what happens if you've enabled tracking protection everywhere, and the private browsing mode pref you read here is turned off. Does tracking protection work in private browsing mode or not? If not (which is what the code implies) please can we have a comment for that here. If it does, this code is wrong and needs to check the other preference as well.

I didn't catch that.

what happens if you've enabled tracking protection everywhere, and the private browsing mode pref you read here is turned off.
^^
What kind of scenario this things would happen? I've verfied that by toggling privacy.trackingprotection.pbmode in about:config (normal mode) and private browsing's checkbox value was synced as expected.

> This doesn't work well with variable font-sizing. If we're trying to make sure there's enough room for the background-image, use min-height instead.
> 
> Then, get rid of display:flex and align-items:center, which also doesn't work with variable font sizing.
> 
> Instead, use background-position: left 0, center 0; or something like this to make sure that text aligns with the image irrespective of its font-size.

background-position has added.

Q: Why flex doesn't work with variable font sizing?
I can see flex works well after changing font-size and text aligns verically as expected.

> Same issue with the height and background image alignment here.

same as above

> So you can set this to -moz-margin-start: calc(var(--icon-margin) - 32px), and you should fix -moz-padding-start to be 52px or whatever so that the text lines up and the icon is far enough away from the text.

I'm still thinking that calc margin by hand it isn't pretty strightforward rather than additional img element and set relative margin-right, but I'll follow your suggestion

> If this is necessary, there should be a comment about why it's necessary (which isn't because there is a "longer selector" as you said, but because the  rule in common.css uses !important, too).
> 
> But in fact, if we switch the element we use from a `label` to something else, I'm fairly sure we won't need important anymore.

I think "label for" binding is the only way to achieve our requirement to customize a toggle effect instead of styling a checkbox. So I decided to leave a comment on it.
After looking through original source code, I finally understood the difference in global tracking protection and private browsing's tracking protection.
I'll follow the original design to disable toggle checkbox when global tracking protection is on.
(In reply to Ricky Chien [:rickychien] from comment #61)
> After looking through original source code, I finally understood the
> difference in global tracking protection and private browsing's tracking
> protection.
> I'll follow the original design to disable toggle checkbox when global
> tracking protection is on.

s/disable/hide/
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47451/diff/6-7/
Attachment #8742760 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

https://reviewboard.mozilla.org/r/47451/#review45131

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:27
(Diff revision 7)
>      if (prefBranch.getBoolPref("enabled")) {
> -      document.body.setAttribute("globalTpEnabled", "true");
> +      tpToggle.classList.add("hide");
>      } else {
> -      document.body.removeAttribute("globalTpEnabled");
> +      tpToggle.classList.remove("hide");
>      }

See below, this can be a classList.toggle() call as well. However, just hiding the checkbox is insufficient - you need to hide the label/span as well.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:33
(Diff revision 7)
> -        prefBranch.getBoolPref("enabled")) {
> -      document.body.setAttribute("tpEnabled", "true");
> +    if (prefBranch.getBoolPref("enabled") ||
> +      prefBranch.getBoolPref("pbmode.enabled")) {
> +      tpToggle.checked = true;
> +      tpToggle.setAttribute("aria-checked", "true")
> +      tpSubHeader.classList.remove("tp-off");
> +      title.classList.add("hide");
> +      titleTracking.classList.remove("hide");
>      } else {
> -      document.body.removeAttribute("tpEnabled");
> +      tpToggle.checked = false;
> +      tpToggle.setAttribute("aria-checked", "false")
> +      tpSubHeader.classList.add("tp-off");
> +      title.classList.remove("hide");
> +      titleTracking.classList.add("hide");
> +    }

You can simplify to:

let trackingEnabled = prefBranch.getBoolPref("enabled") || prefBranch.getBoolPref("pbmode.enabled");

tpToggle.checked = trackingEnabled;
title.classList.toggle("hide", trackingEnabled);
titleTracking.classList.toggle("hide", !trackingEnabled);
tpSubHeader.classList.toggle("tp-off", !trackingEnabled);

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:36
(Diff revision 7)
>      }
> -    if (prefBranch.getBoolPref("pbmode.enabled") ||
> -        prefBranch.getBoolPref("enabled")) {
> -      document.body.setAttribute("tpEnabled", "true");
> +
> +    if (prefBranch.getBoolPref("enabled") ||
> +      prefBranch.getBoolPref("pbmode.enabled")) {
> +      tpToggle.checked = true;
> +      tpToggle.setAttribute("aria-checked", "true")

Because the toggle element is a checkbox, you don't need to keep track of its state separately with aria attributes.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:74
(Diff revision 7)
> +      </section>
> +
> +      <h2 id="tpSubHeader" class="about-subheader">
> +        <p class="tpTitle">&trackingProtection.title;</p>
> +        <input id="tpToggle" class="toggle toggle-input" type="checkbox" role="checkbox"/>
> +        <label class="toggle-btn" for="tpToggle" tabindex="0"></label>

Can you explain why this can't be a `<span>` instead? The label does not do anything special here, as far as I can tell. The only thing you'd need to change otherwise would be to add a click handler that propagates the click to the `<input>`.

Either way you don't need tabindex="0" - there is no keyboard handling on the label, and the checkbox input is focusable and we then use focus styling that shows focus on the control instead of the checkbox. So don't make the label/span focusable.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:36
(Diff revision 7)
> +body.private .showNormal,
> +body.normal .showPrivate,
> +body[tpEnabled] .showTpDisabled,
> +body:not([tpEnabled]) .showTpEnabled {
> +  display: none !important;
> +}

Sorry, I was not very clear. When I said "Having re-read everything after this rule ... I think you should move all of it", I meant everything *after* it. Meaning, this rule and the `.hide` class rule should stay in the content stylesheet.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:67
(Diff revision 7)
> +  width: 780px;
> +}
> +
> +section.section-main {
> +  margin-bottom: 48px;
> +  margin-left: 88px;

This should be -moz-margin-start and should use the icon variable as well - that's why there's margin on here: to make it align with the margin of the .title, which has the same amount of padding, because of the icon.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:74
(Diff revision 7)
> +.hide {
> +  display: none;
> +}

This should also live in the content stylesheet.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:78
(Diff revision 7)
> +.list-row {
> +  display: flex;
> +  flex-direction: row;

You can get rid of the display: flex and flex-direction: row as I noted in the last review...

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:92
(Diff revision 7)
> +  display: flex;
> +  align-items: center;

These properties are unnecessary at this point, as I said in the last review.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:101
(Diff revision 7)
> +  background-position: left, center;
> +  font-weight: lighter;
> +  line-height: 1.5em;
> +  color: var(--color-darkTheme-purple-text);
> +  min-height: 64px;
> +  -moz-margin-start: calc(var(--icon-margin) - 64px);

I think we misunderstood each other. This can just be -moz-margin-start: 0;

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:110
(Diff revision 7)
> +.about-subheader {
> +  display: flex;
> +  align-items: center;
> +  font-size: 1.5em;
> +  font-weight: lighter;
> +  height: 32px;

You left the height here. Use min-height and use a <span> instead of a <p> for the contents of the title, and it looks pretty much the same, as far as I can tell.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:115
(Diff revision 7)
> +  height: 32px;
> +  background-image: url("chrome://browser/skin/privatebrowsing/tracking-protection.svg");
> +  background-repeat: no-repeat;
> +  background-size: 32px;
> +  -moz-margin-start: calc(var(--icon-margin) - 32px);
> +  -moz-padding-start: calc(var(--icon-margin) - 8px);

And this can just be 56px - it is just 32px + the spacing between the icon and the title, for which you don't need a variable (or if anything, you'd want a different variable for the 32px-wide smaller icon, and/or the 24px spacing - don't piggyback on the icon margin because it is unrelated to how much space there should be here - you've already used margin to take the larger icon into account).

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:139
(Diff revision 7)
> +
> +a.button {
> +  padding: 5px 40px;
> +  background-color: #381e59;
> +  border: 1px solid #43256a;
> +  font-size: 1em;

You got rid of the font property, so now you can get rid of the font-size one as well, as font-size: 1em is a no-op.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:146
(Diff revision 7)
> +  text-decoration: none;
> +  display: inline-block;
> +}
> +
> +.toggle + .toggle-btn {
> +  display: inline-block;

This is overridding the `hide` class even when you set it, so the label doesn't end up being hidden.
Attachment #8742760 - Flags: review?(gijskruitbosch+bugs)
https://reviewboard.mozilla.org/r/47451/#review45131

> Can you explain why this can't be a `<span>` instead? The label does not do anything special here, as far as I can tell. The only thing you'd need to change otherwise would be to add a click handler that propagates the click to the `<input>`.
> 
> Either way you don't need tabindex="0" - there is no keyboard handling on the label, and the checkbox input is focusable and we then use focus styling that shows focus on the control instead of the checkbox. So don't make the label/span focusable.

Never mind, I just prefer to don't touch .js to add a click listener by leveraging the id binding of label for.
I've switched to span and get rid of !important rule

> I think we misunderstood each other. This can just be -moz-margin-start: 0;

I know it's same, I do prefer to write this for more readability
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47451/diff/7-8/
Attachment #8742760 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

https://reviewboard.mozilla.org/r/47451/#review45149

Great! With the below addressed, r=me.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:27
(Diff revision 8)
> -    if (prefBranch.getBoolPref("pbmode.enabled") ||
> -        prefBranch.getBoolPref("enabled")) {
> -      document.body.setAttribute("tpEnabled", "true");
> +    let globalTrackingEnabled = prefBranch.getBoolPref("enabled");
> +    let trackingEnabled = prefBranch.getBoolPref("enabled") ||
> +                          prefBranch.getBoolPref("pbmode.enabled");

Nit:

```
let trackingEnabled = globalTrackingEnabled ||
                      prefBranch.getBoolPref("pbmode.enabled");
```

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:73
(Diff revision 8)
> +        </p>
> +      </section>
> +
> +      <h2 id="tpSubHeader" class="about-subheader">
> +        <span class="tpTitle">&trackingProtection.title;</span>
> +        <input id="tpToggle" class="toggle toggle-input" type="checkbox" role="checkbox"/>

Sorry if I missed this last time, but you don't need role="checkbox" either.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:159
(Diff revision 8)
> +  border-radius: 50%;
> +  background: white;
> +  transition: left .2s ease;
> +}
> +
> +input.toggle + span.toggle-btn::before {

Nit: I think you can omit the tag names from this selector rule at this stage.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:180
(Diff revision 8)
> +  left: 35px;
> +}
> +
> +.toggle:checked + .toggle-btn::before {
> +  visibility: visible;
> +}

At the end, can you add:

```
.toggle:-moz-focusring + .toggle-btn {
  outline: 2px solid rgba(0,149,221,0.5);
  outline-offset: 1px;
  -moz-outline-radius: 2px;
}
```


to add focus styling to the toggle button, which is currently missing?
Attachment #8742760 - Flags: review?(gijskruitbosch+bugs) → review+
https://reviewboard.mozilla.org/r/47451/#review45153

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:24
(Diff revision 8)
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
>                                           Ci.nsISupportsWeakReference]),
>    observe: function () {
> -    if (prefBranch.getBoolPref("enabled")) {
> -      document.body.setAttribute("globalTpEnabled", "true");
> -    } else {
> +    let tpSubHeader = document.getElementById("tpSubHeader");
> +    let tpToggle = document.getElementById("tpToggle");
> +    let tpLabel = document.getElementById("tpLabel");

One more thing: This should be "tpButton" - right now, nothing seems to be hiding.

You should also still hide the tpToggle as well, if globalTrackingEnabled is true, otherwise that will stay in the tab order but be invisible...
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47451/diff/8-9/
Gijs,

thanks your patient for this review. I learned a lots.

Final question: Do we need UX review before shipping this patch?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Ricky Chien [:rickychien] from comment #70)
> Gijs,
> 
> thanks your patient for this review. I learned a lots.
> 
> Final question: Do we need UX review before shipping this patch?

No, I think this is a faithful enough rendering of the UX-created spec that we don't need UX review before landing. We normally request UI reviews when engineers create UI that was unspecced, or if they end up deviating significantly from the proposed spec. For this bug, if there are things UX wants changed slightly from this implementation, we can do that in followup bugs.

NB: please also fix this before landing:

(In reply to :Gijs Kruitbosch from comment #68)
> You should also still hide the tpToggle as well, if globalTrackingEnabled is
> true, otherwise that will stay in the tab order but be invisible...
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Ricky Chien [:rickychien] from comment #70)
> Gijs,
> 
> thanks your patient for this review. I learned a lots.

Thanks for doing this, and sticking with it! I'm happy with the result. :-)
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47451/diff/9-10/
comment #68 has beed addressed on previous update but I found there are some test failures. Fixed on latest patch.
Any chance we can use the info-pages.css stylesheet for this? It avoids lots of code duplication.
Yes, we've reused some part of info-pages.css for styling.

Gijs, it my try tests is enough to cover this patch?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f05c86d185c1

There are some failures but I think it's irrelevant. It's ok to ship?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Ricky Chien [:rickychien] from comment #76)
> Yes, we've reused some part of info-pages.css for styling.
> 
> Gijs, it my try tests is enough to cover this patch?
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f05c86d185c1
> 
> There are some failures but I think it's irrelevant. It's ok to ship?

Yes, this looks good to me, feel free to land.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Ricky Chien [:rickychien] from comment #76)
> Yes, we've reused some part of info-pages.css for styling.
What I meant is that it should include info-pages.css instead of copying over the styles. It reduces the amount of duplicated code, and allows about:privatebrowsing to benefit from the responsive styles included in info-pages.css.

Plus, if info-pages.css ever gets updated (because the style guide gets changed), about:privatebrowsing will also align with the changes.
(In reply to Tim Nguyen :ntim (busy, email me instead) from comment #78)
> (In reply to Ricky Chien [:rickychien] from comment #76)
> > Yes, we've reused some part of info-pages.css for styling.
> What I meant is that it should include info-pages.css instead of copying
> over the styles. It reduces the amount of duplicated code, and allows
> about:privatebrowsing to benefit from the responsive styles included in
> info-pages.css.
> 
> Plus, if info-pages.css ever gets updated (because the style guide gets
> changed), about:privatebrowsing will also align with the changes.

File a follow-up for this maybe? I'll make sure Ricky has time to work on it.

That said, I did not look into the code to see how easy it is to get this done. We had been trying to style Gaia UIs from one common style sheet + one special overwrites and it drag us more on regressions than any good on "code reuse". This is the question to be dealt with in another bug (and not, plainly, "let's do it because code reuse is *always* good).
Comment on attachment 8742760 [details]
MozReview Request: Bug 1259340 - New Private Browsing Window Update r=past r=Gijs Kruitbosch

https://reviewboard.mozilla.org/r/47451/#review45387
Attachment #8742760 - Flags: review+
Depends on: 1267047
Panos landed this yesterday, see comment 81.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/463fc6d36ff5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Iteration: --- → 48.3 - Apr 25
Priority: P2 → P1
Blocks: 1216897
Design question/remark: The macsk icon (http://people.mozilla.org/~bbell/about-page-improvements/images/icon-privateBrowsing-64.svg) has a purple background, a darker purple border, and it's on an even darker purple page... Together the effect is that the icon's edge looks rather soft/blurry. This is especially noticeable on a hidpi display, as everything else looks so sharp and crisp with good contrast.

Perhaps it would be better to remove the border (so the icon is a flat circle), or give it a very slightly _brighter_ border ala the "see how it works" button?
Ni bbell to answer above question.
Flags: needinfo?(bbell)
Depends on: 1200587
Depends on: 1269485
Depends on: 1270496
Duplicate of this bug: 1244441
Flags: needinfo?(bbell)
Depends on: 1274272
Depends on: 1278548
See Also: → 1267499
See Also: 1267499
Comment on attachment 8745145 [details]
MozReview Request: Bug 1259340: Adjust hardcoded 'width' in new Private Browsing UI to be a max-width instead, so it fits better in skinny windows. r?rickychien

https://reviewboard.mozilla.org/r/48873/#review57784
Attachment #8745145 - Flags: review+
Depends on: 1320156
You need to log in before you can comment on or make changes to this bug.