1014313 - Add tab control options, including undo, to toolbar context menu and tab overflow menu

Status: VERIFIED FIXED

(Firefox :: Menus, enhancement)

Description

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Two menus where tab control options should be added

This bug adds "Undo Close Tab" to the tab overflow menu, and adds four items to the toolbar context menu in Firefox.

Acceptance criteria:
1. Undo Close Tab added to tab overflow menu
2. Reload All Tabs, Save All Tabs for Later, Bookmark all Tabs..., and Undo Close Tab added to toolbar context menu

(This is a followup implementation bug from bug 989796)

Rationale for two areas:

1. Context menu of the empty area in the toolbar

The rationale here stems from the fact that currently, users can undo a closed tab in a tab, but not right beside it.  This means there are some places in the menu bar that users can undo a tab closure from and other they cannot, and with Australis making background tabs more subtle, the difference is currently fairly arbitrary.  After all, neither a tab or the menu is the closed tab in question.  This rationale goes for not just closing the last tab, but all global tab options.  In 1c, all global tab options are added to the toolbar context menu, where the tab-specific options remain in 1a, when an actual tab is the target.

The option should be added to the top set under “Remove from Toolbar.”  This allows “Customize” to still be the most obvious bottom target, which is important since it’s the place a user right-clicking the toolbar will most likely find helpful.

2. Overflow tab menu

The rationale here is that this menu serves as a tab access point, including a link to launch tab groups.  If this menu is truly for tab management, undoing what was previously closed makes sense as an action here.  

Everything in this menu has an icon, so this needs one too.  We can make a new one, but for now let’s use the New Tab icon.  That and reload are the two most similar, but reload implies that action will happen to the current page while New Tab correctly implies an additional tab will be opened.

Comment 1

[Mike Hoye [:mhoye]]

Looking for a mentor for this bug, so I can shop it around. Marco, any suggestions?

Comment 2

[Blake Winton (:bwinton) (:☕️)]

I've done a bit of digging in this one, and could probably mentor someone.

Comment 3

[Mike Hoye [:mhoye]]

All yours.

Comment 4

[Marco Mucci [:MarcoM]]

Removed from Iteration 33.1.

Comment 5

[:Gijs (he/him)]

Attachment: part 1: add tabs options to tabstrip context menu,

Jared, how does this look?

Comment 6

[:Gijs (he/him)]

(oh, and I should have mentioned - I talked with Blair about this bug a while ago, so I'm working on the assumption that he still doesn't mind if I steal this one)

Comment 7

[Marco Mucci [:MarcoM]]

Added to Iteration 33.2

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8449678 [details] [diff] [review]
part 1: add tabs options to tabstrip context menu,

>+    <broadcaster id="broadcaster_reloadAllTabs"
>+                 label="&reloadAllTabs.label;"
>+                 accesskey="&reloadAllTabs.accesskey;"
>+                 oncommand="gBrowser.reloadAllTabs();"
>+                 />

Reusing the same access keys in different contexts is a footgun and likely already creating conflicts, if not for en-US then for other locales.

/> should be glued to the previous line.

>+  Array.forEach(popup.querySelectorAll(".toolbaritem-customize"),
>+                (n) => n.hidden = onTabStrip);

use for..of?

>+  }
>+  // In some cases, we will exit the above loop with toolbarItem being the

between these two lines there should be a blank one

Comment 9

[:Gijs (he/him)]

Attachment: part 1: add tabs options to tabstrip context menu,

Comment 10

[:Gijs (he/him)]

Boriss, can you provide the "undo close tab" asset? We don't have the existing new tab button asset in 16x16px and 32x32px (for OS X hidpi), and the sizes we do have (18px, 36px, 72px for OS X hidpi) don't scale to 16px well.

Comment 11

[:Gijs (he/him)]

Attachment: part 2: WIP patch to add undo close tab item to alltabs-button menu

Checkpointing this while waiting for assets.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8449800 [details] [diff] [review]
part 1: add tabs options to tabstrip context menu,

