Closed
Bug 593126
Opened 14 years ago
Closed 14 years ago
Reword text in the private browsing mode content area page based on the menu structure
Categories
(Firefox :: Private Browsing, defect)
Tracking
()
RESOLVED
FIXED
Firefox 4.0b7
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta7+ |
People
(Reporter: faaborg, Assigned: ehsan.akhgari)
References
Details
(Whiteboard: [strings])
Attachments
(1 file, 4 obsolete files)
7.97 KB,
patch
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•14 years ago
|
||
Should be a simple change but requesting blocking since the current text can confuse the user.
blocking2.0: --- → ?
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → ehsan
Assignee | ||
Comment 2•14 years ago
|
||
Dao, do you know how one can determine whether the Firefox button is active or not?
Comment 3•14 years ago
|
||
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•14 years ago
|
||
This has neither a patch, nor a blocking decision but strings impact. What's the triage or ETA status here?
Assignee | ||
Comment 5•14 years ago
|
||
I'll post a patch shortly.
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #474165 -
Flags: review?(dao)
Comment 7•14 years ago
|
||
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 | ||
Comment 8•14 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•14 years ago
|
||
Attachment #474165 -
Attachment is obsolete: true
Attachment #474181 -
Flags: review?(dao)
Comment 10•14 years ago
|
||
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•14 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•14 years ago
|
||
Attachment #474181 -
Attachment is obsolete: true
Attachment #474220 -
Flags: review?(dao)
Attachment #474181 -
Flags: review?(dao)
Comment 13•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
Attachment #474220 -
Attachment is obsolete: true
Attachment #474237 -
Flags: review?(dao)
Comment 16•14 years ago
|
||
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+
Updated•14 years ago
|
Flags: in-litmus?
Assignee | ||
Comment 17•14 years ago
|
||
With comment 16 addressed.
Attachment #474237 -
Attachment is obsolete: true
Attachment #474252 -
Flags: approval2.0?
Updated•14 years ago
|
blocking2.0: ? → beta6+
Assignee | ||
Updated•14 years ago
|
Attachment #474252 -
Flags: approval2.0?
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [strings] → [strings][has reviewed patch]
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [strings][has reviewed patch] → [strings][can land]
Assignee | ||
Comment 18•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/4e868145aa6b
Status: NEW → RESOLVED
Closed: 14 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.
Description
•