1272416 - Down arrow: If there aren't too many tabs open, put the new container tab options into the main menu

Status: RESOLVED WONTFIX

(Core :: DOM: Security, defect, P2)

Description

[Tanvi Vyas[:tanvi]]

The down arrow will have an Open tab in new container option.  That will have a submenu where you choose the type of container.  This is implemented in Bug 1233886.

But if there aren't too many tabs open, and there is nothing else in the down arrow, we don't need to use a submenu.  The new container tab options can be in the main menu.

+++ This bug was initially created as a clone of Bug #1233886 +++

Comment 1

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

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

Comment 2

[Jonathan Kingston [:jkt] he/him]

Hey Gijs,

I was told you might want additional review of this, which makes sense as it was a chunk of UX work. I have pushed an attached patch but I'm working on some tests right now for it.

Comment 3

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/56546/#review53160

I'm happy to do a proper review when this is finished, but when I spoke to baku about this it was mostly about the UX/UI side of this. I'm concerned about the implications in that domain more than as to how to implement this technically. Without usercontextid being enabled, I see effectively little to no benefit to having an extra UI element in the tab strip, especially if it provides no functionality (with only 1-2 tabs and no closed tabs, nothing in the all tabs menu is really useful). AIUI the trend has been to be more minimalist rather than to add more widgets/frobs/buttons in the toolbox.

If the only reason we're doing this is so we can do something clever when usercontextid is enabled and the user presses and holds accel-t, then it seems like we could just drop down only the tab options off the [+] button if the all tabs button is hidden. That would certainly be a less disruptive change, and would save us all the code in this patch...

So, if it hasn't happened already, I think it would be good for Stephen to have a look. (I'd suggest Philipp but I think he's busy with Tofino stuff.)

::: browser/base/content/tabbrowser.xml:4881
(Diff revision 1)
>                             onclick="checkForMiddleClick(this, event);"
>                             onmouseover="document.getBindingParent(this)._enterNewTab();"
>                             onmouseout="document.getBindingParent(this)._leaveNewTab();"
>                             tooltip="dynamic-shortcut-tooltip"/>
> +
> +        <xul:toolbarbutton id="tabs-alltabs-button"

