When I create a new tab after already being in a container, many users are requesting that the same container is used to make a tab. I propose the prefname for this purpose: privacy.userContext.newTabInheritContext By default this will be false, but when this is true the user clicking + will get the same container as they are currently focused on (where "no container" behaves the same). Ctrl+T should behave in the same manner too.

This change allows for explicit specifying of 0 userContextId which we could choose to use in our "no container" items, currently these however don't so they open the specified default. We should think about if we want to change the wording of "no container" to be "default" or change that item to list what container it will open.

Comment on attachment 8915987 [details] Bug 1406371 - Add preference to inherit the previous container when opening a tab without a container. https://reviewboard.mozilla.org/r/187278/#review192732 ::: browser/base/content/utilityOverlay.js:272 (Diff revision 1) > + if (!aIsPrivate && isNaN(aUserContextId) && getBoolPref("privacy.userContext.newTabInheritContext", false)) { > + let currentTargetBrowser = params.targetBrowser || w.gBrowser.selectedBrowser; > + aUserContextId = w.gBrowser.getTabForBrowser(currentTargetBrowser).userContextId; > + } This doesn't just affect the new tab button and ctrl-t, it also changes the behaviour when clicking on links in e.g. about:preferences, notification bars and other parts of the UI that (eventually) go through openLinkIn. Is that actually what we want here? ::: browser/components/contextualidentity/test/browser/browser_newtabButton.js:110 (Diff revision 1) > + await SpecialPowers.pushPrefEnv({"set": [ > + ["privacy.userContext.enabled", true] > + ]}); Why add this? This doesn't unset the other preference, I think...

Comment on attachment 8915987 [details] Bug 1406371 - Add preference to inherit the previous container when opening a tab without a container. https://reviewboard.mozilla.org/r/187278/#review192742 ::: browser/base/content/utilityOverlay.js:272 (Diff revision 1) > w && !w.toolbar.visible) { > w = getTopWin(true); > aRelatedToCurrent = false; > } > > + if (!aIsPrivate && isNaN(aUserContextId) && getBoolPref("privacy.userContext.newTabInheritContext", false)) { Note that you're increasing the complexity of this function which is breaking eslint. If we go ahead with this approach, either avoid doing this or add an eslint ignore instruction that ignores the 'complexity' rule only for this function.

> Why add this? This doesn't unset the other preference, I think... Yeah this is because I'm not convinced that code is working correctly. I can raise another bug if you like. > This doesn't just affect the new tab button and ctrl-t, it also changes the behaviour when clicking on links in e.g. about:preferences, notification bars and other parts of the UI that (eventually) go through openLinkIn. Is that actually what we want here? I think this likely makes the most sense given that it feels all very much the same use-case. I'm happy to move this only to the use-cases we care about perhaps. > Note that you're increasing the complexity of this function which is breaking eslint I'm not sure how complexity gets calculated however I can make this a different function or something else to fix the complexity. I meant to note that failure here. I'll get Tanvi to confirm: https://bugzilla.mozilla.org/show_bug.cgi?id=1406371#c3

I mentioned this in IRC, but wanted to ping you here too. It looks like on update of addons we are flipping the prefs.

Whoops wrong bug sorry

Comment on attachment 8915987 [details] Bug 1406371 - Add preference to inherit the previous container when opening a tab without a container. https://reviewboard.mozilla.org/r/187278/#review193466 looks good to me, but apply the comments asked by Gijs

