126903 - XUL file window title should appear on browser title bar

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Graham Robertson]

When displaying a XUL file in the Mozilla browser the title in the
window element should show up on the browsers title bar. Currently the browser
window inherits it's title from the most recent html web page displayed.
This is very annoying and confusing. I have also tried to set the title with
some <html:head> etc elements but with no success.

See attachment for example

<window
    orient="vertical"
    title="Graham's Bugs"
    xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

Comment 1

[Graham Robertson]

Attachment: XUL page with a window title that is not displayed

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Sets the document title to the title attribute of the root element.

Comment 3

[Axel Hecht]

tested the patch, works.
It probably doesn't apply anymore, the code moved a bit, I applied it with 
copy'n'paste.
Note that the html content sink sets the title on DidBuildModel, but the XUL
content sink seems to do stuff like that at the place where bz put it.
Should the code check that it's dealing with a <window> ?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Should the code check that it's dealing with a <window> ?

I considered that....  The main object of the patch was to be as fast as
possible (so as not to regress Txul) while fixing this bug.

Now that I think on it, using HasAttr and then GetAttr only when the attr is
present may be faster yet, in the no-title case (which is the common case).

Comment 5

[Gervase Markham [:gerv]]

bz/axel: can you drive this patch to checkin? Bugzilla is beginning to do remote
XUL stuff (see bug 156548) and having this fixed would be really handy. And from
what you say, it doesn't sound too complex.

Gerv

Comment 6

[Boris Zbarsky [:bzbarsky]]

*** Bug 170519 has been marked as a duplicate of this bug. ***

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

+        nsCOMPtr<nsIAtom> titleAtom = dont_AddRef( NS_NewAtom("title") );

move that inside the if?

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

oops, re-cc'ing keith

Comment 9

[Boris Zbarsky [:bzbarsky]]

yeah, good catch.  Of course we still need to decide whether this is the right
place to hack it in...

Comment 10

[Axel Hecht]

is checking node->mNodeInfo for the right tag name[s] really too costly?

Comment 11

[[e-mail]]

Attachment: Alternative patch

But then, what do I know about XUL documents :-P

Comment 12

[[e-mail]]

Comment on attachment 108221 [details] [diff] [review]
Alternative patch

Whoops, I typed if (nsIDOMElement) { instead of if (mRootElement) { :-[

Comment 13

[Axel Hecht]

Comment on attachment 108221 [details] [diff] [review]
Alternative patch

As nsXULElement::GetAttribute gets the atom and calls GetAttr, we should work
on the nsIContent directly. That spares as a few QIs and the
NormalizeAttrString call that ::GetAttribute does, too.

Axel, who still thinks we should check if it's a window.

Comment 14

[Axel Hecht]

Attachment: check for xul:window and better atom handling

Here is how I envisioned this to end up.
Still think that it's better off in the content sink, as that's where XML does
it,
too.

Comment 15

[Marco Pesenti Gritti]

Patch 108221 would fix also bug 216651.

Comment 16

[Axel Hecht]

Comment on attachment 108221 [details] [diff] [review]
Alternative patch

> if (nsIDOMElement) {
That's bad. And the element shouldn't be named mRootElement, but rootElement.
It's no member after all.

This code should be around line number 3172 these days, AFAICT

Comment 17

[Boris Zbarsky [:bzbarsky]]

Axel, mind updating this to the deCOMified nsINodeInfo, replacing the namespace
check with IsContentOfType, and replacing do_GetAtom with nsHTMLAtoms::title?

With those changes, your patch looks pretty good, assuming <dialog> never has a
title.

Comment 18

[Axel Hecht]

Jonas removed GetAttr on the element prototype, so this line ...
  element->GetAttr(kNameSpaceID_None, titleAtom, title);
... doesn't compile anymore.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Wasn't there a problem with this patch using prototypes which would bypass
overlays? I.e. if someone overlayed the root element and changed the title we
would ignore the title in the overlay?

Comment 20

[[e-mail]]

Attachment: Updated patch

OK, so it turns out that by setting the title just as the document loads makes
it possible to remove the LoadTitleFromXUL code thus fixing bug 259067.

Comment 21

[Dan M]

A Mozilla browser window containing a XUL content window is still a Mozilla
browser window. The content window's title is irrelevant. This bug would open a
spoofing hole if implemented. See for example bug 244965 comment 1.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Er... But we show HTML window titles in the title bar.  How is this any different?

We're not suggesting overriding the whole of the title, just the part before the
"Mozilla {Build ID: whatever"

Reopening, since I think this situation (for non-chrome XUL) is nothing like bug
244965

Comment 23

[Dan M]

My mistake. I've applied the patch in my build. It behaves just like an HTML
window's title, including correctly appending the application name to the
requested title if the XUL document is a content window. So that's all well and
good.

I have a concern about removing LoadTitleFromXUL, though. With the patch, the
Title Fight runs like this when opening a new window containing a XUL content
window:

1) Empty XUL document sets the browser window's title to browser.xul's
   <window> title attribute.
2) Empty HTML document sets the title to empty, which has the effect of
   setting the browser window's title to browser.xul's <window> titlemodifier
   attribute.
3) Repeat #2 why not.
4) LoadTitleFromXUL resets browser window's title to the correct <window>
   title attribute.
X) --- load XUL content window; this can take an arbitrary amount of time ---
5) Repeat step 1 but with the XUL content window's title, with browser.xul's
   <window> titlemodifier appended.
6) tabbrowser.xml apparently was listening as #6 happened. It repeats step 6.

(Title changes 1, 5, and 6 are new with this patch.)

Now removing step 4 would save us a dance step, but it causes us to have an
incorrect title during step (X), which can take an arbitrary amount of time. Oh
heck. In fact if you just open a new empty window: File Menu -> New Window, the
wrong window title remains until you load a document.

True the incorrect title isn't a terrible thing; it still comes from
browser.xul. In fact browser.xul currently uses the same value for both |title|
and |titlemodifier|, so you wouldn't even notice unless someone changed that.
But I suggest omitting the changes to nsXULWindow. Doing so will simply cause
nsXULDocument to behave as nsHTMLDocument has always behaved. What say?

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 158972 [details] [diff] [review]
Updated patch

sr=jst

Comment 25

[Dan M]

Comment on attachment 158972 [details] [diff] [review]
Updated patch

Eh, fine. What am I arguing for, anyway?

Comment 26

[[e-mail]]

FYI, this is the sequence I see when loading a page in a new window:
1+ navigator.xul loads, and sets the title to "Mozilla {Build ID: 0000000000}"
2. CNavDTD::AddHeadLeaf sets the content title to "" for about:blank
3. HTMLContentSink::DidBuildModel resets the content title for about:blank
(it would need an nsXPIDLString to tell if CNavDTD::AddHeadLeaf already set it)
4- after navigator load event fires, nsXULWindow::OnChromeLoaded sets the title
5. nsXULWindow makes the window visible.
6. requested content document loads and sets its title as in 2 and/or 3
7. tabbrowser notices title change and resets the same title
Although steps 2, 3 and 6 do cascade by way of contenttitlesetting="true" in
navigator.xul this still resets to the correct title.

Comment 27

[Stefan Borggraefe]

This caused Bug 261551.

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 267321 has been marked as a duplicate of this bug. ***