You can't/shouldn't add items with an ID into an XBL anon binding, which will require quite some reworking of this patch, unfortunately. :-(

Comment 4

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

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

Comment 5

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

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

Comment 6

[Tanvi Vyas[:tanvi]]

Hi Gijs,

Bram is our UX designer for Containers.  With his help, we have added an open container tab option to the drop down arrow when containers is enabled.  But the drop down arrow only appears when there are a lot of tabs open.  So Bram said that we could add the drop down arrow next to the plus sign when containers is enabled:
https://bug1274246.bmoattachments.org/attachment.cgi?id=8757735
https://bugzilla.mozilla.org/show_bug.cgi?id=1274246#c4

When containers is not enabled, the drop down arrow won't appear when there are not enough tabs open.

Using the plus sign instead for this few-tabs-open option isn't great.  Some users may use containers on occassion or rarely.  They want the plus button to still easily open a new default tab.  Providing a submenu under the plus button means they need to do two clicks instead of one.  Other users will use containers frequently and want an easy access point to open them (as was suggested by some user testing we did).  Hence, to balance these two needs, we decided to use the drop down arrow instead of the plus sign.  The keyboard shortcut is a separate bug (1245262) that builds on top of the down arrow containers menu.

I hope this answers your questions, but we can also needinfo Bram if you have more.


(In reply to :Gijs Kruitbosch from comment #3)
> https://reviewboard.mozilla.org/r/56546/#review53160
> 
> I'm happy to do a proper review when this is finished, but when I spoke to
> baku about this it was mostly about the UX/UI side of this. I'm concerned
> about the implications in that domain more than as to how to implement this
> technically. Without usercontextid being enabled, I see effectively little
> to no benefit to having an extra UI element in the tab strip, especially if
> it provides no functionality (with only 1-2 tabs and no closed tabs, nothing
> in the all tabs menu is really useful). AIUI the trend has been to be more
> minimalist rather than to add more widgets/frobs/buttons in the toolbox.
> 
> If the only reason we're doing this is so we can do something clever when
> usercontextid is enabled and the user presses and holds accel-t, then it
> seems like we could just drop down only the tab options off the [+] button
> if the all tabs button is hidden. That would certainly be a less disruptive
> change, and would save us all the code in this patch...
> 
> So, if it hasn't happened already, I think it would be good for Stephen to
> have a look. (I'd suggest Philipp but I think he's busy with Tofino stuff.)
>

Comment 7

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56546/diff/3-4/

Comment 8

[:Gijs (he/him)]

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #6)
> Hi Gijs,
> 
> Bram is our UX designer for Containers.  With his help, we have added an
> open container tab option to the drop down arrow when containers is enabled.
> But the drop down arrow only appears when there are a lot of tabs open.  So
> Bram said that we could add the drop down arrow next to the plus sign when
> containers is enabled:
> https://bug1274246.bmoattachments.org/attachment.cgi?id=8757735
> https://bugzilla.mozilla.org/show_bug.cgi?id=1274246#c4
> 
> When containers is not enabled, the drop down arrow won't appear when there
> are not enough tabs open.

But we aim to eventually ship containers to everyone, right? Saying "it's behind a pref now" or "it's only on nightly for now" doesn't make the actual details of the plan any better or worse if we do intend to ship it. :-)

> Using the plus sign instead for this few-tabs-open option isn't great.  Some
> users may use containers on occassion or rarely.  They want the plus button
> to still easily open a new default tab.

I don't think we want to use it as a click target. If people want a permanent click target, they can use the specific toolbar button we made for containers (bug 1269029). They can put that in the tabstrip if they want to, or we can put it there if we think it should be there permanently if containers are enabled.

> Providing a submenu under the plus
> button means they need to do two clicks instead of one.  Other users will
> use containers frequently and want an easy access point to open them (as was
> suggested by some user testing we did). 

Right, I agree we shouldn't make the [+] button do something else on click. I just disagree that that means we need to make the dropdown arrow permanently visible in order for users to have a mouse or keyboard option for opening these.

Bram, can you elaborate please?

Comment 9

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56546/diff/4-5/

Comment 10

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

Hey I know you have outstanding questions to the layout and functionality, however would you be able to review the code for me please? Thank you!

Comment 11

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c52802f75a66

Comment 12

[:Gijs (he/him)]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

https://reviewboard.mozilla.org/r/56546/#review53920

::: browser/base/content/tabbrowser.xml:6
(Diff revision 5)
>  <?xml version="1.0"?>
>  
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<!DOCTYPE bindings SYSTEM "chrome://browser/locale/browser.dtd">

Rather than doing this, can we just copy labels / tooltips from the "real button" using JS?

::: browser/base/content/tabbrowser.xml:4889
(Diff revision 5)
> +            <xul:menuitem id="tabs-alltabs-containersLabel"
> +                          disabled="true"
> +                          label="&newUserContext.label;"/>
> +            <xul:menuseparator id="alltabs-popup-separator-2"/>
> +            <xul:menuitem id="alltabs_undoCloseTab"

There are still some id attributes hanging around here.

::: browser/base/content/tabbrowser.xml:4907
(Diff revision 5)
>                      style="width: 0;"/>
>        </xul:arrowscrollbox>
>      </content>
>  
> -    <implementation implements="nsIDOMEventListener">
> +    <implementation implements="nsIDOMEventListener, nsIObserver">
> +      <method name="observe">

Methods after the constructor and fields please. :-)

::: browser/base/content/tabbrowser.xml:4940
(Diff revision 5)
> +          let containersEnabled = Services.prefs.getBoolPref("privacy.userContext.enabled");
> +          document.getAnonymousElementByAttribute(this, "anonid", "tabs-alltabs-button").hidden = !containersEnabled;

Please reduce duplication by just calling this.observe(null, "nsPref:changed", "privacy.userContext.enabled");

::: browser/base/content/utilityOverlay.js:412
(Diff revision 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) {

Rather than this more extensive refactoring, you could just add an insertion point as a third optional argument to this function, e.g.:

, insertBeforeItem = null

and then:

```
event.target.insertBefore(docfrag, insertBeforeItem);
```

::: browser/themes/linux/browser.css:1647
(Diff revision 5)
>    list-style-image: url("chrome://browser/skin/tabbrowser/alltabs-inverted.png");
>  }
>  
> -#alltabs-button > .toolbarbutton-icon {
> -  padding: 9px 6px 6px;
> +#alltabs-button > .toolbarbutton-icon,
> +.tabs-alltabs-few-button > .toolbarbutton-icon {
> +  padding: 9px 6px 6px !important;

Do we really need !important here? Even if we nuke this thing from the primaryToolbarbuttons thing? Why?

::: browser/themes/osx/browser.css:2822
(Diff revision 5)
>    #TabsToolbar > toolbarpaletteitem > #new-tab-button > .toolbarbutton-icon {
>      width: 18px;
>    }
>  }
>  
> -#alltabs-button {
> +#alltabs-button, .tabs-alltabs-few-button {

