Closed
Bug 457548
Opened 16 years ago
Closed 15 years ago
Window titles are incorrect on mac
Categories
(SeaMonkey :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.0b2
People
(Reporter: stefanh, Assigned: stefanh)
References
Details
Attachments
(3 files, 6 obsolete files)
1.68 KB,
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
882 bytes,
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
8.09 KB,
patch
|
mnyromyr
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•16 years ago
|
||
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)
Assignee | ||
Comment 2•16 years ago
|
||
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?
Comment 3•16 years ago
|
||
> 1) Display "PageTitle - Composer"
> 2) Display "Edit: PageTitle"
I'd vote for (1), 'cos ":" in titles is ugly.
Comment 4•16 years ago
|
||
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+
Assignee | ||
Comment 5•16 years ago
|
||
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)
Assignee | ||
Comment 6•16 years ago
|
||
(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.
Assignee | ||
Comment 7•16 years ago
|
||
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?
Comment 8•16 years ago
|
||
Works fine for me in Windows with or without the patch.
Assignee | ||
Comment 9•16 years ago
|
||
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 {...}")
Assignee | ||
Comment 10•16 years ago
|
||
Hmm, looking at the js in debugQANavigatorOverlay.xul, maybe I can remove the titledefault attribute there if I don't have any titlemodifier attribute...
Comment 11•16 years ago
|
||
(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)
Assignee | ||
Comment 12•16 years ago
|
||
ajschult tells me that a new, blank tab just displays "SeaMonkey" in the title (release build with debugQA installed).
Assignee | ||
Comment 13•16 years ago
|
||
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)
Assignee | ||
Comment 14•16 years ago
|
||
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 15•16 years ago
|
||
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+
Assignee | ||
Comment 16•16 years ago
|
||
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)
Assignee | ||
Comment 17•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #341753 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Comment 18•16 years ago
|
||
Once browser is fixed, I'll resolve this and continue fixing the rest in bug 56995 .
Comment 19•16 years ago
|
||
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 20•16 years ago
|
||
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.
Assignee | ||
Comment 21•16 years ago
|
||
(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.
Comment 22•16 years ago
|
||
> 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?")
Assignee | ||
Comment 23•16 years ago
|
||
(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).
Assignee | ||
Comment 24•16 years ago
|
||
> 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)
Assignee | ||
Comment 25•16 years ago
|
||
> - 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.
Updated•16 years ago
|
Attachment #342096 -
Flags: superreview?(neil)
Attachment #342096 -
Flags: superreview+
Attachment #342096 -
Flags: review?(neil)
Attachment #342096 -
Flags: review+
Assignee | ||
Comment 26•16 years ago
|
||
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)
Assignee | ||
Comment 28•16 years ago
|
||
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
Assignee | ||
Comment 29•15 years ago
|
||
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.
Assignee | ||
Comment 30•15 years ago
|
||
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?
Assignee | ||
Comment 31•15 years ago
|
||
I think we might be able to get back the old behaviour by just removing the contenttitlesetting attribute.
Assignee | ||
Updated•15 years ago
|
Assignee | ||
Comment 32•15 years ago
|
||
(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.
Assignee | ||
Comment 33•15 years ago
|
||
Not blocking since we fix the outstanding issues with the patch in bug 462997.
Flags: blocking-seamonkey2.0b1?
Assignee | ||
Comment 34•15 years ago
|
||
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)
Updated•15 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 35•15 years ago
|
||
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
Assignee | ||
Comment 36•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Target Milestone: seamonkey2.0a2 → seamonkey2.0b2
Comment 37•15 years ago
|
||
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-
Assignee | ||
Comment 38•15 years ago
|
||
(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)
Assignee | ||
Comment 39•15 years ago
|
||
(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.
Assignee | ||
Comment 40•15 years ago
|
||
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)
Assignee | ||
Comment 41•15 years ago
|
||
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 42•15 years ago
|
||
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 43•15 years ago
|
||
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+
Comment 44•15 years ago
|
||
(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 ;-)
Assignee | ||
Comment 45•15 years ago
|
||
(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.
Comment 46•15 years ago
|
||
(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?
Comment 47•15 years ago
|
||
> I did. But I'm still none the wiser - what's the point of an unused variable?
Argl. Forget it. :-(
Assignee | ||
Comment 48•15 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•