(In reply to :Gijs (blocked reviews; no response to ni until Nov 2 (PTO)) from comment #3) > ::: browser/base/content/utilityOverlay.js:272 > (Diff revision 1) > > + if (!aIsPrivate && isNaN(aUserContextId) && getBoolPref("privacy.userContext.newTabInheritContext", false)) { > > + let currentTargetBrowser = params.targetBrowser || w.gBrowser.selectedBrowser; > > + aUserContextId = w.gBrowser.getTabForBrowser(currentTargetBrowser).userContextId; > > + } > > This doesn't just affect the new tab button and ctrl-t, it also changes the > behaviour when clicking on links in e.g. about:preferences, notification > bars and other parts of the UI that (eventually) go through openLinkIn. Is > that actually what we want here? > Are we able to enumerate all the things that openLinkIn is used for? If we aren't - I think we should scope this to just Ctrl+T and the plus button. Link clicking within an container tab already inherits the container type. Related tabs already inherit the container type. If we are - we can quickly go through them and make sure inheriting makes sense for all of them. I don't want us to end up in a situation where we inherit in a case that we haven't thought through or shouldn't inherit.

baku is going to enumerate the things that openLinkIn does.

Would this also affect tabs opened from external applications? I would like for links opened from external applications to inherit the container of the last focused tab. However, I can see how some users would NOT like that behavior.

Comment on attachment 8915987 [details] Bug 1406371 - Add preference to inherit the previous container when opening a tab without a container. https://reviewboard.mozilla.org/r/187278/#review211176 ::: browser/base/content/utilityOverlay.js:272 (Diff revision 1) > + if (!aIsPrivate && isNaN(aUserContextId) && getBoolPref("privacy.userContext.newTabInheritContext", false)) { > + let currentTargetBrowser = params.targetBrowser || w.gBrowser.selectedBrowser; > + aUserContextId = w.gBrowser.getTabForBrowser(currentTargetBrowser).userContextId; > + } WE spoke about this but :baku feels happy about it. ::: browser/components/contextualidentity/test/browser/browser_newtabButton.js:110 (Diff revision 1) > + await SpecialPowers.pushPrefEnv({"set": [ > + ["privacy.userContext.enabled", true] > + ]}); This is because this test isn't running in non nightly mode due to the pref default.

Comment on attachment 8915987 [details] Bug 1406371 - Add preference to inherit the previous container when opening a tab without a container. https://reviewboard.mozilla.org/r/187278/#review212344 I don't really want to play bogeyman here repeatedly and I have raised this point before, so I guess rs=me in terms of the actual code, but I haven't really seen a convincing story (after Tanvi's question in comment 9) about why this is OK to do for all callers for openUILink and its friends. Generally, we use openUILink for any UI-opened pages. We use openLinkIn in even more cases. So, as a random example, my understanding is that after this patch and with the pref flipped, if you click "Help > Troubleshooting Information", we'll open about:support in the container matching the current tab. And maybe that's fine - it's an about: page, so who really cares, right? But it feels pretty weird (I would expect it to always open in the default container, all else being equal.). Same for any other links from the UI, especially ones that *aren't* about: pages but e.g. links to SUMO or whatever. I also imagine that some people try to use "one window per container" (so all tabs in a given window should have matching containers, but different windows have different containers). They want the original thing requested in comment #0, but after this patch and with the about:config pref flipped, new windows *also* inherit the container of the current tab, right? Which wouldn't be what those users want... It feels like we don't really have a clear story about how containers should/shouldn't follow other things (whether that's domains, link clicks, cross-origin navigation either in the same tab/window or different ones, etc. etc.), and as a result I am uncomfortable about this patch. The lack of a cohesive story makes it feel like we're adding options (default to off!) more or less whenever asked for, and we end up with a medley of related but subtly different ways of managing containers. Ultimately that makes it harder for users to understand and use containers as they like. We should have a clear idea (or clear set of ideas) about how containers are "meant" to be used and implement that without the need for users to touch about:config separately - not a bunch of different prefs that users have to experiment with until they get something they like.

I still think we need to go through the callers of openLinkIn and see if it makes sense to inherit in all of them. baku, were you still planning to enumerate these? Gijs - the most requested feature we have is for new tabs to inherit the current container type. This already works for link clicks and related tabs. I'd like to extend this to the plus button and Ctrl+T, as this is what the requesting users are probably asking for. I can see some that would also like it for bookmark or history clicks, but that action happens far less frequently then Ctrl+T/plus button. The goal of making the about:config pref is so that webextensions can then toggle that pref as they see fit. In the Firefox MultiAccount Containers addon, we plan to add inheritance as an option. Then users who want inheritence and flip it on through our UI (not about:config). Other addons can decide how they would like inheritence to work. I agree with Gijs that about: pages shoudl open in the default container. (in fact, maybe we need a separate bug to ensure that.) We should not change the behavior for all openLinkIn until we audit it. If inheritence makes sense in most or all openLinkIn entry points, then we can use this patch. If not, we need to scope this to Ctrl+T, plus button, and maybe bookmarks.

Note that there are a *lot* of callers for the open(UI?)Link(In?) set of functions. See e.g. the patch up on bug 1374741 which touches most (but not even all!) of them. If we think the default for [+] (and its shortcut) and maybe bookmarks should be to 'inherit' the user context ID, then personally I would suggest just updating the code that opens bookmarks and the code for the new tab command, pass a new arg to open*Link* and use that arg to determine that we need to "inherit" context. No pref. I'm pretty sure those callsites are outnumbered by other callsites where we don't want to do this. If we then decide there are other callsites that should have this behaviour, we can add the arg there. I would also suggest doing a follow-up bug to figure out new tab preloading if we do have containers, because I don't know how that would work. Perhaps start preloading when hovering the [+]. For people who have, say, 10 different containers, I don't think we should keep 10 preloaded new tab pages around... anyway, difficult problem, not necessary to fix that here, but one that we should bear in mind for performance reasons etc.

(In reply to :Gijs from comment #16) > Note that there are a *lot* of callers for the open(UI?)Link(In?) set of > functions. See e.g. the patch up on bug 1374741 which touches most (but not > even all!) of them. > > If we think the default for [+] (and its shortcut) and maybe bookmarks > should be to 'inherit' the user context ID, then personally I would suggest > just updating the code that opens bookmarks and the code for the new tab > command, pass a new arg to open*Link* and use that arg to determine that we > need to "inherit" context. No pref. I'm pretty sure those callsites are > outnumbered by other callsites where we don't want to do this. If we then > decide there are other callsites that should have this behaviour, we can add > the arg there. I agree with using the inherit behavior with just the plus and the Ctrl+T. Lets even leave it out of bookmarks for now. But we do need a pref. It seems that half the users prefer the current behavior and half the users really need different behavior. For example, imagine a user that just uses Containers for facebook in a single tab. They don't want new tabs to inherit their facebook container. On the other hand, imagine a user who uses Containers for "Work" during work hours. They have a window with various Work tabs. Every new tab they open, they want to be a Work tab. Jonathan, can you change the patch to add a pref to inherit for just plus and Ctrl+T? > > I would also suggest doing a follow-up bug to figure out new tab preloading > if we do have containers, because I don't know how that would work. Perhaps > start preloading when hovering the [+]. For people who have, say, 10 > different containers, I don't think we should keep 10 preloaded new tab > pages around... anyway, difficult problem, not necessary to fix that here, > but one that we should bear in mind for performance reasons etc. What do we preload when a user clicks the plus button? I'm not too familiar with how this works, but yes lets discuss it in next Containers meeting and create a new bug if necessary.

Has there been any progress on this? Jonathan?