587099 - Display current tab set's name in browser window

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Eric Shepherd [:sheppy]]

While browsing, it would be extremely useful to be able to see the title of the tab set you're currently viewing.

My use case here: I keep a tab set for each set of documentation I'm working on (for example, one for JavaScript typed arrays, one for WebGL, etc). Then in another window, I keep a tab set for each of those same sets of documentation, but with the bugs associated with each documentation set. Being able to at a glance see which set I'm looking at would save a lot of time, especially since at a glance, one bug looks pretty much like another.

Some sort of badge in the window's title bar might work nicely for this.

Comment 1

[(no longer active)]

Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".

Comment 2

[TGOS]

Is "tab set" the right name? Should probably be named to "tab group".

Comment 3

[Aza Raskin [:aza]]

The title bar should read:

(1) If in a titled group: "[Group Name] — [Page Title]"
(2) Else: "[Page Title]"

Note that this is for display only, not for inclusion in bookmark names, etc.

Comment 5

[Aza Raskin [:aza]]

Attachment: Patch v1

Fairly simple patch, but using Firefox with it enabled already makes a nice big difference.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 473342 [details]
Patch v1

>+  getActiveGroupName: function Tabview_getActiveGroupName(){
>+    // We get the active group this way, instead of querying
>+    // GroupItems.getActiveGroupItem() because the tabSelect event
>+    // will not have happened by the time the browser tries to
>+    // update the title.
>+    var activeTab = window.gBrowser.selectedTab;
>+    if( activeTab.tabItem && activeTab.tabItem.parent ){
>+      var groupName = activeTab.tabItem.parent.getTitle();
>+      if( groupName )
>+        return groupName;
>+    }
>+    return null;
>+  },  

A future todo: consider setting setActiveGroupItem earlier so that we can use getActiveGroupItem here instead?

Another consideration: What happens when TabView has not loaded yet here? Then it's possible for group names to not show up, even if we're part of a group, if we haven't loaded TabView yet. We could call TabView.init() here if we haven't set it up yet, but this would necessarily force TabView to load on Firefox startup, which may cause a Ts regression. I would leave it as is now and open a separate bug to consider this.

