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

VERIFIED FIXED in Firefox 4.0b12

Status

()

Firefox
Menus
P1
normal
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: faaborg, Assigned: ttaubert)

Tracking

(Blocks: 1 bug, {user-doc-needed})

Trunk
Firefox 4.0b12
user-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus ?

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(3 attachments, 6 obsolete attachments)

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
(Reporter)

Comment 1

6 years ago
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).
Blocks: 626527
(Reporter)

Comment 2

6 years ago
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/
Blocks: 607226
Blocks: 627096
Tim, can you take this?
Assignee: nobody → tim.taubert
(Assignee)

Comment 4

6 years ago
Sure!
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
(Assignee)

Comment 5

6 years ago
Created attachment 507364 [details] [diff] [review]
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.
(Assignee)

Comment 6

6 years ago
Passed try.
(Assignee)

Comment 7

6 years ago
Builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/tim.taubert@gmx.de-ab6b1f1ee7ad/

(For anyone who wants to try out)
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)
Attachment #507364 - Flags: review?(dolske)
Comment on attachment 507364 [details] [diff] [review]
patch v1

>+#menu_tabview image {

This is inefficient CSS, please fix.

This patch also breaks keyboard access.
Attachment #507364 - Flags: review-
Comment on attachment 507364 [details] [diff] [review]
patch v1

Also, the icon you're using isn't 16x16 except for gnomestripe.
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
Attachment #507364 - Flags: review?(dolske) → feedback-
(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.
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.
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).
>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
(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).
(Assignee)

Comment 17

6 years ago
Created attachment 508392 [details] [diff] [review]
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).
Attachment #507364 - Attachment is obsolete: true
Attachment #508392 - Flags: review?(dao)
Created attachment 508950 [details]
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.
Nominating to be a soft blocker since this is the only mouse-based way to get into Panorama on Windows (7/vista)
blocking2.0: --- → ?
Flags: in-litmus?
Would accept the patch if it gets in before beta12, but wouldn't block on it.
blocking2.0: ? → -
(Assignee)

Comment 21

6 years ago
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/tim.taubert@gmx.de-180ed8c81360/
(Assignee)

Comment 22

6 years ago
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 :/
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.
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.
(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.
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?
Created attachment 511500 [details]
Mac menu, correct key combo

I can't reproduce the problem, the combo shows correctly for me.
(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 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.
Attachment #508392 - Flags: review?(dao) → review-
(In reply to comment #29)

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

Filed bug 633681.
(Assignee)

Comment 31

6 years ago
Created attachment 511948 [details] [diff] [review]
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.
Attachment #508392 - Attachment is obsolete: true
Attachment #511948 - Flags: review?(dolske)
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.
Attachment #511948 - Flags: review-
(Assignee)

Comment 33

6 years ago
(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.
>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 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--;
>         }
(Assignee)

Comment 36

6 years ago
(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.
Ugh, okay...

I still think you should loop backwards.
(Assignee)

Comment 38

6 years ago
Created attachment 511983 [details] [diff] [review]
patch v4

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

Looks much better. Fixed.
Attachment #511948 - Attachment is obsolete: true
Attachment #511983 - Flags: review?(dao)
Attachment #511948 - Flags: review?(dolske)
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?
(Assignee)

Comment 40

6 years ago
(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".
You probably have the gnome setting that disables icons in menus.
... in which case this behaves as expected.
(Assignee)

Comment 42

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

Interesting :)
(Assignee)

Comment 43

6 years ago
Created attachment 511984 [details] [diff] [review]
patch v5

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

Removed.
Attachment #511983 - Attachment is obsolete: true
Attachment #511984 - Flags: review?(dao)
Attachment #511983 - Flags: review?(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
Attachment #511984 - Flags: review?(dao) → review-
> > 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.
(Assignee)

Comment 46

6 years ago
Created attachment 512138 [details] [diff] [review]
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.
Attachment #511984 - Attachment is obsolete: true
Attachment #512138 - Flags: review?(dao)
We really want to get this in Fx4!
Priority: P3 → P1
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.
Attachment #512138 - Flags: review?(dao)
Attachment #512138 - Flags: review+
Attachment #512138 - Flags: approval2.0+
(Assignee)

Comment 49

6 years ago
Created attachment 513722 [details] [diff] [review]
patch for checkin
Attachment #512138 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Blocks: 626854

Comment 50

6 years ago
http://hg.mozilla.org/mozilla-central/rev/3017b3459639
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

6 years ago
Keywords: user-doc-needed
Depends on: 635732

Updated

6 years ago
Depends on: 635895

Updated

6 years ago
No longer depends on: 635732

Comment 51

6 years ago
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.
_updateTabsVisibilityStatus is unmaintained code, we should remove it.

Comment 53

6 years ago
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?
No, since the code doesn't do anything critical.
Target Milestone: --- → Firefox 4.0b12
Version: unspecified → Trunk
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
Status: RESOLVED → VERIFIED
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?

Updated

6 years ago
Depends on: 639730

Updated

6 years ago
Depends on: 653655
You need to log in before you can comment on or make changes to this bug.