Closed Bug 1157978 Opened 5 years ago Closed 4 years ago

Remove duplicate what is private browsing messages

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- verified
firefox42 --- verified

People

(Reporter: mcomella, Assigned: mhaigh)

References

Details

Attachments

(7 files, 1 obsolete file)

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.
Attached image 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)
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).
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!
Flags: needinfo?(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.
Flags: needinfo?(matej)
Assignee: nobody → mhaigh
Can you show me what you want for about:privatebrowsing when opened in a normal tab?
Flags: needinfo?(alam)
Here's the design for normal tab, that re-uses the same icon :)
Flags: needinfo?(alam)
Can I get design specs for these please (colours, dimensions etc)?

Thanks
Flags: needinfo?(alam)
Attached image spec_pb_tray2.png
Quick spec for ya.

on dark, it's the same except font colors change to "Tabs tray icon grey" :)
Flags: needinfo?(alam)
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
Flags: needinfo?(alam)
Attached image icon_Masq_SVG.svg
Flags: needinfo?(alam)
Revamp of the about:privateBrowsing page.  One question - do I need to update the string key names?

Image has been optimised :)
Attachment #8622518 - Flags: review?(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 :)
Attachment #8622518 - Flags: review?(margaret.leibovic) → feedback+
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.
Attachment #8622518 - Attachment is obsolete: true
Attachment #8623685 - Flags: review?(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?
Attachment #8623685 - Flags: review?(margaret.leibovic) → review+
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
Flags: needinfo?(alam)
You mean the same links? yeah, I think so.
Flags: needinfo?(alam)
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.
https://hg.mozilla.org/mozilla-central/rev/1679ea0f802f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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.
Flags: needinfo?(mhaigh)
Depends on: 1176883
I've created bug 1176883 to track this.
Flags: needinfo?(mhaigh)
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.
You need to log in before you can comment on or make changes to this bug.