Closed
Bug 1065004
Opened 10 years ago
Closed 9 years ago
Provide an option to always external links in Private Browsing
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(firefox41 fixed, fennec+)
RESOLVED
FIXED
Firefox 41
People
(Reporter: krudnitski, Assigned: ally)
References
Details
(Keywords: productwanted)
Attachments
(3 files, 3 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
liuche
:
review+
|
Details | Diff | Splinter Review |
UX needed to provide an option to our users to always use private browsing by default
Comment 2•10 years ago
|
||
I have been talking about an approach to this feature, combined with a different requested feature: Don't save history.
"Don't save history" works in a normal Tab, which saves everything but the browsing history. Desktop has this feature and we have had a few people asking for it in Fennec too.
So my pitch is to create a Setting that allows people to pick:
Select browsing privacy level:
1. Save everything (Normal)
2. Don't save history
3. Save nothing (Private)
In modes #1 and #2, Fennec would have both Tabs and Private Tabs. In mode #1, Tabs work like they do today. In #2, Tabs would not save history, but would save cookies and anything else.
Thoughts?
Comment 3•10 years ago
|
||
This could use some UX designs
tracking-fennec: ? → 35+
Flags: needinfo?(ywang)
Flags: needinfo?(alam)
Comment 4•10 years ago
|
||
I could see the first part as a setting. But I'm still a bit on the fence about the "don't save history" part.
I can see what this might look like as a "select your privacy level" but it seems like we're adding a lot of complexity to what could be better off left simple with more of a focus on messaging around Private Vs Normal tabs (+ a simple setting toggle for 'Default to Private tabs').
Going to NI- Robin as well since this has a lot to do with Settings and we talked about this earlier too
Flags: needinfo?(randersen)
Comment 5•10 years ago
|
||
So what I can suggest is to add a History item on the Privacy settings, with three options:
1) Always
2) Never
3) Always Private
This doesn't give the user a ton of info/education and it also introduces another scenario...they are using Private mode without being in Private mode so they won't have the UI to remind them, and somewhere down the line they might forget they made this choice and be pissed.
Flags: needinfo?(randersen)
Comment 6•10 years ago
|
||
Comment 7•10 years ago
|
||
(In reply to Robin Andersen [:tecgirl] from comment #5)
> and somewhere down the line they
> might forget they made this choice and be pissed.
We've had exactly this happen with Guest Mode, FWIW.
Presumably we'll display private tabs in our dark-and-purple flair, though, regardless of why they're private?
Comment 8•10 years ago
|
||
Thanks for catching this. I agree we should offer both options, but I don't think "don't save history" should be combined with "always private" in one setting.
> So what I can suggest is to add a History item on the Privacy settings, with
> three options:
> 1) Always
> 2) Never
> 3) Always Private
To me "Always private" offers more than just a setting for history. Also with the strong word "NEVER", it makes "Never save history" a more powerful option than "always private", which I think it should be the opposite.
----------------
Karen's description for "Always private":
"the browser opens up private tabs by default (while still allowing users to actively choose to open a 'public' tab). ie flip the 'public' and 'private' contexts around"
My hypothesis is that though people prefer to always launch in private, but they might still want to keep the normal tabs around. So have the option of "don't remember history" can still be valuable.
If I understand Karen's description correctly, an accurate term for this action should be "Launch in private browsing" instead of "Always use private browsing". If you have "launch in private browsing" turned on, you can still open and access public tabs.
------------------
I would suggest:
* Add "Remember browsing history" option(checked by default). It could sit above "Remember passwords".
* Add "Launch in private browsing" option(unchecked by default) at the bottom(the placement is tentative) of the list. Since this action will cause some significant UI changes, when a user turns it on, we should show a confirmation dialog that displays a screenshot of your new browser UI and what this change will do. The user can CONFIRM or CANCEL that decision.
This will make sure both options are surfaced to the top level instead of hidden in a secondary dialog. And we will be able to address the UI change to our users before they actually confirm the change, in order to avoid those "OMG CHANGE" moments.
Let me know what you think.
Flags: needinfo?(ywang)
Comment 9•10 years ago
|
||
(In reply to Robin Andersen [:tecgirl] from comment #5)
> So what I can suggest is to add a History item on the Privacy settings, with
> three options:
> 1) Always
> 2) Never
> 3) Always Private
>
> This doesn't give the user a ton of info/education and it also introduces
> another scenario...they are using Private mode without being in Private mode
> so they won't have the UI to remind them, and somewhere down the line they
> might forget they made this choice and be pissed.
This is the design I was thinking about. As for "using Private mode without being in Private mode" we might be able to get around that by making sure the UI acts the same as when using a Private Tab. We could maybe use an indicator, like we're doing with Guest Mode, to keep the user informed about the state. I like the indicator less for Private Mode though.
If we decide to not go with a combined setting, I'm not sure we should show "Never save history" as a visible Setting at all. It might only add to confusion.
(In reply to Yuan Wang(:Yuan) – Mobile Firefox Design Lead from comment #8)
> To me "Always private" offers more than just a setting for history. Also
> with the strong word "NEVER", it makes "Never save history" a more powerful
> option than "always private", which I think it should be the opposite.
I agree that "Always private" is stronger than just "history" and we need to convey that.
> If I understand Karen's description correctly, an accurate term for this
> action should be "Launch in private browsing" instead of "Always use private
> browsing". If you have "launch in private browsing" turned on, you can still
> open and access public tabs.
Agreed. Karen's use case is all about the default type of tab we use to open Fennec: Normal or Private. After launch, we shouldn't do anything different
> I would suggest:
> * Add "Remember browsing history" option(checked by default). It could sit
> above "Remember passwords".
I'd skip this Setting for now. It's likely to add some confusion as to how these different Settings interact: "If 'Remember browsing history' is checked, but 'Launch in private browsing' is also checked, is my history still saved?"
> * Add "Launch in private browsing" option(unchecked by default) at the
> bottom(the placement is tentative) of the list. Since this action will cause
> some significant UI changes, when a user turns it on, we should show a
> confirmation dialog that displays a screenshot of your new browser UI and
> what this change will do. The user can CONFIRM or CANCEL that decision.
Sounds reasonable
Comment 10•10 years ago
|
||
It see(In reply to Mark Finkle (:mfinkle) from comment #9)
> (In reply to Yuan Wang(:Yuan) – Mobile Firefox Design Lead from comment #8)
>
> > To me "Always private" offers more than just a setting for history. Also
> > with the strong word "NEVER", it makes "Never save history" a more powerful
> > option than "always private", which I think it should be the opposite.
>
> I agree that "Always private" is stronger than just "history" and we need to
> convey that.
> > * Add "Launch in private browsing" option(unchecked by default) at the
> > bottom(the placement is tentative) of the list. Since this action will cause
> > some significant UI changes, when a user turns it on, we should show a
> > confirmation dialog that displays a screenshot of your new browser UI and
> > what this change will do. The user can CONFIRM or CANCEL that decision.
>
> Sounds reasonable
Perhaps "Always use Private browsing"?
Wording aside, this does seem more and more like a heavier feature that needs to be called out a bit more for discover-ability and all the other reasons pointed out above.
Though a confirmation is all that's necessary here I think. Not sure we need to show a screenshot of the new browser UI. We could also leverage the first blank state in the private browsing window to explain what "just happened".
Flags: needinfo?(alam)
Updated•10 years ago
|
tracking-fennec: ? → +
Reporter | ||
Updated•10 years ago
|
tracking-fennec: + → ?
Comment 14•10 years ago
|
||
Let's figure out the bare minimum that needs to be done here. Limit the scope please.
Comment 15•10 years ago
|
||
Just wanted to throw some additional information in here. I see that Desktop supports an "Always use Private Browsing" mode now. It's in the Preferences, under Privacy > History, and pick "Use custom settings for history". There are a few configurations there.
We currently support the ability to "Never save history" but we do not have a UI to enable it. We could probably support "Always use Private Browsing" without too much effort, but we would need more thinking about the ramifications. For example, if someone turned on "Always use PB", we'd probably need to clear the session state of any non-PB tabs and probably offer to restart Firefox, so there would be no leftover non-PB tabs running. We might also offer to clear all private data too.
Comment 16•10 years ago
|
||
To look at the Desktop code, search for permanentPrivateBrowsing and the "browser.privatebrowsing.autostart" preference.
Updated•10 years ago
|
Updated•10 years ago
|
Assignee: bnicholson → ally
Assignee | ||
Comment 18•9 years ago
|
||
Margaret, I was going to keep the other bug(1162269) for the bare bones technical implementation and use this one for the ui/polishing.
Assignee | ||
Comment 19•9 years ago
|
||
- mfinkle won't like it, but it currently does the job. Yes, ill put up his idea after dinner.
- bug: does not observe changes to the pref. Whatever the state is upon load is the state it will stay at until rebooted. This seems like there's something wrong with the pref helper there. Or maybe I'm misusing pref helper.
Assignee | ||
Comment 20•9 years ago
|
||
incorrect use of prefsHelper it is. Needs isObserver() override. (Though why this is not the default behavior for monitoring gecko prefs is lost on me)
Comment 21•9 years ago
|
||
Because generally Java UI code doesn't depend on Gecko prefs, and if it does it expects you to change them through GeckoPreferences, not through about:config, so a one-shot fetch is expected.
Presumably this setting will live in our own Settings UI, right?
In that case, if there's a Gecko pref at all (e.g., because some JS code needs to behave correctly), the Gecko pref will be a mirror of the Java toggle, exactly as already handled by most of GeckoPreferences, and you don't need to observe from Java.
If we *do* need a Gecko pref (e.g., because we need it set from distributions, and for some reason Bug 866271 isn't enough), then we probably don't need to observe it.
But looking at your WIP, it looks like there will be no Gecko-side logic at all. We'll have a single android.not_a_pref preference in GeckoPreferences, backing a checkbox, and that's it.
Assignee | ||
Comment 22•9 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #21)
> Because generally Java UI code doesn't depend on Gecko prefs, and if it does
> it expects you to change them through GeckoPreferences, not through
> about:config, so a one-shot fetch is expected.
>
> Presumably this setting will live in our own Settings UI, right?
>
> In that case, if there's a Gecko pref at all (e.g., because some JS code
> needs to behave correctly), the Gecko pref will be a mirror of the Java
> toggle, exactly as already handled by most of GeckoPreferences, and you
> don't need to observe from Java.
>
> If we *do* need a Gecko pref (e.g., because we need it set from
> distributions, and for some reason Bug 866271 isn't enough), then we
> probably don't need to observe it.
>
> But looking at your WIP, it looks like there will be no Gecko-side logic at
> all. We'll have a single android.not_a_pref preference in GeckoPreferences,
> backing a checkbox, and that's it.
That would be the eventual home (I hope), but until it becomes an Official Feature exposed in the UI, and that UI is decided upon, my marching orders are to stick it in an about:config pref until that time.
Assignee | ||
Comment 23•9 years ago
|
||
- per original bug, contains no settings ui
- defaults to off(false)
Attachment #8607265 -
Attachment is obsolete: true
Attachment #8607776 -
Flags: review?(margaret.leibovic)
Comment 24•9 years ago
|
||
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #22)
> (In reply to Richard Newman [:rnewman] from comment #21)
> > Because generally Java UI code doesn't depend on Gecko prefs, and if it does
> > it expects you to change them through GeckoPreferences, not through
> > about:config, so a one-shot fetch is expected.
> >
> > Presumably this setting will live in our own Settings UI, right?
> >
> > In that case, if there's a Gecko pref at all (e.g., because some JS code
> > needs to behave correctly), the Gecko pref will be a mirror of the Java
> > toggle, exactly as already handled by most of GeckoPreferences, and you
> > don't need to observe from Java.
> >
> > If we *do* need a Gecko pref (e.g., because we need it set from
> > distributions, and for some reason Bug 866271 isn't enough), then we
> > probably don't need to observe it.
> >
> > But looking at your WIP, it looks like there will be no Gecko-side logic at
> > all. We'll have a single android.not_a_pref preference in GeckoPreferences,
> > backing a checkbox, and that's it.
>
> That would be the eventual home (I hope), but until it becomes an Official
> Feature exposed in the UI, and that UI is decided upon, my marching orders
> are to stick it in an about:config pref until that time.
Rather than having this be a pref in about:config, I think we should just keep behind a Nightly flag until we're ready to ship it.
I think power users who want new features should just use Nightly, rather than go into about:config on release builds.
Comment 25•9 years ago
|
||
Comment on attachment 8607776 [details] [diff] [review]
openExteneralLinksInPrivateBrowsing
Review of attachment 8607776 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/Tabs.java
@@ +121,5 @@
> "Tab:StreamStart",
> "Tab:StreamStop",
> "Reader:Toggle");
>
> + mPrefObserverId = PrefsHelper.getPref("browser.tabs.openNewURLsInPrivate", new PrefsHelper.PrefHandlerBase() {
You don't use this member variable, so you can get rid of it. (I would suggest you unregister this observer, but it doesn't seem like there's any place to do that).
If we do go with this gecko pref approach, I think we should try to be lazier about fetching this pref, and only do it the first time we are opening an external URL.
However, I think it would be better to just use a standard shared pref, and expose this in the settings UI behind a nightly only flag. You should talk to antlam about the best place to put this.
Related (but not required) idea: maybe we could have an "experiments" setting section for nightly/beta users.
@@ +865,5 @@
> try {
> boolean isPrivate = (flags & LOADURL_PRIVATE) != 0;
> boolean userEntered = (flags & LOADURL_USER_ENTERED) != 0;
> boolean desktopMode = (flags & LOADURL_DESKTOP) != 0;
> boolean external = (flags & LOADURL_EXTERNAL) != 0;
Reviewing this patch made me fall into reading this code to understand what we actually do with this parameter, and it seems like it's *only* used so that we can know to close external tabs when you hit the back button to exit the app:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#2322
I wish there was a better way to do that without passing these parameters and flags all over the place :/
@@ +867,5 @@
> boolean userEntered = (flags & LOADURL_USER_ENTERED) != 0;
> boolean desktopMode = (flags & LOADURL_DESKTOP) != 0;
> boolean external = (flags & LOADURL_EXTERNAL) != 0;
>
> + if(mExternURLSinPrivate && external) {
Nit: space between if and (
@@ +869,5 @@
> boolean external = (flags & LOADURL_EXTERNAL) != 0;
>
> + if(mExternURLSinPrivate && external) {
> + isPrivate = true;
> + }
This looks like the right approach to me. I was curious as to whether or not we could just take care of this in JS, but sadly we need to know about this before it gets to JS in order to create a PrivateTab instance, as opposed to a Tab isntance.
Attachment #8607776 -
Flags: review?(margaret.leibovic) → feedback+
Comment 27•9 years ago
|
||
Let's put this preference in Privacy, under Settings.
"Always use Private browsing"
Flags: needinfo?(alam) → needinfo?(ally)
Comment 29•9 years ago
|
||
(In reply to Anthony Lam (:antlam) from comment #27)
> Let's put this preference in Privacy, under Settings.
>
> "Always use Private browsing"
But wait... this bug is just about adding a pref for opening external links in private tabs, not always using private browsing. So I think this string needs to mention that. Perhaps "Always use Private browsing for external links". Or maybe something shorter, but then include a description string? I actually think adding extra descriptive text would be helpful no matter what.
Flags: needinfo?(alam)
Comment 30•9 years ago
|
||
(In reply to :Margaret Leibovic from comment #29)
> (In reply to Anthony Lam (:antlam) from comment #27)
> > Let's put this preference in Privacy, under Settings.
> >
> > "Always use Private browsing"
>
> But wait... this bug is just about adding a pref for opening external links
> in private tabs, not always using private browsing. So I think this string
> needs to mention that. Perhaps "Always use Private browsing for external
> links". Or maybe something shorter, but then include a description string? I
> actually think adding extra descriptive text would be helpful no matter what.
Ah, I was looking for mention of that. My mistake!
Then I think this belongs in "Customize" under Settings. Right next to "Open multiple links".
try this:
Open links in Private browsing
For all external links opened in Firefox
Flags: needinfo?(alam) → needinfo?(ally)
Assignee | ||
Comment 31•9 years ago
|
||
Attachment #8607776 -
Attachment is obsolete: true
Flags: needinfo?(ally)
Attachment #8610777 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 32•9 years ago
|
||
Yes, I know about the 2nd space inside the () on the if () statement. Its fixed locally, but didnt seem worth uploading another patch for.
Comment 33•9 years ago
|
||
Comment on attachment 8610777 [details] [diff] [review]
openExteneralLinksInPrivateBrowsing
Review of attachment 8610777 [details] [diff] [review]:
-----------------------------------------------------------------
I'm okay with letting this ride the trains (or we can deal with disabling it in another bug if we decide to do that), but FYI, if we want to restrict this to Nightly, you should do something like this to hide the pref in the UI:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java#699
r+ with comments addressed.
::: mobile/android/base/Tabs.java
@@ +854,5 @@
> boolean userEntered = (flags & LOADURL_USER_ENTERED) != 0;
> boolean desktopMode = (flags & LOADURL_DESKTOP) != 0;
> boolean external = (flags & LOADURL_EXTERNAL) != 0;
>
> + SharedPreferences sharedprefs = GeckoSharedPrefs.forApp(mAppContext);
Is there a reason you chose to make this an app-level pref instead of a profile-level pref? I think it would make more sense to be per-profile, so that a guest session user couldn't change things (and this will be more future-proof for when the day comes that we want more profile support).
@@ +855,5 @@
> boolean desktopMode = (flags & LOADURL_DESKTOP) != 0;
> boolean external = (flags & LOADURL_EXTERNAL) != 0;
>
> + SharedPreferences sharedprefs = GeckoSharedPrefs.forApp(mAppContext);
> + boolean isPrivatePref = sharedprefs.getBoolean("android.not_a_preference.openExternalURLsPrivately", false);
Nit: Declare local variables as final.
::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +152,5 @@
> <!ENTITY pref_developer_remotedebugging "Remote debugging">
> <!ENTITY pref_category_logins "Logins">
> <!ENTITY pref_remember_signons "Remember passwords">
> +<!ENTITY pref_open_external_urls_privately_title "Open links in Private browsing">
> +<!ENTITY pref_open_external_urls_privately_summary "For all external links opened in Firefox">
We should use &brandShortName; here instead of "Firefox".
Attachment #8610777 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 34•9 years ago
|
||
(In reply to :Margaret Leibovic from comment #33)
> Comment on attachment 8610777 [details] [diff] [review]
> openExteneralLinksInPrivateBrowsing
>
> Review of attachment 8610777 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> r+ with comments addressed.
>
> ::: mobile/android/base/Tabs.java
> @@ +854,5 @@
> > boolean userEntered = (flags & LOADURL_USER_ENTERED) != 0;
> > boolean desktopMode = (flags & LOADURL_DESKTOP) != 0;
> > boolean external = (flags & LOADURL_EXTERNAL) != 0;
> >
> > + SharedPreferences sharedprefs = GeckoSharedPrefs.forApp(mAppContext);
>
> Is there a reason you chose to make this an app-level pref instead of a
> profile-level pref? I think it would make more sense to be per-profile, so
> that a guest session user couldn't change things (and this will be more
> future-proof for when the day comes that we want more profile support).
Using GeckoSharedPrefs.forProfile() always failed to find the pref. The app level succeeded. If you think this should not ride as is, please let me know.
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 35•9 years ago
|
||
discussed with rnewman & margaret on irc.
Bug 1162509 is thought to be behind this.
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 37•9 years ago
|
||
mfinkle would like to limit this to Nightly only. The original version exposed to the feature on dev edition and Nightly (which I wanted, but mfinkle points out that to limiting it to nightly keeps it consistent with other locked down features and makes the bookkeeping on that subject saner.).
Attachment #8610864 -
Flags: review?(liuche)
Updated•9 years ago
|
Attachment #8610864 -
Attachment is patch: true
Comment 38•9 years ago
|
||
Comment on attachment 8610864 [details] [diff] [review]
followupOpenExternal
Review of attachment 8610864 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, one nit. Also, comment about using the pref name from GeckoPreferences rather than maintaining a separate string.
::: mobile/android/base/preferences/GeckoPreferences.java
@@ +707,2 @@
> PREFS_OPEN_URLS_IN_PRIVATE.equals(key)) {
> // Remove UI for opening external links in private borwsing onrelease builds.
Nit: fix spelling and comment.
Attachment #8610864 -
Flags: review?(liuche) → feedback+
Updated•9 years ago
|
Keywords: leave-open
Comment 39•9 years ago
|
||
([leave open] until part 2 lands, or maybe filing a new bug)
Comment 40•9 years ago
|
||
You might also opt to hide the setting in Settings if we're in guest mode. That avoids some of the risk from per-app prefs.
Status: NEW → ASSIGNED
Assignee | ||
Comment 42•9 years ago
|
||
Attachment #8610864 -
Attachment is obsolete: true
Attachment #8613701 -
Flags: review?(liuche)
Comment 43•9 years ago
|
||
Comment on attachment 8613701 [details] [diff] [review]
followupOpenExternal v1
Review of attachment 8613701 [details] [diff] [review]:
-----------------------------------------------------------------
Can you file a follow-up to hide this in Guest Mode? Or do it in this bug if you want.
Attachment #8613701 -
Flags: review?(liuche) → review+
Assignee | ||
Comment 44•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 45•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Comment 46•9 years ago
|
||
Scenarios:
A. disable "Open multiple links" pref and enable "Open links in Private browsing" pref.
1. Go to gmail app and tap on a link
2. choose to open with "Nightly" -> the link is opened in private browsing
B. enable "Open multiple links" and "Open links in Private browsing" pref
1. Go to gmail app and tap on a link
2. choose to open with "Nightly" => "Tab saved in Nightly | Open now" notification is displayed.
3. If you choose "Open now", the link is opened in private browsing.
4. If you put the link in the tab queue and tap the "Nightly | 1 tab waiting", the link is opened in normal browsing. Is it expected?
Flags: needinfo?(mhaigh)
Flags: needinfo?(ally)
Assignee | ||
Comment 47•9 years ago
|
||
No, it should open in private browsing per the user's choice in the pref. It's a bug. Sounds like Tab Queues are not respecting that pref. I understand the feature is still under development.
This could happen if:
- TQ opens tabs through some other method that Tab.loadURL()
- TQ does call Tab.loadURL() but drops/does not include the LOADURL_EXTERNAL flag in the arguments
Martyn?
Flags: needinfo?(ally)
Comment 48•9 years ago
|
||
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #47)
> No, it should open in private browsing per the user's choice in the pref.
> It's a bug. Sounds like Tab Queues are not respecting that pref. I
> understand the feature is still under development.
I've filled Bug 1171860 for this issue.
Comment 49•9 years ago
|
||
Yup - TQ hasn't been designed to work with private tabs.
Flags: needinfo?(mhaigh)
Updated•9 years ago
|
Blocks: pb-external-link
Updated•9 years ago
|
Updated•9 years ago
|
Summary: Provide an option to always open tabs in Private Browsing → Provide an option to always external links in Private Browsing
Updated•9 years ago
|
Updated•9 years ago
|
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•