625320 - Move "Tab Groups" to the "list all tabs" menu

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

The command "Tab Groups" should move from the view menu to the list all tabs menu.  Advantages include:

1) The command will always be available regardless of if the user has the traditional menu bar or the Firefox button activated

2) The command will have a more discoverable and natural mapping to the way that users access a large number of tabs

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Also note that this item should have the panorama icon (to make it clear that it is the same item as the primary UI control that will be added after the user invokes it).

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

The UX team is very eager to get this bug landed over the next few days in order to make Beta 11.  If anyone can get a patch for this bug posted soon, we will push hard for reviews and approval (even though this isn't blocking).

You can view all of the related bugs to clean up the traditional menu bar here: http://areweprettyyet.com/4/traditionalMenu/

Comment 3

[Ian Gilman [:iangilman]]

Tim, can you take this?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Sure!

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Who could I ask for review for this? It touches menubars, alltabs button and sync ui. Seems to work pretty well, tested with sync, too. The number of active groups is also indicated by the icon. Pushed to try.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Passed try.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-ab6b1f1ee7ad/

(For anyone who wants to try out)

Comment 8

[Ian Gilman [:iangilman]]

Comment on attachment 507364 [details] [diff] [review]
patch v1

Dolske has agreed to review it (he's reviewing a number of the menu change bugs)

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 507364 [details] [diff] [review]
patch v1

>+#menu_tabview image {

This is inefficient CSS, please fix.

This patch also breaks keyboard access.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 507364 [details] [diff] [review]
patch v1

Also, the icon you're using isn't 16x16 except for gnomestripe.

Comment 11

[Ian Gilman [:iangilman]]

Comment on attachment 507364 [details] [diff] [review]
patch v1

Just tried this patch on my Mac. Two issues: 

* The icon does not appear
* The key combo is listed as command+e, where it needs to be shift+command+e

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #0)
> 2) The command will have a more discoverable and natural mapping to the way
> that users access a large number of tabs

More first-time-discoverable, yes. But the rest doesn't really apply, as the panorama button would appear next to the all tabs button when the user starts using panorama. So I'm not sure this bug is a net win.

Comment 13

[Ian Gilman [:iangilman]]

As far as I'm aware, the Panorama menu item isn't available at all in the "Firefox button" menu system on Windows (and presumably Linux as well), making it impossible to access if you don't know the key combo or think to install the button on your toolbar. This bug fixes that.

Comment 14

[Dão Gottwald [:dao]]

I was responding to the second point from comment 0, you're talking about the first one. The first one could be addressed by adding "Tab Groups" to the Firefox button menu (although I can't say where exactly).

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

>The first one could be addressed by adding "Tab Groups" to the
>Firefox button menu (although I can't say where exactly).

Here's the rationale for placing this in the list all tabs menu instead of the Firefox button:

1) consistent position across all platforms
2) natural mapping to managing tabs
3) natural mapping to the problem the user doesn't realize they have (way too many tabs in their list all tabs menu)
4) placed directly next to "tabs from other computers."  The long term plan is for us to remove that feature, and only have panorama which syncs across all of your devices.  This way you can manage groups that each have a topic (work, home, shopping), instead of machines which just have a location (desktop, laptop).  Once we cut tabs from other computers, users are going to wonder how they can access those tabs, and the answer is tab groups, which happens to have a control right next to the thing that you were looking for.

In terms of using an icon, rationale is over here:

https://bugzilla.mozilla.org/show_bug.cgi?id=626525#c34

Comment 16

[[not reading bugmail]]

