Closed Bug 118704 Opened 18 years ago Closed 11 years ago

document.title does not dynamically update when the contents of the document's <title> element change

Categories

(Core :: DOM: Core & HTML, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1a2

People

(Reporter: unger, Assigned: roc)

References

(Depends on 1 open bug)

Details

(Keywords: html5)

Attachments

(6 files, 2 obsolete files)

If a title is changed in the following way

<script language="JavaScript" type="text/javascript">

theTitle=document.getElementsByTagName("title").item(0).firstChild;
document.write(theTitle.nodeValue, "<br />");
theTitle.replaceData(0, theTitle.nodeValue.length, "This is a new title.");
document.write(theTitle.nodeValue);

</script>

then the title is changed in the document but not in the title bar of
the document window nor in the history.
Additional proposal: Don't add an additional history entry if the
title is change, only change the title.
to DOM core....
Assignee: asa → jst
Status: UNCONFIRMED → NEW
Component: Browser-General → DOM Core
Ever confirmed: true
QA Contact: doronr → stummala
Future.
OS: Windows 98 → All
Hardware: PC → All
Target Milestone: --- → Future
Component: DOM Core → DOM HTML
changing priority to P5
Priority: -- → P5
*** Bug 155521 has been marked as a duplicate of this bug. ***
*** Bug 181921 has been marked as a duplicate of this bug. ***
*** Bug 189716 has been marked as a duplicate of this bug. ***
Summary: Title not changed in title bar and history → document.title != getElementsByTagName('title')[0].text
Ok. But there is another backfire -- TITLE tag does not support id properety, so
only way to gain acsess to it, use getElementsByTagName('title')[0].
getElementById does not work. Also w3c HTML 4.01 attributes dir and lang does
not supported.
Well. As you see, text is only properety, wich supported (and changed
getElementsByTagName('title')[0].text lead to change of document.tittle). All
others no supported.
Hmm. Source of this behaviour lies in function:

HTMLContentSink::AddHeadContent(const nsIParserNode& aNode)

(/content/html/document/src/nsHTMLContentSink.cpp)

3533     nsHTMLTag type = nsHTMLTag(aNode.GetNodeType());
3534     if (eHTMLTag_title == type) {
3535       nsCOMPtr<nsIDTD> dtd;
3536       mParser->GetDTD(getter_AddRefs(dtd));
3537       if (dtd) {
3538         nsAutoString title;
3539         PRInt32 lineNo = 0;
3540         dtd->CollectSkippedContent(eHTMLTag_title, title, lineNo);
3541         rv = SetDocumentTitle(title);
3542       }
3543     }
3544     else {
3545       rv = AddLeaf(aNode);
3546     }

Well, for <title> tag this code do following: it ignored ALL attributes,
remember only title content. Even more, SetDocumentTitle(title); create
a new HTMLTitleElement only for first <title> tag, trashed all other
<title> tags. I don't think that this is aceptable.
There is only one <title> allowed per the HTML spec.  Read the DTD.  Or read
section 7.4.3, which says "...the TITLE element, which provides information
about an entire document and may only appear once..."

Also, there are no ways to get the dir and lang attributes of the TITLE element
other than using getAttribute.  See
http://www.w3.org/TR/2003/REC-DOM-Level-2-HTML-20030109/html.html#ID-79243169
which does not define anything for HTMLTitleElement other than text.

Please don't make assumptions about what you think is acceptable or not without
consulting the specs first.
Comment on attachment 112451 [details]
HTMLTitleElenent propereties

This testcase as written is invalid.
Attachment #112451 - Attachment is obsolete: true
Sorry, Christopher, but you should consult manual more precisely. dir and lang
is attributes of HTMLElement, and they DECLARED by line:

interface HTMLTitleElement : HTMLElement 

and

http://www.w3.org/TR/1999/REC-html401-19991224/struct/global.html#edef-TITLE:

7.4.2 The TITLE element

<!-- The TITLE element is not considered part of the flow of text.
       It should be displayed, for example as the page header or
       window title. Exactly one title is required per document.
    -->
<!ELEMENT TITLE - - (#PCDATA) -(%head.misc;) -- document title -->
<!ATTLIST TITLE %i18n>

Start tag: required, End tag: required

Attributes defined elsewhere

    * lang (language information), dir (text direction)


So dir and lang AND id! should be inherited from HTMLElement. Even more, in
current version this attributes SUPPORTED but INCORRECTLY.  Of you think, that
id for tittle is not allowed?

P.S. Where is stated that there should be exactly one title? There is only:

Every HTML document must have a TITLE element in the HEAD section.

Nothing more.
Attachment #112451 - Attachment description: HTMLTitleElenebt propereties → HTMLTitleElenent propereties
Attachment #112451 - Attachment is obsolete: false
Aha, I found an answer to last question. So second title is realy incorect, and
this part of guessing goeas away.
Comment on attachment 112451 [details]
HTMLTitleElenent propereties

Er, right, I forgot about HTMLElement.	But still, I point you to the sentence
I copied.  Which as I said is in  7.4.3 (not 7.4.2 as you quoted.  Look down
just a few paragraphs).
Well, I realy think about such code:

<title>This is element title</title>
<script type="text/javascript">
  var tt=document.createElement('title')
  document.getElementsByTagName("HEAD")[0].appendChild(tt)
  tt.text="I am the new title"
</script>

After it, there is 2 title elements in HTML file (I check it), and change text
properety of each of it set document.title. 
Depends on: 190516
*** Bug 190516 has been marked as a duplicate of this bug. ***
*** Bug 190516 has been marked as a duplicate of this bug. ***
Mass-reassigning bugs.
Assignee: jst → dom_bugs
Just click on every button...
id, dir, lang properties in attachment 112451 [details] work, if the file is
served as application/xhtml+xml.
Is Bug 280044 a dup of this?
*** Bug 317923 has been marked as a duplicate of this bug. ***
*** Bug 362219 has been marked as a duplicate of this bug. ***
Summary: document.title != getElementsByTagName('title')[0].text → document.title does not dynamically update when the contents of the document's <title> element change
Blocks: acid3
The multiple-titles issue mentioned here seems to be generally agreed to be invalid, but the dynamic-changes issue is still valid, so I changed the summary to match the valid issue.
(In reply to comment #23)
> but the dynamic-changes issue is still valid, so I changed the summary

> document.title does not dynamically update when the contents
> of the document's <title> element change

Then, when title attribute changed, <title> element should update, or not?
(In reply to comment #24)
> Then, when title attribute changed, <title> element should update, or not?

Probably not, when you read an element you should only get its current DOM state, and document.title is a property of the document, not an element (sorry if that sounds confusing).

Are you going to request review on this patch?
(In reply to comment #24)
> Created an attachment (id=312294) [details]
> <title> element -> document.title property
> 
> (In reply to comment #23)
> > but the dynamic-changes issue is still valid, so I changed the summary
> 
> > document.title does not dynamically update when the contents
> > of the document's <title> element change
> 
> Then, when title attribute changed, <title> element should update, or not?
> 

Actually, ignore my above comment, it should update. Opera and Webkit both do so, so I think we should too.
I think we need a little more work here. Setting document.title should change the contents of the <title> element, if there is one. I'll have a go.
Assignee: general → roc
The HTML5 spec says we should actually make document.title defer to the contents of the <title> element:
http://www.whatwg.org/specs/web-apps/current-work/#document.title

We should probably implement it that way and get rid of nsDocument::mDocumentTitle.

Torisugari, do you want to carry on with this or should I take over? Your patch is a good start, but instead of calling nsHTMLDocument::SetTitle it should call nsDocument::SetTitle (probably renamed to SetTitleInternal and exposed through nsIDocument). We should get rid of mDocumentTitle. nsDocument::GetTitle and nsDocument::SetTitle should be reimplemented to use the HTML5 algorithm.
(In reply to comment #28)
> Torisugari, do you want to carry on with this or should I take over?

I'd like to try it out this weekend, unless you've already started writing patch.
Adding keyword 'html5' per comment 28
Keywords: html5
Gettin this on the 1.9.1 nom list.
Flags: wanted1.9.1?
O. Atsushi (Torisugari) did you get much further?
Flags: wanted1.9.1? → wanted1.9.1+
Another testcase for 

> 3. If the title element is null, then a new title element must be
>    created and appended to the head element.
I have a question.

According to whatwg HTML5, *the title element* is defined in this way.

> The title element of a document is the first title element in
> the document (in tree order), if there is one, or null otherwise. 

That is,

> <html>
>   <title>A</title>
>   <head>
>     <title>B</title>
>   </head>
>   <body>
>     <title>C</title>
>     <title>D</title>
>     <title>E</title>
>   </body>
> </html>

*The title element* is |<title>A</tiltle>|. So the title bar displays "A".
This is OK.

Then, a user can remove *the title element*.

> var element = document.getElementsByTagName("title")[0];
> element.parentNode.removeChild(element);

What should happen? In theory (probably), the moment removeChild() is
called, *the title element* is |<title>B</title>|.

i.e. removeChild() should change the title bar from "A" to "B"?

Another question is,

> document.getElementsByTagName("title")[2].text = "F";

can change the title bar? Or it should not?

|document.getElementsByTagName("title")[2]| is not *the title element*
while it's *a title element* nonetheless. Such behavior is not described in
the whatwg spec. We should respect or ignore the third title element?

Anyway, when given document has 2 or more <title> elements, it's a little confusing.
Though I'm afraid less than 0.00000001% of users may have experienced removing <title>
element from a HTML document.
(In reply to comment #35)
> Then, a user can remove *the title element*.
> 
> > var element = document.getElementsByTagName("title")[0];
> > element.parentNode.removeChild(element);
> 
> What should happen? In theory (probably), the moment removeChild() is
> called, *the title element* is |<title>B</title>|.
> i.e. removeChild() should change the title bar from "A" to "B"?

That's right.

We want to avoid having a mutation listener on the entire document. So I think we'll need a hook in nsHTMLTitleElement::BindToTree/UnbindFromTree to detect when title elements are added or removed and update the window title.

> Another question is,
> 
> > document.getElementsByTagName("title")[2].text = "F";
> 
> can change the title bar? Or it should not?
> 
> |document.getElementsByTagName("title")[2]| is not *the title element*
> while it's *a title element* nonetheless. Such behavior is not described in
> the whatwg spec. We should respect or ignore the third title element?

Ignore it. The window title should always be based on whatever the title attribute is returning, and the title attribute depends only on "the title element" and its contents.
I don't know how you're doing this but IMHO the right way to implement it is to not store the "title attribute" anywhere. Just make GetTitle compute it from scratch by analyzing the DOM. And when the DOM changes in a way that could affect which element "the title element" is, or could affect the contents of "the title element", then we need to trigger updating of the window title (which will call GetTitle). SetTitle just needs to make the DOM changes that the HTML5 spec describes, and then the window title will update automatically.
roc - I'll take a look at this if you don't mind. 
Component: DOM: HTML → DOM: Core & HTML
QA Contact: stummala → general
Attached patch Ryan's partial patch (obsolete) — Splinter Review
I'll work on this shortly.
Attached patch fix (obsolete) — Splinter Review
Here's a working patch.

The basic idea is described above. Instead of storing the title in a dedicated variable, it's just something we compute from the DOM tree. SetTitle modifies the DOM. The content sinks don't need to explicitly track the title anymore, we just have to make sure that the <title> element is properly notified so it can fire DOMTitleChanged when it should. This actually simplifies code. I've made DOMTitleChanged fully async, there doesn't seem to be any reason why it needs to e synchronous.

One thing I know I haven't handled is the title change when a XUL root element is removed or added to the DOM. I just don't care enough to make that work.
Attachment #332570 - Attachment is obsolete: true
Attachment #333329 - Flags: superreview?
Attachment #333329 - Flags: review?
Attachment #333329 - Flags: superreview?(jst)
Attachment #333329 - Flags: superreview?
Attachment #333329 - Flags: review?(jst)
Attachment #333329 - Flags: review?
So one way that *may* be simpler to keep track of the current title is this:

1. Make nsIDocument keep a (weak) pointer: nsIContent* mFirstTitleElement
2. Inside the title elements ::BindToTree call check if the documents
   mFirstTitleElement is set. If it's set, call nsContentUtils::PositionIsBefore
   and if the existing title is before abort.
3. Set the documents mFirstTitleElement
4. In title elements ::UnbindFromTree, walk the DOM to search for a new title
   and update mFirstTitleElement accordingly.

Not sure if that's better or worse than what you're doing.
That sounds more complicated than what I'm doing.

It would speed up GetTitle, but I don't think we should be concerned about that (at least until someone shows us a real-world testcase where it matters). The only possible GetTitle performance concern I can see is where nsDocShell::RestoreFromHistory calls NotifyPossibleTitleChange; if the document is large and has no title, the resulting call to GetTitle will scan the entire DOM. So we could slow down page restore a fraction for those pages. I'm not convinced that's something to be concerned about.
If that was a concern, my preferred solution would be to add a single bit to nsDocument which records whether there was ever an SVG or HTML <title> element bound to the document. Then GetTitle could use that to avoid scanning the whole document if we know there can't be a title element.
Comment on attachment 333329 [details] [diff] [review]
fix

- In nsDocument::GetHtmlChildContent(nsIAtom* aTag):

+  // Look for body inside html. This needs to run forwards to find
+  // the first body element.

Fix this comment, we're not necessarily looking for "body" here.

- In nsDocument::GetTitleContent(PRUint32 aNodeType):

+{
+  nsRefPtr<nsContentList> list =
+    NS_GetContentList(this, nsGkAtoms::title, kNameSpaceID_Unknown);

Jonas brought up the idea that over all it'd probably be beneficial to keep a weak pointer to the first title element (or an array/list of all of them if need be) and have the notifications for added/(re)moved title elements tell the document what title element was added/(re)moved title elements use nsContentUtils::ComparePosition() to know whether the notification is for the first title or not etc. If we did that we'd only ever need to walk the DOM when a title element is removed from the DOM tree (unless we keep pointers to all title elements), which basically never happens.

If we don't do that then this could get really expensive in the case where we're dealing with a really large document that has no title element. Not the normal case either, but definitely a possible case.

- In nsDocument::GetTitleFromElement():

+{
+  nsIContent* title = GetTitleContent(aNodeType);
+  if (!title)
+    return NS_OK;
+  nsContentUtils::GetNodeTextContent(title, PR_FALSE, aTitle);
+  return NS_OK;

Either return what GetNodeTextContent() returns here, or make this a void function (the latter is probably preferred).

- In nsRunnableMethod::Revoke():

+    NS_IF_RELEASE(mObj);
+    mObj = nsnull;

NS_IF_RELEASE(mObj) sets mObj to nsnull for you.

Over all this looks really good. I'd like to look over an updated patch with the optimization to remember the current title element in the document before stamping this though.
Attachment #333329 - Flags: superreview?(jst)
Attachment #333329 - Flags: superreview-
Attachment #333329 - Flags: review?(jst)
Attachment #333329 - Flags: review-
Yeah, i'm not really concerned about performance about anything here. We should do whatever results in the least amount of code.
I'm quite sure that tracking the first title element will be more code. We'll still have to walk the document when a title element is removed (either SVG title or HTML title), so that code will still be around somewhere, but we'd have additional code to track the first title element.

> If we don't do that then this could get really expensive in the case where
> we're dealing with a really large document that has no title element.

Note that in regular page load, such a document won't (well, "shouldn't", I haven't tested) fire a DOMTitleChanged event, so I wouldn't expect anyone to be calling GetTitle, unless chrome does it somewhere unexpected.

I'll post a new patch with the other comments addressed.
Also note that if someone calls GetTitle more than once on the same document, then it seems the content list is likely to be cached so we should be pretty efficient there too.
Attached patch fix v2Splinter Review
Updated to comments, plus I added the 1-bit mMayHaveTitleElement optimization since it's very simple and should be completely effective for title-less documents.
Attachment #333329 - Attachment is obsolete: true
Attachment #333880 - Flags: superreview?(jst)
Attachment #333880 - Flags: review?(jst)
Comment on attachment 333880 [details] [diff] [review]
fix v2

Yup, makes sense. r+sr=jst
Attachment #333880 - Flags: superreview?(jst)
Attachment #333880 - Flags: superreview+
Attachment #333880 - Flags: review?(jst)
Attachment #333880 - Flags: review+
Ready to be landed? Without tests????
The tests are in the patch.
Ok, sorry I hadn't seen it.
So it looks like sometimes the code is looking for a <title> element by just looking through the children of <head>, sometimes by looking through the whole DOM. Is this intended?

Sorry, just glanced over the patch.
Where are we only looking through the children of <head>?

HTML5 says that "the title element" is the first <title> in the document, whether it's in the head or not.

It also says that when you create a new title by setting .title, the new <title> element is appended to "the head element" (and *nothing happens* if there is no "the head element"). That should be the only dependence on <head> in this patch.
Yeah, you are right, i misread the part that inserts a new title when needed. Thought I had posted a comment fessing up to my mistake, but apparently not :)
Pushed 9934298811d7.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Future → mozilla1.9.1a2
Backed out. I think this may have caused Windows orange due to leaks.

I'm not sure what the problem is, but it could be that in NotifyPossibleTitleChange, NS_DispatchToCurrentThread could fail (because the current thread has shut down? I don't know), and in that case mPendingTitleChangeEvent stays around with a zero refcount, because it was never addrefed ... holding a strong ref to our nsDocument, so it and everything hanging off it is leaked.

We should fix that by assigning the result of new nsRunnableMethod to an nsCOMPtr<nsIRunnable> in NotifyPossibleTitleChange, so if the dispatch fails then the refcount of the event will be decreased to zero as we leave NotifyPossibleTitleChange and the event will be destroyed. Of course mPendingTitleChangeEvent should only be set if dispatch succeeds. I'll reland with that fix later and see if that is the problem...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Acid 3 now says:
Test 98 failed: doc.body is undefined

Mozilla/5.0 (Windows; U; Windows NT 6.0; sk; rv:1.9.1a2pre) Gecko/20080816031323 Minefield/3.1a2pre
(In reply to comment #58)
That's from bug 450160.

when I had used this patch (before it was pulled), a weird space did appear in the tab,

http://img294.imageshack.us/img294/8725/titlespacevl5.jpg
> when I had used this patch (before it was pulled), a weird space did appear in
> the tab,
> 
> http://img294.imageshack.us/img294/8725/titlespacevl5.jpg
> 

This seems to only be a problem with last.fm user music profiles.

                        <title>
        mdew’s Music Profile
 – Users at Last.fm
</title>
shows in the source of the page so this would be an evangelism bug.
I don't think so. Mozilla should keep stripping the white space. This is what IE is doing and what Mozilla currently does. It seems like an unintended effect from the patch.
Should we strip the whitespace from document.title or in chrome when we display the title? What does IE do?
I tested this. FF3, IE7 and Opera 9.51 all compress internal whitespace and strip leading and trailing whitespace. Only Safari 3.1 does not. I posted to whatwg recommending that the spec change to make dominant UA behaviour. In the meantime, I'll reland with a call to aTitle.CompressWhitespace in nsDocument::GetTitle.
Attached file compression testcase
testcase for whitespace compression
I relanded, changeset c771615414fe.
For the record, in the initial landing the leak was triggered in mochitests.
(In reply to comment #67)
> For the record, in the initial landing the leak was triggered in mochitests.

good thing it was unlanded =P
This time looks better, tests passed on Windows.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Depends on: 451256
Depends on: 454949
Which version of Firefox will this fix be in? Thanks.

(http://code.google.com/p/fbug/issues/detail?id=1315
 Issue 1315:  	 Changes made to the <title> tag are not seen.)
Given the target milestone of mozilla1.9.1a2, I'd guess Firefox 3.1 Alpha 2.
Depends on: 475717
Depends on: 400095
Depends on: 510809
You need to log in before you can comment on or make changes to this bug.