Steps to repeat: 1) Open Bugzilla in a work container 2) open related bug in a new tab 3) realize the new tab is not logged in. can not comment. I would expect Firefox to defaulted to same-container when opening new tabs that visit the same origin. I know this is tricky to get right. So I want to focus on the same-origin case first. (For other origins, especially those already opened in another container, the user might also want to use the existing session. But they also might really, really not. Let's do this ina a follow-up bug.)

Baku asked me to NI him, forgot to set this.

I think we should open the same container in case the origin is the same. Tanvi, feedback? I remember we had a similar feature, could it be that is a regression?

Comment on attachment 8758310 [details] [diff] [review] right_click.patch Review of attachment 8758310 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +5512,5 @@ > + > + if (doc.nodePrincipal.originAttributes.userContextId) { > + // If the 2 URIs have the same origin, we use the same userContextId, > + // otherwise we disable the referrer. > + if (makeURI(href).prePath == referrerURI.prePath) { makeURI can return null, in which case this will throw, which needs fixing here and in ContentClick.jsm. This also won't work well for pages that open about:blank or open a data or javascript URI. It's also fragile for e.g. subdomains (so if I go from foo.com to tickets.foo.com, do I really want to break out of the usercontextid?). Why are we using origins in this way? ::: browser/modules/ContentClick.jsm @@ +42,5 @@ > } > return; > } > > + let uri = Services.io.newURI(json.href, null, null); This can throw, so please deal with that. @@ +87,5 @@ > > + if (json.originAttributes.userContextId) { > + // If the 2 URIs have the same origin, we use the same userContextId, > + // otherwise we disable the referrer. > + if (uri.prePath == browser.documentURI.prePath) { This will then need to deal with the null uri case - but as noted in the non-e10s code, not sure why we're doing this this way.

Bram, we need your help! Here the problem: Tab A (Container 1) has a link. The user does middle click on a link. Firefox opens a tab in background with the selected URL. Which UserContextId should we use? There are many options: 1. same Container always. With referrer. 2. default container always... (this is the current setup). No referrer. 3. if the host/origin is the same we use the same container (with referrer), otherwise we use the default one (no referrer). My patch implements 3, but it's easy to change. If you like 3, what about if tab1 loads 'www.foobar.com' and the link is 'ticket.foobar.com' ?

P1 because of referrer leakage.

The referrer leakage is a bigger and separate problem. I filed bug 1277765 for that. About this bug: Bram, note that from the context menu, "Open link in a New Tab" opens a tab using the default container. So, if we want to be consistent with the context menu, we should mark this bug as 'wontfix'.

One might also consider "fixing" this by allowing a shortcut "Open link in a New <type> Tab" where <type> is the current container, including color and icon. One could still find the other container types behind the nested selection of all containers that already exists.

Frederik Braun’s suggestion here aligns with what I’ve suggested over email in a related problem: When I’m in container ‘foo’: * cmd+click opens the link in container ‘foo’ * Right click produces these options (in this order) * Open Link in a New ‘foo’ Tab * Open Link in a New Tab * Open Link in a New Container Tab * containername2 * containername3 * (‘foo’ is not listed, because I’m currently on ‘foo’ container’) This means that the menu changes depending on the containers you’re currently in. What do you think?

In order to apply this patch, you must apply the dependence as well: bug 1277765

Comment on attachment 8760742 [details] [diff] [review] link.patch Review of attachment 8760742 [details] [diff] [review]: ----------------------------------------------------------------- Can I make an Nth suggestion? If I have a link in a particular container tab, and open it in a new tab / window / whatever, I would still expect this to be using the same container. If I want to open it somewhere else (different container / no container), that should be the non-default / additional menu option. Much like e.g. in a private window, all the modifier-click and context menu click options will keep the link private. Taking myself out of that state requires the jumping through hoops, not the other way around. I think that behaviour should govern containers as well. Either way, there's a number of issues below, so no r+ on account of those issues. ::: browser/base/content/browser-context.inc @@ +53,5 @@ > <menuitem id="context-openlinkincurrent" > label="&openLinkCmdInCurrent.label;" > accesskey="&openLinkCmdInCurrent.accesskey;" > oncommand="gContextMenu.openLinkInCurrent();"/> > +# label and usercontextid are dinamically set. Nit: dynamically @@ +68,5 @@ > label="&openLinkCmdInContainerTab.label;" > accesskey="&openLinkCmdInContainerTab.accesskey;" > hidden="true"> > <menupopup oncommand="gContextMenu.openLinkInTab(event);" > + onpopupshowing="return gContextMenu.createUserContextMenu(event);" /> I'm not an amazing fan of the abstraction of what this does into an identically named function on gContextMenu that does something different from the version in utilityOverlay.js Don't we want the same change for the other menus like this one anyway, if this is the UI we're pursuing? ::: browser/base/content/utilityOverlay.js @@ +408,5 @@ > > // Populate a menu with user-context menu items. This method should be called > // by onpopupshowing passing the event as first argument. addCommandAttribute > // param is used to set the 'command' attribute in the new menuitem elements. > +function createUserContextMenu(event, addCommandAttribute = true, excludeUserContextId = 0) { This conflicts with the work in bug 1272416. ::: browser/locales/en-US/chrome/browser/browser.dtd @@ +447,5 @@ > <!ENTITY searchSetAsDefault.accesskey "D"> > > <!ENTITY openLinkCmdInTab.label "Open Link in New Tab"> > <!ENTITY openLinkCmdInTab.accesskey "T"> > +<!ENTITY openLinkCmdInSameContainerTab.accesskey "O"> This conflicts with the access key for 'open link', a few lines below.

(In reply to :Gijs Kruitbosch from comment #11) > Comment on attachment 8760742 [details] [diff] [review] > link.patch > > Review of attachment 8760742 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can I make an Nth suggestion? If I have a link in a particular container > tab, and open it in a new tab / window / whatever, I would still expect this > to be using the same container. If I want to open it somewhere else > (different container / no container), that should be the non-default / > additional menu option. Much like e.g. in a private window, all the > modifier-click and context menu click options will keep the link private. > Taking myself out of that state requires the jumping through hoops, not the > other way around. I think that behaviour should govern containers as well. --> ni bram.

> > Can I make an Nth suggestion? This is what I implemented. Am I wrong? By default, right click, or context-menu (first item) is: Open link in the 'FooBar' container. So we keep the same container, always.

> > <!ENTITY openLinkCmdInTab.label "Open Link in New Tab"> > > <!ENTITY openLinkCmdInTab.accesskey "T"> > > +<!ENTITY openLinkCmdInSameContainerTab.accesskey "O"> > > This conflicts with the access key for 'open link', a few lines below. I invented it :) I should ask bram for this. Bram what is the best accessKey for this new 'Open link in a new 'Foobar' Tab' entry in the context menu?

> Don't we want the same change for the other menus like this one anyway, if > this is the UI we're pursuing? This is what we want only for the context menu. The other menus are kept as they are so far.

(In reply to :Gijs Kruitbosch from comment #11) > Much like e.g. in a private window, all the > modifier-click and context menu click options will keep the link private. > […] I think that behaviour should govern containers as well. This sounds logical and reasonable. +1. My original thinking was to make same-origin links open in the same container, while links that point to different domains should open elsewhere. But this gets complex. For example, what if the domain being opened has been opened before in the other container? Following the Private Browsing behaviour makes it somewhat easier to think about. All links open in the same container by default, and if I want it to open in a different container (including the default container), I need to right-click and select which container I’d like to open it with. For what it’s worth, I think that there should be a way to make container limited to a domain or a certain set of domains (although this is a hard problem to solve). For example, on my home session, I want to be signed into both google.com and youtube.com. On my work session, I only want to be signed into google.com. One day, my colleague sends me a YouTube link on my work email. The ideal situation is for Firefox to always open the right site in the right session, with little to no tweaking on my part.

(In reply to Andrea Marchesini (:baku) from comment #14) > > > <!ENTITY openLinkCmdInTab.label "Open Link in New Tab"> > > > <!ENTITY openLinkCmdInTab.accesskey "T"> > > > +<!ENTITY openLinkCmdInSameContainerTab.accesskey "O"> > > > > This conflicts with the access key for 'open link', a few lines below. > > I invented it :) I should ask bram for this. Bram what is the best accessKey > for this new 'Open link in a new 'Foobar' Tab' entry in the context menu? Is it possible for the access key to change based on my current container? If I’m currently inside ‘foo’ container, then the access key for “Open link in a new ‘foo’ Tab” is T. If I’m inside the default container, then the access key for “Open link in a new Tab” is T.

(In reply to Andrea Marchesini (:baku) from comment #13) > > > Can I make an Nth suggestion? > > This is what I implemented. Am I wrong? By default, right click, or > context-menu (first item) is: Open link in the 'FooBar' container. So we > keep the same container, always. I meant that the item labeled "open link in new tab" should do what your new item "open link in the foobar container" does, and we shouldn't add new menuitems. Additionally, your patch hasn't changed the behaviour for ctrl/alt/meta-click and middleclick.

> Additionally, your patch hasn't changed the behaviour for > ctrl/alt/meta-click and middleclick. This is done in bug 1277765

Discussing this issue with Bram, we decided to go for this: If you are not in a container: Open link in a new Tab Open link in a container tab -> submenu [ list of containers ] If you are in a container: Open link in a new '%S' Tab Open link in a container tab -> submenu [ no-container | list of containers ] This patch implements this feature.

I can confirm this implementation. This achieves the best compromise of keeping the context menu clean and making ‘open in same container’ a default behaviour, while still giving ‘no container’ a place to exist. Andrea, the wording for the submenu item should be “No Container” to make it consistent with the rest of the items. Capitalised start of the letter, and no dash. What do you think?

userContextNone.label = No Container Already done :)

Comment on attachment 8761930 [details] [diff] [review] link.patch Review of attachment 8761930 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/nsContextMenu.js @@ +162,4 @@ > var shouldShow = this.onSaveableLink || isMailtoInternal || this.onPlainTextLink; > var isWindowPrivate = PrivateBrowsingUtils.isWindowPrivate(window); > var showContainers = Services.prefs.getBoolPref("privacy.userContext.enabled"); > + Nit: Don't add a newline please. ::: browser/base/content/utilityOverlay.js @@ +422,5 @@ > > let bundle = document.getElementById("bundle_browser"); > let docfrag = document.createDocumentFragment(); > > + // If we are exlucing a userContextId, we want to add a 'no-container' item. Nit: excluding @@ +429,5 @@ > + menuitem.setAttribute("usercontextid", "0"); > + menuitem.setAttribute("label", bundle.getString("userContextNone.label")); > + menuitem.setAttribute("accesskey", bundle.getString("userContextNone.accesskey")); > + > + // We don't set oncommand/command attribute because if we have to exlclude nit: "an oncommand/command attribute" nit: "exclude" @@ +430,5 @@ > + menuitem.setAttribute("label", bundle.getString("userContextNone.label")); > + menuitem.setAttribute("accesskey", bundle.getString("userContextNone.accesskey")); > + > + // We don't set oncommand/command attribute because if we have to exlclude > + // a userContextId we are generating the contextMenu for the rightclick and nit: "generating a context menu", leave out "for the rightclick" (right click is implied when talking about a context menu)

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/20ea7d3d347f 'Open link in a new <container_name> Tab' in the context menu, r=gijs

Backed out for failing browser-chrome test browser_referrer_open_link_in_container_tab2.js: https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0a3fa38dc2 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=20ea7d3d347f575315495d90e5bf571ed734c6ea Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=29968656&repo=mozilla-inbound 06:09:41 INFO - 81 INFO browser_referrer_open_link_in_container_tab: policy=[undefined] rel=[undefined] http:// -> http:// 06:09:41 INFO - 82 INFO TEST-PASS | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js | We have a menupopup. - 06:09:41 INFO - 83 INFO TEST-PASS | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js | We have a first container entry. - 06:09:41 INFO - 84 INFO TEST-PASS | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js | We have a first container entry. - 06:09:41 INFO - 85 INFO TEST-PASS | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js | We have a usercontextid value. - 06:09:41 INFO - 86 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js | We have the right usercontextid value. - Got 1, expected 0 06:09:41 INFO - Stack trace: 06:09:41 INFO - chrome://mochikit/content/browser-test.js:test_is:967 06:09:41 INFO - chrome://mochitests/content/browser/browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js:onPopupShown:29

(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #25) > Backed out for failing browser-chrome test > browser_referrer_open_link_in_container_tab2.js: > > https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0a3fa38dc2 > > Push with failures: > https://treeherder.mozilla.org/#/jobs?repo=mozilla- > inbound&revision=20ea7d3d347f575315495d90e5bf571ed734c6ea > Failure log: > https://treeherder.mozilla.org/logviewer.html#?job_id=29968656&repo=mozilla- > inbound > > 06:09:41 INFO - 81 INFO browser_referrer_open_link_in_container_tab: > policy=[undefined] rel=[undefined] http:// -> http:// > 06:09:41 INFO - 82 INFO TEST-PASS | > browser/base/content/test/referrer/ > browser_referrer_open_link_in_container_tab2.js | We have a menupopup. - > 06:09:41 INFO - 83 INFO TEST-PASS | > browser/base/content/test/referrer/ > browser_referrer_open_link_in_container_tab2.js | We have a first container > entry. - > 06:09:41 INFO - 84 INFO TEST-PASS | > browser/base/content/test/referrer/ > browser_referrer_open_link_in_container_tab2.js | We have a first container > entry. - > 06:09:41 INFO - 85 INFO TEST-PASS | > browser/base/content/test/referrer/ > browser_referrer_open_link_in_container_tab2.js | We have a usercontextid > value. - > 06:09:41 INFO - 86 INFO TEST-UNEXPECTED-FAIL | > browser/base/content/test/referrer/ > browser_referrer_open_link_in_container_tab2.js | We have the right > usercontextid value. - Got 1, expected 0 > 06:09:41 INFO - Stack trace: > 06:09:41 INFO - chrome://mochikit/content/browser-test.js:test_is:967 > 06:09:41 INFO - > chrome://mochitests/content/browser/browser/base/content/test/referrer/ > browser_referrer_open_link_in_container_tab2.js:onPopupShown:29 And also, I just realized, I would expect this to start failing the context menu tests when we flip the switch on usercontextid stuff. And/or, ideally, we should add a test to that set of tests to verify that this works correctly.

userContextOpenLink.label = Open Link in New %S Tab If you plan to replace %S with one of the possible containers (e.g. "Open Link in New Shopping Tab"), this is all but localizable, with issues varying from capitalization (minor) to different grammar case.

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6a6aa8a42eb1 'Open link in a new <container_name> Tab' in the context menu, r=gijs

> If you plan to replace %S with one of the possible containers (e.g. "Open > Link in New Shopping Tab"), this is all but localizable, with issues varying > from capitalization (minor) to different grammar case. We also want to support custom containers. At this point the name will not be localized.

Sounds like something we want to uplift for test pilot in 49.

(In reply to Paul Theriault [:pauljt] from comment #31) > Sounds like something we want to uplift for test pilot in 49. The patch landed in this bug has new strings, not really happy to break string freeze so late in the aurora cycle. Also, if I understand it correctly, this feature is not enabled by default in 49?

baku, can we uplift this patch without a string change? Can the string change part be taken out for the uplift?

The string is "Open Link in New %S Tab" and it's quite important for this bug. I cannot uplift this patch without it. Francesco, what do you suggest here?

My suggestion would be to let it ride the trains with 50, especially considering the feature is preffed off outside of Nightly, and I haven't heard of any plans to change that decision. The only reason to uplift this "as is" in mozilla-aurora is if we plan to enable the feature in 49.

The reason for uplift is that we want to use it for Test Pilot in Firefox 49. In Test Pilot, we can use an addon that does UI changes, but it cannot make platform changes. Since this bug has platform changes, we want to uplift to 49. Is there a way that we can skip localizing this string for 49 and just localize for 50 while still uplifting the code? Thanks!

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #36) > Is there a way that we can skip localizing this string for 49 > and just localize for 50 while still uplifting the code? Create an ad-hoc version of the patch with strings hard-coded (not exposed in .properties file). Having one string in English doesn't seem an issue for international Test Pilot users, given that all experiments have been running only in English.

Andrea, can I ask for you to make a patch as per comment 37 for uplift?

Approval Request Comment [Feature/regressing bug #]: Containers [User impact if declined]: an important feature will be missing for test-pilot. [Describe test coverage new/current, TreeHerder]: it's green on m-i [Risks and why]: none, this feature is not exposed to content [String/UUID change made/needed]: none

Comment on attachment 8769565 [details] [diff] [review] patch for m-a Part of Test Pilot work, please uplift.

patch does not apply cleanly to aurora: (eg '1-3,5', or 's' to toggle the sort order between id & patch description) 2 adding 1276880 to series file renamed 1276880 -> link.patch applying link.patch patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh link.patch