Here and below we could avoid touching blame quite so much if we added the new selectors before the old ones (on their own line).

::: browser/themes/shared/browser.inc:5
(Diff revision 5)
>  %filter substitution
>  
>  % Note that zoom-reset-button is a bit different since it doesn't use an image and thus has the image with display: none.
>  %define nestedButtons #zoom-out-button, #zoom-reset-button, #zoom-in-button, #cut-button, #copy-button, #paste-button
> -%define primaryToolbarButtons #back-button, #forward-button, #home-button, #print-button, #downloads-button, #bookmarks-menu-button, #new-tab-button, #new-window-button, #fullscreen-button, #sync-button, #feed-button, #social-share-button, #open-file-button, #find-button, #developer-button, #preferences-button, #privatebrowsing-button, #save-page-button, #add-ons-button, #history-panelmenu, #nav-bar-overflow-button, #PanelUI-menu-button, #characterencoding-button, #email-link-button, #sidebar-button, @nestedButtons@, #e10s-button, #panic-button, #webide-button, #containers-panelmenu
> +%define primaryToolbarButtons #back-button, #forward-button, #home-button, #print-button, #downloads-button, #bookmarks-menu-button, #new-tab-button, .tabs-alltabs-few-button, #new-window-button, #fullscreen-button, #sync-button, #feed-button, #social-share-button, #open-file-button, #find-button, #developer-button, #preferences-button, #privatebrowsing-button, #save-page-button, #add-ons-button, #history-panelmenu, #nav-bar-overflow-button, #PanelUI-menu-button, #characterencoding-button, #email-link-button, #sidebar-button, @nestedButtons@, #e10s-button, #panic-button, #webide-button, #containers-panelmenu

The #alltabs-button isn't in here, so I don't think this should be, either...

::: browser/themes/windows/browser.css:2132
(Diff revision 5)
> +#TabsToolbar[brighttext] > .tabs-alltabs-few-button,
> +#TabsToolbar[brighttext] > toolbarpaletteitem > .tabs-alltabs-few-button {

Neither of these selectors ever apply, because the .tabs-alltabs-few-button is never a direct child of the toolbar, only ever a descendant. See the other OSes for what to do here.

Comment 13

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch (gone until June 6) from comment #8)
> Right, I agree we shouldn't make the [+] button do something else on click.
> I just disagree that that means we need to make the dropdown arrow
> permanently visible in order for users to have a mouse or keyboard option
> for opening these.
> 
> Bram, can you elaborate please?

(we could also just add these items to the context menu of the [+] button instead of having a separate button for it, or do a similar long-mouse-press thing that we're using for the keyboard shortcut)

Comment 14

[Jonathan Kingston [:jkt] he/him]

https://reviewboard.mozilla.org/r/56546/#review53920

> Rather than this more extensive refactoring, you could just add an insertion point as a third optional argument to this function, e.g.:
> 
> , insertBeforeItem = null
> 
> and then:
> 
> ```
> event.target.insertBefore(docfrag, insertBeforeItem);
> ```

Mostly because I don't need the element removal while loop which would end up deleting the anchor I need. So I could shuffle the code in a different way but I don't think there is a simpler solution that what I had. Another option is to add another argument for flagging off the loop but again that feels way more complex.

Comment 15

[Jonathan Kingston [:jkt] he/him]

https://reviewboard.mozilla.org/r/56546/#review53920

> Rather than doing this, can we just copy labels / tooltips from the "real button" using JS?

I'll make the change now though anyway but I'm not sure I see the advantage of that, is importing the bindings here something that would impact perf?

Comment 16

[Jonathan Kingston [:jkt] he/him]

https://reviewboard.mozilla.org/r/56546/#review54546

<p>&lt;p&gt;Marking as done as it won't let me push again otherwise (need to raise a bug for that, even if you are away it should be possible to push updates)&lt;/p&gt;</p>

Comment 17

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

https://reviewboard.mozilla.org/r/56546/#review54548

Comment 18

[Jonathan Kingston [:jkt] he/him]

https://reviewboard.mozilla.org/r/56546/#review54550

Comment 19

[Jonathan Kingston [:jkt] he/him]

https://reviewboard.mozilla.org/r/56546/#review54552

Closing

Comment 20

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

Changing reviewer to see if push review behaves.

Currently getting:

error publishing review request 56544: Error publishing: Bugzilla error: :Gijs Kruitbosch (gone until June 6) <[e-mail]> is not currently accepting 'review' requests. (HTTP 500, API Error 225)
(review requests not published; visit review url to attempt publishing there)

Comment 21

[Jonathan Kingston [:jkt] he/him]

https://reviewboard.mozilla.org/r/56546/#review53920

> Mostly because I don't need the element removal while loop which would end up deleting the anchor I need. So I could shuffle the code in a different way but I don't think there is a simpler solution that what I had. Another option is to add another argument for flagging off the loop but again that feels way more complex.

Marking issue as done to see if it will resolve this review.

Comment 22

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

https://reviewboard.mozilla.org/r/56546/#review54556

Comment 23

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5939c055f9c0

Comment 24

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56546/diff/5-6/

Comment 25

[:Gijs (he/him)]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

https://reviewboard.mozilla.org/r/56546/#review54724

Code-wise, this looks OK with the below addressed. I've not tested it, I assume you have and that the test passes. I think the UX discussion should be resolved before we ship this.

::: browser/base/content/tabbrowser.xml:4931
(Diff revision 6)
> +          let mainAlltabsButton = document.getElementById("alltabs-button");
> +          alltabsButton.setAttribute("tooltiptext", mainAlltabsButton.getAttribute("tooltiptext"));
> +          let mainLabel = document.getElementById("alltabs_containersTab");
> +          this.alltabsFewContainersLabel.label = mainLabel.label;
> +          let mainUndo = document.getElementById("alltabs_undoCloseTab");
> +          this.alltabsFewUndoLabel.label=mainUndo.label;

Nit: spaces around '='.

::: browser/base/content/tabbrowser.xml:4945
(Diff revision 6)
> +
> +      <field name="alltabsFewContainersLabel" readonly="true">
> +        document.getAnonymousElementByAttribute(this, "anonid", "tabs-alltabs-containersLabel");
> +      </field>
> +
> +      <field name="alltabsFewUndoLabel" readonly="true">

This isn't a label, so the name shouldn't say that. :-)

