Closed Bug 1397100 Opened 7 years ago Closed 7 years ago

Preferences show controlling container extension

Categories

(Toolkit :: Preferences, enhancement, P2)

Product:
Toolkit
Component:
Preferences
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: jkt, Assigned: jkt)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Bug 1397100 - Disable container about:preference checkbox if a container addon is enabled.
59 bytes, text/x-review-board-request
bsilverberg
: review+
Felipe
: review+
Details
Container-Permission-flow-required-and-optional.png
280.42 KB, image/png
designakt
: ui-review?
mheubusch
Details
containers-control2.PNG
14.21 KB, image/png
Details 
As a follow up bug to Bug 1395659. We would like the ability to show the extension that is blocking the containers setting from being disabled.

When a user has a containers extension enabled it will prevent them from disabling containers. The user will be able to still manage container settings.
One core difference between this and Bug 1354344 is that there may be multiple extensions enabling containers. I don't think it would be a great UI to show a list of them all, however I'm willing to be convinced otherwise.

Perhaps "Sea Containers and 3 other extensions are enabling containers" would be a better string?

- Would the button disable all extensions?
  - When we support optional pref's for containers would it just remove the optional?
- Should it just list the first in the list and let the user disable just one at a time?


:andym, :jaws Who would be the decider for what direction should be done here?
Flags: needinfo?(jaws)
Flags: needinfo?(amckay)
Assignee: nobody → jkt
Markus has been working on this for other add-ons. For context, this is about exposing containers in the preferences, part of project Jazz.
Flags: needinfo?(amckay) → needinfo?(mjaritz)
Markus is probably the right person. Do we expect users to have more than one of these types of add-ons installed? I think listing them all by name here would be helpful. You can list them in a way that will require less localization work:

Add-ons forcing the Containers feature enabled: ABC Containers, 123 Containers, XYZ Containers
Flags: needinfo?(jaws)
> Do we expect users to have more than one of these types of add-ons installed?

