Closed Bug 1325014 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: ke5trel, Assigned: ke5trel)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [userContextId])

Attachments

(1 file, 4 obsolete files)

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.
Depends on: 528005
Whiteboard: [userContextId]
Attached patch bug1325014.patch (obsolete) — Splinter Review
BrowserOpenTab() should inherit selectedBrowser's usercontextid attribute for whereToOpenLink()'s tabshifted case. Will need help with a try push.
Attachment #8820616 - Flags: review?(dao+bmo)
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.
Attachment #8820616 - Flags: review?(dao+bmo)
Assignee: nobody → kestrel
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch bug1325014v2.patch (obsolete) — Splinter Review
Requested changes.
Attachment #8820616 - Attachment is obsolete: true
Attachment #8821072 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8821072 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Flags: needinfo?(kestrel)
(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)
Flags: needinfo?(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?
Flags: needinfo?(tanvi)
(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!
Attached patch bug1325014v3addtab.patch (obsolete) — Splinter Review
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).
Attachment #8821072 - Attachment is obsolete: true
Flags: needinfo?(kestrel)
Attachment #8825354 - Flags: feedback?(gijskruitbosch+bugs)
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.
Attachment #8825354 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Attached patch bug1325014v4addtab.patch (obsolete) — Splinter Review
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.
Attachment #8825354 - Attachment is obsolete: true
Attachment #8826107 - Flags: review?(gijskruitbosch+bugs)
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. :-)
Attachment #8826107 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch Final patchSplinter Review
Updated patch with "use strict" included.

Trypush:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ed5606cd58f1e8becddda8ae335e0fc414f6e0a
Attachment #8826107 - Attachment is obsolete: true
Attachment #8826278 - Flags: review+
Try looks good. :-)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/456bf2ee3d1d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(In reply to Pulsebot from comment #15)
> Pushed by ryanvm@gmail.com:
> 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!
Flags: needinfo?(kestrel)
(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!
(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.
Flags: needinfo?(kestrel)
(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?
Flags: needinfo?(kestrel)
(In reply to :Gijs from comment #20)
> is there a bug on file for this work?

Bug 1331595
Flags: needinfo?(kestrel)
Depends on: 1355433
You need to log in before you can comment on or make changes to this bug.