1325014 - Creating a new related tab (middle/accel-click on new tab button) should inherit the current tab's container usercontextid

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Kestrel]

Middle/accel-clicking on links, the refresh button & forward/back buttons creates a related tab near the current tab which inherits its usercontextid so it would make sense to extend this to related tabs created by middle/accel-clicking on the new tab button.

Comment 1

[Kestrel]

Attachment: bug1325014.patch

BrowserOpenTab() should inherit selectedBrowser's usercontextid attribute for whereToOpenLink()'s tabshifted case. Will need help with a try push.

Comment 2

[:Gijs (he/him)]

Comment on attachment 8820616 [details] [diff] [review]
bug1325014.patch

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

Stealing.

::: browser/base/content/browser.js
@@ +1966,2 @@
>          relatedToCurrent = true;
> +        userContextId = gBrowser.selectedBrowser.getAttribute("usercontextid");

This should convert to a number.

@@ +1973,5 @@
>    }
>  
> +  openUILinkIn(BROWSER_NEW_TAB_URL, where, {
> +    relatedToCurrent,
> +    userContextId

Nit: should have a trailing comma.

Comment 3

[Kestrel]

Attachment: bug1325014v2.patch

Requested changes.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8821072 [details] [diff] [review]
bug1325014v2.patch

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

With the changes below, r=me

::: browser/base/content/browser.js
@@ +1938,5 @@
>  
>  function BrowserOpenTab(event) {
>    let where = "tab";
>    let relatedToCurrent = false;
> +  let userContextId = null;

This should default to 0, not null.

@@ +1952,2 @@
>          relatedToCurrent = true;
> +        userContextId = parseInt(gBrowser.selectedBrowser.getAttribute("usercontextid"));

Use either '|| 0', or use Number(...) instead of parseInt, because parseInt("") (if the attribute is not present) returns NaN.

Comment 5

[Tanvi Vyas[:tanvi]]

Before this lands, can you elaborate on the behavior?

What do you mean by related tabs created by middle/accel-clicking on the new tab button?  I'm not familiar with functionality.

When you hit the command button and click the plus button at the same time, I get a new tab next to my current tab (instead of at the end of the tab row).  Are you proposing that the newly created tab inherits the usercontextid from the active tab in that situation?


(In reply to Kestrel from comment #0)
> Middle/accel-clicking on links, the refresh button & forward/back buttons
> creates a related tab near the current tab which inherits its usercontextid
> so it would make sense to extend this to related tabs created by
> middle/accel-clicking on the new tab button.

Comment 6

[:Gijs (he/him)]

(In reply to Tanvi Vyas[:tanvi] from comment #5)
> Before this lands, can you elaborate on the behavior?
> 
> What do you mean by related tabs created by middle/accel-clicking on the new
> tab button?  I'm not familiar with functionality.

"Related tabs" is what happens if you start with e.g.:

/ Tab 1 \/ Tab 2 \/ Tab 3 \

and tab 2 selected, and you middle-click a link in tab 2 (or, as comment #0 pointed out, the refresh or back/fwd buttons), you will end up with:

/ Tab 1 \/ Tab 2 \/ New tab \/ Tab 3 \

rather than having the tab created at the end of the tabstrip.

We somewhat recently enabled this behaviour for the new tab button as well. If you middle click it, we will create the new tab immediately next to the currently selected tab - it is deemed "related" to the current tab.


> When you hit the command button and click the plus button at the same time,
> I get a new tab next to my current tab (instead of at the end of the tab
> row).  Are you proposing that the newly created tab inherits the
> usercontextid from the active tab in that situation?

Yes, because the same already happens for all the other cases described in comment #0 (middle- or accel-clicking links, refresh/back/fwd button).

Do you have concerns about this?

(leaving ni for Kestrel because the patch needs some small updates before landing still)

Comment 7

[Tanvi Vyas[:tanvi]]

Thanks Gijs for the clarification!  So this bug only adds: "the middle click on plus button opens a new tab next to the previous tab with an inherited usercontextid".  The back/front/refresh options already work that way.  Correct?

Comment 8

[:Gijs (he/him)]

(In reply to Tanvi Vyas - behind on bugmail [:tanvi] from comment #7)
> Thanks Gijs for the clarification!  So this bug only adds: "the middle click
> on plus button opens a new tab next to the previous tab with an inherited
> usercontextid".  The back/front/refresh options already work that way. 
> Correct?

Yup!

Comment 9

[Kestrel]

Attachment: bug1325014v3addtab.patch

Different approach. Searching highlighted text via the context menu _loadSearch() creates a related tab and should probably also inherit the usercontextid. Other cases may also arise, they could be treated independently but it seems more appropriate to move this further down into addTab() and make related tabs always inherit usercontextid when it is not specified.

If the user disables browser.tabs.insertRelatedAfterCurrent, container inheritance should still occur since this pref only controls positioning of related tabs.

One possible side effect is view-source: tabs inherit the current container, I'm not sure if this is a problem, if it is they could be explicitly created with the default container. There may be other edge cases with this more generalized approach (it passes the "contextualidentity" tests at least).

Comment 10

[:Gijs (he/him)]

Comment on attachment 8825354 [details] [diff] [review]
bug1325014v3addtab.patch

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

This seems sensible to me. view-source tabs *should* inherit userContextIds, because otherwise any requests necessary will be made in another usercontextid and lack the right cookies etc., so the result might be confusing.

Do you think you can write an additional test for this?

::: browser/base/content/tabbrowser.xml
@@ +2196,5 @@
>              if (aIsPrerendered) {
>                t.setAttribute("hidden", "true");
>              }
>  
> +            // Related tab inherits current tab's user context

Nit: append "unless a different usercontextid is specified" to the comment.

Comment 11

[Kestrel]

Attachment: bug1325014v4addtab.patch

Of course if view-source makes requests (which I now see that it does!), it should inherit the user context.

duplicateTab() already has a test that checks user context but the userContextId is supplied, removing it and relying on relatedToCurrent to inherit the user context would bring it into the test scope. However this would make the code less readable and it could be easily changed back so I think a dedicated test would be better.

Since this is a more general case, the test should focus on simply getting the right user context from addTab() rather than testing specific actions that produce related tabs.

I will need help with a try push.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8826107 [details] [diff] [review]
bug1325014v4addtab.patch

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

Perfect, thanks!

::: browser/components/contextualidentity/test/browser/browser_relatedTab.js
@@ +1,2 @@
> +/*
> + * Bug 1325014 - Adding tab related to current tab inherits current tab's container usercontextid unless otherwise specified

Nit: add "use strict" at the top of the file, please. :-)

Comment 13

[:Gijs (he/him)]

Attachment: Final patch

Updated patch with "use strict" included.

Trypush:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ed5606cd58f1e8becddda8ae335e0fc414f6e0a

Comment 14

[:Gijs (he/him)]

Try looks good. :-)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/456bf2ee3d1d
Adding tab related to current tab inherits current tab's container usercontextid unless otherwise specified. r=gijs

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/456bf2ee3d1d

Comment 17

[emil_prager]

(In reply to Pulsebot from comment #15)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/456bf2ee3d1d
> Adding tab related to current tab inherits current tab's container
> usercontextid unless otherwise specified. r=gijs

What exactly does it mean "unless otherwise specified"?
Is there a way by which we can override this behaviour? A new about:config property, perhaps?
Thanks!

Comment 18

[emil_prager]

(In reply to Kestrel from comment #0)
> Middle/accel-clicking on links, the refresh button & forward/back buttons
> creates a related tab near the current tab which inherits its usercontextid
> so it would make sense to extend this to related tabs created by
> middle/accel-clicking on the new tab button.

Actually this was a feature, not a bug or limitation, from my (and other users') perspective at least.
And it was consistent with the other Mozilla privacy (tracking limitation) efforts.
Perhaps the introduction of such changes should be preceded by an experiment asking the users about their opinion on that change.

Use case description:
For example, it was an advantage to: 
- not have to copy the selected text from a page opened in a Personal container tab (e.g.: facebook) 
- and to open a No Container tab (possibly very far from the original one, because of not solving the issue with ID 528005) just to paste it there in order to be searched. 
That's because in the Personal container tabs I might be authenticated in my personal Google account and I don't want necessarily that _all_ my Google searches to be remembered, but only some of them, at my choice.
(Of course, the same stands for other container tabs combinations.)

Can this behavior be overridden?
Do you plan to add new about:config property? Or perhaps suggest a extension for this?
Thank you!

Comment 19

[Kestrel]

(In reply to emil_prager from comment #18)
> That's because in the Personal container tabs I might be authenticated in my
> personal Google account and I don't want necessarily that _all_ my Google
> searches to be remembered, but only some of them, at my choice.

I did consider this use case but containers must be contained by default. Searches can't be allowed to implicitly leak into a different container, if you want to escape a container you need to make a conscious effort. Drag & drop already works this way (Bug 1237077), try dragging a link or selected text into the tab strip between tabs (which will do a search) and watch the created tab inherit the current tab's container.

To create a related tab with the container of your choice you can right-click a link and choose "Open Link in New Container Tab". You can also long press the New Tab button but currently middle-click doesn't work here for creating related container tabs which is something I'm looking at fixing.

The search context menu could potentially let you choose which container to do the search in. Recent web extension work supporting contextual identities would allow something like this (Bug 1302697).

As a workaround you could try pinning a tab with the desired search engine container and then drag & drop selected text on to it to search. If you want new tabs you can drag over the pinned tab first to make it current and then drop in the tab strip where you want the new tab and it will inherit the pinned tab's container and do a search.

There are definitely ways of improving the container UI and making common tasks more streamlined.

Comment 20

[:Gijs (he/him)]

(In reply to Kestrel from comment #19)
> To create a related tab with the container of your choice you can
> right-click a link and choose "Open Link in New Container Tab". You can also
> long press the New Tab button but currently middle-click doesn't work here
> for creating related container tabs which is something I'm looking at fixing.

This sounds sensible to fix - is there a bug on file for this work?

Comment 21

[Kestrel]

(In reply to :Gijs from comment #20)
> is there a bug on file for this work?

Bug 1331595