463400 - Make about:privatebrowsing useful outside the private browsing mode

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P2)

Description

[Martijn Wargers (dead)]

I had cases (not sure how it happened) where the about:safebrowsing came up after a session restore.
When that happens, the information on that page is confusing, becase it says that private browsing is enabled for you, while it isn't.
It seems to me that the about:privatebrowsing page should at least indicate whether private browsing is enabled or not.

Comment 1

[(no longer active)]

Martijn: this is a serious problem...  Are you sure you never had about:pb open before entering the private mode?  It'd be great if you can try to reproduce this.  CCing Marcia for help here.

Alex: what do you suggest we do here, given that we're past string freeze?  I think it may be possible to disable the about:privatebrowsing page altogether outside the private browsing mode, to avoid this confusion.  Or we can add strings to that page indicating the current status, and mark this as late-l10n.

Comment 2

[Martijn Wargers (dead)]

I have to be honest, I thought I didn't have about:privatebrowsing opened myself, but now I'm not so sure myself.
I'm afraid I don't know how to reproduce.

I was wondering, maybe just visiting about:privatebrowsing should enable private browsing?

Comment 3

[(no longer active)]

(In reply to comment #2)
> I have to be honest, I thought I didn't have about:privatebrowsing opened
> myself, but now I'm not so sure myself.
> I'm afraid I don't know how to reproduce.

OK, maybe Marcia can keep an eye on this to see if she can reproduce?

> I was wondering, maybe just visiting about:privatebrowsing should enable
> private browsing?

Hmm, that would lead to some strange behavior, because of the fact that we close the current session when switching to PB mode.  Suppose you type about:privatebrowsing in the location bar, suddenly the session is closed and a private session is opened.  You leave the private mode, and the original session is restored, which includes about:privatebrowsing, which would close the current session again, and, well you get the idea!  :-)

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

>Alex: what do you suggest we do here, given that we're past string freeze?  I
>think it may be possible to disable the about:privatebrowsing page altogether
>outside the private browsing mode, to avoid this confusion.

This sounds like the best solution, overall this doesn't seem like too serious of a UI problem since anyone who tires directly navigating to about:privatebrowsing probably has enough technical knowledge to understand that the contents of the page may not reflect the present state of the browser.

>I was wondering, maybe just visiting about:privatebrowsing should enable
>private browsing?

I don't think we want other pages to be able to create links that trigger this action, and having the user enter private browsing mode by visiting a bookmark or using their awesome bar is just weird.

Comment 5

[(no longer active)]

