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

RESOLVED FIXED in Firefox 4.0b7

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: faaborg, Assigned: Ehsan)

Tracking

unspecified
Firefox 4.0b7
x86
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus ?

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: [strings])

Attachments

(1 attachment, 4 obsolete attachments)

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)
Should be a simple change but requesting blocking since the current text can confuse the user.
blocking2.0: --- → ?
Assignee: nobody → ehsan
Dao, do you know how one can determine whether the Firefox button is active or not?
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).
This has neither a patch, nor a blocking decision but strings impact.

What's the triage or ETA status here?
I'll post a patch shortly.
Posted patch Patch (v1) (obsolete) — Splinter Review
Attachment #474165 - Flags: review?(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.
Attachment #474165 - Flags: review?(dao) → review-
Blocks: 585370
(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.
Posted patch Patch (v2) (obsolete) — Splinter Review
Attachment #474165 - Attachment is obsolete: true
Attachment #474181 - Flags: review?(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...
(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...
Posted patch Patch (v3) (obsolete) — Splinter Review
Attachment #474181 - Attachment is obsolete: true
Attachment #474220 - Flags: review?(dao)
Attachment #474181 - Flags: review?(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 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 += ...
Attachment #474220 - Flags: review?(dao) → review-
Posted patch Patch (v4) (obsolete) — Splinter Review
Attachment #474220 - Attachment is obsolete: true
Attachment #474237 - Flags: review?(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() {
Attachment #474237 - Flags: review?(dao) → review+
Flags: in-litmus?
Posted patch Patch (v4)Splinter Review
With comment 16 addressed.
Attachment #474237 - Attachment is obsolete: true
Attachment #474252 - Flags: approval2.0?
blocking2.0: ? → beta6+
Attachment #474252 - Flags: approval2.0?
Keywords: checkin-needed
Whiteboard: [strings] → [strings][has reviewed patch]
Keywords: checkin-needed
Whiteboard: [strings][has reviewed patch] → [strings][can land]
http://hg.mozilla.org/mozilla-central/rev/4e868145aa6b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [strings][can land] → [strings]
Target Milestone: --- → Firefox 4.0b6
You need to log in before you can comment on or make changes to this bug.