Closed Bug 1279029 Opened 4 years ago Closed 4 years ago

UX for a about:preferences#privacy option for containers

Categories

(Core :: DOM: Security, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: tanvi, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active][usercontextId])

Attachments

(3 files, 3 obsolete files)

Provide a mockup for a Containers section in about:preferences#privacy.  So that we can go to about:preferences, the Privacy section, and then enable or disable containers. Enabling or disabling it will toggle the about:config pref.  The text could say something like:

<heading>Containers</heading> <a href="sumo page">Learn More</a>
<radio button> Enable </radio button>
<radio button> Disable </radio button>

The heading is in bold.

The UI would only show up for Nightly users, it would be behind a pref of its own the way tracking protection is.  For example, privacy.userContext.ui.enabled.  It would be false by default.

Bram, are you able to do this?

Then Jonathan or baku can implement, and we can also create a sumo page to link the Learn More page from.
Whiteboard: [domsecurity-backlog]
Assignee: nobody → bram
Priority: -- → P2
Whiteboard: [domsecurity-backlog] → [domsecurity-backlog][usercontextId]
Attached image about:preferences - containers - i01 (obsolete) —
Other names we may be able to use in place of “Containers” if it’s too vague:

* Container Mode
* Container Tabs
* Container Windows
If we aren't lumping Containers into the tabs section, then maybe it fits better in about:preferences#security or about:preferences#privacy instead of general.  Bram - thoughts?
Flags: needinfo?(bram)
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #2)
> If we aren't lumping Containers into the tabs section, then maybe it fits
> better in about:preferences#security or about:preferences#privacy instead of
> general.

Yes. If we don’t group it with tabs, Containers might be a good section to add under Privacy. Not so appropriate under security, though, since we have add-on, content blocking and saved username/password there.
Flags: needinfo?(bram)
Should we put it at the bottom of about:preferences#privacy?

I think we should use "Container Tabs".  Once we've finalized, can you do a final mockup and we'll file an implementation bug?

Thanks!
Whiteboard: [domsecurity-backlog][usercontextId] → [domsecurity-active][usercontextId]
OK. Let’s change the title to “Container Tabs” and put it under the Privacy section, then.

