1330745 - When privacy.userContext.longPressBehavior is set to 1, it's impossible to open a default container tab from the + button.

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P3)

Description

[Andrea Marchesini [:baku]]

We need some UX here. I suggest to add an item has we did in the Context menu.

Comment 1

[Kamil Jozwiak [:kjozwiak]]

Adding STR in case someone from the community decides to verify this bug once it's fixed.

* launch the latest version of nightly
* load about:config and set the "privacy.userContext.longPressBehavior" to 1
* Click on the "+" button to open a new tab

When the container submenu appears, you'll notice that there's no way to open a regular container using this method.

Comment 2

[Kamil Jozwiak [:kjozwiak]]

(In reply to Andrea Marchesini [:baku] from comment #0)
> We need some UX here. I suggest to add an item has we did in the Context
> menu.

Another possible solution is making the "+" button open a default container once it's clicked on the second time around once the container menu has appeared. Example:

* user clicks on the "+" while privacy.userContext.longPressBehavior;1
* the container menu under the "+" button instantly appears
* user clicks on the "+" the second time around and a new default tab is created

Making this a P3 as the user has to manually change the pref via about:containers.

Comment 3

[Firefox Bug Husbandry Bot]

Bulk change per https://bugzilla.mozilla.org/show_bug.cgi?id=1401020

Comment 5

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1330745 - Add 'No Container' when longPressBehaviour is 1 for containers new tab context menu.

Review commit: https://reviewboard.mozilla.org/r/183208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183208/

Comment 6

[Jonathan Kingston [:jkt] he/him]

We can change the wording to "Default Tab" or whatever in a follow up commit if we need it. However given that this isn't ever default behavior or exposed to any users I think this is fine for now.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8911787 [details]
Bug 1330745 - Add 'No Container' when longPressBehaviour is 1 for containers new tab context menu.

https://reviewboard.mozilla.org/r/183208/#review188410

::: browser/components/contextualidentity/test/browser/browser_newtabButton.js:58
(Diff revision 1)
> +  EventUtils.synthesizeMouseAtCenter(newTabButton, {type: "mousedown"});
> +  await popupShownPromise;
> +  let contextIdItems = popup.querySelectorAll("menuitem");
> +  // 4 + default + manage containers
> +  is(contextIdItems.length, 6, "Has 6 menu items");
> +  console.log(contextIdItems[0].label);

Leftover debugging code.

::: browser/components/contextualidentity/test/browser/browser_newtabButton.js:76
(Diff revision 1)
> +      // waitForNewTab doesn't work as the location doesn't change
> +      waitForTabPromise = new Promise((resolve, reject) => {
> +        gBrowser.tabContainer.addEventListener("TabOpen", function(openEvent) {
> +          resolve(openEvent.target);
> +        });
> +      });

This should use `waitForEvent`, and it's also leaking the listener right now. Why does `waitForNewTab` work in the default case, but not in the container case? The previous code only used non-default tabs and those used `waitForNewTab`...

If the `waitForEvent` code works for the default case too, just use that one?

Comment 8

[Jonathan Kingston [:jkt] he/him]

waitForNewTab doesn't work in default case, the event is handled differently in the default case the waitForNewTab mentions this in their comment.

Comment 9

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8911787 [details]
Bug 1330745 - Add 'No Container' when longPressBehaviour is 1 for containers new tab context menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183208/diff/1-2/

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8911787 [details]
Bug 1330745 - Add 'No Container' when longPressBehaviour is 1 for containers new tab context menu.

https://reviewboard.mozilla.org/r/183208/#review189234

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/193675c58378
Add 'No Container' when longPressBehaviour is 1 for containers new tab context menu. r=baku,Gijs

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/193675c58378

Comment 13

[:Gijs (he/him)]

Should we uplift this to 57?

Comment 14

[Jonathan Kingston [:jkt] he/him]

No, I don't think it's worth it despite the low risk.

Comment 15

[Jonathan Kingston [:jkt] he/him]

Tanvi asked me to explain the pref values:

0 - is the default behaviour, containers menus are never created and show
1 - the menu is shown on first click, including the item "No Container" with a line separator to the other containers (exactly like how right clicking on a link does)
2 - Doesn't show the "No Container" option or separator and has a 500ms pause before showing the menu in which time the user can stop holding and instead create a "no container" tab.

There is also a line separator in the menu and "Manage Containers" as the last items in both 1,2 cases.

Also 2 will open like the back button does when you click and drag down into the menu without waiting the 500ms.