Bug 3586 – Setting and getting title's text attribute does not work

• Status: VERIFIED
• Resolution: FIXED
• Component: DOM

[Anders Carlsson]

It should be possible to set a document's title by setting title.text and it should also be possible to retreive 
it by getting title.text. This doesn't work right now.

[Anders Carlsson]

Created attachment 2421 [details]
Support text property on titles

Here's a patch with test cases. I modified DumpRenderTree so it would
(optionally) print out title changes.

[Anders Carlsson]

Created attachment 2422 [details]
Make DumpRenderTree optionally print out title changes

[Joost de Valk (AlthA)]

I believe Anders when he files a bug, will ask someone on the apple webkit team to give him canconfirm 
and editbugs privileges.

[Anders Carlsson]

Created attachment 2435 [details]
Reset value of dumpTitleChanges after each run

We need to set dumpTitleChanges to NO after each run to make the other test
cases pass.

[Darin Adler]

I have an old title patch that addresses other problems with titles. I'm attaching it, just so Anders and 
other can see it.

[Darin Adler]

Created attachment 2442 [details]
Possibly-relevant title patch I had sitting around.

This patch attempts to deal with the issue of competing attempts to set the
title by setting the text of the title element, setting the title on the
document element, and manipulating the children of the title element directly
with DOM calls. But I don't think I have enough test cases to be sure it does
the right thing.

[Darin Adler]

Created attachment 2443 [details]
Possibly-relevant title patch I had sitting around (uploading again).

Trouble uploading. Trying again.

[Darin Adler]

Comment on attachment 2435 [details]
Reset value of dumpTitleChanges after each run

This looks fine. I think it would be even more elegant to figure out a way to
add this without a new method.

For example, any "unexpected" title changes could be logged. Perhaps the code
could check to see if the <title> element, if any, was parsed, and simply dump
all title changes after that point.

But I also think it's fine to land this as-is.

r=me

[Darin Adler]

Comment on attachment 2421 [details]
Support text property on titles

We can probably do even better later (see my patch), but this patch seems like
a clear improvement.

One area for future improvement is to share code with other places that need to
concatenate text from all child elements. Maybe that's only the <script> tag?

r=me

[Anders Carlsson]

I do have a newer patch that incorporates your changes as well as adds a test case. I also noticed that 
WebKit doesn't handle setting the title to '' so I have a patch for that as well.

I'll upload these patches tomorrow.

[Darin Adler]

Sounds great!

[Anders Carlsson]

Created attachment 2481 [details]
Integrate Darin's patch and add tests

[Darin Adler]

Here are a few things I've been thinking about:

    1) Can we share code in the text/setText functions for HTMLOptionElementImpl, 
HTMLTitleElementImpl, and HTMLScriptElementImpl?
    2) Should setText optimize for the case where the text isn't change and not send out 
childrenChanged and the DOM tree modification events? Does that break anything for <title>? For 
<script>?
    3) The HTMLOptionElementImpl looks at the entire subtree, and doesn't assume that text nodes will 
all be at the top level. Do we want that for <title>? For <script>? If so, then childrenChanged() isn't a 
good enough notification since it doesn't bubble up to parents.
    4) Do we want to add a new replaceChildren so we can remove the children and add the one new text 
node without sending out double childrenChanged and double DOM tree modification events?
    5) Should we bother fixing the "removed a <title> element" case to look for a remaining <title> 
element?

I think he has answers to all of these questions, and there's a chance we won't have to change the patch 
at all.

It would also be good to ensure the test case covers enough of the corner cases, since the patch has a 
lot of different code paths. Maybe we're already there. I think I'm going to wait on reviewing this patch 
until we get a tiny bit more data.

[Anders Carlsson]

(In reply to comment #13)
> Here are a few things I've been thinking about:
> 
>     1) Can we share code in the text/setText functions for HTMLOptionElementImpl, 
> HTMLTitleElementImpl, and HTMLScriptElementImpl?

We could definitely share code for text in HTMLTitleElementImpl and HTMLScriptElementImpl. 
HTMLOptionElementImpl does work differently (As is pointed out in 3) but having something like 
DOMString NodeImpl::getChildrenTextContents(bool traverse) could work, I'm not sure. 
Looks like setText could be shared between all three implementations.

>     2) Should setText optimize for the case where the text isn't change and not send out 
> childrenChanged and the DOM tree modification events? Does that break anything for <title>? For 
> <script>?

I don't think that should break anything for <title> or <script>. The way ::text() and ::setText() works is 
highly imlementation-dependent, see my comment on 3) below.

>     3) The HTMLOptionElementImpl looks at the entire subtree, and doesn't assume that text nodes 
will 
> all be at the top level. Do we want that for <title>? For <script>? If so, then childrenChanged() isn't a 
> good enough notification since it doesn't bubble up to parents.

Like I stated before, the way ::text and ::setText work is very implementation-specific. 
Mozilla, for example does only look at the first child node's text contents, and doesn't allow you to set 
title.text unless its first child node is a text node. So something like <title><span>foo</span></title> 
created by the DOM could not be changed by setting title.text.
WinIE does not allow you to change the title at all by appending children to the title element, only by 
setting title.text or document.title.
So basically, I think what we have now should be good enough.

>     4) Do we want to add a new replaceChildren so we can remove the children and add the one new 
text 
> node without sending out double childrenChanged and double DOM tree modification events?

For the general case the <title> element should only have one child (I think this is always true when 
dealing with HTML, and it'll be true for XML documents as well unless there's a mix between text and 
cdata) in which case we'll just update the child node's text. I don't think it's worth adding a new method 
just for this.

>     5) Should we bother fixing the "removed a <title> element" case to look for a remaining <title> 
> element?

Neither Mozilla nor WinIE does this, and I can't think of a good use case for this.

> 
> I think he has answers to all of these questions, and there's a chance we won't have to change the 
patch 
> at all.
> 
> It would also be good to ensure the test case covers enough of the corner cases, since the patch has 
a 
> lot of different code paths. Maybe we're already there. I think I'm going to wait on reviewing this 
patch 
> until we get a tiny bit more data.

I think what we have test-case wise is pretty good, although I did add

spanElement = document.createElement('span');
	spanElement.appendChild(document.createTextNode('4. appending a span element with a text 
child (should not trigger a title change)'));
	title.appendChild(spanElement);

to the second test case.

[Darin Adler]

Comment on attachment 2481 [details]
Integrate Darin's patch and add tests

OK, I'm convinced, r=me.

[Darin Adler]

Radar for this is:

    <rdar://problem/3831364> getting or setting text for a <title> element doesn't work

This patch also fixes this bug:

    <rdar://problem/4091225> REGRESSION (1.2.4-1.3): many titles with non-ASCII characters do not 
display properly (XHTML strict only)

although this fix doesn't deal with the underlying problem that libxml2 breaks the title up into more than 
one text node, which is the other cause of that latter problem.

[Darin Adler]

Created attachment 2484 [details]
Test case for the non-ASCII XML title bug that this patch also fixes