The UI is again fairly straightforward. Does this look good?
Attachment #8767807 - Attachment is obsolete: true
(In reply to Bram Pitoyo [:bram] from comment #5)
> Created attachment 8769935 [details]
> about:preferences - containers - i02
> 
> OK. Let’s change the title to “Container Tabs” and put it under the Privacy
> section, then.
> 
> The UI is again fairly straightforward. Does this look good?

This looks good!  Thanks Bram!
Attached patch pref.patch (obsolete) — Splinter Review
This doesn't fix the fact that we need to restart FF to have all icon in the customizable area. But this is a separate bug.
Attachment #8770508 - Flags: review?(gijskruitbosch+bugs)
Can we point the learn more link to the wiki page for now?  Or do we need to create a sumo article?  Given this is only for Nightly, I personally think wiki is fine for now.
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #8)
> Can we point the learn more link to the wiki page for now?  Or do we need to
> create a sumo article?

I’m not sure about the best place to host, but one content that we need there is a tutorial/how-to, telling readers not only what containers is and what it’s good for, but also how to use it. We’ve already covered some of this in our “How to use section”.

Here’s an example of a SUMO article that’s written this way: https://support.mozilla.org/en-US/kb/enable-and-disable-cookies-website-preferences
Comment on attachment 8770508 [details] [diff] [review]
pref.patch

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

::: browser/components/preferences/in-content/privacy.js
@@ +60,5 @@
> +    if (!Services.prefs.getBoolPref("privacy.userContext.ui.enabled")) {
> +      return;
> +    }
> +
> +    let link = document.getElementById("containersLearnMore");

Some of this terminology is really confusing. I don't suppose we could just prefix with "browsingContainers" or just "userContextID" or something to make it slightly more clear that this isn't the same container as '.largeDialogContainer' or cookie containers (no, not these, the other ones...) or whatever?

@@ +63,5 @@
> +
> +    let link = document.getElementById("containersLearnMore");
> +    // TODO: this link doesn't exist yet!
> +    let url = Services.urlFormatter.formatURLPref("app.support.baseURL") + "containers";
> +    link.setAttribute("href", url);

nit: link.href =

@@ +65,5 @@
> +    // TODO: this link doesn't exist yet!
> +    let url = Services.urlFormatter.formatURLPref("app.support.baseURL") + "containers";
> +    link.setAttribute("href", url);
> +
> +    document.getElementById("containersbox").hidden = false;

I see you opted against "containerscontainer"? :P

@@ +70,5 @@
> +
> +    let enabledPref = document.getElementById("privacy.userContext.enabled");
> +    let radiogroup = document.getElementById("containersRadioGroup");
> +
> +    radiogroup.value = enabledPref.value ? "yes" : "no";

Why isn't this just a checkbox? That's directly hooked up to the pref? That seems more sensible, and would be a lot less code and simpler for the user to understand.

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
@@ +104,5 @@
>  
>  <!ENTITY  clearOnCloseSettings.label     "Settings…">
>  <!ENTITY  clearOnCloseSettings.accesskey "t">
> +
> +<!ENTITY  containersHeader.label         "Containers">

I think a sentence of explanation (rather than only a "learn more" link) would not go amiss.
Attachment #8770508 - Flags: review?(gijskruitbosch+bugs) → review-
> Why isn't this just a checkbox? That's directly hooked up to the pref? That
> seems more sensible, and would be a lot less code and simpler for the user
> to understand.


> > +<!ENTITY  containersHeader.label         "Containers">
> 
> I think a sentence of explanation (rather than only a "learn more" link)
> would not go amiss.

Bram, can you answer these 2 questions?
Flags: needinfo?(bram)
Attached patch pref.patch (obsolete) — Splinter Review
I applied all the comments except the UX changes. I want to see what Bram things about it.
Assignee: bram → amarchesini
Attachment #8770508 - Attachment is obsolete: true
(In reply to Andrea Marchesini (:baku) from comment #11)
> > Why isn't this just a checkbox? That's directly hooked up to the pref?

With a checkbox, we can be very brief and write something like this:

Container Tabs
[x] Enable Container Tabs        Learn more

Sounds good?

> > I think a sentence of explanation (rather than only a "learn more" link)
> > would not go amiss.


If there’s a way for us to explain this concept in one line of text, it would make things a lot simpler. Otherwise, having a tutorial or a blog post behind a “Learn more” link would mean that we have a place to teach Nightly users how to use this feature.
Flags: needinfo?(bram)
> Container Tabs
> [x] Enable Container Tabs        Learn more
> 
> Sounds good?

working on it.
Attached patch pref.patchSplinter Review
Attachment #8770903 - Attachment is obsolete: true
Attachment #8771919 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8771919 [details] [diff] [review]
pref.patch

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

If we can't explain what the feature does in one sentence, I wonder what that says about our understanding of the goals and appeal of the feature. :-\

In any case, the code here is fine. r=me when you've created the SUMO link. You can ping Joni to do this for you, assuming you have a SUMO page in mind you want this to link to.
Attachment #8771919 - Flags: review?(gijskruitbosch+bugs) → review+
I need a page for 'learn more'. A temporary page is OK for now I guess.
Flags: needinfo?(tanvi)
Flags: needinfo?(bram)
:baku this was the modal I was working on, it follows the same style as the XUL ones permitting resize etc. The modal content isn't there however I made that a callback.

https://gist.github.com/jonathanKingston/b28ac17ffb1b7268f87ea7f7f0b9fac0

It's not perfect however I don't want to be blocking you as I'm working on something else.
Flags: needinfo?(amarchesini)
I think that the wiki page would be a perfect one to link to. We can update it with a tutorial:
https://wiki.mozilla.org/Security/Contextual_Identity_Project/Containers

Tanvi, what do you think?
Flags: needinfo?(bram)
(In reply to Bram Pitoyo [:bram] from comment #19)
> I think that the wiki page would be a perfect one to link to. We can update
> it with a tutorial:
> https://wiki.mozilla.org/Security/Contextual_Identity_Project/Containers
> 
> Tanvi, what do you think?

Anybody can change a wikipage. I don't think we should have in-product links to wikimo.
Ok. Let's not block this patch. Let's remove the 'learn more', file a follow up to reintroduce it.
How does it sound?
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #21)
> Ok. Let's not block this patch. Let's remove the 'learn more', file a follow
> up to reintroduce it.
> How does it sound?

wfm.
Blocks: 1287765
Attached patch pref2.patchSplinter Review
Instead changing the first patch, I prefer to add a separate patch so that, once we have the final link, I can revert it.
Attachment #8772390 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8772390 [details] [diff] [review]
pref2.patch

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

::: browser/components/preferences/in-content/privacy.xul
@@ +291,5 @@
>  
>  <!-- Containers -->
>  <groupbox id="browserContainersGroup" data-category="panePrivacy" hidden="true">
>    <vbox id="browserContainersbox" hidden="true">
> +    <caption><label>&browserContainersHeader.label;</caption>

<caption><label>&browserContainersHeader.label;</label></caption>
Comment on attachment 8772390 [details] [diff] [review]
pref2.patch

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

Note that actually, looking at these changes, it occurs to me that the incorporating of the link within the text might need adjusting for l10n reasons if/when we do this. We can discuss if/when that moment comes.
Attachment #8772390 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70b550c1b6d1
part 1 - Containers in preferences, r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/d22b099cf1e3
part 2 - removed 'learn more' for Containers, r=gijs
Clearing my needinfo.  Learn More link is being discussed in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1287765
Flags: needinfo?(tanvi)
You need to log in before you can comment on or make changes to this bug.