>@@ -739,6 +739,11 @@
>               this.ownerDocument.title = TabView.windowTitle;
>             } else {
>               this.ownerDocument.title = this.getWindowTitleForBrowser(this.mCurrentBrowser);
>+              if (window.TabView && TabView.getActiveGroupName()) {
>+                this.ownerDocument.title = this.mStringBundle.getFormattedString(
>+                  "tabs.titleWithGroupName",
>+                  [TabView.getActiveGroupName(), this.ownerDocument.title] );
>+              }

Call TabView.getActiveGroupName() once and store it in some variable, then check if it's not null and use it.

feedback+ with that one fix and those bugs filed.

Comment 7

[Aza Raskin [:aza]]

Perhaps using session store is the right approach for keeping the number of groups currently in use available even with TabView isn't loaded yet. I'll follow up with that in a new bug.

Comment 8

[Aza Raskin [:aza]]

Attachment: Patch v2

Feedback fixed. Created two follow-up bugs:

Group name isn't displayed in title before TabView is loaded the first time: https://bugzilla.mozilla.org/show_bug.cgi?id=595020

Consider setting setActiveGroupItem earlier to alleviate race conditions with onTabSelect
https://bugzilla.mozilla.org/show_bug.cgi?id=595017

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 473818 [details] [diff] [review]
Patch v2

r+ with a couple small changes:

>+++ b/browser/base/content/tabbrowser.xml
...
>               this.ownerDocument.title = this.getWindowTitleForBrowser(this.mCurrentBrowser);
>+              if (window.TabView) {
>+                var groupName = TabView.getActiveGroupName();
>+                if (groupName) {
>+                  this.ownerDocument.title = this.mStringBundle.getFormattedString(
>+                    "tabs.titleWithGroupName",
>+                    [groupName, this.ownerDocument.title] );                
>+                }
>+              }

This would be cleaner as something like:

  var title =  this.getWindowTitleForBrowser()
  if (...) {
    if (...) {
      title = this.mStringBundle.getFormattedString("foo", [groupName, title])
    }
  }
  this.ownerDocument.title = title;


>+++ b/browser/locales/en-US/chrome/browser/tabbrowser.properties
...
>+tabs.titleWithGroupName=%S â %S

Use %1$S and %2$S, with a localization note briefly explaining what each one is.

eg

# LOCALIZATION NOTE (tabs.titleWithGroupName): first string is the tab group name, second string is the document title.

>\ No newline at end of file

Oops?

Comment 10

[Justin Dolske [:Dolske]]

Actually, this should get ui-review as well, though it's fairly minor.

Two issues randomly popped into my mind:

* On Windows the window title is "document title -- Firefox" (though I think we've wanted to see if we can drop the "- Firefox" bit?). So with this patch I think you'll now get "group name -- document title -- Firefox", which seems unfortunate.

* If we still care about XP, the the group name will essentially displace the document title from the task bar. Instead of "Document Ti..." you'll now get "Group Na...". Meh?

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 473818 [details] [diff] [review]
Patch v2

>             } else {
>               this.ownerDocument.title = this.getWindowTitleForBrowser(this.mCurrentBrowser);
>+              if (window.TabView) {
>+                var groupName = TabView.getActiveGroupName();
>+                if (groupName) {
>+                  this.ownerDocument.title = this.mStringBundle.getFormattedString(
>+                    "tabs.titleWithGroupName",
>+                    [groupName, this.ownerDocument.title] );                
>+                }
>+              }

All of this actually belongs in getWindowTitleForBrowser.

>+tabs.titleWithGroupName=%S â %S
>\ No newline at end of file

Don't hard code the separator, see getWindowTitleForBrowser for how to do it.

Comment 12

[Alex Limi (:limi) — Firefox UX Team]

Since I was asked for an opinion:

* "Firefox" should go away — in fact, I didn't think we did that anywhere but for blank tabs at the moment?

* As for "Group — Page" vs. "Page — Group", I like the former better, but I think the latter is more useful on Windows because of the way XP (still a substantial part of our user base!) displays entries in the task bar. But I could be convinced either way on this particular subject, I think — they are both useful in different ways.

Comment 13

[Aza Raskin [:aza]]

I'm going to go with "Group — Page" for now. We can decide if, on XP, we want to switch to "Page — Group" as polish.

Agreed Firefox should go away, but that is outside the scope of this bug :)

Comment 14

[Aza Raskin [:aza]]

@Dao: For the separator, unfortunately |sep| in getWindowTitleForBrowser() uses a single en-dash (which is visually on the same hierarchy order as a hyphen), whereas for the groups we want to use a longer dash for greater visual separator; a double-en dash.

You can see the effect here:
http://img.skitch.com/20100910-rfa2cbgyxbfamrhbwbppfk713u.png (en dash)
http://img.skitch.com/20100910-nhb6heg558rqtsknh214xd1cuj.png (double-en dash)

Comment 15

[Justin Dolske [:Dolske]]

(In reply to comment #11)

> >+tabs.titleWithGroupName=%S â %S
> >\ No newline at end of file
> 
> Don't hard code the separator, see getWindowTitleForBrowser for how to do it.

That seems like crud we should just remove, unless you know of a good reason for it. I dug through history, it dates back the 1999 import of code from Netscape, so there's no explanation of why this is as it is.

I was guessing it _might_ be useful for local-specific separators, but the DTD has:

<!-- LOCALIZATION NOTE (mainWindow.titlemodifiermenuseparator): DONT_TRANSLATE -->

Comment 16

[Aza Raskin [:aza]]

Attachment: Patch v3

Comment 17

[Aza Raskin [:aza]]

Comment on attachment 474258 [details] [diff] [review]
Patch v3

Realized that this had r+ from Dolske if the changes were made from v2, which they were.

Comment 18

[Dão Gottwald [:dao]]

(In reply to comment #14)
> @Dao: For the separator, unfortunately |sep| in getWindowTitleForBrowser() uses
> a single en-dash (which is visually on the same hierarchy order as a hyphen),
> whereas for the groups we want to use a longer dash for greater visual
> separator; a double-en dash.
> 
> You can see the effect here:
> http://img.skitch.com/20100910-rfa2cbgyxbfamrhbwbppfk713u.png (en dash)
> http://img.skitch.com/20100910-nhb6heg558rqtsknh214xd1cuj.png (double-en dash)

The separator is used to separate the page title from app name, though, where the hierarchy argument would apply even more. Introducing a different separator doesn't make sense here. What you really want here is change the separator we already use, but I suggest filing a new bug for that.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 474258 [details] [diff] [review]
Patch v3

>+            if (window.TabView && !TabView.isVisible()) {

TabView.isVisible() will always be false here.

>+              var groupName = TabView.getActiveGroupName();

nit: use let

>+                newTitle = groupName + " \u2014 " + newTitle;

r- stands for this

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 474258 [details] [diff] [review]
Patch v3

>       <method name="updateTitlebar">
>         <body>
>           <![CDATA[
>-            if (window.TabView && TabView.isVisible()) {
>-              // ToDo: this will be removed when we gain ability to draw to the menu bar.
>-              // Bug 586175
>-              this.ownerDocument.title = TabView.windowTitle;
>-            } else {

I didn't actually mean that this part should be moved to getWindowTitleForBrowser. It doesn't have anything to do with the single browser that's passed to getWindowTitleForBrowser, so it probably makes more sense to keep this code here.

Comment 21

[Aza Raskin [:aza]]

Attachment: Patch v4

Fixed all of the comments. In interested of landing, used the current single en-dash in |sep| instead of a double-en dash. Will file follow up bug.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 474386 [details] [diff] [review]
Patch v4

You should really get an explicit r+ after an r-.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 474386 [details] [diff] [review]
Patch v4

>+  getActiveGroupName: function Tabview_getActiveGroupName(){
>+    // We get the active group this way, instead of querying
>+    // GroupItems.getActiveGroupItem() because the tabSelect event
>+    // will not have happened by the time the browser tries to
>+    // update the title.
>+    var activeTab = window.gBrowser.selectedTab;
>+    if( activeTab.tabItem && activeTab.tabItem.parent ){
>+      var groupName = activeTab.tabItem.parent.getTitle();
>+      if( groupName )
>+        return groupName;
>+    }

nit: Whitespace is slightly off in a few places.
Tabview_getActiveGroupName() {
if (activeTab.tabItem && activeTab.tabItem.parent) {
if (groupName)

another nit: Use let instead of var at least in the 'if' branch.

>       <method name="updateTitlebar">
>         <body>
>           <![CDATA[
>+            let title = this.getWindowTitleForBrowser(this.mCurrentBrowser);
>             if (window.TabView && TabView.isVisible()) {
>               // ToDo: this will be removed when we gain ability to draw to the menu bar.
>               // Bug 586175
>-              this.ownerDocument.title = TabView.windowTitle;
>-            } else {
>-              this.ownerDocument.title = this.getWindowTitleForBrowser(this.mCurrentBrowser);
>-            }
>+              title = TabView.windowTitle;
>+            }        
>+            this.ownerDocument.title = title;
>           ]]>
>         </body>
>       </method>

This change doesn't seem to make sense. It just makes us needlessly call getWindowTitleForBrowser in TabView mode.

>--- a/browser/locales/en-US/chrome/browser/tabbrowser.properties
>+++ b/browser/locales/en-US/chrome/browser/tabbrowser.properties
>@@ -1,8 +1,8 @@
> tabs.loading=Loadingâ¦
> tabs.emptyTabTitle=New Tab
> tabs.closeTab=Close Tab
> tabs.close=Close
> tabs.closeWarningTitle=Confirm close
> tabs.closeWarningMultipleTabs=You are about to close %S tabs. Are you sure you want to continue?
> tabs.closeButtonMultiple=Close tabs
>-tabs.closeWarningPromptMe=Warn me when I attempt to close multiple tabs
>+tabs.closeWarningPromptMe=Warn me when I attempt to close multiple tabs
>\ No newline at end of file

Drop this change too.

Comment 24

[Dietrich Ayala (:dietrich)]

Marking blocking+ since there's currently no visual indication of any kind that the reason all your tabs aren't showing is because you're "viewing a group".

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #24)
> Marking blocking+ since there's currently no visual indication of any kind that
> the reason all your tabs aren't showing is because you're "viewing a group".

That sounds more like bug 589010. This bug won't really solve that problem since we hide the window title by default on Windows Vista and 7.

Comment 26

[Aza Raskin [:aza]]

Attachment: Patch v5

Comment 27

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/13ea24576474

Comment 28

[Tracy Walker [:tracy]]

we should have a check for this in our test case for opening a Tab group

Comment 29

[Ed Lee :Mardak]

(In reply to comment #3)
> (1) If in a titled group: "[Group Name] — [Page Title]"
Any particular reason to have the group name before the page title instead of after?

Comment 30

[Aza Raskin [:aza]]

(In reply to comment #29)
> Any particular reason to have the group name before the page title instead of
> after?

Because of the hierarchy window > group > page in LTR languages.

Comment 31

[Ed Lee :Mardak]

So then should Private Browsing be moved to the front of the title instead of being at the end?

Comment 32

[Aza Raskin [:aza]]

Probably!

Comment 33

[Tracy Walker [:tracy]]

As part of Bug Week, thanks to gmontague for the new test case in Litmus.

Litmus ID 12877

Comment 34

[Jonathan Haas]

Correct me if I'm wrong, but the current patch seems to have no effect on Win 7 as the window title is nowhere to be shown. On the Window it has been replaced by the Firefox button, and even the taskbar icons are by default collapsed (they show no text).

So is there a bug / followup to fix this for window users, too?

Comment 35

[John Hawkinson]

(In reply to comment #34)
I'd like to second Jonathan's comment, under XP (FF4.0.1), this UI element isn't very visible.
The title is visible in truncated form in the taskbar, but so far truncated to not be useful because task bar is crowded (I get 1 character...). The Firefox button at the top of the window seems the ideal place, as sits on the title bar of the window which is otherwise empty wasted horizontal space the full width of the window.

But this bug appears untouched for the past 6 months -- is there a better venue for this, should I open a new bug, or find a discussion thread somewhere?

Thanks.