463253 - Application name displayed in title bar

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Stefan [:stefanh]]

It seems to me that the fix for bug 454949 missed that there's mac-specific code in nsContentTreeOwner.cpp (line 911 and so on) that takes care of the window title. That is, on mac we shouldn't display the application name in the window title bar.

Comment 1

[Dave Townsend [:mossop]]

In theory, bug 454949 should have brought us back to how Firefox 3 behaved, where tabbrowser always wins at setting the title. Can you give STR for what you believe to be a problem case?

Comment 2

[Dave Townsend [:mossop]]

I should note that you should not have any extensions like Nightly Tester Tools installed when you test this.

Comment 3

[Stefan [:stefanh]]

1) Launch Minefield, lets say that your start page is "http://www.mozilla.org/"
2) Look at the title in the window title bar
3) Notice that it says "Mozilla.org - Home of the Mozilla Project - Minefield"
4) Put back contenttitlesetting="true" in browser.xul
5) Launch Minefield and look at the title in the window title bar
6) Notice that it says "Mozilla.org - Home of the Mozilla Project"

3) is wrong, 6) is right.

Comment 4

[Stefan [:stefanh]]

Maybe I should clarify why it's happening. On mac, if you have contenttitlesetting="true", the titlemodifier is removed here: http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsContentTreeOwner.cpp#912

But now we have a titlemodifier, so the code in http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#649 now adds both titlemodifier and separator on mac.

Comment 5

[Stefan [:stefanh]]

Note here that bug 411929 kind of broke it on mac too, instead of "page title - Minefield (Private Browsing)", you'd probably want "page title (Private Browsing)" on mac.

Comment 6

[Stefan [:stefanh]]

Maybe there's some confusion here on why it's wrong to display the application name in the window title on mac. I'll try and explain why:

1) On mac, there's no confusion regarding what application a window belongs to. An application window doesn't have a menubar on mac, instead the menubar is situated at the top of the desktop. Now, this menubar also contains a "application" menu with the title "Minefield" (in this case). So, displaying "page title - Minefield" in the window title bar will just look silly. For comparison, Safari and Camino doesn't display any application name in their window titles.

I also think that the regression here is quite notable, since on mac we've been without the application name since 2003 (when Hyatt removed it).

Comment 7

[(no longer active)]

Attachment: Patch (v1)

Comment 8

[(no longer active)]

Comment on attachment 348025 [details] [diff] [review]
Patch (v1)

Argg, hit Enter too soon!

Totally untested patch, but I think it should work based on the comments in this bug.

If someone can test this on a Mac, it would be really great!

Comment 9

[(no longer active)]

Stefan suggested on IRC that another (possibly better) approach would be to nuke the titlemodifier attributes on Mac and handle this inside the js code, instead of the XUL file.  I have not worked out the details on how that fix would work, but it would also require the string changes as patch v1, and _may_ be a little more complex...

I'll leave the choice to mconnor as the reviewer here.

Comment 10

[Stefan [:stefanh]]

Comment on attachment 348025 [details] [diff] [review]
Patch (v1)

-<!-- LOCALIZATION NOTE (mainWindowPrivateBrowsing.titlemodifier): This will be appended to the window's title
-                                                                  inside the private browsing mode -->
-<!ENTITY mainWindowPrivateBrowsing.titlemodifier "&mainWindow.titlemodifier; (Private Browsing)">


You still have mainWindowPrivateBrowsing.titlemodifier in browser.xul ;-)

Comment 11

[(no longer active)]

Comment on attachment 348025 [details] [diff] [review]
Patch (v1)

(In reply to comment #10)
> You still have mainWindowPrivateBrowsing.titlemodifier in browser.xul ;-)

Oh, where's my mind?  New patch following...

Comment 12

[(no longer active)]

Attachment: Patch (v1.1)

Comment 13

[Stefan [:stefanh]]

Comment on attachment 348087 [details] [diff] [review]
Patch (v1.1)

