1190427 - Update the design of about:privatebrowsing

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P1)

Description

[:Paolo Amadini]

This bug tracks the completion of the work already started last week as part of the test in bug 1188455, to implement a new design for the about:privatebrowsing page that includes a section on Tracking Protection.

Comment 1

[:Paolo Amadini]

This is the updated design we're implementing:

http://cl.ly/image/241a013L2Z3e

And an updated mockup is here:

https://invis.io/2K3PADF8N

There is one specific change from the previous design that I'd like to call attention to, that is instead of having a "Privacy Preferences" link we have a "Turn off Tracking Protection" link. That's much better for users as we don't open the Preferences tab, since there is confusion when it's open in a private window as to why it says "Firefox will: Remember history". On the other hand the preference change is stored to disk when clicking the link, it does not only affect the current session. Is this an acceptable trade-off?

Comment 2

[:Paolo Amadini]

Aislinn, the mockup says that we're showing "See how this works" both when the feature is enabled and disabled. However, if I remember correctly the tour only works when the feature is enabled. Are you fine with removing the button on the disabled version?

Separate issue, can we have the link to turn on Tracking Protection again not be inline with the sentence, but on a separate line? Having it inline causes issues with localization.

Comment 3

[Ash Grigas]

(In reply to :Paolo Amadini from comment #2)
> Aislinn, the mockup says that we're showing "See how this works" both when
> the feature is enabled and disabled. However, if I remember correctly the
> tour only works when the feature is enabled. Are you fine with removing the
> button on the disabled version?
> 
> Separate issue, can we have the link to turn on Tracking Protection again
> not be inline with the sentence, but on a separate line? Having it inline
> causes issues with localization.

Yes, please remove.

Comment 4

[Ash Grigas]

As to the turn on link, lets put it where the button was centered. I'll get an updated mock for you from Brian.

Comment 5

[(no longer active)]

(In reply to :Paolo Amadini from comment #1)
> This is the updated design we're implementing:
> 
> http://cl.ly/image/241a013L2Z3e

Looks nice!  :-)

> And an updated mockup is here:
> 
> https://invis.io/2K3PADF8N
> 
> There is one specific change from the previous design that I'd like to call
> attention to, that is instead of having a "Privacy Preferences" link we have
> a "Turn off Tracking Protection" link. That's much better for users as we
> don't open the Preferences tab, since there is confusion when it's open in a
> private window as to why it says "Firefox will: Remember history". On the
> other hand the preference change is stored to disk when clicking the link,
> it does not only affect the current session. Is this an acceptable trade-off?

What does that preference capture?  Is there any other UI that can change the same preference?

If this is a preference that can only be changed from a private window, storing it on the disk will reveal whether the user has been using private browsing, which violates their privacy.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #5)
> What does that preference capture?  Is there any other UI that can change
> the same preference?
> 
> If this is a preference that can only be changed from a private window,
> storing it on the disk will reveal whether the user has been using private
> browsing, which violates their privacy.

I believe it is the exact same as unticking the checkbox in about:preferences#privacy, which flips the privacy.trackingprotection.pbmode.enabled pref.  This checkbox can be changed from the preferences page in normal mode as well.  Leaving the ni? for Paolo to confirm.

Comment 7

[:Paolo Amadini]

What Brian said.

Comment 8

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1190427 - Update the design of about:privatebrowsing. r=ttaubert

Bug 1190427 - Update the design of about:privatebrowsing. r=ttaubert

Comment 9

[:Paolo Amadini]

This replaces the existing about:privatebrowsing design completely. I reduced the version displayed in non-private windows to the bare essentials, reusing just two strings we already had for that case, as I don't think it will be often discovered.

For the moment there is some self-contained code that disables the Tracking Protection section, designed to be easily removed in one go in bug 1175606.

Tryserver build:

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

Comment 10

[:Paolo Amadini]

Attachment: Screenshot of normal window

Comment 11

[:Paolo Amadini]

Attachment: Screenshot when Tracking Protection disabled

Comment 12

[:Paolo Amadini]

Attachment: Screenshot of maximized window, used for larger screen sizes

Comment 13

[:Paolo Amadini]

Attachment: Screenshot of new design without the Tracking Protection section

Comment 14

[:Paolo Amadini]

Aislinn, you asked for Helvetica Light for the headings, but I'm not sure what font this maps to exactly. Can you please attach an HTML file (as opposed to a screenshot) that includes the CSS I might use to obtain the desired typeface on Mac OS X?

Comment 15

[Ash Grigas]

(In reply to :Paolo Amadini from comment #14)
> Aislinn, you asked for Helvetica Light for the headings, but I'm not sure
> what font this maps to exactly. Can you please attach an HTML file (as
> opposed to a screenshot) that includes the CSS I might use to obtain the
> desired typeface on Mac OS X?

can you use this?
body {
   font-family: "HelveticaNeue-Light", "Helvetica Neue Light", "Helvetica Neue", Helvetica, Arial, "Lucida Grande", sans-serif; 
   font-weight: 300;
}

Comment 16

[:Paolo Amadini]

(In reply to agrigas from comment #15)
> body {
>    font-family: "HelveticaNeue-Light", "Helvetica Neue Light", "Helvetica
> Neue", Helvetica, Arial, "Lucida Grande", sans-serif; 
>    font-weight: 300;
> }

For me, that renders exactly like in the screenshot.

Comment 17

[Ash Grigas]

(In reply to :Paolo Amadini from comment #16)
> (In reply to agrigas from comment #15)
> > body {
> >    font-family: "HelveticaNeue-Light", "Helvetica Neue Light", "Helvetica
> > Neue", Helvetica, Arial, "Lucida Grande", sans-serif; 
> >    font-weight: 300;
> > }
> 
> For me, that renders exactly like in the screenshot.

ok well maybe you don't have that font installed on your machine?

Comment 18

[:Paolo Amadini]

Quite possible, so the question is whether we want to use fonts that may not be present on user machines?

Comment 19

[Ash Grigas]

(In reply to :Paolo Amadini from comment #18)
> Quite possible, so the question is whether we want to use fonts that may not
> be present on user machines?

It will default to helvetica regular in which case that is fine. When user is on a mac - it should show up, when not, it will default to something sufficient.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

https://reviewboard.mozilla.org/r/15113/#review13653

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:36
(Diff revision 1)
> -    // Private browsing window, classic version.
> -    document.getElementById("learnMore").setAttribute("href", learnMoreURL);
> +    document.getElementById("favicon")
> +            .setAttribute("href", "chrome://global/skin/icons/question-16.png");

Maybe define this as a constant, e.g.

const FAVICON_QUESTION = "...png";

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:44
(Diff revision 1)
> +  document.getElementById("favicon")
> +          .setAttribute("href", "chrome://browser/skin/Privacy-16.png");

Should use a constant here too.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:69
(Diff revision 1)
> +    document.getElementById("trackingProtectionSection")
> +            .setAttribute("style", "display: none;");

.setAttribute("hidden", "true");

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:34
(Diff revision 1)
> -      <div class="showPrivate">
> +    <div id="bar" class="showPrivate">&privateWindow.title;</div>

Couldn't we use a <h1> for this here?

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:38
(Diff revision 1)
> +        <div class="sectionHeader">&aboutPrivateBrowsing.title;</div>

<h2>?

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:63
(Diff revision 1)
> -      <p class="showTpEnabled"><a id="startTour">&trackingProtection.startTour;</a></p>
> +        <div class="sectionHeader">&trackingProtection.title;

<h2>?

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:83
(Diff revision 1)
> -              accesskey="&trackingProtection.enable.accesskey;"/>
> -    </div>
> +        <p id="tpStartTour"
> +           class="showTpEnabled"><a id="startTour">&trackingProtection.startTour;</a></p>

What's the a11y story here? Looks like I cant' focus it using the keyboard.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8643697 [details]
MozReview Request: Bug 1190427 - Update the design of about:privatebrowsing. r=ttaubert

Thanks RB.

Comment 22

[:Paolo Amadini]

(In reply to Tim Taubert [:ttaubert] from comment #20)
> Couldn't we use a <h1> for this here?

I tried that but I then was inheriting the common in-content styles like margins, colors and font sizes that I then had to reset in the CSS to match the result we wanted. It turned out it was much cleaner to use a generic block element containing the text (which is still perfectly valid HTML) than using the h1 and h2 tags.

> What's the a11y story here? Looks like I can't focus it using the keyboard.

Hm, it can be focused but there is no visual feedback. Will try to fix.

Comment 23

[:Paolo Amadini]

For the font issue, this clarified things a bit:

https://en.wikipedia.org/wiki/List_of_typefaces_included_with_OS_X

So, Helvetica does not have the Light variant while Helvetica Neue does. Using a font-family of "Helvetica Neue" and a font-weight of 300 makes the result very similar to the mockup.

Comment 24

[:Paolo Amadini]

Comment on attachment 8643697 [details]
MozReview Request: Bug 1190427 - Update the design of about:privatebrowsing. r=ttaubert

Bug 1190427 - Update the design of about:privatebrowsing. r=ttaubert

Comment 25

[Ash Grigas]

(In reply to :Paolo Amadini from comment #23)
> For the font issue, this clarified things a bit:
> 
> https://en.wikipedia.org/wiki/List_of_typefaces_included_with_OS_X
> 
> So, Helvetica does not have the Light variant while Helvetica Neue does.
> Using a font-family of "Helvetica Neue" and a font-weight of 300 makes the
> result very similar to the mockup.

Helvetica Neue is what was inteneded. So that works!

Comment 26

[:Paolo Amadini]

Attachment: Screenshot of normal window with fonts updated

Here is the screenshot for the latest version of the patch. This should address all the nits, let me know if there is anything else.

Comment 27

[Ash Grigas]

NI'ing Bryan so he can verify it looks ok since its his design...

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Paolo Amadini from comment #22)
> (In reply to Tim Taubert [:ttaubert] from comment #20)
> > Couldn't we use a <h1> for this here?
> 
> I tried that but I then was inheriting the common in-content styles like
> margins, colors and font sizes that I then had to reset in the CSS to match
> the result we wanted. It turned out it was much cleaner to use a generic
> block element containing the text (which is still perfectly valid HTML) than
> using the h1 and h2 tags.

Oh yeah no doubt it's valid HTML, semantically nicer (esp. for screen readers) would of course be to use <h1>/<h2>.

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8643697 [details]
MozReview Request: Bug 1190427 - Update the design of about:privatebrowsing. r=ttaubert

https://reviewboard.mozilla.org/r/15115/#review13723

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:140
(Diff revision 2)
> -#tourFooter {
> -  margin: 16px auto;
> +  margin-left: auto;
> +  margin-right: auto;

Maybe margin: 0 auto ?

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

NOTE: We could either incorporate the strings from bug 1192088 before landing or do it there as a follow-up.

Comment 31

[:Paolo Amadini]

(In reply to Tim Taubert [:ttaubert] from comment #29)
> ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.css:140
> > +  margin-left: auto;
> > +  margin-right: auto;
> 
> Maybe margin: 0 auto ?

Here I have to keep the default vertical margin of the P element, which is a browser-level value technically unknown to us (although it's probably 1.12em according to <http://www.w3.org/TR/CSS21/sample.html>).

Comment 32

[:Paolo Amadini]

(In reply to Tim Taubert [:ttaubert] from comment #30)
> NOTE: We could either incorporate the strings from bug 1192088 before
> landing or do it there as a follow-up.

Let's land what we have and handle follow-ups there.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d6ff8fa48095

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d6ff8fa48095

Comment 35

[Tim Nguyen :ntim]

Why is this hardcoding fonts ? The common.css stylesheet already provides the right font per platform (Helvetica Neue on OSX, Segoe UI on Windows, Ubuntu on Linux, ...) using the message-box shorthand.

Also, there's the info-pages.css stylesheet which can be used for an error-page like layout.

Comment 36

[:Paolo Amadini]

(In reply to Tim Nguyen [:ntim] (mostly away until 26 August) from comment #35)
> Why is this hardcoding fonts ? The common.css stylesheet already provides
> the right font per platform (Helvetica Neue on OSX, Segoe UI on Windows,
> Ubuntu on Linux, ...) using the message-box shorthand.

Apparently message-box maps to Lucida Grande UI on OS X, while the designers asked for Helvetica Neue. I don't know if the current fallbacks for other platforms have been explicitly requested. The font specification in the comments above looks similar to others I found in the tree, so I guess this might be the desired result, although the designers may better answer this question.

> Also, there's the info-pages.css stylesheet which can be used for an
> error-page like layout.

Thanks for the pointer, I didn't know this existed, can be useful for the future. I've taken a look and I think the layout we want here is distinct enough that we should just use the in-content style sheet as a starting point.

Comment 37

[Tim Nguyen :ntim]

(In reply to :Paolo Amadini from comment #36)
> (In reply to Tim Nguyen [:ntim] (mostly away until 26 August) from comment
> #35)
> > Why is this hardcoding fonts ? The common.css stylesheet already provides
> > the right font per platform (Helvetica Neue on OSX, Segoe UI on Windows,
> > Ubuntu on Linux, ...) using the message-box shorthand.
> 
> Apparently message-box maps to Lucida Grande UI on OS X, while the designers
> asked for Helvetica Neue. I don't know if the current fallbacks for other
> platforms have been explicitly requested. The font specification in the
> comments above looks similar to others I found in the tree, so I guess this
> might be the desired result, although the designers may better answer this
> question.
Lucida Grande is used for versions older than Yosemite, Yosemite uses Helvetica Neue (as message-box font). My guess is that designers aimed to be consistent with the current UI.

Comment 38

[Stefan Plewako [:stef]]

Why "Turn Tracking Protection On/Off" is presented as link and "See how this works" is presented as button? I'm asking because this looks quite jarring in Polish.

Comment 39

[:Paolo Amadini]

(In reply to Stefan Plewako [:stef] from comment #38)
> Why "Turn Tracking Protection On/Off" is presented as link and "See how this
> works" is presented as button?

To give different emphasis to the two controls.

> I'm asking because this looks quite jarring in Polish.

What is exactly the difference from the English version and the Polish version?

Comment 40

[Stefan Plewako [:stef]]

(In reply to :Paolo Amadini from comment #39)
> > I'm asking because this looks quite jarring in Polish.
> 
> What is exactly the difference from the English version and the Polish
> version?

I don't know if Polish makes big difference here, roles seem reverted - buttons usually execute commands and hyperlinks reference documents.

Comment 41

[:Paolo Amadini]

Yeah, we're giving more weight to the visual hierarchy here (think of the de-emphasized "Cancel" links in some modern web UIs, which are not links but actual JavaScript commands). For technical users, we're making the action explicit by displaying the URL on hover for the button and nothing for the Turn On / Turn Off link.

Comment 42

[Ash Grigas]

Not sure why I'm NI'd?

Comment 43

[Tim Nguyen :ntim]

(In reply to agrigas from comment #42)
> Not sure why I'm NI'd?

For comment 36.

Comment 44

[Ash Grigas]

(In reply to :Paolo Amadini from comment #36)
> (In reply to Tim Nguyen [:ntim] (mostly away until 26 August) from comment
> #35)
> > Why is this hardcoding fonts ? The common.css stylesheet already provides
> > the right font per platform (Helvetica Neue on OSX, Segoe UI on Windows,
> > Ubuntu on Linux, ...) using the message-box shorthand.
> 
> Apparently message-box maps to Lucida Grande UI on OS X, while the designers
> asked for Helvetica Neue. I don't know if the current fallbacks for other
> platforms have been explicitly requested. The font specification in the
> comments above looks similar to others I found in the tree, so I guess this
> might be the desired result, although the designers may better answer this
> question.
> 
> > Also, there's the info-pages.css stylesheet which can be used for an
> > error-page like layout.
> 
> Thanks for the pointer, I didn't know this existed, can be useful for the
> future. I've taken a look and I think the layout we want here is distinct
> enough that we should just use the in-content style sheet as a starting
> point.

Paolo is right - we should adapt what we need to so that on Mac platforms using the latest OX QA can verify Helvetica Neue is being displayed.

Comment 45

[Tim Nguyen :ntim]

(In reply to agrigas from comment #44)
> Paolo is right - we should adapt what we need to so that on Mac platforms
> using the latest OX QA can verify Helvetica Neue is being displayed.

What about other platforms (Windows and Linux) ? On Windows, the page is the only page to use Arial, while the other pages use Segoe UI. On Linux, the same issue is happening, except other pages use Ubuntu instead of Segoe UI.

Also, Helvetica Neue is only native on Yosemite. It looks out of place on both Mavericks (uses Lucida Grande) and El Capitan (uses San Francisco).

Are you OK with using message-box ? It uses the native font per-platform, and is consistent with what other pages (in-content prefs, add-on manager, error-pages) use now. That includes using Helvetica Neue on OSX Yosemite.

Comment 46

[Ash Grigas]

(In reply to Tim Nguyen [:ntim] (mostly away until 26 August) from comment #45)
> (In reply to agrigas from comment #44)
> > Paolo is right - we should adapt what we need to so that on Mac platforms
> > using the latest OX QA can verify Helvetica Neue is being displayed.
> 
> What about other platforms (Windows and Linux) ? On Windows, the page is the
> only page to use Arial, while the other pages use Segoe UI. On Linux, the
> same issue is happening, except other pages use Ubuntu instead of Segoe UI.
> 
> Also, Helvetica Neue is only native on Yosemite. It looks out of place on
> both Mavericks (uses Lucida Grande) and El Capitan (uses San Francisco).
> 
> Are you OK with using message-box ? It uses the native font per-platform,
> and is consistent with what other pages (in-content prefs, add-on manager,
> error-pages) use now. That includes using Helvetica Neue on OSX Yosemite.

I am ok with using the current default font for each version of OS for the platforms we support. For Mac, we should surface the specific fonts for Yosemite, Mavericks and El Capitain. Same for MS, we should reflect the default for each version. If the message-box does this - thats fine. I thought Paolo said it didn't handle the Mac default correctly.

Comment 47

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 42.0a2 (2015-09-10) Win 7, Ubuntu 12.04, OS X 10.10.