(In reply to comment #14)
> The first one could be addressed by adding "Tab Groups" to the
> Firefox button menu (although I can't say where exactly).

I thought this made sense too (see bug 628572).

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(In reply to comment #9)
> >+#menu_tabview image {
> 
> This is inefficient CSS, please fix.

Fixed.

> This patch also breaks keyboard access.

The shortcut? It still works for me.

(In reply to comment #10)
> Also, the icon you're using isn't 16x16 except for gnomestripe.

Didn't know that they differ. Fixed.

(In reply to comment #11)
> Just tried this patch on my Mac. Two issues: 
> 
> * The icon does not appear

Does it appear now?

> * The key combo is listed as command+e, where it needs to be shift+command+e

Weird. I didn't make any changes to the <key> element or the key combo. Does work for me (the right shortcut is also displayed).

Comment 18

[Ian Gilman [:iangilman]]

Attachment: Mac menu, wrong key combo

(In reply to comment #17)
> > * The icon does not appear
> 
> Does it appear now?

It does :)

> > * The key combo is listed as command+e, where it needs to be shift+command+e
> 
> Weird. I didn't make any changes to the <key> element or the key combo. Does
> work for me (the right shortcut is also displayed).

Still broken; screen shot attached. Perhaps there's some issue in how Firefox handles multi key combos in context menus on the Mac? Perhaps worth asking around. Also worth checking on Windows.

Comment 19

[Alex Faaborg [:faaborg] (Firefox UX)]

Nominating to be a soft blocker since this is the only mouse-based way to get into Panorama on Windows (7/vista)

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Would accept the patch if it gets in before beta12, but wouldn't block on it.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

I just checked on Windows: the key combo works and the shortcut is display correctly. So it's not working on Mac, only.

If anyone wants to try: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-180ed8c81360/

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Does anyone have any experience with multi key combos in context menus on Mac? Who could we ask about that? This is AFAIK the only thing blocking this patch :/

Comment 23

[Alex Faaborg [:faaborg] (Firefox UX)]

This doesn't work for me on Windows.  The panorama team will likely want to prioritize this pretty high, as it is the only mouse driven way to access the feature with a default configuration.

Comment 24

[[not reading bugmail]]

The trybuild work for me in Windows 7 in both a dirty and clean profile.  Tim, we already are getting mixed reactions from users about which key combos makes sense anyway and three finger combos are a bit much.  But Web Search, which is invoked by Ctrl+E or Ctrl+K in prior versions of Fx, apparently Panorama tried taking Ctrl+E already?  And now Ctrl+E does nothing in the current build.

Comment 25

[Ian Gilman [:iangilman]]

(In reply to comment #23)
> This doesn't work for me on Windows.  The panorama team will likely want to
> prioritize this pretty high, as it is the only mouse driven way to access the
> feature with a default configuration.

I've just verified with Alex that he was referring to the existing behavior, not the patch; Windows is working fine with the patch.

Comment 26

[Alex Faaborg [:faaborg] (Firefox UX)]

cc'ing Markus and Josh for the question in comment #22:

>Does anyone have any experience with multi key combos in context menus on Mac?

Comment 27

[Markus Stange [:mstange]]

Attachment: Mac menu, correct key combo

I can't reproduce the problem, the combo shows correctly for me.

Comment 28

[Ian Gilman [:iangilman]]

(In reply to comment #27)
> Created attachment 511500 [details]
> Mac menu, correct key combo
> 
> I can't reproduce the problem, the combo shows correctly for me.

OMG! I'd forgotten I had Tim's "change the key combo" add-on activated (and it didn't occur to me that that would actually update the visible key-combo on the menu). Once I deactivated that, I'm seeing the correct key-combo as well.

So then, looks like this patch is working properly on all platforms. We just need the review from Dao.

Comment 29

[Justin Dolske [:Dolske]]

Comment on attachment 508392 [details] [diff] [review]
patch v2

>--- a/browser/base/content/browser-syncui.js
>     // Find the alltabs-popup, only if there is a gBrowser
>     if (gBrowser) {
>       let popup = document.getElementById("alltabs-popup");
>       if (popup) {
>         let self = this;
>         popup.addEventListener("popupshowing", function() {
>+          popup.removeEventListener("popupshowing", arguments.callee, true);
>           self.alltabsPopupShowing();
>         }, true);

Blah. This code is already broken, Sync only adds this listener (and, hence, menuitem) to the browser windows open when Sync is first initialized. I'll file a new bug on that...

But this change also isn't quite right. If you change the state of any of the checks at the top of alltabsPopupShowing() [eg, enable/disable Sync], the Sync menu item won't be in the correct state the next time the popup is shown.

Remove this change, fix differently in the popuphidden <handler> (I have further comments below...).

>-    // Fake the tab object on the menu entries, so that we don't have to worry
>-    // about removing them ourselves. They will just get cleaned up by popup
>-    // binding.
>-    menuitem.tab = { "linkedBrowser": { "currentURI": { "spec": label } } };
>-    sep.tab = { "linkedBrowser": { "currentURI": { "spec": " " } } };

The DOMMenuItemActive in the tabbrowser.xml#tabbrowser-alltabs-popup binding uses this to show the link-preview destination. I guess we don't really need that, though, it's a little weird to show a label there like a tooltip.


>--- b/browser/base/content/tabbrowser.xml
>       <handler event="popuphidden">
>       <![CDATA[
>         // clear out the menu popup and remove the listeners
>-        while (this.hasChildNodes()) {
>+        while (this.lastChild.tagName != 'menuseparator') {
>           var menuItem = this.lastChild;
>           menuItem.removeEventListener("command", this, false);
>           menuItem.tab.mCorrespondingMenuitem = null;
>           this.removeChild(menuItem);
>         }

This code already isn't as paranoid as it should be (tough luck to anyone who tries to add a menuitem and wants it to persist!). We can fix that and your problem from up above...

This loop should just change to a simple loop over each child. Ideally we should only delete exactly the things added in onpopupshowing. But let's be conservative at this point and nuke everything except the 2 children Panorama adds (ie, expects to persist), since it's vaguely possible an addon might now be expecting that behavior.

I'd suggest adding an attribute (say, keepme="true") to those two children. Then loop like:

  for (let i = this.childNodes.length; i > 0; i--) {
    let kid = childNodes[i];
    if (kid.hasAttribute("keepme"))
      continue;
    /* nuke |kid| stuff as before */
  }


>--- a/browser/themes/gnomestripe/browser/browser.css
...
>-#tabview-button[groups="0"] {
>+#tabview-button[groups="0"],
>+#menu_tabview[groups="0"] {
>   -moz-image-region: rect(0, 16px, 16px, 0);
> }

Why is gnomestripe sharing -moz-image-region rules, while pinstripe/winstripe are not?

...Oh, because the offsets/size are different. I thought we just fixed all that in bug 616472. :( Bah.

I assume you've tested on the 3 platforms to ensure the icon regions look correct?

>+/* download manager button */
>+

Nit: Fine to make gnomestripe consistent with the others, but doesn't need to be done here.

Comment 30

[Justin Dolske [:Dolske]]

(In reply to comment #29)

> I'll file a new bug on that...

Filed bug 633681.

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

(In reply to comment #29)
> This loop should just change to a simple loop over each child. Ideally we
> should only delete exactly the things added in onpopupshowing. But let's be
> conservative at this point and nuke everything except the 2 children Panorama
> adds (ie, expects to persist), since it's vaguely possible an addon might now
> be expecting that behavior.
> 
> I'd suggest adding an attribute (say, keepme="true") to those two children.

Fixed.

> I assume you've tested on the 3 platforms to ensure the icon regions look
> correct?

I tested on Linux + Windows. I have no Mac available unfortunately :/

> Nit: Fine to make gnomestripe consistent with the others, but doesn't need to
> be done here.

Removed.

I removed the browser-syncui.js changes but modified it again to not create a duplicate separator and insert the sync-menuitem in the right position.

Comment 32

[Dão Gottwald [:dao]]

Comment on attachment 511948 [details] [diff] [review]
patch v3

This looks like it would display the separator when neither the sync nor the tab view item would be shown. Let me know if I'm missing something.

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #32)
> This looks like it would display the separator when neither the sync nor the
> tab view item would be shown. Let me know if I'm missing something.

The tab view item and the separator are always (static) in the alltabs popup menu. Only the sync item is generated at runtime when the weave sync service got initialized.

Comment 34

[Alex Faaborg [:faaborg] (Firefox UX)]

>The tab view item and the separator are always (static) in the alltabs popup
>menu.

Which exposes the name and keyboard shortcut, and allows users to discover the command when they are looking at their very long tab list.  Same (redundant) design we went with for the Firefox button, and notification split buttons.

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 511948 [details] [diff] [review]
patch v3

Ah, I see. In that case I think you should just loop backwards here until you encounter the separator:

>         // clear out the menu popup and remove the listeners
>-        while (this.hasChildNodes()) {
>-          var menuItem = this.lastChild;
>+        let numChildren = this.childNodes.length;
>+        for (let i = 0; i < numChildren; ) {
>+          let menuItem = this.childNodes[i];
>+          if (menuItem.hasAttribute("keepme")) {
>+            i++;
>+            continue;
>+          }
>           menuItem.removeEventListener("command", this, false);
>           menuItem.tab.mCorrespondingMenuitem = null;
>           this.removeChild(menuItem);
>+          numChildren--;
>         }

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #35)
> Ah, I see. In that case I think you should just loop backwards here until you
> encounter the separator.

That unfortunately would break Sync's menuitem insertion. That is done on every "popupshow" event (or whatever that is called :) and would lead to multiple sync menuitems. If we only do that once when weave is loaded we'd have the following problem:

From comment #29:

> But this change also isn't quite right. If you change the state of any of the
> checks at the top of alltabsPopupShowing() [eg, enable/disable Sync], the Sync
> menu item won't be in the correct state the next time the popup is shown.

Comment 37

[Dão Gottwald [:dao]]

Ugh, okay...

I still think you should loop backwards.

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

(In reply to comment #37)
> I still think you should loop backwards.

Looks much better. Fixed.

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 511983 [details] [diff] [review]
patch v4

>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -839,17 +839,27 @@
> 
>       <toolbarbutton id="alltabs-button"
>                      class="toolbarbutton-1 chromeclass-toolbar-additional tabs-alltabs-button"
>                      type="menu"
>                      label="&listAllTabs.label;"
>                      tooltiptext="&listAllTabs.label;"
>                      removable="true">
>         <menupopup id="alltabs-popup"
>-                   position="after_end"/>
>+                   position="after_end">
>+          <menuitem id="menu_tabview"
>+                    class="menuitem-iconic menuitem-with-favicon"

menuitem-with-favicon doesn't make sense here. The icon isn't a favicon.

Has somebody verified that the menu item and its icon look right on OS X?

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #39)
> menuitem-with-favicon doesn't make sense here. The icon isn't a favicon.

Indeed, but removing that class makes the icon disappear. Do you know why that is? DOM Inspector shows no special style associated with "menuitem-with-favicon".

Comment 41

[Dão Gottwald [:dao]]

You probably have the gnome setting that disables icons in menus.
... in which case this behaves as expected.

Comment 42

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #41)
> You probably have the gnome setting that disables icons in menus.
> ... in which case this behaves as expected.

Interesting :)

Comment 43

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v5

(In reply to comment #39)
> menuitem-with-favicon doesn't make sense here. The icon isn't a favicon.

Removed.

Comment 44

[Dão Gottwald [:dao]]

Comment on attachment 511984 [details] [diff] [review]
patch v5

the access key blocks access to item starting with the same letter, so you need to remove this

Comment 45

[Dão Gottwald [:dao]]

> > This patch also breaks keyboard access.
> 
> The shortcut? It still works for me.

You need to use the mouse to find out about the shortcut.

Comment 46

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v6

(In reply to comment #44)
> the access key blocks access to item starting with the same letter, so you need
> to remove this

Removed.

Comment 47

[Ian Gilman [:iangilman]]

We really want to get this in Fx4!

Comment 48

[Justin Dolske [:Dolske]]

Comment on attachment 512138 [details] [diff] [review]
patch v6

Stealing this to help make sure this goes in for B12. If there's any further nits, let's do this as followups.

Comment 49

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 50

[Jacek Caban]

http://hg.mozilla.org/mozilla-central/rev/3017b3459639

Comment 51

[Steffen Wilberg]

This breaks _updateTabsVisibilityStatus when tab overflow occurs and you click the "List all tabs" button:
Error: this.childNodes[i].tab is undefined
Source file: chrome://browser/content/tabbrowser.xml
Line: 3587

It sets tabIsVisible, which is not used anywhere (bug 345741 has stalled), but it could break third party themes.

My patch in bug 626854 contains a fix for this, but hasn't been reviewed yet.
I'd suggest to at least take the regression fix for 4.0.

Comment 52

[Dão Gottwald [:dao]]

_updateTabsVisibilityStatus is unmaintained code, we should remove it.

Comment 53

[Steffen Wilberg]

We ship that since 2007-03-07, i.e. Firefox 3.0. Isn't a removal of it so close to the release a bit too risky?

Comment 54

[Dão Gottwald [:dao]]

No, since the code doesn't do anything critical.

Comment 55

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on all platforms with builds like with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b13pre) Gecko/20110226 Firefox/4.0b13pre ID:20110226030401

Comment 56

[Les Orchard [:lorchard]]

FWIW: Seems like the hotkey for Adblock Plus add-on preferences is now taking precedence over the hotkey for Tab Groups now, at least on OS X (ie. Cmd-Shift-E). Is that expected?