1331595 - The new tab button's container menu should allow middle/accel-click to create new related tabs with the desired container

Status: ASSIGNED

(Firefox :: Tabbed Browser, defect, P3)

Description

[Kestrel]

Currently, opening the new tab button's container menu and then middle-clicking on a menu item creates a related tab to the current tab but not with the desired container. Accel-clicking creates the desired container but the new tab is not related to the current tab.

Middle/accel-clicking in the container menu should open a new tab related to the current tab similar to middle/accel-clicking on the new tab button but with the container of choice. This is a useful way of creating related tabs without inheriting the current tab's container (Bug 1325014).

"No Container" should always appear on the list so it can be middle/accel-clicked to create a related tab with the default container. The new setting privacy.userContext.longPressBehavior = 1 (Bug 1328756) also needs this to allow default containers to be created since clicking on the new tab button opens the container menu rather than creating a new tab.

Comment 1

[Kestrel]

Attachment: bug1331595.patch

Proposed patch.

I want to expand the tests to include "No container" (i=0) but the new tab promise isn't being completed for some reason.

Comment 2

[:Gijs (out for now; he/him)]

Comment on attachment 8827411 [details] [diff] [review]
bug1331595.patch

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

Thanks for diving into this! This looks like it's heading in the right direction - please get review from :tanvi as well for this change.

::: browser/base/content/browser.js
@@ +4151,5 @@
>  function openNewUserContextTab(event) {
> +  let relatedToCurrent = false;
> +  let userContextId = 0;
> +
> +  if (event) {

Under what circumstances do we not get an event?

::: browser/base/content/utilityOverlay.js
@@ +455,5 @@
>  
>    let bundle = document.getElementById("bundle_browser");
>    let docfrag = document.createDocumentFragment();
>  
> +  // If we are not generating a context menu or excluding a userContextId, we want

Can we be more explicit about what "not generating a context menu" means, ie make it a positive statement like "if we are generating the new tab button's dropdown menu, ..."

::: browser/components/contextualidentity/test/browser/browser_newtabButton.js
@@ +109,5 @@
> +  let popup = document.getAnonymousElementByAttribute(newTab, "anonid", "newtab-popup");
> +
> +  for (let i = 1; i <= 4; i++) {
> +    let popupShownPromise = BrowserTestUtils.waitForEvent(popup, "popupshown");
> +    EventUtils.synthesizeMouseAtCenter(newTabButton, {type: "mousedown"});

Don't you want a click here?

@@ +119,5 @@
> +
> +    let waitForTabPromise = BrowserTestUtils.waitForNewTab(gBrowser);
> +    EventUtils.synthesizeMouseAtCenter(contextIdItem, {button: 1});
> +
> +    let tab = yield waitForTabPromise;

https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#277-278

is why this does not work for i=0, I suspect. We will have preloaded the about:newtab instance and so we won't get an onLocationChange event, I think.

Maybe just wait for TabOpen using BTU.waitForEvent? Then you get a tab, so you can just immediately check the uci.

@@ +121,5 @@
> +    EventUtils.synthesizeMouseAtCenter(contextIdItem, {button: 1});
> +
> +    let tab = yield waitForTabPromise;
> +
> +    is(tab.getAttribute('usercontextid'), i, `New tab has UCI equal ${i}`);

Nit: double quotes here and elsewhere. Also, I thought is() does strict comparisons, so then we should be template-quoting i for the comparison, right?

Comment 3

[Tanvi Vyas[:tanvi]]

Wasn't this done in https://bugzilla.mozilla.org/show_bug.cgi?id=1325014.  How is this different/

Comment 4

[:Gijs (out for now; he/him)]

(In reply to Tanvi Vyas - not reading bugmail, PM me [:tanvi] from comment #3)
> Wasn't this done in https://bugzilla.mozilla.org/show_bug.cgi?id=1325014. 
> How is this different/

This bug is about the items in the dropdown menu.