1269029 - Create a hamburger menu for containers

Status: RESOLVED FIXED

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

Description

[Tanvi Vyas[:tanvi]]

The File Menu isn't always visible on Windows.  We should make it easier for users to open new container tabs by adding a hamburger menu item for containers.  Clicking on it would open up a slider (similar to how clicking on history in the hamburger menu works) with otpions to open new container tabs, go to the management about:containers interface, and maybe more.

We have discussed this with Bram.  Needinfo'ing him to see if he can come up with a mockup design.  He already has created an icon here from bug 1181953:
https://bug1181953.bmoattachments.org/attachment.cgi?id=8640888

Thanks Bram!

From an engineering standpoint, this will mostly require UI work.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: contextualmenu.patch

I didn't have luck with the icon. But all the other icons in the CustomizableWidgets are done using a global image and offsets. Maybe we should use the same pattern.

Comment 2

[:Gijs (he/him)]

Comment on attachment 8747659 [details] [diff] [review]
contextualmenu.patch

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

Stealing this.

The icon needs to fit in with the theme - it needs different colors on different OSes and the like, even if we don't add it to the spritesheet (which would indeed be preferable). It also needs a white 'active' state like the other icons with a submenu. Note also that we size default toolbar button icons to 18x18px which doesn't work nicely for a 32x32 SVG, so there might now be scaling artifacts.

If we do keep the SVG, please put the icon file in browser/themes/shared instead. Please also check https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines and remove the doctype and the fill-rule attribute. You can use <use> elements in the SVG and a #target in the CSS to reuse the glyph and fill it with different colors that match the fills we use for other icons on each OS - but maybe you should ping UX folks to update the spritesheets?

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1109,5 @@
> +    id: "containers-panelmenu",
> +    type: "view",
> +    viewId: "PanelUI-containers",
> +    label: "containers-panelmenu.label",
> +    tooltiptext: "containers-panelmenu.tooltiptext",

You don't need to specify the label and tooltiptext properties because they're in the default location.

@@ +1110,5 @@
> +    type: "view",
> +    viewId: "PanelUI-containers",
> +    label: "containers-panelmenu.label",
> +    tooltiptext: "containers-panelmenu.tooltiptext",
> +    defaultArea: CustomizableUI.AREA_PANEL,

It isn't in the panel by default, no? We're just creating a customizable widget that people can put there? In which case, this property shouldn't be there.