::: browser/components/contextualidentity/test/browser/browser.ini:11
(Diff revision 6)
>  [browser_usercontextid_tabdrop.js]
> +[browser_alltabsButton.js]
>  skip-if = os == "mac" || os == "win" # Intermittent failure - bug 1268276

This moves the skip-if from usercontextid_tabdrop.js to your new test. Annotations always apply to the test header above them (think regular Windows/Linux .ini file syntax).

::: browser/components/contextualidentity/test/browser/browser_alltabsButton.js:7
(Diff revision 6)
> +  yield new Promise((resolve) => {
> +    SpecialPowers.pushPrefEnv({"set": [
> +      ["privacy.userContext.enabled", true]
> +    ]}, resolve);
> +  });

`yield SpecialPowers.pushPrefEnv(...)` was made to work recently.

Note that then it becomes so small that it probably makes sense to just add it to your main add_task method.

::: browser/components/contextualidentity/test/browser/browser_alltabsButton.js:14
(Diff revision 6)
> +function onPopupEvent(popup, evt) {
> +  let fullEvent = "popup" + evt;
> +  let deferred = Promise.defer();
> +  let onPopupHandler = (e) => {
> +    if (e.target == popup) {
> +      popup.removeEventListener(fullEvent, onPopupHandler);
> +      deferred.resolve();
> +    }
> +  };
> +  popup.addEventListener(fullEvent, onPopupHandler);
> +  return deferred.promise;

Couple of notes:

- please don't ever use Promise.defer() in new code.
- this function is unused.
- instead of awaitTooltipOpen, you can just use:

BrowserTestUtils.waitForEvent(button, "popupshown");

:-)