(In reply to comment #4)
> >Alex: what do you suggest we do here, given that we're past string freeze?  I
> >think it may be possible to disable the about:privatebrowsing page altogether
> >outside the private browsing mode, to avoid this confusion.
> 
> This sounds like the best solution, overall this doesn't seem like too serious
> of a UI problem since anyone who tires directly navigating to
> about:privatebrowsing probably has enough technical knowledge to understand
> that the contents of the page may not reflect the present state of the browser.

OK, taking over.

Comment 6

[(no longer active)]

Attachment: Patch (v1)

Disable about:privatebrowsing outside of the private mode, with a unit test to make sure the page cannot be retrieved outside of that mode.  This test has the additional benefit of making sure that about:privatebrowsing *is* available inside the private mode.

Comment 7

[(no longer active)]

Attachment: Patch (v2)

OK, based on IRC discussions with mconnor, we decided not to disable about:privatebrowsing in normal mode, and instead show some text which might be useful.

This is what I'm proposing.  Please study the text.  I'll post a screenshot right away as well.

Comment 8

[(no longer active)]

Attachment: Screenshot

Comment 9

[(no longer active)]

Attachment: Patch (v2.1)

This patch makes the CSS rules cleaner, and also styles the button a bit for it to match the "Clear Recent History" button styling.

Regarding calling the API directly instead of using gPrivateBrowsingUI, I don't think that's what we want, given the fact that gPrivateBrowsingUI implements the dialog box which asks the user to confirm closing their session.  Using the API directly here would mean that the behavior of this button and the menu item won't be the same.

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 347040 [details] [diff] [review]
Patch (v2.1)

This can't land for b2.  We'll get it in after b2 freeze though.

Comment 11

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 346969 [details]
Screenshot

A few changes:

-I think we should rephrase "In the private browsing session" to "In a private browsing session."

-I think we should replace the icon with the standard question dialog icon, to make the page clearly visually different.  The title would then change to "Would you like to Start Private Browsing?"

-Remove the sentence "You may want to start the private mode now."

We should get the font size to be the same for the three paragraphs under "minefield is not currently in private browsing mode" for both this and the message displayed while in private browsing mode.

Comment 12

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 346969 [details]
Screenshot

ui-r+ with the changes requested above.

Comment 13

[(no longer active)]

Oh, I thought I posted this here.  I have found a strange problem with this patch on Windows at least: clicking the Start Private Browsing icon on the page will close the main window and exit Firefox if only a single browser window is open.  I have not been able to determine what causes this, but I have prepared a try server build for you to test.

<https://build.mozilla.org/tryserver-builds/2008-11-09_06:[e-mail]-try-68d9820673a/>

Marcia: can you please test this and see if you can reproduce it on all three platforms?  Thanks!

Comment 14

[(no longer active)]

Attachment: Patch (v2.2)

Addressed Alex's concerns in comment 11.

Comment 15

[(no longer active)]

Axel: FYI, this will be a late-l10n change.

Comment 16

[Axel Hecht [:Pike]]

Why do we intend to take this, if I may ask? I assume that the rationale for breaking string freeze was part of the irc discussion, but it should be documented here, too.

Comment 17

[Alex Faaborg [:faaborg] (Firefox UX)]

Ehsan or mconnor will need to provide rationale, I'm personally fine with just blocking access to about:privatebrowsing, but also pretty neutral on the idea.

Comment 18

[(no longer active)]

(In reply to comment #17)
> Ehsan or mconnor will need to provide rationale, I'm personally fine with just
> blocking access to about:privatebrowsing, but also pretty neutral on the idea.

I am also fine with blocking access to about:privatebrowsing, as my first patch did, but mconnor thought that it's not a good idea in our IRC conversation.  I agree that making it work all the time is nice, but I'm not sure if it's worth breaking the string freeze.

Comment 19

[Mike Connor [:mconnor]]

The rationale is that failing/showing a blank page is less friendly if someone ends up trying to go there.  I think we'll take some other changes to this page post-b2 anyway, there's some concerns with the language/approach that need to change.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Please get a ui-review from me before making any post beta2 string changes

Comment 21

[(no longer active)]

Comment on attachment 347635 [details] [diff] [review]
Patch (v2.2)

ui-r requested on the string changes.

Comment 22

[(no longer active)]

I still need some QA help (and some code help as well) on comment 13...

Comment 23

[Martijn Wargers (dead)]

(In reply to comment #22)
> I still need some QA help (and some code help as well) on comment 13...

I tested the windows build of those builds, but I can't reproduce that issue.
Instead, for me, all windows close just fine, but the browser fails to restart. It seems it is restarted for a short time, but then immediately closes.

Comment 24

[(no longer active)]

(In reply to comment #23)
> I tested the windows build of those builds, but I can't reproduce that issue.
> Instead, for me, all windows close just fine, but the browser fails to restart.
> It seems it is restarted for a short time, but then immediately closes.

The browser should not restart (i.e., firefox.exe process must not quit).  Can you please confirm that firefox.exe is quitting?  Also, I have tested this on Windows myself, any chance you can give other platforms a try?  Thanks!

Comment 25

[Martijn Wargers (dead)]

I'm confused. The browser does quit completely on me, using the build you provided, after clicking on the "Start Private Browsing" button on the "about:privatebrowsing" page. That seems like a different result than you get.

Comment 26

[(no longer active)]

(In reply to comment #25)
> I'm confused. The browser does quit completely on me, using the build you
> provided, after clicking on the "Start Private Browsing" button on the
> "about:privatebrowsing" page. That seems like a different result than you get.

No, that is the exact same thing that I'm observing: Firefox quits completely (the firefox.exe process goes away).

Comment 27

[Marcia Knous [:marcia]]

Ehsan: The build in Comment 13 is no longer found. If you can get me a new build I can do some testing for you now that I am back.

Comment 28

[(no longer active)]

Sure, I just pushed another try server build.  Will update here with the link once the build completes.

Comment 29

[(no longer active)]

Builds should appear here: <https://build.mozilla.org/tryserver-builds/2008-12-17_02:[e-mail]-try-9c12301bbf3/>

Comment 30

[Marcia Knous [:marcia]]

I checked the directory several times today, but unfortunately no Windows build every showed in the directory, just linux and mac. Not sure what is going on...

(In reply to comment #29)
> Builds should appear here:
> <https://build.mozilla.org/tryserver-builds/2008-12-17_02:[e-mail]-try-9c12301bbf3/>

Comment 31

[(no longer active)]

Oh, sorry, that was a problem with another patch in my queue.  I pushed this single patch as another try server build here <https://build.mozilla.org/tryserver-builds/2008-12-17_12:[e-mail]-try-432ba337044/> which is available for all platforms.

Comment 32

[(no longer active)]

Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.

Comment 33

[Marcia Knous [:marcia]]

Using the build from Comment 31, I am getting some inconsistent behavior when I have one tab open and I switch to Private Browsing Mode.

1. Have one tab open, a bugzilla bug
2. Tools -> Private Browsing
3. Session closes, and then I am in PB mode.
4. Tools -> Private Browsing again and uncheck the mode.

Expected: I would return to regular browsing mode.
Actual: I am stuck in PB mode.

Subsequent attempts to get back to regular mode seem to fail (Oddly, the reload button seems to take me back to regular mode).  There is definitely something buggy here.  Next I will try to also replicate what Ehsan was seeing in Comment 13.

Comment 34

[Marcia Knous [:marcia]]

I forgot to mention in Comment 33 that I was testing with Windows Vista. Using Vista again, if I have multiple tabs open, I am able to move in/out of PB fine and I don't see the browser closing down using this tryserver build.

Testing on other platforms will have to wait until I am back in MV, but I can continue to test on Windows and see what other anomalies might surface.

Comment 35

[(no longer active)]

Attachment: Patch (v2.3)

Patch applied on top of the one from bug 471627.

This patch solves the problem that I was observing: it lets the browser.js code handle the button command event.  So, this is ready for review.

Beltzner: can you please review it before the string freeze?

Comment 36

[(no longer active)]

I pushed a test build to the try server.  I'll update here with the URL once it's available.

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 354979 [details] [diff] [review]
Patch (v2.3)

Same comments as bug 471627 apply, and with that nit, uir+r+a191 on the string parts from me.

Comment 38

[Nochum Sossonko [:Natch]]

tabtitle.normal and pagetitle.normal are exactly the same text. Also "Start Private Browsing", the "S" in "Start" should probably be lowercase as is the rest of the entity (besides Private Browsing which is a noun, at least in Firefox) ;) Also there's tons of discrepancies in the strings, sometimes "private browsing" is lowercase and sometimes it's uppercase, i.e. "Private Browsing"?

Comment 39

[Nochum Sossonko [:Natch]]

Comment on attachment 354979 [details] [diff] [review]
Patch (v2.3)

> <!ENTITY privatebrowsingpage.tabtitle                  "Private Browsing">
>+<!ENTITY privatebrowsingpage.tabtitle.normal           "Would you like to Start Private Browsing?">
> 
> <!ENTITY privatebrowsingpage.pagetitle                 "Private Browsing">
>+<!ENTITY privatebrowsingpage.pagetitle.normal          "Would you like to Start Private Browsing?">
> 
> <!ENTITY privatebrowsingpage.issueDesc                 "&brandShortName; won't remember any history for this session.">
>+<!ENTITY privatebrowsingpage.issueDesc.normal          "&brandShortName; is not currently in private browsing mode.">
> <!ENTITY privatebrowsingpage.description               "When private browsing, &brandShortName; won't keep any browser history, search history, download history, web form history, cookies, or temporary internet files.  However, files you download and bookmarks you make are preserved.">
>+<!ENTITY privatebrowsingpage.description.normal        "In a private browsing session, no history will be recorded by &brandShortName;.  This includes browser history, search history, download history, web form history, cookies, and temporary internet files.  However, any files you download or bookmarks you create will be preserved.">
> 
> <!-- LOCALIZATION NOTE (privatebrowsingpage.clearRecentHistoryAfter): include a starting space as needed -->
> <!ENTITY privatebrowsingpage.clearRecentHistoryBefore  "You may want to start by also">
> <!ENTITY privatebrowsingpage.clearRecentHistoryInner   "clearing your recent history">
> <!ENTITY privatebrowsingpage.clearRecentHistoryAfter   ".">
> 
>+<!ENTITY privatebrowsingpage.startPrivateBrowsing.label "Start Private Browsing">
>+<!ENTITY privatebrowsingpage.startPrivateBrowsing.accesskey "P">
>+
> <!ENTITY privatebrowsingpage.howToStop                 "To stop private browsing, select &toolsMenu.label; &gt; &privateBrowsingCmd.stop.label;, or close &brandShortName;.">
>+<!ENTITY privatebrowsingpage.howToStart                "To start private browsing, you can also select &toolsMenu.label; &gt; &privateBrowsingCmd.start.label;.">

Specifically, these sentences don't really make sense if "private browsing" isn't a noun, and should probably be "Private Browsing", unless the policy of lower/uppercase has to do with prominence, not grammar. The label is fine as it's all in Camel Case.

Comment 40

[(no longer active)]

I think we should probably use "Private Browsing" everywhere.

Beltzner: what do you think?

Comment 41

[(no longer active)]

(In reply to comment #38)
> tabtitle.normal and pagetitle.normal are exactly the same text.

And BTW, I don't think this is a problem, since a localization may choose to have different translations (for whatever reason) for these two.

Comment 42

[(no longer active)]

Marcia: could you please test this try server build as well?

<https://build.mozilla.org/tryserver-builds/2008-12-31_13:[e-mail]-try-edd846d4a3f/>

Comment 43

[Axel Hecht [:Pike]]

The English strings should be correct before landing. Changing them later, even if just for casing, is going to raise questions on whether we broke string freeze or not.

Comment 44

[(no longer active)]

Attachment: Patch (v2.3 with string fixes)

Hopefully better strings.  Beltzner, can you take another look at this, please?

BTW, if I don't get a code review in time to make it for the string freeze, can I land the new strings alongside the old ones, and then later on delete the old strings when the code is landing?

Comment 45

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #43)
> The English strings should be correct before landing. Changing them later, even
> if just for casing, is going to raise questions on whether we broke string
> freeze or not.

First: we take English grammar correction string changes all the time and simply don't revise the entities. We can't afford to mix our signals: revving an entity means that the meaning of the string has changed. Anything other than that should and must be considered a correctness fix.

Second: this isn't just a capitalization fix. It's changing Private Browsing to be a noun instead of an adjective. I think that's the correct fix, so ultimately I agree that this does need to be something that causes an entity revision and gets signalled to the localizers.

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 355032 [details] [diff] [review]
Patch (v2.3 with string fixes)

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>@@ -2413,6 +2413,11 @@ function BrowserOnCommand(event) {

>+    else if (/^about:privatebrowsing/.test(errorDoc.documentURI)) {
>+      if (ot == errorDoc.getElementById("startPrivateBrowsing")) {
>+        gPrivateBrowsingUI.toggleMode();
>       }
>     }

Are you just doing this here to avoid having to duplicate the gPrivateBrowsingUI.toggleMode code? about:privatebrowsing is privileged, so the choice of this approach is probably worth a comment. (We should probably also just be checking the ID directly rather than using getElementById, but that's a separate issue.)

>diff --git a/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml b/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml

>+      body.normal .showPrivate {

This is kind of a kludge, but I guess I don't have any better ideas offhand.

>+      function onLoad() {
>+        let pb = Cc["@mozilla.org/privatebrowsing;1"].
>+                 getService(Ci.nsIPrivateBrowsingService);
>+        if (!pb.privateBrowsingEnabled) {
>+          let body = document.getElementById("body");

document.body

>diff --git a/browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd b/browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd

> <!ENTITY privatebrowsingpage.tabtitle                  "Private Browsing">
>+<!ENTITY privatebrowsingpage.tabtitle.normal           "Would you like to start Private Browsing?">
> 
> <!ENTITY privatebrowsingpage.pagetitle                 "Private Browsing">
>+<!ENTITY privatebrowsingpage.pagetitle.normal          "Would you like to start Private Browsing?">

Seems like perhaps we should merge these once we aren't string frozen...

>+<!ENTITY privatebrowsingpage.description
>+<!ENTITY privatebrowsingpage.description.normal

Why split these out if they're the same? We can always add an entity later (with the same l10n hit) if they need to be different.

>diff --git a/browser/themes/gnomestripe/browser/aboutPrivateBrowsing.css b/browser/themes/gnomestripe/browser/aboutPrivateBrowsing.css

>+body.normal > #errorPageContainer {
>+  background-image: url("chrome://global/skin/icons/question-48.png");
> }

This image doesn't exist in gnomestripe. I think you want:
moz-icon://stock/gtk-dialog-question?size=dialog
instead (that's what about:sessionRestore uses).

>diff --git a/browser/themes/pinstripe/browser/aboutPrivateBrowsing.css b/browser/themes/pinstripe/browser/aboutPrivateBrowsing.css

>+body.normal > #errorPageContainer {
>+  background-image: url("chrome://global/skin/icons/question-48.png");

Similarly, I think you want
chrome://global/skin/icons/question-64.png
here.

r=me with those addressed.

Comment 47

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 355032 [details] [diff] [review]
Patch (v2.3 with string fixes)

oops, restoring ui-r=beltzner that I accidentally clobbered.

Comment 48

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

I took the liberty of updating this patch in hopes of getting it landed tonight. I addressed my comments, including merging pagetitle and tabtitle entities, but excluding the one about the comment in browser.js, since on second thought that doesn't really seem necessary.

Comment 49

[(no longer active)]

Landed on mozilla-central <http://hg.mozilla.org/mozilla-central/rev/f035bd74e1ca>.  Thanks for the updated patch, Gavin!

Comment 50

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-1.9.1/rev/26e839f160ad

Comment 51

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=7444 added to Litmus.

Verified fixed on 1.9.1 using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090105 Shiretoko/3.1b3pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090105 Shiretoko/3.1b3pre

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090105 Minefield/3.2a1pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090105 Minefield/3.2a1pre

Comment 52

[(no longer active)]

Marcia: could you please modify this test to make sure that the Start Private Browsing button on this page also works?

Comment 53

[Marcia Knous [:marcia]]

Ehsan: Just to clarify, you want a similar test case that tests about:privatebrowsing in PB mode?

(In reply to comment #52)
> Marcia: could you please modify this test to make sure that the Start Private
> Browsing button on this page also works?

Comment 54

[(no longer active)]

(In reply to comment #53)
> Ehsan: Just to clarify, you want a similar test case that tests
> about:privatebrowsing in PB mode?

Oh, that would also be great!  :-)

But what I was referring to was a test case which makes sure the Start Private Browsing button in about:privatebrowsing (outside the PB mode) works as intended.

Comment 55

[Henrik Skupin [:whimboo][⌚️UTC+1]]

-Revert in-litmus flag until we have everything in-place.

Comment 56

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=7463 and https://litmus.mozilla.org/show_test.cgi?id=7464 added.

Comment 57

[(no longer active)]

This patch actually did include an automated test.

Comment 58

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marcia, why is the following Litmus test disabled or better not assigned to any group? Is there a reason for?

https://litmus.mozilla.org/show_test.cgi?id=7464