1157978 - Remove duplicate what is private browsing messages

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Antlam noticed show two nearly identical private browsing messages - one in the tabs tray when you have no private tabs and another when you create a new private tab. We shouldn't duplicate the information.

I think we should remove the tabs tray message because you can avoid it by using the toolbar menu to create a new private tab, whereas you will always see the new private tab screen.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_pb_normal2.png

I'd agree. This moves the messaging of the empty private tabs tray (the latest) into the empty state for a new private tab (currently with the old colorful masq design).

Now, we just need an empty state for the private tabs tray. (Bug 1164959)

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Worth pointing out that this follows the same spec as our empty state for Panels (bug 1091826) except just a bit wider (300 dp rather than 240dp).

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: emptystate_masq.zip

attaching icons.

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

NI-ing Matej here to review the copy :)

Matej, does this still work? Feel free to open up our current build to see the current situation we're looking to resolve.

Appreciate the eyes!

Comment 5

[Matej Novak [:matej]]

(In reply to Anthony Lam (:antlam) from comment #4)
> NI-ing Matej here to review the copy :)
> 
> Matej, does this still work? Feel free to open up our current build to see
> the current situation we're looking to resolve.
> 
> Appreciate the eyes!

I think that works. Thanks.

Comment 6

[Martyn Haigh (:mhaigh)]

Can you show me what you want for about:privatebrowsing when opened in a normal tab?

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_pb_aboutcompare.png

Here's the design for normal tab, that re-uses the same icon :)

Comment 8

[Martyn Haigh (:mhaigh)]

Can I get design specs for these please (colours, dimensions etc)?

Thanks

Comment 9

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: spec_pb_tray2.png

Quick spec for ya.

on dark, it's the same except font colors change to "Tabs tray icon grey" :)

Comment 10

[Martyn Haigh (:mhaigh)]

Actually this page is web content, so I'm not sure if we can use the Roboto font and measurements in dp and sp don't make sense.  Can I get you to redo for HTML?  ping me if you need any more info

Comment 11

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_Masq_SVG.svg

Comment 12

[Martyn Haigh (:mhaigh)]

Attachment: Remove duplicate what is private browsing messages

Revamp of the about:privateBrowsing page.  One question - do I need to update the string key names?

Image has been optimised :)

Comment 13

[:Margaret Leibovic]

Comment on attachment 8622518 [details] [diff] [review]
Remove duplicate what is private browsing messages

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

This looks like it's going in the right direction, but I have some comments I'd like to see addressed.

::: mobile/android/chrome/content/aboutPrivateBrowsing.js
@@ +17,5 @@
> +    .QueryInterface(Ci.nsIDocShellTreeItem)
> +    .rootTreeItem
> +    .QueryInterface(Ci.nsIInterfaceRequestor)
> +    .getInterface(Ci.nsIDOMWindow)
> +    .QueryInterface(Ci.nsIDOMChromeWindow));

Because we only have one global chrome window, a more succient way to do this is just:

Services.wm.getMostRecentWindow("navigator:browser");

You could get rid of the XPCOMUtils import and just do this directly in the event handler.

@@ +20,5 @@
> +    .getInterface(Ci.nsIDOMWindow)
> +    .QueryInterface(Ci.nsIDOMChromeWindow));
> +
> +let PrivateBrowsing = {
> +    init: function() {

Nit: 2-space indentation in JS.

@@ +31,5 @@
> +        }
> +    },
> +};
> +
> +window.addEventListener("load", PrivateBrowsing.init.bind(PrivateBrowsing), false);

You can use DOMContentLoaded, which happens sooner than load.

Also, instead of creating this whole PrivateBrowsing object, I feel like it would be simpler to just pass in a function here that does what the init method above does.

::: mobile/android/locales/en-US/chrome/aboutPrivateBrowsing.dtd
@@ +10,2 @@
>  
> +<!ENTITY privatebrowsingpage.description "Bookmarks you add and files you download will still be saved on your device.">

Yes, you should update entity names when the content of the string changes.

@@ +14,3 @@
>  
> +<!ENTITY privatebrowsingpage.link.private "Want to learn more?">
> +<!ENTITY privatebrowsingpage.link.private.url "https://support.mozilla.org/kb/mobile-private-browsing-browse-web-without-saving-syncing-info">

We don't want localizers to change link URLs. We should dynamically generate this link using the "app.support.baseURL" pref. Here's an example:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6393

::: mobile/android/themes/core/aboutPrivateBrowsing.css
@@ +23,2 @@
>    background-color: #292c29;
> +  color: #afb1b3; /* tabs_tray_icon_grey */

Nice use of the color palette :)

Comment 14

[Martyn Haigh (:mhaigh)]

Attachment: Remove duplicate what is private browsing messages

Unfortunately the simplified JS approach didn't work, so I've kept the original code in there.  Also having spoken to Roland, he suggested a hardcoded url for the 'More info' link.

Comment 15

[:Margaret Leibovic]

Comment on attachment 8623685 [details] [diff] [review]
Remove duplicate what is private browsing messages

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

Thanks for looking into my suggestions, and sorry the window logic suggestion didn't pan out!

::: mobile/android/chrome/content/aboutPrivateBrowsing.xhtml
@@ +36,2 @@
>  
> +      <p class="showPrivate"><a href="https://support.mozilla.org/kb/private-browsing-firefox-android">&privatebrowsingpage.link.private;</a></p>

Just double-checking: this will just load this support URL in the current tab. That's the desired behavior, right?

Comment 16

[Martyn Haigh (:mhaigh)]

Margaret: I believe so. 
Anthony: Can we double check that the more info link on the empty private tab page should open the info in the current (private) tab

Comment 17

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

You mean the same links? yeah, I think so.

Comment 18

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

OH wait, do you mean that Learn more link, should be opened in a Private tab? Yes! that has always bugged me. So let's open it in the current private tab.

Comment 19

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76db61438807

Comment 20

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/integration/fx-team/rev/1679ea0f802f

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1679ea0f802f

Comment 22

[Francesco Lodolo [:flod]]

http://hg.mozilla.org/mozilla-central/diff/1679ea0f802f/mobile/android/locales/en-US/chrome/aboutPrivateBrowsing.dtd

-<!ENTITY privatebrowsingpage.description.private    "&privatebrowsingpage.issueDesc.private; &privatebrowsingpage.description;">

+<!ENTITY privatebrowsingpage.description.private "&privatebrowsingpage.issueDesc.private2; &privatebrowsingpage.description2;">

That will probably create a YSOD in localized builds: you kept the existing string ID, but now privatebrowsingpage.issueDesc.private is not available anymore, so existing translations will break badly.

You need a new ID for that string (I guess privatebrowsingpage.description2.normal). 

It would be nice to avoid another backout, but this needs to be fixed as soon as possible, and I'm not sure what it means with Whistler in the middle.

Comment 23

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=163100769cdd

Comment 24

[Martyn Haigh (:mhaigh)]

I've created bug 1176883 to track this.

Comment 25

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-07-16-16-18-14.png

Tested with:
Device: Nexus 4 (Android 5.0)
Builds: Firefox for Android 41.0a2 (2015-07-16) and Firefox for Android 42.0a1 (2015-07-16)

Also, tapping "Want to learn more?" opens the support page in the current private tab.