Sorry, I should probably have thought about this yesterday, but it was late and I was tired: This won't work on pages without a title set, since both titledefault and titlemodifier is empty (well, titledefault isn't there at all). On mac, you will just have an empty titlebar when there's no page title. See comment #4 for how the behaviour was before the contenttitlesetting attribute was removed. Basically, when there is no page title, the code in tabbrowser relies on that there is a titledefault attribute set on mac only (set to the same value as the titlemodifier attribute) and no titlemodifier.

Comment 14

[Stefan [:stefanh]]

Some clarification for non-mac people; The "normal" behaviour is:

1) On pages with a title set, display "page title"
2) On pages with no title set, display "Application name"

I guess 2) could be discussed (Safari and Camino bot displays "Untitled"). But that should probably go in a separate bug - if we should display "Untitled", we should probably display "Untitled" instead of "(Untitled)" in the tab title.

Comment 15

[(no longer active)]

(In reply to comment #13)
> (From update of attachment 348087 [details] [diff] [review])
> Sorry, I should probably have thought about this yesterday, but it was late and
> I was tired: This won't work on pages without a title set, since both
> titledefault and titlemodifier is empty (well, titledefault isn't there at
> all). On mac, you will just have an empty titlebar when there's no page title.
> See comment #4 for how the behaviour was before the contenttitlesetting
> attribute was removed. Basically, when there is no page title, the code in
> tabbrowser relies on that there is a titledefault attribute set on mac only
> (set to the same value as the titlemodifier attribute) and no titlemodifier.

Stefan, I'm lost here.  Have you applied and tested this patch?  Based on bug 411929 comment 38, I added something which I _think_ should handle this case on Mac here: <http://hg.mozilla.org/mozilla-central/file/fbad4dfa05e5/browser/base/content/browser.js#l6906>.

Can you please give this patch a test?  If you want, I can prepare a try server build...

Comment 16

[Stefan [:stefanh]]

(In reply to comment #15)
> (In reply to comment #13)
> > (From update of attachment 348087 [details] [diff] [review] [details])
> > Sorry, I should probably have thought about this yesterday, but it was late and
> > I was tired: This won't work on pages without a title set, since both
> > titledefault and titlemodifier is empty (well, titledefault isn't there at
> > all). On mac, you will just have an empty titlebar when there's no page title.
> > See comment #4 for how the behaviour was before the contenttitlesetting
> > attribute was removed. Basically, when there is no page title, the code in
> > tabbrowser relies on that there is a titledefault attribute set on mac only
> > (set to the same value as the titlemodifier attribute) and no titlemodifier.
> 
> Stefan, I'm lost here.  Have you applied and tested this patch?

Yeah, I did and then I added comment #14. You made me re-apply the patch again, though - I wanted to check all cases:

When you're in private browsing mode, the following titles appear:

*about:privatebrowsing == "Private Browsing - (Private Browsing)
(I can imagine that the best would be just "Private Browsning" here)
*Page with title = "Page Title - (Private Browsing)"
*Page without title/blank tab = "(Private Browsing)"

When you're not in private browsing mode, the following titles appear:

*Page with title = "Page Title"
*Page without title/blank tab = ""

>Based on bug
> 411929 comment 38, I added something which I _think_ should handle this case

If you mean the comment re copying titlemodifier to titledefault, this doesn't happen anymore. This assumes that browser.xul has the contenttitlesetting attribute set to "true". That attribute was removed in bug 454949.

<http://hg.mozilla.org/mozilla-central/file/fbad4dfa05e5/browser/base/content/browser.js#l6906>.
This code sets titlemodifier and titledefault to "", when leaving private browsing mode.

Comment 17

[(no longer active)]

Attachment: Patch (v1.2)

Ah, I meant <http://hg.mozilla.org/mozilla-central/file/fbad4dfa05e5/browser/base/content/browser.js#l6872>, but anyway, can you try this new patch?  This one sets titledefault to title_normal, which should be "Application Name".  Let's see if this takes care of all the cases.  Note that I have not tested this myself.

I'll ask review on this when we make sure that it's working.

Comment 18

[Stefan [:stefanh]]

Comment on attachment 348776 [details] [diff] [review]
Patch (v1.2)

I don't understand what you're trying to achive here.

Comment 19

[(no longer active)]

(In reply to comment #18)
> (From update of attachment 348776 [details] [diff] [review])
> I don't understand what you're trying to achive here.

I tried setting titledefault to "Application Title" when entering the private browsing mode.  Previously, it was emptied when entering the private browsing mode.

Comment 20

[Stefan [:stefanh]]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 348776 [details] [diff] [review] [details])
> > I don't understand what you're trying to achive here.
> 
> I tried setting titledefault to "Application Title" when entering the private
> browsing mode.  Previously, it was emptied when entering the private browsing
> mode.

Right, but what about when you're not in private browsing mode and the page has no title? Have you read the tabbrowser code that I've mentioned?

Comment 21

[Stefan [:stefanh]]

I think this might be a bit tricky to fix if you don't change tabbrowser.

Comment 22

[Stefan [:stefanh]]

> *Page without title/blank tab = "(Private Browsing)"

Maybe this one should be "Minefield (Private Browsing)", to fit with the plain "Minefield" title on pages with no title in "normal" mode? beltzner or mconnor should probably settle this.

Comment 23

[Stefan [:stefanh]]

I'm a bit tired now, so some oddities could have slipped in here, but afaiu this is how it is if you don't change tabbrowser: You can try to mimic the old behavior on mac. That is, in browser.xul have a titledefault attribute set to the same as titlemodifier and have no titlemodifier. That would fix the problem with empty titles on pages without title/blank tabs on mac, since titledefault would set the title on empty pages. And since we don't have a titlemodifier on mac, nothing will be appended after the page title in the titlebar.

Now, the tricky part here is then to make this work with the code in browser.js (enter/exit safebrowsing). You can have the same title attribute values on all OS (on enter/exit pb), that should work fine.

When you enter private browsing mode it gets more complicated:

1) If you want something like "Minefield (Private Browsing)" to display on blank pages/blank tabs it could be achived by changing the titledefault attribute and not use any titlemodifier.
2) You might want "Private Browsing" to display on about:privatebrowsing. That'll work with 1).

3) You want something like "Page Title" - (Private Browsing) on pages with a title set. I'm not sure about the "(" and ")" here (or should it just be "Page Title (Private Browsing)"?), but you want something after the page title to indicate that we're in private browsing mode. That can be achived by using the titlemodifier attribute when entering private browsing mode, but then it'll clash with the titledefault attribute and you'll end up with both displayed on blank pages/tabs.