>+    <!-- Tabs broadcasters -->
>+    <broadcaster id="broadcaster_reloadAllTabs"
>+                 label="&reloadAllTabs.label;"
>+                 oncommand="gBrowser.reloadAllTabs();"/>
>+    <broadcaster id="broadcaster_bookmarkAllTabs"
>+                 label="&bookmarkAllTabs.label;"
>+                 command="Browser:BookmarkAllTabs"/>

Broadcasters are useful for sharing dynamic attributes, but it looks like you never access them by script. In that case they're overkill. Please just set the label, command and oncommand attributes directly on the menu items, although for oncommand you could create a command element.

>+  let showTabStripItems = toolbarItem && toolbarItem.id == "tabbrowser-tabs";
>+  // NB: don't show tab strip items in customize mode:
>+  showTabStripItems = showTabStripItems &&
>+                      toolbarItem.parentNode.localName != "toolbarpaletteitem";

As far as I can tell we don't and shouldn't wrap tabbrowser-tabs, so I don't think this check works. But if it did, I'd ask you to reformat it:

let showTabStripItems = toolbarItem &&
                        toolbarItem.id == "tabbrowser-tabs" &&
                        /* don't show tab strip items in customize mode: */
                        toolbarItem.parentNode.localName != "toolbarpaletteitem";

Comment 13

[:Gijs (he/him)]

Attachment: part 1: add tabs options to tabstrip context menu,

Good catch re: customize mode. We still have the 'normal' menu on tabs, so I've left the tab options in place on the empty tabstrip as well (more useful than the other two which would be greyed out because you can't move the tabstrip out of the tabstoolbar anyway). I've also added a small test.

Comment 14

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Icon: Undo Close Tab 32x32 .png

Comment 15

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Icon: Undo Close Tab 16x16 .png

Comment 16

[:Gijs (he/him)]

Attachment: part 2: add undo close tab item to alltabs-button menu

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8451071 [details] [diff] [review]
part 2: add undo close tab item to alltabs-button menu

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

>+function onAllTabsPopupShowing(e) {
>+  let disableUndoCloseTab = SessionStore.getClosedTabCount(window) == 0;
>+  document.getElementById("alltabs_undoCloseTab").disabled = disableUndoCloseTab;
>+}

Please move this to the popupshowing event handler in tabbrowser.xml.

Also, I wouldn't introduce a variable only to avoid the long line. Just wrap this line and format it appropriately?

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8450272 [details] [diff] [review]
part 1: add tabs options to tabstrip context menu,

>+  for (let node of popup.querySelectorAll(".toolbaritem-customize")) {
>+    node.hidden = showTabStripItems;
>+  }
>+  for (let node of popup.querySelectorAll(".toolbaritem-tabsmenu")) {
>+    node.hidden = !showTabStripItems;
>+  }

I find these classes weird, since based on how we normally name classes, they seem to imply that they are meant for toolbaritem not menuitem nodes. Classes kind of share a global namespace, so we usually name them in a way that makes them unambiguous globally. In this case I think you should just use an attribute, e.g. contexttype=tabbar/toolbaritem.

Comment 19

[:Gijs (he/him)]

Attachment: part 1: add tabs options to tabstrip context menu,

I will just note that we don't seem to have any clear guidance on how to indent such multiline assignments, and browser.js does it in manifold different ways. I'm also not sure why this is to be preferred over a temp variable, which takes equally many lines and is, IMO, less messy.

Comment 20

[:Gijs (he/him)]

Attachment: part 2: add undo close tab item to alltabs-button menu,

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8451608 [details] [diff] [review]
part 1: add tabs options to tabstrip context menu,

