Closed Bug 1233886 Opened 4 years ago Closed 4 years ago

Add options to new tab button or down arrow to open tabs in different container types

Categories

(Core :: DOM: Core & HTML, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: huseby, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Whiteboard: [userContextId])

Attachments

(2 files, 10 obsolete files)

15.24 KB, patch
Details | Diff | Splinter Review
3.06 KB, patch
mconley
: review+
Details | Diff | Splinter Review
this bug covers fixing the call to createCodebasePrincipal in toolkit/modules/NewTabUtils.jsm to use a default user context origin attribute.
Status: NEW → ASSIGNED
Attached patch Bug_1233886.patch (obsolete) — Splinter Review
this fixes up the call to createCodebasePrincipal in the new tab button handler to always use the default user context.
Attachment #8700230 - Flags: review?(jonas)
Component: Security → DOM
Product: Firefox → Core
Whiteboard: [userContextId]
Attachment #8700230 - Flags: review?(jonas)
Attached patch Bug_1233886.patch (obsolete) — Splinter Review
Attachment #8700230 - Attachment is obsolete: true
Attached patch Bug_1233886.patch (obsolete) — Splinter Review
Attachment #8717705 - Attachment is obsolete: true
Comment on attachment 8717712 [details] [diff] [review]
Bug_1233886.patch

this one is good to go.
Attachment #8717712 - Flags: review?(jonas)
Comment on attachment 8717712 [details] [diff] [review]
Bug_1233886.patch

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

System principal already has the same originAttributes as {}, so no need to do anything here.
Attachment #8717712 - Flags: review?(jonas) → review-
Bug 1245262 will honor the current container when user using Ctrl+t , however this bug is trying to use the DEFAULT_CONTEXT_ID when user presss the '+' (New Tab Button).

Do we really need two different behaviors here?
Flags: needinfo?(huseby)
Yoshi,

so is it decided to change the + button to have a drop-down for container selection (Bug 1245262)?  If so, then I'll change this behavior to match.
Flags: needinfo?(huseby) → needinfo?(allstars.chh)
Hi Dave
Actually I am not sure which behavior we want, so I ni? you.
I don't know who's the UX for this, can you forward this to him/her?
Flags: needinfo?(allstars.chh)
Dave said we should ni? tanvi for checking this.
Tanvi, please see my question in Comment 6

thanks
Flags: needinfo?(tanvi)
For now, ctrl+T and the plus sign should open a tab in the default container.

For the future, we need better UX around this.  Perhaps there is a drop down menu on the plus sign that lets you choose the container you want.  Perhaps we use alt+# to open a tab in different contexts.

Enhancements to the plus sign (this bug) are probably easier to determine than the keyboard shortcuts in bug 1245262.

Adding Bram.  Bram, do you have any ideas on how we can update the plus button to open different container tabs?  Maybe an arrow at the bottom of the plus sign that lets you choose a container?  Or maybe a if you have containers turned on, then clicking the plus sign automatically gives you container options under the plus sign?

If we decide that clicking the plus sign gives you the secondary menu to choose which container you want, we can add an about:config pref that turns off the secondary menu.  That way users who want to use containers but only once in a while, are not disturbed by the secondary menu.
Flags: needinfo?(tanvi) → needinfo?(bram)
From bug 1245262; Bram has already thought a bit about this problem:

