Closed Bug 1269029 Opened 3 years ago Closed 3 years ago

Create a hamburger menu for containers

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: tanvi, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Whiteboard: [userContextId][userContextId-UI][domsecurity-backlog])

Attachments

(4 files, 4 obsolete files)

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.
Whiteboard: [userContextId][userContextId-UI] → [userContextId][userContextId-UI][domsecurity-backlog]
Assignee: nobody → amarchesini
Depends on: 1267923
Attached patch contextualmenu.patch (obsolete) — Splinter Review
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.
Attachment #8747659 - Flags: review?(mconley)
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.
Attachment #8747659 - Flags: review?(mconley)
> 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?
Flags: needinfo?(bram)
> Bram, can you help me with this?

And can you also help me with:

> +containers-panelmenu.label = Containers
> +containers-panelmenu.tooltiptext = Open container tabs
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.
Flags: needinfo?(bram)
Attached file 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.
These icons do not exist in gecko. What should I do with them?

Toolbars-linux-assets/Toolbar-inverted@2x.png
Toolbars-linux-assets/Toolbar@2x.png
Toolbars-windows-assets/Toolbar-XP@2x.png
Toolbars-windows-assets/Toolbar-lunaSilver@2x.png
Flags: needinfo?(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.
Flags: needinfo?(bram) → needinfo?(gijskruitbosch+bugs)
Attached patch part 1 - icons (obsolete) — Splinter Review
Attachment #8747659 - Attachment is obsolete: true
Attachment #8749104 - Flags: review?(gijskruitbosch+bugs)
Attached patch part 2 - implementation (obsolete) — Splinter Review
Attachment #8749105 - Flags: review?(gijskruitbosch+bugs)
(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. :-\ ).
Flags: needinfo?(gijskruitbosch+bugs)
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/
Attachment #8749104 - Flags: review?(gijskruitbosch+bugs)
Attachment #8749104 - Attachment is obsolete: true
Attachment #8749155 - Flags: review+
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
Attachment #8749105 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch part 2 - implementation (obsolete) — Splinter Review
Attachment #8749105 - Attachment is obsolete: true
Attachment #8749160 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8749160 [details] [diff] [review]
part 2 - implementation

Applying your comments!
Attachment #8749160 - Flags: review?(gijskruitbosch+bugs)
Attachment #8749160 - Attachment is obsolete: true
Attachment #8749163 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #8749163 - Flags: review?(gijskruitbosch+bugs) → review+
(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!
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/15ae438a63dc
https://hg.mozilla.org/mozilla-central/rev/4932a868d8a7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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!
(In reply to Stephen Horlander [:shorlander] from comment #23)

You should file a new bug. Thanks.
You need to log in before you can comment on or make changes to this bug.