863780 - Update about:privatebrowsing instructions after appmenu button removal

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Going to about:privatebrowsing shows a document that tells you about private browsing, and tells you how to get to it. It gives you a button to press, and then it tells you which menus you can navigate to open a private browsing window.

With the Firefox button being retired, we need to update this to use the new app button that's in the nav-bar.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block landing on m-c.

Comment 2

[Justin Dolske [:Dolske]]

Hmm, is there any way to get to about:privatebrowsing without manually typing it?

Or, rather, the version of this page shown when opening a new private window doesn't actually say anything that needs adjusting, only when visiting directly.

This is the string that needs fixed (or maybe just removed?)

<!ENTITY privatebrowsingpage.howToStart3 "To start Private Browsing, you can also select &basePBMenu.label; &gt; &newPrivateWindow.label;.">

Comment 3

[(no longer active)]

(In reply to Justin Dolske [:Dolske] from comment #2)
> Hmm, is there any way to get to about:privatebrowsing without manually
> typing it?

No.

> Or, rather, the version of this page shown when opening a new private window
> doesn't actually say anything that needs adjusting, only when visiting
> directly.

I think so.

Comment 4

[Antonio Miras [:AMiras]]

Hello Mike.

I've been taking a look to Mozilla Community this week and would like to know about your projects.

Would this bug be good for me to take?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Sure! You'll want to create a local build of Firefox to develop on - you can follow the instructions here:

https://developer.mozilla.org/en/docs/Simple_Firefox_build

Alternatively, there are very helpful videos here: http://codefirefox.com/

Once you have your copy of Firefox built, let me know, and I'll guide you through the rest of this bug.

Feel free to email me, or comment in this bug if you get stuck, or drop into #introduction on irc.mozilla.org[1] and ask in there.

Thanks AMiras!

-Mike

[1]: https://wiki.mozilla.org/IRC

Comment 6

[Antonio Miras [:AMiras]]

Actually, this is my second bug, so I've already gone through those steps.

I've downloaded the UX branch. I'll keep you informed.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Ok, UX branch *should* be OK (it's usually kept up to date), but Australis work is occurring on mozilla-central right now.

For this patch, you should be OK, but for future patches, you'll want to clone and work off of mozilla-central.

So here's what about:privatebrowsing looks like right now: http://i.imgur.com/vRcigME.png

"To start Private Browsing, you can also select Nightly > New Private Window." is the offending string.

There are two approaches here:

1) Remove the string entirely (simplest). We might be able to keep it on OS X, since the instructions there refer to the ever-present global menu.
2) Alter the string to make reference to the Private Browsing widget that might be in the menu panel (I say might, because while it's in there by default, it can get customized out).

My vote is for #1, personally. This is a pretty obscure piece of the UI, so I think we should just go for it unless there are any objections.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

AMiras:

The files you're interested in are:

browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
and maybe
browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd

I think the approach I'm advocating right now is to wrap the footerTextNormal <p> element in an #ifdef XP_MACOSX.

We might also want to simplify some of the locale munging that's going on in the top of aboutPrivateBrowsing.xhtml.

Any preference, Ehsan? Or can we proceed as above?

-Mike

Comment 9

[(no longer active)]

This code <http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd#17> uses &basePBMenu.label;, which is set here: <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml#15>.  The non-Mac path in that code uses this value <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml#89> in the CSS to make either the "File menu" or "Nightly" show up in the text (see <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml#34>).  So, you basically need to modify |<span class='appMenuButton'>&brandShortName;</span>| on line 19 to point to the name of the menu button and also fix the logic on line 89.

A bigger question is, what would you use instead of &brandShortName; there!  AFAIK the new menu button doesn't even have an official name.  :/

Comment 10

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #9)
> This code
> <http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/aboutPrivateBrowsing.dtd#17> uses &basePBMenu.label;, which is set
> here:
> <http://mxr.mozilla.org/mozilla-central/source/browser/components/
> privatebrowsing/content/aboutPrivateBrowsing.xhtml#15>.  The non-Mac path in
> that code uses this value
> <http://mxr.mozilla.org/mozilla-central/source/browser/components/
> privatebrowsing/content/aboutPrivateBrowsing.xhtml#89> in the CSS to make
> either the "File menu" or "Nightly" show up in the text (see
> <http://mxr.mozilla.org/mozilla-central/source/browser/components/
> privatebrowsing/content/aboutPrivateBrowsing.xhtml#34>).  So, you basically
> need to modify |<span class='appMenuButton'>&brandShortName;</span>| on line
> 19 to point to the name of the menu button and also fix the logic on line 89.
> 
> A bigger question is, what would you use instead of &brandShortName; there! 
> AFAIK the new menu button doesn't even have an official name.  :/

According to madhava in IRC, the button is called "the menu button".

This isn't the first time I've heard I've heard about this problem - it was brought up in IRC for some support documentation.

madhava suggested we could try using a graphical representation of the button, like https://support.google.com/chrome/answer/3090029?ref_topic=3097320&rd=1.

madhava - is that going to be a common pattern when referring to the menu button? To supply the graphic as well?

Comment 11

[Antonio Miras [:AMiras]]

Ok, no problem. I'll keep working on mozilla-central.

So no further action until we get a response.

Comment 12

[:Gijs (he/him)]

We kind of really need strings to use here.

Comment 13

[Madhava Enros [:madhava]]

for a new string, instead of:

"To start Private Browsing, you can also select File > New Private Window."

how about:

"To start Private Browsing, you can also select New Private Window from the menu."

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Antonio, I'm sorry to have kept you in a holding position for so long only to yank this bug from you, but we're in a time crunch to get a patch for this in before the merge, so I'm going to snag it.

You can find more bugs to hack on at http://www.joshmatthews.net/bugsahoy/?ff=1.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch 1

This works on Windows. Testing on OS X and Linux next before requesting review.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8368090 [details] [diff] [review]
Patch 1

This seems to do the job on OS X, Linux and Windows. How's my driving, Matt?

Comment 17

[(no longer active)]

Comment on attachment 8368090 [details] [diff] [review]
Patch 1

Review of attachment 8368090 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
@@ -10,5 @@
>    <!ENTITY % netErrorDTD SYSTEM "chrome://global/locale/netError.dtd">
>    %netErrorDTD;
>    <!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd">
>    %globalDTD;
>    <!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd">

Please remove netError.dtd and browser.dtd as well.  I don't think either of them are used after this patch.

(But please double check that.)

::: browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd
@@ -12,5 @@
>  
>  <!ENTITY privatebrowsingpage.openPrivateWindow.label "Open a Private Window">
>  <!ENTITY privatebrowsingpage.openPrivateWindow.accesskey "P">
>  
> -<!-- LOCALIZATION NOTE (privatebrowsingpage.howToStart3): please leave &basePBMenu.label; intact in the translation -->

Please retain this l10n note, and adjust it for &newPrivateWindow.label;.

Comment 18

[:Gijs (he/him)]

Attachment: [Australis] Update about:privatebrowsing instructions after appmenu button removal. r=?

We can remove netError.dtd, but then we need to re-add brand.dtd (which is included in it), and we can't remove browser.dtd because we use it for newPrivateWindow.label. I've reinstated the l10n comment, and I've removed the obsolete JS logic and associated styles. Ehsan, is this better?

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Over to Gijs to drive this home. :)

Comment 20

[(no longer active)]

Comment on attachment 8368489 [details] [diff] [review]
[Australis] Update about:privatebrowsing instructions after appmenu button removal. r=?

Review of attachment 8368489 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks!

Comment 21

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #19)
> Over to Gijs to drive this home. :)

Pff, you did all the work. ;-)

remote:   https://hg.mozilla.org/integration/fx-team/rev/c8beb738fee7

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c8beb738fee7

Comment 23

[Bogdan Maris, Desktop QA]

Verified as fixed in latest Aurora on Windows 7 64bit, Ubuntu 13.10 64bit and Mac OS X 10.8.5. 
"To start Private Browsing, you can also select New Private Window from the menu." is present in about:privatebrowsing.