(In reply to Bram Pitoyo [:bram] from comment #19)
> Max Popp from the UX team has mocked up a New Tab button that has an arrow
> beside it. Page 1–6:
> 
> https://mozilla.invisionapp.com/share/6269XCTAR#/screens/139413470
> 
> This was designed for the Restore Tabs feature, but perhaps it’s a good idea
> to use for containers?
Initially, I thought that adding a down arrow to the side of the plus sign – which opens up a context menu – is a good idea. It naturally fits the way that user invokes new tab, and those who don’t use containers won’t ever see it.

But the more I think about it, the more I think that, while the idea is sound, using the arrow graphic is problematic:

1. Currently, the arrow is already two times in the toolbar to “expand” or “show all”. Opening container tabs should probably not be considered a “show” action:
  a. On the URL bar, as a shorthand for “Show history”
  b. On the tab bar, as a shorthand for “List all ioen tabs”

2. Related to point 1b, if you open too many tabs to fit the toolbar area, Firefox shows you an arrow that list all open tabs. This down arrow appears directly to the right of the plus sign. So, if we have yet another arrow to invoke container tabs, the user will see two identical arrows next to each other that does two completely different things.

So my proposal is to show a separate icon for containers. Example:
http://f.cl.ly/items/252C2F3n2e0H0U2y0f33/containers-tab-bar-context-menu.png
Flags: needinfo?(bram)
(In reply to Bram Pitoyo [:bram] from comment #12)
> So my proposal is to show a separate icon for containers. Example:
> http://f.cl.ly/items/252C2F3n2e0H0U2y0f33/containers-tab-bar-context-menu.png

Thanks Bram!  Would clicking the plus sign have a different action than clicking on the container icon?  Where the plus sign opens the default tab and the container button brings down the pop down menu?

The top picture doesn't have a tab border around it.  That is what my browser looks like now.  The second two graphics do.  In which cases do we have the tab border and in which ones do we not have one?  Thanks!

~Tanvi
Flags: needinfo?(bram)
I think that clicking the plus button should always open a non-container tab.

You can also argue that the plus button should open a container tab of the same type that the user already has open, but this risks confusing users who can’t figure out the exact behaviour. It’s better to always keep container tabs managed through a separate interface and avoid mixing it up with normal browsing.

I had forgotten to label the image. The top state is the initial state (non-hover), the middle is for when your mouse hovers over the area, and the bottom is when you click on one of the icons.
Flags: needinfo?(bram)
Assignee: huseby → nobody
Status: ASSIGNED → NEW
Whiteboard: [userContextId] → [OA][userContextId]
Flags: needinfo?(mconley)
Assignee: nobody → amarchesini
Clearing needinfo - baku and I talked in person, and I think we have a way forward here.
Flags: needinfo?(mconley)
Attached patch ui.patch (obsolete) — Splinter Review
Attachment #8717712 - Attachment is obsolete: true
Attachment #8741465 - Flags: review?(mconley)
Comment on attachment 8741465 [details] [diff] [review]
ui.patch

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

I think I have more questions than answers here. I think the general idea of using a toolbarbutton of type menu is the right choice given the spec, but I think I want to figure out some kind of scheme for sharing the menupopup between the places where it's likely to be used.

Dropping review request for now, and needinfo'ing Gijs about sharing menupopups.

::: browser/base/content/tabbrowser.xml
@@ +4848,5 @@
> +  We don't set type="menu-button" by default because we are not shipping
> +  "containers" yet, so more likely the following menu will not be shown
> +-->
> +          <xul:menupopup>
> +            <xul:menuitem usercontextid="1"

I'm a bit wary of dropping in the browser.dtd and these menuitems in here. Especially since these menu items already exist elsewhere.

Ideally, we have a single menupopup somewhere with the list of potential user context, and all of the various places where we might ask the user about which context they want to use, we'll use the same one.

I'm not 100% certain if XUL gives us the capability of sharing this between nodes, but it strikes me as likely. The places I can see us sharing this list are:

1) The File > New Container Tab menu
2) The new tab button
3) The new tab button that is exposed when the tab strip is overflowing

Gijs, do you know if sharing a menupopup between a toolbarbutton of type menu and a standard menu is straight forward, or will we need to be doing some script-trickery?