Comment 24

[(no longer active)]

Attachment: Patch (v1.3)

Well, I really wanted to do this without changing tabbrowser.xml, but it seems like that won't be an option really...

So, Stefan, can you test this patch?  This adds basically the same Mac title handling as is being done in nsContentTreeOwner.cpp inside tabbrowser.xml.

Thanks for being so patient in debugging my patches.  :-)

Comment 25

[Dave Townsend [:mossop]]

I don't think overwriting the attributes there is what you want to do at all. Maybe I've missed this suggestion already being shown as wrong but doesn't the following do what we want:

On Mac:

titledefault=Minefield
titlemenuseparator=
titlemodifier=
In private browsing mode: titlemodifier=(Private Browsing)

Elsewhere:

titledefault=
titlemenuseparator=-
titlemodifier=Minefield
In private browsing mode: titlemodifier=Minefield (Private Browsing)

Comment 26

[Stefan [:stefanh]]

(In reply to comment #25)
> I don't think overwriting the attributes there is what you want to do at all.
> Maybe I've missed this suggestion already being shown as wrong but doesn't the
> following do what we want:
> 
> On Mac:
> 
> titledefault=Minefield
> titlemenuseparator=
> titlemodifier=
> In private browsing mode: titlemodifier=(Private Browsing)

This looks right if we want "Page Title(Private Browsing)". Otherwise you'd want the titlemenuseparator to be "-" in private browsing mode, I think. When I ment changing tabbrowser, I didn't ment overwriting the attributes - I was more thinking of doing something different when we're in private browsing mode.

Comment 27

[Dave Townsend [:mossop]]

(In reply to comment #26)
> (In reply to comment #25)
> > I don't think overwriting the attributes there is what you want to do at all.
> > Maybe I've missed this suggestion already being shown as wrong but doesn't the
> > following do what we want:
> > 
> > On Mac:
> > 
> > titledefault=Minefield
> > titlemenuseparator=
> > titlemodifier=
> > In private browsing mode: titlemodifier=(Private Browsing)
> 
> This looks right if we want "Page Title(Private Browsing)". Otherwise you'd
> want the titlemenuseparator to be "-" in private browsing mode, I think. When I
> ment changing tabbrowser, I didn't ment overwriting the attributes - I was more
> thinking of doing something different when we're in private browsing mode.

Well I'd maybe just have titlemodifier=" (Private Browsing)" but whatever effect you want yeah.

Comment 28

[Stefan [:stefanh]]

(In reply to comment #27)

> Well I'd maybe just have titlemodifier=" (Private Browsing)"

Yeah, I think you're right. It will probably look better without titlemenuseparator.

Ehsan, since we probably are in different timezones - if you want to solve this quick, you might be able to play/test with this even without a mac if you flip the ifdef's.

Comment 29

[Stefan [:stefanh]]

Hmm, Dave: When I look at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#769 it seems to me that we need a "-" for the titlemenuseparator. Or is this code not used?

Comment 30

[Dave Townsend [:mossop]]

(In reply to comment #29)
> Hmm, Dave: When I look at
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#769
> it seems to me that we need a "-" for the titlemenuseparator. Or is this code
> not used?

Hmm I think that's not very smart and we should probably have an additional attribute to control that.

Comment 31

[(no longer active)]

(In reply to comment #28)
> Ehsan, since we probably are in different timezones - if you want to solve this
> quick, you might be able to play/test with this even without a mac if you flip
> the ifdef's.

That's a good idea.  I didn't have time to look at this since my last patch, but I'll probably take care of it tomorrow.

Comment 32

[(no longer active)]

Attachment: Patch (v2)

I think this is what we want.

Effect of this patch (on Mac):

  Outside private mode:
    Page with title: "Page title"
    Page without title: "Minefield"
  Inside private mode:
    about:privatebrowsing: "Minefield - (Private Browsing)"
    Page with title: "Page title - (Private Browsing)"
    Page without title: "Minefield - (Private Browsing)"

On other platforms, this patch doesn't change anything.

Comment 33

[(no longer active)]

Attachment: Patch (v2.1)

We also need to change browser_privatebrowsing_ui.js so that it passes on Mac.  This patch fixes the test, with no changes to the code.

Comment 34

[Dave Townsend [:mossop]]

We probably ought to try to build some tests for this.

Comment 35

[Mike Connor [:mconnor]]

Comment on attachment 350468 [details] [diff] [review]
Patch (v2.1)

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

> #ifdef XP_MACOSX // see bug 411929 comment 38 for the reason behind this
>       docElement.setAttribute("titlemodifier",
>         docElement.getAttribute("titlemodifier_privatebrowsing"));
>-      docElement.setAttribute("titledefault", "");
>-#else
>-      docElement.setAttribute("title",
>-        docElement.getAttribute("title_privatebrowsing"));
>+#else
>       docElement.setAttribute("titlemodifier",
>         docElement.getAttribute("titlemodifier_privatebrowsing"));
> #endif

>+#ifdef XP_MACOSX // see bug 411929 comment 38 for the reason behind this
>+      docElement.setAttribute("titlemodifier",
>+        docElement.getAttribute("titlemodifier_normal"));
>+#else
>       docElement.setAttribute("titlemodifier",
>         docElement.getAttribute("titlemodifier_normal"));
> #endif

er, both sides of the #ifdef seem identical in both of these cases, unless I'm on crack.  The ifdefs in browser.xul seem to cover this.

>       docElement.setAttribute("browsingmode", "normal");
>     }
>     else
>       this._privateBrowsingAutoStarted = false;
>   },
>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>+#ifdef XP_MACOSX
>+        title_privatebrowsing="&mainWindow.title;&mainWindow.titlemodifiermenuseparator;&mainWindow.titlePrivateBrowsingSuffix;"
>+        titledefault="&mainWindow.title;"
>+        titlemodifier=""
>+        titlemodifier_normal=""
>+        titlemodifier_privatebrowsing="&mainWindow.titlePrivateBrowsingSuffix;"
>+        titlemenuseparator="&mainWindow.titlemodifiermenuseparator;"
>+#else
>+        title_privatebrowsing="&mainWindow.titlemodifierPrivateBrowsing;"
>         titlemodifier="&mainWindow.titlemodifier;"
>         titlemodifier_normal="&mainWindow.titlemodifier;"
>-        titlemodifier_privatebrowsing="&mainWindowPrivateBrowsing.titlemodifier;"
>+        titlemodifier_privatebrowsing="&mainWindow.titlemodifierPrivateBrowsing;"
>         titlemenuseparator="&mainWindow.titlemodifiermenuseparator;"
>+#endif

titlemenuseparator should be outside of the ifdef.

r=me with comments addressed

Comment 36

[(no longer active)]

(In reply to comment #35)
> er, both sides of the #ifdef seem identical in both of these cases, unless I'm
> on crack.  The ifdefs in browser.xul seem to cover this.

Nope, you're not on crack!  I wanted to make *sure* that no non-Mac code gets changed, and I just pushed that too far I guess.  :-)

I'll land this on mozilla-1.9.1 after it bakes on mozilla-central, and then it would be late-l10n.

Comment 37

[(no longer active)]

(In reply to comment #34)
> We probably ought to try to build some tests for this.

Is it alright to hard code titles in the tests to compare the real titles against expected ones?

Comment 38

[Dave Townsend [:mossop]]

(In reply to comment #37)
> (In reply to comment #34)
> > We probably ought to try to build some tests for this.
> 
> Is it alright to hard code titles in the tests to compare the real titles
> against expected ones?

Yeah I think that would be a good way to start

Comment 39

[(no longer active)]

Attachment: Test

Here are the tests needed to test the window title on both Mac and other platforms.

Comment 40

[(no longer active)]

Pushed to mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/16d246e86650>

Comment 41

[(no longer active)]

Attachment: mq patch for check-in on 1.9.1

Comment 42

[(no longer active)]

Landed on 1.9.1 <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9940f2cb88b9>

Comment 43

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081222 Shiretoko/3.1b3pre. Adding relevant keyword.

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20081222 Minefield/3.2a1pre

Comment 44

[(no longer active)]

Comment on attachment 351594 [details] [diff] [review]
Test

Gavin: could you please take a look at this test?

Comment 45

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

Comment on attachment 351594 [details] [diff] [review]
Test

What is about:privatebrowsing's title blank on Mac, out of curiosity?

Comment 46

[(no longer active)]

(In reply to comment #45)
> (From update of attachment 351594 [details] [diff] [review])
> What is about:privatebrowsing's title blank on Mac, out of curiosity?

Hmm, I'm not sure what this question means.  If you meant "why" instead of "what", it's because otherwise it would be something like "Private Browsing - (Private Browsing)" which doesn't look too good.

Comment 47

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

Yeah, I meant why.

Comment 48

[(no longer active)]

Tests landed on mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/3bf433cc62cc>

Comment 49

[(no longer active)]

Tests landed on mozilla-1.9.1 as well: <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7fd811585212>