>+  for (let node of popup.querySelectorAll('[contexttype="toolbaritem"]')) {

I'd prefer menuitem[contexttype="toolbaritem"] for clarity and sanity.

>+    PlacesCommandHook.updateBookmarkAllTabsCommand();

Add a blank line here

>+    document.getElementById("toolbar-context-undoCloseTab").disabled =
>+                                              SessionStore.getClosedTabCount(window) == 0;

Two spaces of extra indentation are sufficient for this.

>+      <menuitem id="toolbar-context-reloadAllTabs"
>+                class="toolbaritem-tabsmenu"
>+                hidden="true"
>+                contexttype="tabbar"
>+                oncommand="gBrowser.reloadAllTabs();"
>+                label="&reloadAllTabs.label;"
>+                accesskey="&toolbarContextMenu.reloadAllTabs.accesskey;"/>

Setting hidden="true" in the markup seems unnecessary.

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 8451610 [details] [diff] [review]
part 2: add undo close tab item to alltabs-button menu,

>       <toolbarbutton id="alltabs-button"
>                      class="toolbarbutton-1 chromeclass-toolbar-additional tabs-alltabs-button"
>                      type="menu"
>                      label="&listAllTabs.label;"
>                      tooltiptext="&listAllTabs.label;"
>+                     cui-areatype="toolbar"
>                      removable="false">

Is this just a drive-by fix? What is this attribute good for here?

>       <handler event="popupshowing">
>       <![CDATA[
>+
>+        document.getElementById("alltabs_undoCloseTab").disabled =
>+                                    SessionStore.getClosedTabCount(window) == 0;
>         var tabcontainer = gBrowser.tabContainer;

nit: Remove the blank line before your code, add one after it. Also, indentation again.

>+/* All tabs button menu */
>+
>+/* undo close tab menuitem in alltabs button menu */
>+#alltabs_undoCloseTab {
>+  list-style-image: url(chrome://browser/skin/undoCloseTab.png);
>+}
>+
>+@media (min-resolution: 2dppx) {
>+  #alltabs_undoCloseTab {
>+    list-style-image: url(chrome://browser/skin/[e-mail]);
>+  }
>+  #alltabs_undoCloseTab > .toolbarbutton-icon {
>+    width: 16px;
>+  }
>+}

This probably fits better in browser.css along with the styles for #menu_tabview, rather than tabs.inc.css.
I don't think we support hi-DPI on Linux yet, and on Windows we should downscale the @2x icon for scaling factors above 1.

Comment 23

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #22)
> Comment on attachment 8451610 [details] [diff] [review]
> part 2: add undo close tab item to alltabs-button menu,
> 
> >       <toolbarbutton id="alltabs-button"
> >                      class="toolbarbutton-1 chromeclass-toolbar-additional tabs-alltabs-button"
> >                      type="menu"
> >                      label="&listAllTabs.label;"
> >                      tooltiptext="&listAllTabs.label;"
> >+                     cui-areatype="toolbar"
> >                      removable="false">
> 
> Is this just a drive-by fix? What is this attribute good for here?

Yes. It was on the menuitem before, where it made no sense (mistakenly added there in bug 899576 - I should have caught that in review, but didn't). We tweaked the startup path in bug 899576 to have this attribute prefilled on items in their normal location so we don't need to add it dynamically on startup - when items are moved, CustomizableUI sets the attribute (which happens to never happen to this particular node anyway, as it's not removable).

Comment 24

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #23)
> tweaked the startup path in bug 899576 to have this attribute prefilled on
> items in their normal location so we don't need to add it dynamically on
> startup - when items are moved, CustomizableUI sets the attribute (which
> happens to never happen to this particular node anyway, as it's not
> removable).

So why should this attribute ever be set on this node?

Comment 25

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #24)
> (In reply to :Gijs Kruitbosch from comment #23)
> > tweaked the startup path in bug 899576 to have this attribute prefilled on
> > items in their normal location so we don't need to add it dynamically on
> > startup - when items are moved, CustomizableUI sets the attribute (which
> > happens to never happen to this particular node anyway, as it's not
> > removable).
> 
> So why should this attribute ever be set on this node?

It's used to determine the location of a customizable widget in CSS without having to resort to descendant selectors. It should be set on all customizable items which are in a toolbar. Right now the node matches e.g. http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/browser.inc#13 .

Comment 26

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #25)
> It's used to determine the location of a customizable widget in CSS without
> having to resort to descendant selectors. It should be set on all
> customizable items which are in a toolbar.

This isn't a customizable item as far as I can tell...

> Right now the node matches e.g.
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/browser.
> inc#13 .

So does every other node that isn't customizable in browser.xul. As written inAnyPanel cannot be used on its own but only in combination with other selectors. I agree it's a footgun, though; seems like it should be made more specific by requiring the presence of the cui-areatype attribute.

Comment 27

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #26)
> (In reply to :Gijs Kruitbosch from comment #25)
> > It's used to determine the location of a customizable widget in CSS without
> > having to resort to descendant selectors. It should be set on all
> > customizable items which are in a toolbar.
> 
> This isn't a customizable item as far as I can tell...

It is? http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/src/CustomizableUI.jsm#238

(it's non-removable, but even so - it's certainly something about which CUI/currentset knows, and it'd fall under a selector like toolbarbutton[cui-areatype="toolbar"], and should; the attribute is also set on e.g. the urlbar-container and tabbrowser-tabs)

> > Right now the node matches e.g.
> > http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/browser.
> > inc#13 .
> 
> So does every other node that isn't customizable in browser.xul. As written
> inAnyPanel cannot be used on its own but only in combination with other
> selectors. I agree it's a footgun, though; seems like it should be made more
> specific by requiring the presence of the cui-areatype attribute.

Right, but we're getting very far away from the original purpose of the bug now. Can we agree that the patch basically corrects what the work in bug 899576 was meant to do, and move any further discussion about the usefulness or otherwise of the attribute to a followup bug? :-)

Comment 28

[:Gijs (he/him)]

Comment on attachment 8451608 [details] [diff] [review]
part 1: add tabs options to tabstrip context menu,

Landed with nits: https://hg.mozilla.org/integration/fx-team/rev/4a7bfc90109f

Comment 29

[:Gijs (he/him)]

Attachment: part 2: add undo close tab item to alltabs-button menu,

This should address the other nits. I'd actually have preferred to move the menu_tabview stuff to tabs.inc.css instead of adding more identical-but-platform-specific-stuff, but it looks like we have platform-specific assets and offsets for that... I'd be in favour of getting rid of that, but that's a separate issue.

Comment 30

[:Gijs (he/him)]

Attachment: part 2: add undo close tab item to alltabs-button menu,

Forgot to qref the blank line change. Sorry for the spam.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a7bfc90109f

Comment 32

[Francesco Lodolo [:flod]]

It took me a while to understand what was going on: you're reusing an existing string, with a different accesskey.

This won't make tools happy for sure, since they try to create a relation between string and accesskey.

Having said that, can we have at least a comment referencing the original label key for each accesskey? As it is, it's almost impossible to localize this file.

Comment 33

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #32)
> It took me a while to understand what was going on: you're reusing an
> existing string, with a different accesskey.
> 
> This won't make tools happy for sure, since they try to create a relation
> between string and accesskey.
> 
> Having said that, can we have at least a comment referencing the original
> label key for each accesskey? As it is, it's almost impossible to localize
> this file.

Would it be preferable if we had identical strings everywhere? That doesn't seem nice either. :-\

Comment 34

[Francesco Lodolo [:flod]]

I think it would make things a lot clearer/cleaner

toolbarContextMenu.bookmarkAllTabs.accesskey = A 
toolbarContextMenu.bookmarkAllTabs.label = Bookmark All Tabs…
etc.

No need to understand which string each label refers to, and some tools will provide suggestions based on translation memory anyway.

Comment 35

[:Gijs (he/him)]

Attachment: add labels specifically for this context menu,

Comment 36

[Dão Gottwald [:dao]]

Comment on attachment 8452682 [details] [diff] [review]
part 2: add undo close tab item to alltabs-button menu,

(In reply to :Gijs Kruitbosch from comment #27)
> > > It's used to determine the location of a customizable widget in CSS without
> > > having to resort to descendant selectors. It should be set on all
> > > customizable items which are in a toolbar.
> > 
> > This isn't a customizable item as far as I can tell...
> 
> It is?
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> customizableui/src/CustomizableUI.jsm#238
> 
> (it's non-removable, but even so - it's certainly something about which
> CUI/currentset knows, and it'd fall under a selector like
> toolbarbutton[cui-areatype="toolbar"], and should; the attribute is also set
> on e.g. the urlbar-container and tabbrowser-tabs)

Still not customizable for any sensible meaning of that word. Indeed, our CSS seems to use cui-areatype only for items that can move elsewhere.

It's not clear to me what one would use toolbarbutton[cui-areatype="toolbar"] for. Since it isn't going to reliably match buttons in complex widgets, it's not a useful general replacement for 'toolbar toolbarbutton'. CustomizableWidgets.jsm has updateCombinedWidgetStyle, but of course that's only a limited hack.

> > > Right now the node matches e.g.
> > > http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/browser.
> > > inc#13 .
> > 
> > So does every other node that isn't customizable in browser.xul. As written
> > inAnyPanel cannot be used on its own but only in combination with other
> > selectors. I agree it's a footgun, though; seems like it should be made more
> > specific by requiring the presence of the cui-areatype attribute.
> 
> Right, but we're getting very far away from the original purpose of the bug
> now. Can we agree that the patch basically corrects what the work in bug
> 899576 was meant to do, and move any further discussion about the usefulness
> or otherwise of the attribute to a followup bug? :-)

I don't agree with what bug 899576 meant to do, then. So please file that bug if you want my r+ on this part or drop it from this patch.

>--- a/browser/themes/linux/jar.mn
>+++ b/browser/themes/linux/jar.mn
>@@ -60,16 +60,18 @@ browser.jar:
>   skin/classic/browser/searchbar.css
>   skin/classic/browser/Secure.png
>   skin/classic/browser/Security-broken.png
>   skin/classic/browser/setDesktopBackground.css
>   skin/classic/browser/slowStartup-16.png
>   skin/classic/browser/Toolbar.png
>   skin/classic/browser/Toolbar-inverted.png
>   skin/classic/browser/Toolbar-small.png
>+  skin/classic/browser/undoCloseTab.png                        (../shared/undoCloseTab.png)
>+  skin/classic/browser/[e-mail]                     (../shared/[e-mail])

Don't add [e-mail] here.

Comment 37

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #36)
> > > > Right now the node matches e.g.
> > > > http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/browser.
> > > > inc#13 .
> > > 
> > > So does every other node that isn't customizable in browser.xul. As written
> > > inAnyPanel cannot be used on its own but only in combination with other
> > > selectors. I agree it's a footgun, though; seems like it should be made more
> > > specific by requiring the presence of the cui-areatype attribute.
> > 
> > Right, but we're getting very far away from the original purpose of the bug
> > now. Can we agree that the patch basically corrects what the work in bug
> > 899576 was meant to do, and move any further discussion about the usefulness
> > or otherwise of the attribute to a followup bug? :-)
> 
> I don't agree with what bug 899576 meant to do, then. So please file that
> bug if you want my r+ on this part or drop it from this patch.

I can just not add it to the button, but am I good to go to remove it from the menuitem where, AFAICT, we both agree it makes 0 sense?

Comment 38

[Dão Gottwald [:dao]]

Sure, the attribute makes no sense whatsoever on the menuitem.

Comment 39

[:Gijs (he/him)]

w/ nits:
remote:   https://hg.mozilla.org/integration/fx-team/rev/c209e9852e51
remote:   https://hg.mozilla.org/integration/fx-team/rev/cef679881a18

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c209e9852e51
https://hg.mozilla.org/mozilla-central/rev/cef679881a18

Comment 41

[Marco Mucci [:MarcoM]]

Hi Florin, can a QA contact be assigned for verification of this bug.

Comment 42

[Cornel Ionce [:noni]]

Verified as fixed using latest Nightly, build ID: 20140713030204 on Windows 7 64bit, Mac OS X 10.9.3 and Ubuntu 14.04.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:33.0) Gecko/20100101 Firefox/33.0
Mozilla/5.0 (X11; Linux i686; rv:33.0) Gecko/20100101 Firefox/33.0