@@ +5639,5 @@
> +        <![CDATA[
> +          var button = document.getAnonymousElementByAttribute(this, "anonid", "tabs-newtab-button");
> +          var visible = Services.prefs.getBoolPref("privacy.userContext.enabled");
> +          if (visible) {
> +            button.setAttribute('type', 'menu-button');

It's a little odd to have a menupopup under a normal toolbar button, but it's probably okay.
Attachment #8741465 - Flags: review?(mconley)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #17)
> Comment on attachment 8741465 [details] [diff] [review]
> ui.patch
> 
> Review of attachment 8741465 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I have more questions than answers here. I think the general idea of
> using a toolbarbutton of type menu is the right choice given the spec, but I
> think I want to figure out some kind of scheme for sharing the menupopup
> between the places where it's likely to be used.
> 
> Dropping review request for now, and needinfo'ing Gijs about sharing
> menupopups.
> 
> ::: browser/base/content/tabbrowser.xml
> @@ +4848,5 @@
> > +  We don't set type="menu-button" by default because we are not shipping
> > +  "containers" yet, so more likely the following menu will not be shown
> > +-->
> > +          <xul:menupopup>
> > +            <xul:menuitem usercontextid="1"
> 
> I'm a bit wary of dropping in the browser.dtd and these menuitems in here.
> Especially since these menu items already exist elsewhere.
> 
> Ideally, we have a single menupopup somewhere with the list of potential
> user context, and all of the various places where we might ask the user
> about which context they want to use, we'll use the same one.
> 
> I'm not 100% certain if XUL gives us the capability of sharing this between
> nodes, but it strikes me as likely. The places I can see us sharing this
> list are:
> 
> 1) The File > New Container Tab menu
> 2) The new tab button
> 3) The new tab button that is exposed when the tab strip is overflowing
> 
> Gijs, do you know if sharing a menupopup between a toolbarbutton of type
> menu and a standard menu is straight forward, or will we need to be doing
> some script-trickery?

I don't know. Maybe Neil can help.

I will note that the patch also lacks support for the regular new-tab-button: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#601 .
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(enndeakin)
That's true - and that reminds me that I wanted to ask bram if we're still okay with the design of the dropmarker if it's in the overflowed state.

The all-tabs-list dropdown thing is a similar dropdown. Putting them next to one another might look odd:

http://i.imgur.com/2hAzDM3.png
Flags: needinfo?(bram)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #19)
> That's true - and that reminds me that I wanted to ask bram if we're still
> okay with the design of the dropmarker if it's in the overflowed state.

Hi Mike, on comment 12, I suggested having another icon that’s not an arrow – even though the context menu behaviour itself is a good idea.

http://f.cl.ly/items/252C2F3n2e0H0U2y0f33/containers-tab-bar-context-menu.png

The icon assets can be found here (there’s one for each platform and @2x resolution), by going into the ‘-Assets’ folder:

https://www.dropbox.com/sh/nay36uvjvs6f2fh/AADnBfpmCezbLxXtbWKEq0Y3a?dl=0
Flags: needinfo?(bram)
> http://f.cl.ly/items/252C2F3n2e0H0U2y0f33/containers-tab-bar-context-menu.png

I used that component not to have an arrow, but to be styled as Bram suggested.
I tested the patch and without privacy.userContextEnabled it works as normal. So, except the CSS part, the functionality correct.
Am I wrong?
Flags: needinfo?(mconley)
> > Gijs, do you know if sharing a menupopup between a toolbarbutton of type
> > menu and a standard menu is straight forward, or will we need to be doing
> > some script-trickery?

You can't share menupopups that are associated with a parent menu or menu-type button. I thought there may be have been a bug somewhere but I can't find it.
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #22)
> > > Gijs, do you know if sharing a menupopup between a toolbarbutton of type
> > > menu and a standard menu is straight forward, or will we need to be doing
> > > some script-trickery?
> 
> You can't share menupopups that are associated with a parent menu or
> menu-type button. I thought there may be have been a bug somewhere but I
> can't find it.

OK, this is what I thought, but I wasn't sure.