Yeah, already we are seeing this like Sea Containers, switch container tabs etc.
(In reply to Jonathan Kingston [:jkt] from comment #1)
> One core difference between this and Bug 1354344 is that there may be
> multiple extensions enabling containers. I don't think it would be a great
> UI to show a list of them all, however I'm willing to be convinced otherwise.

All other prefs can also have extensions that previously controlled them, and when the users clicks disable, we fall back to that previous extension. So, I don't see a big difference in that extension.

> Perhaps "Sea Containers and 3 other extensions are enabling containers"
> would be a better string?

I think we should only talk about the last extensions that requested this, as in the other cases.

> - Would the button disable all extensions?
No, just the last one, and show the second to last if there is one. 
(which is unlikely as most users only ever have 1 or 2 extensions installed.)

>   - When we support optional pref's for containers would it just remove the
> optional?
I havn't thought about that case yet. Can you needinfo me on the bug, as soon as this becomes relevant. 
(And will this also be possible for other permissions?)

> - Should it just list the first in the list and let the user disable just
> one at a time?
Yes.
Flags: needinfo?(mjaritz)
> (And will this also be possible for other permissions?)

It may be in time, the main difference here is that on use of this preference it will override native browser preferences where as others seem to be more explicit on overriding a shortcut, home page.

The other use case that is similar would be the APIs to enable preferences like tracking protection, the button to disable an addon might be overkill here as it might not be the primary feature of the extension.

This is different as an extension that uses the permisssion (which in Bug 1386673 would become optional) to get the APIs would force containers to be enabled.
So the wording might not be clear nor would disabling the whole extension be required.

For example: a sidebar tab extension could give users an option to enable containers, in about:preferences the user decides to disable containers and see "Sidebar tabs is enabling containers [click to disable]"

I think it should have some way to remove the option without a full disable as we could see Chrome/Opera/Edge extensions progressively enhance based on this optional setting for containers.
Flags: needinfo?(mjaritz)
(In reply to Jonathan Kingston [:jkt] from comment #8)
> > (And will this also be possible for other permissions?)
> 
> It may be in time, the main difference here is that on use of this
> preference it will override native browser preferences where as others seem
> to be more explicit on overriding a shortcut, home page.

I see how that is a difference, but not why it should impact the behaviour.

> The other use case that is similar would be the APIs to enable preferences
> like tracking protection, the button to disable an addon might be overkill
> here as it might not be the primary feature of the extension.

If the extension *requires* the permission, we only can assume it really needs that permission to provide it's core function, hence disable it, if the user does want control back. (Same as we offer on install: accept, or don't use.)
If the extension asks for it *optionally*, this is different. Then we can assume it is not required, so we can take it away, without disabling. (see second row in the image)

> This is different as an extension that uses the permission (which in Bug
> 1386673 would become optional) to get the APIs would force containers to be
> enabled.
> So the wording might not be clear nor would disabling the whole extension be
> required.

Michelle, can you please review wording for those flows. The required flow is mostly the same as with homepage/newtab/cookie, the optional flow is new. (second row)
Flags: needinfo?(mjaritz)
Attachment #8905857 - Flags: ui-review?(mheubusch)
(In reply to Markus Jaritz [:designakt] (UX) from comment #9)
> Created attachment 8905857 [details]
> Container-Permission-flow-required-and-optional.png

I have a small feedback. ;) I would expect that the "An extension requires containers" row is below the container setting and not above. We usually scan websites from top to bottom, so this seems to be wrong:

- "an extension requires containers"
- user: what are containers?
- "enable container tabs [learn more]"
- user: ah, there I can learn more about containers!

And probably it should be used either "containers" or "container tabs" in both cases. It's a bit confusing that there are two similar, but different terms. ;)
(In reply to Sören Hentzschel from comment #10)
> (In reply to Markus Jaritz [:designakt] (UX) from comment #9)
> > Created attachment 8905857 [details]
> > Container-Permission-flow-required-and-optional.png
> 
> I have a small feedback. ;) I would expect that the "An extension requires
> containers" row is below the container setting and not above. We usually
> scan websites from top to bottom, so this seems to be wrong:
> 
> - "an extension requires containers"
> - user: what are containers?
> - "enable container tabs [learn more]"
> - user: ah, there I can learn more about containers!

I would like to keep the structure consistent across all implementations. When I started, I have tried before and after with cookie override and homepage (https://mozilla.invisionapp.com/d/main/#/console/11288035/242994351/preview)

Showing the message after resulted in a confusion similar to what you describe: The extension controls the preference, and disabling the extension to "unlock" the preference that sits above, goes against our learned hierarchy/reading sequence and it would start the user off with something they can not control.

Maybe tweaking the copy of the message can improve the understanding.
"An extension, <icon> <name>, is enforcing Container Tabs."

(I use one exception from that mentioned rule for NewTab, there the message is not directly controlling the preference, but is just a note to the preference, so it follows the preference.)

> And probably it should be used either "containers" or "container tabs" in
> both cases. It's a bit confusing that there are two similar, but different
> terms. ;)

Yes, we definitely should. Good call. My brain was too used to the internal name that I used that over the one already in the interface. "Container Tabs" should be used everywhere.
So my current WIP patch puts them after the settings row and also indents the row inwards from the left also, when you have preferred wording I can send a sreenshot. Like those screenshots the checkbox and it's label are greyed out but the settings button isn't.
Comment on attachment 8904827 [details]
Bug 1397100 - Disable container about:preference checkbox if a container addon is enabled.

https://reviewboard.mozilla.org/r/176586/#review184976

I don't see anything in here that requires/would benefit from a review from me, so clearing my flag.
Attachment #8904827 - Flags: review?(bob.silverberg)
(In reply to Jonathan Kingston [:jkt] from comment #12)
> So my current WIP patch puts them after the settings row and also indents
> the row inwards from the left also, when you have preferred wording I can
> send a sreenshot. Like those screenshots the checkbox and it's label are
> greyed out but the settings button isn't.

Please put it in front and use the copy I suggested in my previous post and share a screenshot.
To finalize the copy I will ping Michelle to get her opinion on that.
Attached image conrainers-control.PNG (obsolete) — 
I thought the comments suggested placing it after as I implemented. I used the text suggested?
Flags: needinfo?(mjaritz)
(In reply to Markus Jaritz [:designakt] (UX) from comment #11)
> https://mozilla.invisionapp.com/d/main/#/console/11288035/242994351/preview

This link is not publicly viewable. Can you share a link that is accessible without login?
As discussed with bsilverberg the simplest implementation to get this working is another pref which is set with the extension id, I'm going to change my implementation to that as it is currently buggy on when the extension is disabled as Marks bug is actually observing the home page path pref changes.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> (In reply to Markus Jaritz [:designakt] (UX) from comment #11)
> > https://mozilla.invisionapp.com/d/main/#/console/11288035/242994351/preview
> 
> This link is not publicly viewable. Can you share a link that is accessible
> without login?

Sorry about that, here is the public link:
https://mozilla.invisionapp.com/share/6HCITJKP8#/242994351_Jazz_-_Preferences_-_Privacy_-_V4_-_01_Override_Cookies
(In reply to Jonathan Kingston [:jkt] from comment #16)
> Created attachment 8908096 [details]
> conrainers-control.PNG
> 
> I thought the comments suggested placing it after as I implemented. 
If so, then I did not express myself clear enough. 
Please show it before the preference it is affecting.

> I used the text suggested?
please add a comma after the extension name. And you used a mix of my *required* and *optinal* flow.

I proposed 2 different verbs, depending on whether the extension *requires* the override, 
(and if we want to, offer optional override that has been separately granted by the user)

for required overrides: "An extension, <icon> <name>, requires Container Tabs."    "Disable Extension"
         or maybe even: "An extension, <icon> <name>, enforces Container Tabs."    "Disable Extension"

for optional overrides: "An extension, <icon> <name>, uses Container Tabs."        "Revoke Permission"


For copy I hope for Michelles feedback on what verb to use to indicate the connection.
Flags: needinfo?(mjaritz) → needinfo?(mheubusch)
All are required at the moment, we can look at the text of that flow in when it gets implemented.
Thanks for clarifying.
Attached image containers-control2.PNG — 
This is the updated screenshot with the suggested text.
Attachment #8908096 - Attachment is obsolete: true
status-firefox57: --- → affected
Priority: -- → P2
Attachment #8904827 - Flags: review?(jaws)
Attachment #8904827 - Flags: review?(felipc)
Attachment #8904827 - Flags: review?(bob.silverberg)
Comment on attachment 8904827 [details]
Bug 1397100 - Disable container about:preference checkbox if a container addon is enabled.

Adding back bsilverberg, I didn't intend to clear his review request.
Attachment #8904827 - Flags: review?(bob.silverberg)
Comment on attachment 8904827 [details]
Bug 1397100 - Disable container about:preference checkbox if a container addon is enabled.

https://reviewboard.mozilla.org/r/176586/#review185420

::: browser/app/profile/firefox.js:1532
(Diff revision 5)
>  // 0 disables long press, 1 when clicked, the menu is shown, 2 the menu is shown after X milliseconds.
>  pref("privacy.userContext.longPressBehavior", 2);
>  #else
>  pref("privacy.userContext.enabled", false);
>  pref("privacy.userContext.ui.enabled", false);
> +pref("privacy.userContext.extension", "");

As this is the same in both configurations, could you move this outside the #ifdef?

::: browser/components/preferences/in-content/main.js:485
(Diff revision 5)
> +
> +  /**
> +   * Enables/disables the Settings button used to configure containers
> +   */
> +  readBrowserContainersCheckbox() {
> +    var pref = document.getElementById("privacy.userContext.enabled");

It seems like this pref name could use a const in this file.

::: toolkit/components/extensions/ext-contextualIdentities.js:112
(Diff revision 5)
>  
>  ExtensionPreferencesManager.addSetting(CONTAINERS_ENABLED_SETTING_NAME, {
>    prefNames: Object.keys(CONTAINER_PREF_INSTALL_DEFAULTS),
>  
>    setCallback(value) {
> -    if (value === true) {
> +    if (value !== true) {

Why `value !== true`?
Attachment #8904827 - Flags: review?(bob.silverberg)
Comment on attachment 8904827 [details]
Bug 1397100 - Disable container about:preference checkbox if a container addon is enabled.

https://reviewboard.mozilla.org/r/176586/#review185514

::: browser/components/preferences/in-content/main.js:512
(Diff revision 5)
>  
>      document.getElementById("browserContainersbox").hidden = false;
> +    this.initContainersCheckbox();
> +  },
>  
> -    document.getElementById("browserContainersCheckbox").checked =
> +  initContainersCheckbox() {

`init` is not a great name because it leads to believe that the function will be called only once, whereas it can be called again by `readBrowserContainersCheckbox`. Let's try to use a diffent name, like `update` or `setup`.

But the split between `readBrowserContainersCheckbox` and `initContainersCheckbox` is a bit hard to understand.. Is it possible to merge both?  Or add a more detailed comment explaining it. For example, the comment for `readBrowserContainersCheckbox` says "Enables/disables the Settings button used to configure containers", but that appears to be the same thing that init does.

::: browser/components/preferences/in-content/main.xul:432
(Diff revision 5)
> -    <hbox id="browserContainersbox" hidden="true" align="center">
> +    <vbox id="browserContainersbox" hidden="true">
> +      <hbox id="browserContainersExtensionContent" align="center">
> +        <description control="disableContainersExtension" flex="1" />
> +        <button id="disableContainersExtension"
> +                class="extension-controlled-button accessory-button"
> +                label="&disableExtension.label;" />

no accesskey for this button? All the other ones have it
Attachment #8904827 - Flags: review?(felipc)
Comment on attachment 8904827 [details]
Bug 1397100 - Disable container about:preference checkbox if a container addon is enabled.

https://reviewboard.mozilla.org/r/176586/#review186004

::: browser/components/preferences/in-content/main.js:485
(Diff revision 5)
> +
> +  /**
> +   * Enables/disables the Settings button used to configure containers
> +   */
> +  readBrowserContainersCheckbox() {
> +    var pref = document.getElementById("privacy.userContext.enabled");

Sorry copy and paste.

::: toolkit/components/extensions/ext-contextualIdentities.js:112
(Diff revision 5)
>  
>  ExtensionPreferencesManager.addSetting(CONTAINERS_ENABLED_SETTING_NAME, {
>    prefNames: Object.keys(CONTAINER_PREF_INSTALL_DEFAULTS),
>  
>    setCallback(value) {
> -    if (value === true) {
> +    if (value !== true) {

Because the existing value was true which likely needs to be reset, that was my existing experience with testing anyway.

::: toolkit/components/extensions/ext-contextualIdentities.js:112
(Diff revision 5)
>  
>  ExtensionPreferencesManager.addSetting(CONTAINERS_ENABLED_SETTING_NAME, {
>    prefNames: Object.keys(CONTAINER_PREF_INSTALL_DEFAULTS),
>  
>    setCallback(value) {
> -    if (value === true) {
> +    if (value !== true) {

AS the default value before was true so in my testing this fixed existing users.
Comment on attachment 8904827 [details]
Bug 1397100 - Disable container about:preference checkbox if a container addon is enabled.

https://reviewboard.mozilla.org/r/176586/#review186060

Looks good, thanks Jonathan. r+ wc

::: browser/components/preferences/in-content/main.js:2672
(Diff revision 6)
>    return !!controllingExtensionId;
>  }
>  
>  async function showControllingExtension(settingName, extensionId) {
>    let extensionControlledContent = getControllingExtensionEl(settingName);
> -  // Tell the user what extension is controlling the homepage.
> +  // Tell the user what extension is controlling the extension.

In an intermediate commit this said "Tell the user what extension is controlling the setting", which I think makes more sense than this version.
Attachment #8904827 - Flags: review?(bob.silverberg) → review+
Flags: needinfo?(felipc)
Comment on attachment 8904827 [details]
Bug 1397100 - Disable container about:preference checkbox if a container addon is enabled.

https://reviewboard.mozilla.org/r/176586/#review186082
Attachment #8904827 - Flags: review+
Hi. The merge of that init function in readBrowserContainersCheckbox looks good. I think you missed my other question about this button not having an accesskey. Please add one, or explain why this button doesn't need one. r+ with that.
Flags: needinfo?(felipc)
Sorry Felipe, I did reply to your accesskey comment however I messed up mozreview and lost i, it seems.

The reason I didn't add one was the other home disable bug (Bug 1354344) didn't add one either and it seemed odd to make an action like this have it's own key especially since the other didn't and the button is still accessable by tabbing.

Let me know if that is ok.
Flags: needinfo?(felipc)
Felipe confirmed on IRC that this is fine to not have an accesskey as the other bug doesn't. I checked and tabbing and enter works as expected, there isn't a confirm to disable so I don't think it needs any more exposure.

From what I understand the UI feedback applies to the optional containers changes and that will be done in the follow up bug.
Flags: needinfo?(mheubusch)
Flags: needinfo?(felipc)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b2bc48a42a39
Disable container about:preference checkbox if a container addon is enabled. r=bsilverberg,Felipe
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b2bc48a42a39
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: