Closed Bug 457548 Opened 16 years ago Closed 15 years ago

Window titles are incorrect on mac

Categories

(SeaMonkey :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0b2

People

(Reporter: stefanh, Assigned: stefanh)

References

Details

Attachments

(3 files, 6 obsolete files)

You'll see this if you disable debugQA, has been like this since we became a MOZ_XUL_APP.

- Browser window titles are "PageTitle - " (used to be PageTitle - SeaMonkey")
- Composer window titles are "PageTitle - " (used to be PageTitle - Composer")

The reason for this is can be found in http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsContentTreeOwner.cpp#913.

Note that displaying the appName in window titles is not correct on mac, so we don't want the old behaviour.
Blocks: 56995
Attached patch Part 1: fix tabbrowser (obsolete) — Splinter Review
I'm going to do this in parts. This is the tabbrowser part - it basically makes us behave the same as firefox.
Attachment #340807 - Flags: superreview?(neil)
Attachment #340807 - Flags: review?(neil)
For editor, I think we have 2 choices:

1) Display "PageTitle - Composer"
2) Display "Edit: PageTitle"

The point here is to distinguish from a page loaded in Composer and a page loaded in the browser - so I don't think we should go with just displaying "PageTitle" on mac.

Thoughts?
> 1) Display "PageTitle - Composer"
> 2) Display "Edit: PageTitle"

I'd vote for (1), 'cos ":" in titles is ugly.
Comment on attachment 340807 [details] [diff] [review]
Part 1: fix tabbrowser

Don't you want to get someone to test this on a Mac?
Attachment #340807 - Flags: superreview?(neil) → superreview+
Comment on attachment 340807 [details] [diff] [review]
Part 1: fix tabbrowser

(In reply to comment #4)
> (From update of attachment 340807 [details] [diff] [review])
> Don't you want to get someone to test this on a Mac?
Right, sorry. Karsten: in order to test this you need to disable debugQA.
Attachment #340807 - Flags: review?(neil) → review?(mnyromyr)
(In reply to comment #3)
> > 1) Display "PageTitle - Composer"
> > 2) Display "Edit: PageTitle"
> 
> I'd vote for (1), 'cos ":" in titles is ugly.

We have "Compose: Subject". My idea was that having "Edit: PageTitle" sort of refers to that you can do something with the page that you have loaded.
I do notice that with debugQA we don't seem to update the title that well (before and after the patch):

1) Load mozilla.org
2) Open a new, blank tab
3) Switch to mozilla.org
4) Switch back to the blank tab

But I guess this is known?
Works fine for me in Windows with or without the patch.
With the patch, window title in 2) is "SeaMonkey", window title in 4) is "SeaMonkey - SeaMonkey {...}"

With an unpatched trunk build 2) is the same, but 4) is OK (only "SeaMonkey {...}")
Hmm, looking at the js in debugQANavigatorOverlay.xul, maybe I can remove the titledefault attribute there if I don't have any titlemodifier attribute...
(In reply to comment #9)
> With the patch, window title in 2) is "SeaMonkey", window title in 4) is
> "SeaMonkey - SeaMonkey {...}"
> 
> With an unpatched trunk build 2) is the same, but 4) is OK (only "SeaMonkey
> {...}")
Correct for both is "SeaMonkey {...}" (which flashs up very briefly for me)
ajschult tells me that a new, blank tab just displays "SeaMonkey" in the title (release build with debugQA installed).
While I'm thinking of how to handle debugQA - here's a patch for editor (should have the same impact on win/mac/nix). It seems to me that SetDocumentTitle(title) is never called when you switch between normal and html source mode in composer. My solution here is to always call the function (not sure how good that is). Since I think we want "- Composer" to be appended to the title and titlemodifier is "Composer", contentsetting="false" looks like the best way here.
Attachment #341750 - Flags: superreview?(neil)
Attachment #341750 - Flags: review?(neil)
Attached patch Fix debugQA (obsolete) — Splinter Review
This fixes 4) for me. What happens for me is that without any page title, I get both attribute values in the title - titlemodifier is removed, but debugQA adds it back.
Comment on attachment 341750 [details] [diff] [review]
Composer fix (pushed)

>+        SetDocumentTitle(title);        
Nit: trailing spaces. And there weren't any before, so you've no excuse!

>+        contenttitlesetting="false"
Nit: you can probably simply delete this attribute.
Attachment #341750 - Flags: superreview?(neil)
Attachment #341750 - Flags: superreview+
Attachment #341750 - Flags: review?(neil)
Attachment #341750 - Flags: review+
Comment on attachment 341750 [details] [diff] [review]
Composer fix (pushed)

Pushed (without whitespace and contenttitlesetting attribute):
http://hg.mozilla.org/comm-central/rev/49fff21693a1
Attachment #341750 - Attachment description: Composer fix → Composer fix (pushed)
Comment on attachment 341753 [details] [diff] [review]
Fix debugQA

This one assumes that attachment #340807 [details] [diff] [review] has been applied. This fixes 4) for me. I have no solution for 2), but I think that can go in another bug since the behaviour has been there for a while. One interesting thing is that if you open a popup window with 'javascript:window.open("","","menubar=no,location=no,resizable=yes,scrollbars=no,status=no");' , I get "about:blank - SeaMonkey" in the titlebar (without debugQA). Looking at the code, the behavour looks expected. However, with Minefield, I just get "Minefield" - but I can't seem to find what they're doing different here.
Attachment #341753 - Flags: superreview?(neil)
Attachment #341753 - Flags: review?(mnyromyr)
Attachment #341753 - Flags: superreview?(neil) → superreview+
Once browser is fixed, I'll resolve this and continue fixing the rest in bug 56995 .
Using a trunk debug build of 2008-09-29, opening Composer showed "Composer" during load, "untitled -" afterwards and "xxx" when setting xxx as the title; debugQA had no influence there.
With attachment 341750 [details] [diff] [review], it becomes ""/"untitled - Composer"/"xxx - Composer", respectively. Again, no debugQA influence.
I guess the "" during load is acceptable.


In the following, I specifically don't take bug 56995 into account, i.e. I consider "xxx - SeaMonkey {Build ID: 20080929205810}" to be the expected value for normal pages.

So, same build, now browser: new empty tabs cause title "SeaMonkey", normal pages with title xxx show "xxx - SeaMonkey {Build ID: 20080929205810}", switching normal tab to empty tab gets just "SeaMonkey {Build ID: 20080929205810}". 
-> empty tab title isn't consistent, normal page title is okay.

Without debugQA, this gets "SeaMonkey"/"xxx -"/"", respectively.
-> empty tab title isn't consistent, normal page title is broken, too.

With attachment 340807 [details] [diff] [review] applied, this becomes "SeaMonkey"/"xxx - SeaMonkey {Build ID: 20080929205810}"/"SeaMonkey - SeaMonkey {Build ID: 20080929205810}".
-> empty tab title isn't consistent, normal page title is okay.

Without debugQA it's "SeaMonkey"/"xxx"/"SeaMonkey", respectively.
-> empty tab title _is_ consistent, but normal page title lacks extension.

Now enter attachment 341753 [details] [diff] [review]:

With debugQA: "SeaMonkey"/"xxx - SeaMonkey {Build ID: 20080929205810}"/"SeaMonkey {Build ID: 20080929205810}".
-> empty tab title isn't consistent, normal page title is okay.

Without debugQA: "SeaMonkey"/"xxx"/"SeaMonkey".
-> empty tab title _is_ consistent, but normal page title lacks extension.


I think this is still a complete mess, regardless of whether bug 56995 is considered useful/wanted or not. 
First, we should define
- how normal page titles should look 
- how empty page titles should look
- how these should differ between debug and optimized builds
- how these should differ between development and release builds (if at all)

BTW: for empty page titles, e.g. Safari displays "untitled"...
Comment on attachment 341750 [details] [diff] [review]
Composer fix (pushed)

>-        contenttitlesetting="true"
>+        contenttitlesetting="false"
>         titlemodifier="&editorWindow.titlemodifier;"
>         titlemenuseparator="&editorWindow.titlemodifiermenuseparator;"
You could replace contenttitlesetting with title="&editorWindow.titlemodifier;" so that base title appears while the page is loading.
(In reply to comment #19)
> In the following, I specifically don't take bug 56995 into account, i.e. I
> consider "xxx - SeaMonkey {Build ID: 20080929205810}" to be the expected value
> for normal pages.

The point was to fix bug 56995 for browser at the same time. Once this is fixed, it's only MailNews and View Partial Source left.
> The point was to fix bug 56995 for browser at the same time.

I don't think you should fix that here, just stay in this bug's scope.
(One of the open questions to be discussed in 56995 is "where would we put the build id?")
(In reply to comment #22)
> > The point was to fix bug 56995 for browser at the same time.
> I don't think you should fix that here, just stay in this bug's scope.

The scope is "window titles are incorrect on mac" ;-) Normal page titles should not have an app name appended to the title. This is something that have annoyed me for years.

> (One of the open questions to be discussed in 56995 is "where would we put the
> build id?")

I don't really think that is such an important issue - what really is important is that we have correct window titles in our forthcoming releases. I can also live with the debugQA behaviour that my patches introduces. That is, I don't really change the debugQA window titles here. Empty page titles are inconsistent also without the patch when debugQA is installed (on all OS, fwiw).
> You could replace contenttitlesetting with title="&editorWindow.titlemodifier;"
> so that base title appears while the page is loading.
Good point.
Attachment #342096 - Flags: superreview?(neil)
Attachment #342096 - Flags: review?(neil)
> - how normal page titles should look 

Window title should just be the page title (no app name)

> - how empty page titles should look

The default behaviour is to display the app name, see http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsContentTreeOwner.cpp#706 and note that mTitleDefault is the titlemodifier on mac (see http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsContentTreeOwner.cpp#912). So, I would think that the best option would be to just display the app name here.

> - how these should differ between debug and optimized builds
> - how these should differ between development and release builds (if at all)

There shouldn't be any difference. Final release builds doesn't have debugQA installed, though. If you install debugQA, I think the optional on mac would be to just have the build ID appended to the page title and to show appName+buildID on pages without a title (consistent with non-debugQA behaviour).
> 
> BTW: for empty page titles, e.g. Safari displays "untitled"...

Yeah, I know (Camino does that too). Opera shows the address. We could do it the Safari/Camino way, but that wold require more work and I'm not sure it's the right thing. For example, should we then show the build ID together with untitled? I think I saw a comment in a bug where someone said that he considered the Safari behaviour to be a bug.
Attachment #342096 - Flags: superreview?(neil)
Attachment #342096 - Flags: superreview+
Attachment #342096 - Flags: review?(neil)
Attachment #342096 - Flags: review+
Comment on attachment 342096 [details] [diff] [review]
editor.xul followup per comment #20 (pushed)

Pushed changeset 554:004c7427680a
Attachment #342096 - Attachment description: editor.xul followup per comment #20 → editor.xul followup per comment #20 (pushed)
Putting this on blocking radar (2.0).
Flags: blocking-seamonkey2?
Karsten and I talked about this (irc), for some time ago. Karsten didn't liked that we're displaying the appName for pages without a title when we in fact doesn't display the appName for pages with a title. I sort of agree with him - it is inconsistent. However, I'm not sure I want to hack nsContentTreeOwner.cpp at this stage. I also expect a lot of discussion on what we should display for empty page titles. For example, I'd say that "Untitled" would be reasonable - but that doesn't fit with what we display in the tab, and showing "(Untitled)" in the titlebar isn't acceptable imo (and I'm not sure that I'll be able to win when it comes to changing the tab title). So, I'm not sure of how to proceed here.
Status: ASSIGNED → NEW
Blocks: 462997
OK, so the solution here is probably to combine removing the contenttitlesetting attribute in navigator.xul and make tabbrowser not displaying the appname on mac. On empty pages we have then the option to either display just the appname (a bit inconsistent, but in line with Firefox) or just display "Untitled" (like Safari). If we go for "Untitled", I think we should display it for tab titles as well. Whatever the solution is, it has to work with debugQA and other extensions that changes the window title.
When bug 202315 is fixed we won't have debugQA for beta - this makes me think this is a blocker for beta1. We should at least fix this so we keep the "old" (even though it's incorrect) behaviour. FYI, comment #0 only applies for browser (rest is fixed).
Flags: blocking-seamonkey2? → blocking-seamonkey2.0b1?
I think we might be able to get back the old behaviour by just removing the contenttitlesetting attribute.
No longer blocks: 462997
Depends on: 462997
(In reply to comment #31)
> I think we might be able to get back the old behaviour by just removing the
> contenttitlesetting attribute.

The patch in bug 462997 will get back the old behaviour.
Not blocking since we fix the outstanding issues with the patch in bug 462997.
Flags: blocking-seamonkey2.0b1?
Attached patch New attempt (obsolete) — Splinter Review
Here's a new attemp in trying to fix the last issue with our window titles on mac.
Attachment #340807 - Attachment is obsolete: true
Attachment #341753 - Attachment is obsolete: true
Attachment #340807 - Flags: review?(mnyromyr)
Attachment #341753 - Flags: review?(mnyromyr)
Status: NEW → ASSIGNED
Attached patch Another attempt (obsolete) — Splinter Review
It appears that we can set the window title with nsIBaseWindow (Neils idea), then we can still use the contenttitlesetting attribute.
Attachment #393457 - Attachment is obsolete: true
Attached patch new attempt - fix tabbrowser (obsolete) — Splinter Review
This patch will result in the following behaviour:

1) Mac window titles will be the same as win/nix when debugQA is installed
2) On mac, we behave like this:
   - Pages with titles: window title and tab label = page title
   - Pages without a title: window title and tab label = uri
   - about:blank: window title and tab label = "Untitled"
Attachment #396061 - Attachment is obsolete: true
Attachment #396137 - Flags: superreview?(neil)
Attachment #396137 - Flags: review?(mnyromyr)
Target Milestone: seamonkey2.0a2 → seamonkey2.0b2
Comment on attachment 396137 [details] [diff] [review]
new attempt - fix tabbrowser

>+            if (aURI) {
[Should always be true.]

>+              try {
>+                aURI = this.mURIFixup.createExposableURI(aURI).spec;
>+              } catch (e) {
>+              }
Still need the spec even if we fail to create an exposable URI.

>+            if (aURI && aURI != "about:blank") {
[aURI should still always be true!]
Reversing the test will make the logic flow easier to follow.

>+              if (!docTitle)
>+                docTitle = this.mStringBundle.getString("tabs.untitled");
Please comment here that we're overriding contenttitlesetting because we don't want to use titledefault here.

>-            var browser = aTab.linkedBrowser;
Still need this ;-)

>-            var crop = "end";
>+            var cropping = "end";
Why the renaming?

>+              title = this.getTitleForURI(this.mCurrentBrowser.currentURI);
Wrong browser.

>       // Set the new title modifier
>       document.documentElement.setAttribute("titlemodifier", titlemodifier);
What about titledefault?
Attachment #396137 - Flags: superreview?(neil) → superreview-
Attached patch New version (obsolete) — Splinter Review
(In reply to comment #37)
> (From update of attachment 396137 [details] [diff] [review])
> >+            if (aURI) {
> [Should always be true.]


Right, I start with the try direct
> >+              try {
> >+                aURI = this.mURIFixup.createExposableURI(aURI).spec;
> >+              } catch (e) {
> >+              }
> Still need the spec even if we fail to create an exposable URI.

Fixed.

> >+            if (aURI && aURI != "about:blank") {
> [aURI should still always be true!]
> Reversing the test will make the logic flow easier to follow.
Done.
> >+              if (!docTitle)
> >+                docTitle = this.mStringBundle.getString("tabs.untitled");
> Please comment here that we're overriding contenttitlesetting because we don't
> want to use titledefault here.
Done.
> >-            var browser = aTab.linkedBrowser;
> Still need this ;-)
yes, fixed. 
> >-            var crop = "end";
> >+            var cropping = "end";
> Why the renaming?
I thought it looked nicer with "aTab.crop = cropping" than "aTab.crop = crop;". If you don't like it, I keep the "crop".
> >+              title = this.getTitleForURI(this.mCurrentBrowser.currentURI);
> Wrong browser.
Oops. Fixed.
> >       // Set the new title modifier
> >       document.documentElement.setAttribute("titlemodifier", titlemodifier);
> What about titledefault?


I don't know if there's any point removing it - we don't use it and it seems that this happens after it has been removed by contenttitlesetting.
Attachment #396137 - Attachment is obsolete: true
Attachment #396266 - Flags: superreview?(neil)
Attachment #396266 - Flags: review?(mnyromyr)
Attachment #396137 - Flags: review?(mnyromyr)
(In reply to comment #38)
> Created an attachment (id=396266) [details]
> New version
> 
> (In reply to comment #37)

> I don't know if there's any point removing it - we don't use it and it seems
> that this happens after it has been removed by contenttitlesetting.

document.documentElement.setAttribute("titlemodifier", titlemodifier);
happens after the titlemodifier has been removed by contenttitlesetting.
Comment on attachment 396266 [details] [diff] [review]
New version

Sorry, I think I made some mistake here...
Attachment #396266 - Attachment is obsolete: true
Attachment #396266 - Flags: superreview?(neil)
Attachment #396266 - Flags: review?(mnyromyr)
Attached patch Another versionSplinter Review
I forgot to include the diff for navigator.xul... This version keeps the crop var and also removes the titledefault (in debugQABuildIDOnLoad).
Attachment #396280 - Flags: superreview?(neil)
Attachment #396280 - Flags: review?(mnyromyr)
Comment on attachment 396280 [details] [diff] [review]
Another version

>+            } catch (e) {
>+                aURI = aURI.spec;
Nit: incorrect indentation

>             // If location bar is hidden and the URL type supports a host,
>             // add the scheme and host to the title to prevent spoofing.
Bah, this is harder than it looks; on the Mac you might already have the URL.
Let's get this change in and then we can think about what to do in this case.
For instance, we might skip this for the Mac, as you can just click the pill.

>+      // titlemodifier attribute. We don't use titledefault, but lets remove it
Nit: let's
Attachment #396280 - Flags: superreview?(neil) → superreview+
Comment on attachment 396280 [details] [diff] [review]
Another version

Just a couple of nits:

>+++ b/suite/browser/tabbrowser.xml
>+      <method name="getTitleForURI">
...
>+            if (aURI == "about:blank")
>+              aURI = "";
>+            else {

If one if-branch has braces, the other should have as well.

>+              try {
>+                let characterSet = this.mCurrentBrowser.contentDocument.characterSet;
>+                const textToSubURI = Components.classes["@mozilla.org/intl/texttosuburi;1"]
>+                                               .getService(Components.interfaces.nsITextToSubURI);
>+                aURI = textToSubURI.unEscapeNonAsciiURI(characterSet, aURI);

Using const here is a bit esoteric, but if you insist, please add the missing k prefix.

>+              if (!docTitle)
>+                // Here we actually override contenttitlesetting, because we
>+                // don't want the titledefault value.
>+                docTitle = this.mStringBundle.getString("tabs.untitled");

Please use braces here.

>             if (docTitle) {
>               newTitle += docElement.getAttribute("titlepreface");
>               newTitle += docTitle;

Drop the unnecessary assignment, while you're at it.

>@@ -1089,44 +1130,25 @@
>             var browser = aTab.linkedBrowser;

Unused variable.


r=me with nits fixed.
Attachment #396280 - Flags: review?(mnyromyr) → review+
(In reply to comment #43)
> (From update of attachment 396280 [details] [diff] [review])
> >+            if (aURI == "about:blank")
> >+              aURI = "";
> >+            else {
> If one if-branch has braces, the other should have as well.
Or an alternative is to return ""; and not have an else block at all.

> >+                const textToSubURI = Components.classes["@mozilla.org/intl/texttosuburi;1"]
> Using const here is a bit esoteric, but if you insist, please add the missing k prefix.
This was just moved code, but you could change it to use var while you're there.

> >+              if (!docTitle)
> >+                // Here we actually override contenttitlesetting, because we
> >+                // don't want the titledefault value.
> >+                docTitle = this.mStringBundle.getString("tabs.untitled");
> Please use braces here.
(This is to avoid confusion because the comment makes the block look longer.)

> >             if (docTitle) {
> >               newTitle += docElement.getAttribute("titlepreface");
> >               newTitle += docTitle;
> Drop the unnecessary assignment, while you're at it.
I don't know what this means... maybe because we don't provide titlepreface? I'm not sure what we should do if an extension adds one.

> >             var browser = aTab.linkedBrowser;
> Unused variable.
I think Mnyromyr might want to read comment #37 and related comments ;-)
(In reply to comment #44)
> (In reply to comment #43)
> > (From update of attachment 396280 [details] [diff] [review])
> > >+            if (aURI == "about:blank")
> > >+              aURI = "";
> > >+            else {
> > If one if-branch has braces, the other should have as well.
> Or an alternative is to return ""; and not have an else block at all.

Good idea.
> 
> > >+                const textToSubURI = Components.classes["@mozilla.org/intl/texttosuburi;1"]
> > Using const here is a bit esoteric, but if you insist, please add the missing k prefix.
> This was just moved code, but you could change it to use var while you're
> there.
Sure.
> 
> > >+              if (!docTitle)
> > >+                // Here we actually override contenttitlesetting, because we
> > >+                // don't want the titledefault value.
> > >+                docTitle = this.mStringBundle.getString("tabs.untitled");
> > Please use braces here.
> (This is to avoid confusion because the comment makes the block look longer.)
> 
> > >             if (docTitle) {
> > >               newTitle += docElement.getAttribute("titlepreface");
> > >               newTitle += docTitle;
> > Drop the unnecessary assignment, while you're at it.
> I don't know what this means... maybe because we don't provide titlepreface?
> I'm not sure what we should do if an extension adds one.

I think we should keep it for now.
(In reply to comment #44)
> > >             var browser = aTab.linkedBrowser;
> > Unused variable.
> I think Mnyromyr might want to read comment #37 and related comments ;-)

I did. But I'm still none the wiser - what's the point of an unused variable?
> I did. But I'm still none the wiser - what's the point of an unused variable?

Argl. Forget it. :-(
Landed with comments addressed (used return "";), fixed braces and used a let instead of the const, dropped the assignment etc:

http://hg.mozilla.org/comm-central/rev/d5cabc748ec7
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Depends on: 591550
You need to log in before you can comment on or make changes to this bug.