593126 - Reword text in the private browsing mode content area page based on the menu structure

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

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

The current private browsing mode content area page (about:privatebrowsing) contains the text:

(ofF)
To start Private Browsing, you can also select Tools > Start Private Browsing.

(on)
To stop Private Browsing, go to Tools > Stop Private Browsing.

If the Firefox button is active, we need to replace "Tools" with the brand shortname. (in our case Firefox > Stop Private Browsing)

Comment 1

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

Should be a simple change but requesting blocking since the current text can confuse the user.

Comment 2

[(no longer active)]

Dao, do you know how one can determine whether the Firefox button is active or not?

Comment 3

[Dão Gottwald [:dao]]

I would suggest looking at toolbar-menubar's autohide attribute.

I'm not sure it would be labeled "Firefox" as a toolbar button on Linux (bug 585370).

Comment 4

[Axel Hecht [:Pike]]

This has neither a patch, nor a blocking decision but strings impact.

What's the triage or ETA status here?

Comment 5

[(no longer active)]

I'll post a patch shortly.

Comment 6

[(no longer active)]

Attachment: Patch (v1)

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 474165 [details] [diff] [review]
Patch (v1)

This still needs to look at whether the menu bar is hidden and display the right string accordingly.

Comment 8

[(no longer active)]

(In reply to comment #7)
> Comment on attachment 474165 [details] [diff] [review]
> Patch (v1)
> 
> This still needs to look at whether the menu bar is hidden and display the
> right string accordingly.

Ah, right.  Sorry, forgot about that.  New patch upcoming.

Comment 9

[(no longer active)]

Attachment: Patch (v2)

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 474181 [details] [diff] [review]
Patch (v2)

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

>+      // Show the correct menu structure based on whether the App Menu button is
>+      // shown or not.
>+      var appMenuButton = mainWindow.document.getElementById("appmenu-button-container");
>+      var appMenuButtonIsVisible = appMenuButton && !appMenuButton.hidden;
>+      document.getElementById("appMenuButton").style.display = appMenuButtonIsVisible ? "" : "none";
>+      document.getElementById("toolsMenu").style.display = appMenuButtonIsVisible ? "none" : "";

Use the menu bar's autohide attribute instead of appMenuButton.hidden.

Also, this looks like it should be in the DOMContentLoaded handler...

Comment 11

[(no longer active)]

(In reply to comment #10)
> Comment on attachment 474181 [details] [diff] [review]
> Patch (v2)
> 
> >--- a/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
> >+++ b/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
> 
> >+      // Show the correct menu structure based on whether the App Menu button is
> >+      // shown or not.
> >+      var appMenuButton = mainWindow.document.getElementById("appmenu-button-container");
> >+      var appMenuButtonIsVisible = appMenuButton && !appMenuButton.hidden;
> >+      document.getElementById("appMenuButton").style.display = appMenuButtonIsVisible ? "" : "none";
> >+      document.getElementById("toolsMenu").style.display = appMenuButtonIsVisible ? "none" : "";
> 
> Use the menu bar's autohide attribute instead of appMenuButton.hidden.

But the visibility of the app button depends on more than that: <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4793>.  I switched to using titlebar's visibility because that would work correctly if for example someone chooses to display the menu bar all the time on Windows.

> Also, this looks like it should be in the DOMContentLoaded handler...

Yes.  New patch upcoming...

Comment 12

[(no longer active)]

Attachment: Patch (v3)

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #11)
> But the visibility of the app button depends on more than that:
> <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4793>.

But nothing that should matter here.

>  I switched to using titlebar's visibility because that would work correctly if
> for example someone chooses to display the menu bar all the time on Windows.

Not sure what this means. If someone chooses to display the menu bar all the time, the autohide attribute would reflect that.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 474220 [details] [diff] [review]
Patch (v3)

See previous comment. #titlebar is actually not going to exist on Linux.

Also use classList.add instead of className += ...

Comment 15

[(no longer active)]

Attachment: Patch (v4)

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 474237 [details] [diff] [review]
Patch (v4)

>--- a/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
>+++ b/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
>@@ -45,6 +45,13 @@
>   %globalDTD;
>   <!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd">
>   %browserDTD;
>+#ifdef XP_WIN
>+  <!ENTITY basePBMenu.label   "<span class='appMenuButton'>&brandShortName;</span><span class='toolsMenu'>&toolsMenu.label;</span>">
>+#elif defined(XP_MACOSX)
>+  <!ENTITY basePBMenu.label   "<span class='appMenuButton'></span><span class='toolsMenu'>&toolsMenu.label;</span>">

The empty <span class='appMenuButton'></span> isn't of any use, as far as I can see. You could in fact remove <span class='toolsMenu'> as well and don't let the below changes apply on OS X.

>       body.private .showNormal {
>         display: none;
>       }
>+      body.appMenuButtonVisible .toolsMenu {
>+        display: none;
>+      }
>+      body.appMenuButtonInvisible .appMenuButton {
>+        display: none;
>+      }
>     ]]></style>
>     <script type="application/javascript;version=1.7"><![CDATA[
>       const Cc = Components.classes;
>@@ -113,6 +126,13 @@
>         let moreInfoLink = document.getElementById("moreInfoLink");
>         if (moreInfoLink)
>           moreInfoLink.setAttribute("href", moreInfoURL + "private-browsing");
>+
>+        // Show the correct menu structure based on whether the App Menu button is
>+        // shown or not.
>+        var menuBar = mainWindow.document.getElementById("toolbar-menubar");
>+        var appMenuButtonIsVisible = menuBar.getAttribute("autohide") == "true";
>+        document.body.classList.add(appMenuButtonIsVisible ? "appMenuButtonVisible" :
>+                                                             "appMenuButtonInvisible");
>       }, false);
>       
>       function togglePrivateBrowsing() {

Comment 17

[(no longer active)]

Attachment: Patch (v4)

With comment 16 addressed.

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/4e868145aa6b