@@ +1117,5 @@
> +      let win = doc.defaultView;
> +
> +      let items = doc.getElementById("PanelUI-containersItems");
> +      while (items.firstChild) {
> +        items.firstChild.remove();

This continuously repopulates this menu. I don't think the containers change at runtime. Feels like we should just bail early if items.firstChild is non-null ? Maybe we can even have the items be static ?

@@ +1126,5 @@
> +
> +      let onItemCommand = function (aEvent) {
> +        let item = aEvent.target;
> +        win.openUILinkIn(aboutNewTabService.newTabURL, "tab", {
> +          userContextId: parseInt(item.getAttribute('usercontextid'))

Nit: double quotes.

@@ +1136,5 @@
> +      let enumerator = cis.getContextualIdentityInfos();
> +      while (enumerator.hasMoreElements()) {
> +        let ci = enumerator.getNext().QueryInterface(Ci.nsIContextualIdentityInfo);
> +
> +        let label = BrowserBundle.GetStringFromName(ci.label);

I think you can use let bundle = doc.getElementById("bundle_browser") like the other code here, instead of defining a getter that creates another stringbundle wrapper for the same file.

@@ +1143,5 @@
> +        item.setAttribute("label", label);
> +        item.setAttribute("usercontextid", ci.userContextId);
> +        item.setAttribute("class", "subviewbutton");
> +
> +        item.addEventListener("command", onItemCommand);

Add the listener to the container and use aEvent.target to determine which item got clicked, instead of adding a listener to each item.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +249,5 @@
>      </panelview>
>  
> +    <panelview id="PanelUI-containers" flex="1">
> +      <label value="&containersMenu.label;" class="panel-subview-header"/>
> +      <vbox id="PanelUI-containersItems" tooltip="bhTooltip"/>

I don't think you want the history tooltip.

::: browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties
@@ +79,5 @@
>  
>  feed-button.label = Subscribe
>  feed-button.tooltiptext2 = Subscribe to this page
>  
> +containers-panelmenu.label = Containers

Is this really the appropriate label? Maybe "Open Container Tab" should just be the label here?

@@ +80,5 @@
>  feed-button.label = Subscribe
>  feed-button.tooltiptext2 = Subscribe to this page
>  
> +containers-panelmenu.label = Containers
> +containers-panelmenu.tooltiptext = Open container tabs

And this could explain what "container" tabs are?

::: browser/themes/shared/toolbarbuttons.inc.css
@@ +235,5 @@
>      -moz-image-region: rect(0, 1584px, 36px, 1548px);
>    }
>  
> +  #containers-button[cui-areatype="toolbar"] {
> +    list-style-image: url(chrome://browser/content/menuContainersPanel.svg);

If this stays SVG, I don't think you need an additional rule for hidpi referencing the same image. Ditto for the menupanel.

Comment 3

[Andrea Marchesini [:baku]]

> The icon needs to fit in with the theme - it needs different colors on
> different OSes and the like, even if we don't add it to the spritesheet
> (which would indeed be preferable). It also needs a white 'active' state
> like the other icons with a submenu. Note also that we size default toolbar
> button icons to 18x18px which doesn't work nicely for a 32x32 SVG, so there
> might now be scaling artifacts.
> 
> If we do keep the SVG, please put the icon file in browser/themes/shared
> instead. Please also check
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> SVG_Guidelines and remove the doctype and the fill-rule attribute. You can
> use <use> elements in the SVG and a #target in the CSS to reuse the glyph
> and fill it with different colors that match the fills we use for other
> icons on each OS - but maybe you should ping UX folks to update the
> spritesheets?


Bram, can you help me with this?

Comment 4

[Andrea Marchesini [:baku]]

> Bram, can you help me with this?

And can you also help me with:

> +containers-panelmenu.label = Containers
> +containers-panelmenu.tooltiptext = Open container tabs

Comment 5

[Bram Pitoyo [:bram]]

I’m currently working on designing all the assets. Thanks for the pointers, Gijs. Looks like I’m dealing with quite a bit of variations here.

Comment 6

[Bram Pitoyo [:bram]]

Attachment: MenuPanels-assets.zip

Comment 7

[Bram Pitoyo [:bram]]

Attachment: Toolbars-assets.zip

All asset graphics are included as part of its respective spritesheets, but if we need to separate our container icons, we can do that, too.

Comment 8

[Andrea Marchesini [:baku]]

These icons do not exist in gecko. What should I do with them?

Toolbars-linux-assets/[e-mail]
Toolbars-linux-assets/[e-mail]
Toolbars-windows-assets/[e-mail]
Toolbars-windows-assets/[e-mail]

Comment 9

[Bram Pitoyo [:bram]]

That’s odd. I would think that a spritesheet exists for every relevant version, but maybe that’s not true.

In that case, let’s maybe omit the icon from these versions? Needinfo’ing Gijs just to double check that we indeed don’t need @2x on Linux, and XP@2x and lunaSilver@2x on Windows.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 1 - icons

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 2 - implementation

Comment 12

[:Gijs (he/him)]

(In reply to Bram Pitoyo [:bram] from comment #9)
> That’s odd. I would think that a spritesheet exists for every relevant
> version, but maybe that’s not true.
> 
> In that case, let’s maybe omit the icon from these versions? Needinfo’ing
> Gijs just to double check that we indeed don’t need @2x on Linux, and XP@2x
> and lunaSilver@2x on Windows.

Correct; we don't have hidpi versions for XP (on the assumption that few people would want/need it) and Linux (because we're trying to switch to SVG because of all the gazillions of GTK themes... but that's been hard going. :-\ ).

Comment 13

[:Gijs (he/him)]

Comment on attachment 8749104 [details] [diff] [review]
part 1 - icons

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

Note for :bram: Please always throw your pngs through ImageOptim or similar; these all had between 26 and 42% (!) taken off their size.

I'll put up a new patch in a sec and stamp that. Handy mac one-liner for future reference:

hg status --change . -mn | grep '.png' | xargs open -a /Applications/ImageOptim.app/

Comment 14

[:Gijs (he/him)]

Attachment: part 1 - icons (v2)

Comment 15

[:Gijs (he/him)]

Comment on attachment 8749105 [details] [diff] [review]
part 2 - implementation

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

r=me with the below fixed, though I wouldn't object to reviewing this again if the blocking bug changes in the meantime...

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1115,5 @@
> +        return;
> +      }
> +
> +      let cis = Cc["@mozilla.org/contextual-identity;1"]
> +                  .getService(Ci.nsIContextualIdentityService);

When I run with this patch, this throws:

Cc['@mozilla.org/contextual-identity;1'] is undefined.

presumably because I need the patch from the other bug in order to run this...

Looking at that, I just realized we're making the available containers user-configurable, is that right?

If so, I was wrong in suggesting we should only fill this once - sorry! - it's possible, of course, for the containers to change, and the menu should always reflect the latest set of containers... So in that case, please put back the 

while (items.firstChild) { items.firstChild.remove(); }

loop, instead of the early return...

@@ +1120,5 @@
> +
> +      let fragment = doc.createDocumentFragment();
> +      let enumerator = cis.getContextualIdentityInfos();
> +      while (enumerator.hasMoreElements()) {
> +        let ci = enumerator.getNext().QueryInterface(Ci.nsIContextualIdentityInfo);

This is somewhat of an unfortunate abbreviation because "Ci" is already short for Components.interfaces. I'll take anything else that isn't already defined, albeit in a different case.

(which is legal in JS, but it's confusing!)

@@ +1140,5 @@
> +        let item = aEvent.target;
> +        win.openUILinkIn(aboutNewTabService.newTabURL, "tab", {
> +          userContextId: parseInt(item.getAttribute("usercontextid"))
> +        });
> +        CustomizableUI.hidePanelForNode(item);

I *think* you don't need to do that. Can you check what happens if you leave this line out?

::: browser/themes/shared/menupanel.inc.css
@@ +281,5 @@
>    }
>  
> +  #containers-panelmenu[cui-areatype="menu-panel"],
> +  toolbarpaletteitem[place="palette"] > #containers-panelmenu {
> +    -moz-image-region: rect(0px, 2112px, 128px, 2048px);

This should be:

0px, 2112px, 64px, 2048px

::: browser/themes/shared/toolbarbuttons.inc.css
@@ +235,5 @@
>      -moz-image-region: rect(0, 1584px, 36px, 1548px);
>    }
>  
> +  #containers-panelmenu[cui-areatype="toolbar"] {
> +    -moz-image-region: rect(0, 1620px, 18px, 1584px);

This should be:

0, 1620px, 36px, 1584px

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 2 - implementation

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 8749160 [details] [diff] [review]
part 2 - implementation

Applying your comments!

Comment 18

[Andrea Marchesini [:baku]]

Attachment: part 2 - implementation

Comment 19

[:Gijs (he/him)]

Comment on attachment 8749163 [details] [diff] [review]
part 2 - implementation

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

r=me with the below fixed.

::: browser/components/contextualidentity/ContextualIdentityService.jsm
@@ +97,5 @@
> +        !PrivateBrowsingUtils.permanentPrivateBrowsing &&
> +        !aboutNewTabService.overridden) {
> +      return "about:privatebrowsing";
> +    }
> +    return aboutNewTabService.newTabURL;

You can omit the changes to this file...

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1113,5 @@
> +        let item = aEvent.target;
> +        win.openUILinkIn(ContextualIdentityService.getAboutPageURL(win),
> +                         "tab", {
> +                           userContextId: parseInt(item.getAttribute("usercontextid"))
> +                         });

and use win.BROWSER_NEW_TAB_URL instead. If you then use a temp, we can avoid some wrapping:

let userContextId = parseInt(aEvent.target.getAttribute("usercontextid"));
win.openUILinkIn(win.BROWSER_NEW_TAB_URL, "tab", {userContextId}); // yay es6

Comment 20

[Bram Pitoyo [:bram]]

(In reply to :Gijs Kruitbosch from comment #13)
> Note for :bram: Please always throw your pngs through ImageOptim or similar;
> these all had between 26 and 42% (!) taken off their size.

I’ll remember this for next time. Thank you!

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15ae438a63dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/4932a868d8a7

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/15ae438a63dc
https://hg.mozilla.org/mozilla-central/rev/4932a868d8a7

Comment 23

[Stephen Horlander [:shorlander]]

Can we please remove these icons from Toolbar*.png and menuPanel*.png and use stand alone images instead? Containers is behind a pref, would not be loaded by default and is not on track to be a shipping feature.

Adding and removing things from those sprites is an inefficient process. I would prefer to keep experimental things separate if possible so they don't conflict with other work.

Thanks!

Comment 24

[magicp]

(In reply to Stephen Horlander [:shorlander] from comment #23)

You should file a new bug. Thanks.