::: browser/components/contextualidentity/test/browser/browser_alltabsButton.js:37
(Diff revision 6)
> +    });
> +  });
> +}
> +
> +add_task(function* test() {
> +  let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");

mochitest-browser tests like this one run in the scope of a window. You can just use window/document/browser-window-scope-globals (like gBrowser) without prefixing with anything.

::: browser/components/contextualidentity/test/browser/browser_alltabsButton.js:46
(Diff revision 6)
> +    EventUtils.synthesizeMouseAtCenter(allTabsButton, {type: "mousedown"});
> +    EventUtils.synthesizeMouseAtCenter(allTabsButton, {type: "mouseup"});

Does `allTabsButton.click()` not work? Otherwise, just pass `{}` and call synthesizeMouseAtCenter once, and it'll execute a click.

::: browser/components/contextualidentity/test/browser/browser_alltabsButton.js:57
(Diff revision 6)
> +    ok(contextIdItem, `User context id ${i} exists`);
> +
> +    EventUtils.synthesizeMouseAtCenter(contextIdItem, {type: "mousedown"});
> +    EventUtils.synthesizeMouseAtCenter(contextIdItem, {type: "mouseup"});
> +
> +    let tab = yield BrowserTestUtils.waitForNewTab(gBrowser);

Nit: create the promise before you're clicking on the item, and yield after it.

::: browser/themes/linux/browser.css:1641
(Diff revision 6)
>  #alltabs-button {
>    list-style-image: url("chrome://browser/skin/tabbrowser/alltabs.png");
>  }
>  
>  #TabsToolbar[brighttext] > #alltabs-button,
> +#TabsToolbar[brighttext] .tabs-alltabs-few-button,

nitty nit: put this line first.

Comment 26

[:Gijs (he/him)]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

https://reviewboard.mozilla.org/r/56546/#review54730

Gah, review state dropdown novelty - I did intend to give you r+ with the other points addressed!

Comment 27

[Jonathan Kingston [:jkt] he/him]

https://reviewboard.mozilla.org/r/56546/#review54724

> Does `allTabsButton.click()` not work? Otherwise, just pass `{}` and call synthesizeMouseAtCenter once, and it'll execute a click.

Didn't seem to, not sure why. I noticed the above being done in other code so copied that. Will test with `{}` thanks

Comment 28

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56546/diff/6-7/

Comment 29

[Jonathan Kingston [:jkt] he/him]

So all the review issues are resolved, how do we best move forward with this RE :gijs's comment:

Code-wise, this looks OK with the below addressed. I've not tested it, I assume you have and that the test passes. I think the UX discussion should be resolved before we ship this.

Thanks!

Comment 30

[:Gijs (he/him)]

(I think my questions are clear, Bram and Tanvi can decide what to do / if they need more info; clearing needinfo for now.)

Comment 31

[Tanvi Vyas[:tanvi]]

I spoke to Stephen Horlander about this earlier today.  He is going to take a look at this patch and advise on whether he is okay with the UX, or has a better suggestion.  Jonathan, can you add a screenshot as well?

Stephen, please let us know if we can land this drop down arrow change or not.  I know you have concerns about it changing existing UI.  If you have other suggestions of how we can make New Container Tabs more accessible to container users, please let us know.  We plan to enable containers later this week for Nightly only in https://bugzilla.mozilla.org/show_bug.cgi?id=1276412

Thanks!

Comment 32

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56546/diff/7-8/

Comment 33

[Jonathan Kingston [:jkt] he/him]

Attachment: new menu screenshot

Comment 34

[Jonathan Kingston [:jkt] he/him]

The review above is just a rebase, reviewboard shows all the diff as if it was part of it. (I rebased to get the icons to work which got merged into central)

Comment 35

[Bram Pitoyo [:bram]]

(In reply to :Gijs Kruitbosch from comment #8)
> Right, I agree we shouldn't make the [+] button do something else on click.
> I just disagree that that means we need to make the dropdown arrow
> permanently visible in order for users to have a mouse or keyboard option
> for opening these.
> 
> Bram, can you elaborate please?

Hi Gijs,

One of the consequences of piggybacking containers inside the tab list arrow (instead of creating our own access point, which is going to introduce more visual clutter), is that we also need to make that arrow more prominent (always visible).

We thought that the action of opening a new container tab is related enough to opening a new tab, that the access point should be located on the same level and close to each other.

To maintain consistency of content in the arrow menu, we decided to keep showing the tab list, even when there are only a few tabs. But maybe this isn’t a good idea?

What are some of your other concerns? Do you have some ideas on how to solve this problem?

Comment 36

[:Gijs (he/him)]

(In reply to Bram Pitoyo [:bram] from comment #35)
> What are some of your other concerns? Do you have some ideas on how to solve
> this problem?

I made some suggestions (context menu / long mouse-press on the [+] button) in comment #3 and comment #13. Beyond that, I'll leave UX feedback/stamping to Stephen.

Comment 37

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8758231 [details]
Bug 1272416 - Adding in a new alltabs dropdown when containers are enabled and tabs are not overflowing

Clearing my review flag (was done as was unable to push).

Comment 38

[Tanvi Vyas[:tanvi]]

Per Stephen Horlander in https://bugzilla.mozilla.org/show_bug.cgi?id=1274246#c6, I am closing this bug won't fix for now.

Jonathan, I'm sorry about the work that you did for this patch!  Hopefully you may be able to apply some of it to the long press bug - https://bugzilla.mozilla.org/show_bug.cgi?id=1272256.  Gijs, sorry for the unnecessary review!