The next best thing would be to have empty menupopups that call the same function/thing onpopupshowing, and populate the menu in question with the same things if it is empty.
Attached patch ui.patch (obsolete) — Splinter Review
onpopupshowing callback.
Attachment #8741465 - Attachment is obsolete: true
Attachment #8742362 - Flags: review?(mconley)
Comment on attachment 8742362 [details] [diff] [review]
ui.patch

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

This looks right to me - just missing the menupopup on the new-tab-button.

::: browser/base/content/tabbrowser.xml
@@ +4838,5 @@
>                             onclick="checkForMiddleClick(this, event);"
>                             onmouseover="document.getBindingParent(this)._enterNewTab();"
>                             onmouseout="document.getBindingParent(this)._leaveNewTab();"
> +                           tooltip="dynamic-shortcut-tooltip">
> +<!--

Happy to take this documentation. Perhaps we should include the bug number for enabling it by default (assuming that's the eventual plan).

@@ +4842,5 @@
> +<!--
> +  We don't set type="menu-button" by default because we are not shipping
> +  "containers" yet, so more likely the following menu will not be shown
> +-->
> +          <xul:menupopup anonid="tabs-newtab-menupopup"

So we'll need to add something like this to the other newtab button - the one that's displayed if the tabstrip is overflowing.

That's this button: https://hg.mozilla.org/mozilla-central/file/10f66b316457/browser/base/content/browser.xul#l601

::: browser/base/content/utilityOverlay.js
@@ +402,5 @@
>      closeMenus(event.target);
>    }
>  }
>  
> +function createUserContextMenu(event, addCommandAttribute = true) {

Can you add a docstring for this function, describing its functionality, args, etc?

@@ +407,5 @@
> +  if (event.target.hasChildNodes()) {
> +    return true;
> +  }
> +
> +  var userContextItems = [

let, not var

@@ +431,5 @@
> +  if (!bundle) {
> +    return false;
> +  }
> +
> +  for (var i = 0; i < userContextItems.length; ++i) {

Probably less verbose to use a for..of loop here, like:

for (let item of userContextItems) {
  let menuitem = document.createElement...
}

@@ +442,5 @@
> +    if (addCommandAttribute) {
> +      menuitem.setAttribute("command", "Browser:NewUserContextTab");
> +    }
> +
> +    event.target.appendChild(menuitem);

Instead of adding these items one at a time, we should do it in one shot by using a document fragment.

See: https://developer.mozilla.org/en-US/docs/Web/API/Document/createDocumentFragment

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +770,5 @@
>  decoder.noCodecsXP.message = To play video, you may need to enable Adobe’s Primetime Content Decryption Module.
>  decoder.noCodecsLinux.message = To play video, you may need to install the required video codecs.
>  decoder.noHWAcceleration.message = To improve video quality, you may need to install Microsoft’s Media Feature Pack.
> +
> +userContextPersonal.label = Personal

Do you know if we have to update the keys when we move them from .dtd to .properties? I'm not sure if we do still... :flod might know better.
Attachment #8742362 - Flags: review?(mconley) → feedback+
(In reply to Andrea Marchesini (:baku) from comment #21)
> > http://f.cl.ly/items/252C2F3n2e0H0U2y0f33/containers-tab-bar-context-menu.png
> 
> I used that component not to have an arrow, but to be styled as Bram
> suggested.
> I tested the patch and without privacy.userContextEnabled it works as
> normal. So, except the CSS part, the functionality correct.
> Am I wrong?

I think you're right.
Flags: needinfo?(mconley)
Attached patch ui.patch (obsolete) — Splinter Review
I don't know how to test the changes in browser.xul.
Attachment #8742362 - Attachment is obsolete: true
Attachment #8743523 - Flags: review?(mconley)
Comment on attachment 8743523 [details] [diff] [review]
ui.patch

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

r=me with these changes.

You might want to double-check with flod to make sure that the strings don't need their keys changed.

To test your changes, do a try push testing mochitest-bc / mochitest-bc-e10s. I think that should exercise it.

::: browser/base/content/browser.xul
@@ +616,5 @@
>                       cui-areatype="toolbar"
> +                     removable="true">
> +<!--
> +  We don't set type="menu-button" by default because we are not shipping
> +  "containers" yet, so more likely the following menu will not be shown

Same comment as before - maybe let's put in the bug number for where we're going to flip this thing on by default (again, assuming that's the plan).

::: browser/base/content/tabbrowser.xml
@@ +5611,5 @@
>  
> +      <method name="_updateNewTabButton">
> +        <body>
> +        <![CDATA[
> +          var buttonA = document.getAnonymousElementByAttribute(this, "anonid", "tabs-newtab-button");

let, not var. Always use let.

Let's give these better names. "underflowButton" and "customizableButton" might be better.
Attachment #8743523 - Flags: review?(mconley) → review+
Great! I'm applying your comments. And I guess the next step is for you doing these menuitem elements looking better :) Right?
Flags: needinfo?(mconley)
Attached patch ui.patchSplinter Review
Attachment #8743523 - Attachment is obsolete: true
(In reply to Andrea Marchesini (:baku) from comment #29)
> Great! I'm applying your comments. And I guess the next step is for you
> doing these menuitem elements looking better :) Right?

Yep. I took a crack at it recently, and made some progress. I'll post a patch for review when it's done.
Flags: needinfo?(mconley)
Comment on attachment 8744968 [details]
MozReview Request: Initial stabs at styling the user context newtab button

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48745/diff/1-2/
Attachment #8744968 - Attachment description: MozReview Request: Style the user context newtab button → MozReview Request: Initial stabs at styling the user context newtab button
Comment on attachment 8744968 [details]
MozReview Request: Initial stabs at styling the user context newtab button

Hey MattN, so I'm trying to implement the styling for some modifications to the newtab button that allow for a dropmarker to choose a user context (see Bram's screenshot in comment 12).

I'm afraid it's been a little while since I've had to deal with these tabs. I've hacked something together which looks kinda in the ballpark for OS X (haven't tested the others yet), but I've got a bunch of hardcoded pixel values in there, and I'm wondering if there's a less fragile way of implementing the UI.

I'm thinking in particular of the massive negative margin-left on the dropmarker, for example. Without it, the dropmarker sails right out of the side of the newtab button.
Attachment #8744968 - Flags: feedback?(MattN+bmo)
Hey bram, I just wanted to quickly confirm - are you certain that we want to put a dropmarker in the newtab button? I'm just worried about us adding a new target within what's already a commonly used piece of UI. Are we not concerned about users accidentally hitting the dropmarker due to muscle memory?
Flags: needinfo?(bram)
I’m actually quite concerned about the dropmarker reducing not only familiarity, but also the usability of the existing plus button. I did it because there needs to be an accessible way to create new container, that’s as accessible as creating a new tab, and not hidden under a menu that’s hidden by default on Windows.

Currently, the click area for the plus symbol is quite big, which makes it a prominent target. Adding a separate icon for container means that clicking the plus becomes a lot harder. You’d have to be more precise.

One solution I thought is make it so that the container icon gets its own ‘small tab’ area, just like the plus symbol does. This way, it’s visually separated, and it won’t compromise the plus.

What do you think?
Flags: needinfo?(bram)
(In reply to Bram Pitoyo [:bram] from comment #38)
>
> One solution I thought is make it so that the container icon gets its own
> ‘small tab’ area, just like the plus symbol does. This way, it’s visually
> separated, and it won’t compromise the plus.
> 
> What do you think?

Hey Bram,

These are good questions. Probably would be worth talking to phlsa or other folk from the rest of the UX team to get their suggestions as well as mine.

A few ideas to consider:

1) Something additive to the newtab button, that doesn't make the current button smaller or take up any space from it, but sits along-side it.
2) Expose the menu on a long-click of the newtab button - though this has discoverability issues.
3) Expose the menu on a key-combo click of the newtab button. Same discoverability issues as above.

The newtab button is a pretty critical piece of UI. I advise being extremely careful and deliberate about any changes we make to it.
Flags: needinfo?(bram)
Since containers is hidden behind a pref, I am comfortable experimenting with the UI. I suppose, in order to make it usable enough to user test, we a UI that expose it better.

To give some perspective: in our recent test, the biggest complain was the need to press ‘Alt’ and go to the File menu (which was hidden by default on Windows 7 and up) in order to open Containers. So we thought, let’s make it a bit more accessible but still keep it on hidden behind a pref before we re-test it.

This being said, I am very hesitant to change the way our current UI work, and want to make sure that anything that’s exposed will look and feel Firefoxy.

Ideas #1 was what I proposed on comment #38, but another idea I just thought of is to use the Open Tabs arrow.

When Containers is activated, the down arrow that appears when you have too many open tabs show up in its usual place – aligned right on the tab bar. Normally, this menu contains the list of all your open tabs. But when Container is active, it also gives you the option of opening new container tabs.

Like this:
https://s3.amazonaws.com/f.cl.ly/items/2y322s0w0D3c3H3e471D/new-container-tab-arrow.png

Thoughts?
Flags: needinfo?(bram)
Comment on attachment 8744968 [details]
MozReview Request: Initial stabs at styling the user context newtab button

It seems like we're still figuring out the UI we want so re-flag me when that's figured out.
Attachment #8744968 - Flags: feedback?(MattN+bmo)
(In reply to Bram Pitoyo [:bram] from comment #40)
> Like this:
> https://s3.amazonaws.com/f.cl.ly/items/2y322s0w0D3c3H3e471D/new-container-
> tab-arrow.png
> 
> Thoughts?

That's not a bad idea at all, and (I think) quite easy / cheap to implement. We could experiment to see if that's sufficient exposure.

Hey :baku, this might be something you'd be comfortable implementing. There's no styling work, beyond just making sure that the All Tabs dropmarker is always displayed if privacy.userContext.enabled is set to true.

Perhaps an attribute on tabbrowser-tabs could be set in the privacy.userContext.enabled is true case to bypass this rule:

http://searchfox.org/mozilla-central/source/browser/base/content/browser.css#134
Flags: needinfo?(amarchesini)
If we go with using the existing down arrow for open tabs, then what happens when the user has a lot of open tabs plus has containers enabled?  Will they be able to scroll down to the New Container tab section?  Going all the way to the end of that list decreases usability of opening these tabs for users who have lots of tabs open or small screen sizes.

Thoughts?
(In reply to Tanvi Vyas [:tanvi] from comment #43)
> If we go with using the existing down arrow for open tabs, then what happens
> when the user has a lot of open tabs plus has containers enabled?  Will they
> be able to scroll down to the New Container tab section?  Going all the way
> to the end of that list decreases usability of opening these tabs for users
> who have lots of tabs open or small screen sizes.
> 
> Thoughts?

This is a good concern. There are two potential ways to solve this problem:

1. Keep “New Container Tabs” always on top of the list. Pros: great if you’re a frequent containers user. Cons: may be annoying, you might need to scroll down to get to your tab list if there are a lot of items in it

http://f.cl.ly/items/1y3C1a0T0n0s061Q1g3d/containers-tab-bar-context-menu-i02%20-%20option%201.png

2. Make “New Container Tabs” a parent item that will spawn a list of containers. Pros: least interruption to current menu, compact size. Cons: having a submenu within a context menu is really clunky.

http://f.cl.ly/items/0i3P1S2U3l2o3g2u1c1A/containers-tab-bar-context-menu-i02%20-%20option%202.png


One thing to try might be a hybrid approach:
* When there are only a few tabs open, the containers tab menu should always appear inline
* When there are heaps of tabs (>10, let’s say), the containers tab menu appears as a parent item

But the risk here are: the implementation logic gets more complicated, and the UI is also more inconsistent.


Thoughts?
> 2. Make “New Container Tabs” a parent item that will spawn a list of
> containers. Pros: least interruption to current menu, compact size. Cons:
> having a submenu within a context menu is really clunky.

I vote for this. When we will have about:containers, we don't know how many containers the users will create.
This submenu approach seems also consistent with |File->Open new Container Tab|.
Flags: needinfo?(amarchesini)
Attached patch contextualmenu1.patch (obsolete) — Splinter Review
Attachment #8747642 - Flags: review?(mconley)
Attachment #8747642 - Attachment is obsolete: true
Attachment #8747642 - Flags: review?(mconley)
Attachment #8747666 - Flags: review?(mconley)
Bram is right, submenus are very clunky, and can get hard to click on.  The point of this was to have an easy way for users to open a container in the right context.

On the other hand, as baku said with about:containers users may be adding many more containers, making the open tab history even farther down the page.

So I think we go from a submenu for now and see how it goes.

Note that if only a few tabs are open, the down arrow doesn't show up in the browser.  Its only if you have enough tabs open that they can't all be visible.  So in the case where all the tabs that are open are visible and we add the drop down arrorw for opening container tabs, we can use the main menu instead of the submenu.  Since there will be no history in the main menu anyway.

Also, using a slider the way the hamburger menu's History section works would be nicer than a regular submenu.  It would be a submenu that slides in.  This is probably substantially more implementation work though so we can skip it for now.
(In reply to Tanvi Vyas [:tanvi] from comment #48)
> Note that if only a few tabs are open, the down arrow doesn't show up in the
> browser.  Its only if you have enough tabs open that they can't all be
> visible.  So in the case where all the tabs that are open are visible and we
> add the drop down arrow for opening container tabs, we can use the main
> menu instead of the submenu.  Since there will be no history in the main
> menu anyway.

This was my proposal, as well. If too few tabs are open, show the whole container list in the menu. If many tabs are open, put the container list inside a submenu.

This means that, if you have Containers activated, the downward arrow will always show up on the tab bar, and you will always be able to see your list of open tabs alongside the list of containers. This is a side effect, but it’s alright; we do it to have a consistent tab list behaviour.
Comment on attachment 8747666 [details] [diff] [review]
contextualmenu1.patch

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

Note that this doesn't implement Bram's suggestion of showing the full list unless there are many tabs in the dropdown (in which case, display as a submenu). Not sure if we're going down that route, or if that's follow-up work, but just calling that out as an audible.

::: browser/base/content/browser.xul
@@ +628,5 @@
>                      class="menuitem-iconic"
>                      key="key_undoCloseTab"
>                      label="&undoCloseTab.label;"
>                      observes="History:UndoCloseTab"/>
> +          <menuseparator id="alltabs-popup-separator1"/>

nit - I think I'd prefer "alltabs-popup-seperator-1" and "alltabs-popup-separator-2" for readability.

::: browser/base/content/tabbrowser.xml
@@ +6577,5 @@
>  
>      <handlers>
>        <handler event="popupshowing">
>        <![CDATA[
> +        if (event.target.getAttribute('id') == "alltabs_containersMenuTab") {

I think you can make this more general by checking to see if the event.target is this. If not, ignore.

@@ +6605,5 @@
>        ]]></handler>
>  
>        <handler event="popuphidden">
>        <![CDATA[
> +        if (event.target.getAttribute('id') == "alltabs_containersMenuTab") {

Same as above.
Attachment #8747666 - Flags: review?(mconley) → review+
Trying to rename this bug to more closely describe what it is supposed to do.
Priority: -- → P1
Summary: fix up new tab button call to createCodebasePrincipal to use default user context → Add options to new tab button or down arrow to open tabs in different container types
Whiteboard: [OA][userContextId] → [userContextId]
Blocks: 1272416
Status: NEW → ASSIGNED
Blocks: 1270471
https://hg.mozilla.org/mozilla-central/rev/b3a7e11eb6cd
https://hg.mozilla.org/mozilla-central/rev/4497c5cf57f7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.