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

RESOLVED FIXED in Firefox 4.0b7

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: faaborg, Assigned: Ehsan)

Tracking

unspecified
Firefox 4.0b7
x86
Windows 7
Points:
---
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)

Updated

8 years ago
Assignee: nobody → ehsan
(Assignee)

Comment 2

8 years ago
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?
(Assignee)

Comment 5

8 years ago
I'll post a patch shortly.
(Assignee)

Comment 6

8 years ago
Created attachment 474165 [details] [diff] [review]
Patch (v1)
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-
(Assignee)

Updated

8 years ago
Blocks: 585370
(Assignee)

Comment 8

8 years ago
(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.
(Assignee)

Comment 9

8 years ago
Created attachment 474181 [details] [diff] [review]
Patch (v2)
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...
(Assignee)

Comment 11

8 years ago
(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...
(Assignee)

Comment 12

8 years ago
Created attachment 474220 [details] [diff] [review]
Patch (v3)
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-
(Assignee)

Comment 15

8 years ago
Created attachment 474237 [details] [diff] [review]
Patch (v4)
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?
(Assignee)

Comment 17

8 years ago
Created attachment 474252 [details] [diff] [review]
Patch (v4)

With comment 16 addressed.
Attachment #474237 - Attachment is obsolete: true
Attachment #474252 - Flags: approval2.0?
blocking2.0: ? → beta6+
(Assignee)

Updated

8 years ago
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]
(Assignee)

Comment 18

8 years ago
http://hg.mozilla.org/mozilla-central/rev/4e868145aa6b
Status: NEW → RESOLVED
Last Resolved: 8 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.