Bug 57537 - Setting document.title doesn't affect contents of title tag of XHTML documents
Summary: Setting document.title doesn't affect contents of title tag of XHTML documents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Rob Buis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-31 03:41 PDT by Evan Martin
Modified: 2011-09-12 20:49 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.24 KB, patch)
2011-09-09 12:53 PDT, Jacky Jiang
no flags Details | Formatted Diff | Diff
Patch (1.38 KB, patch)
2011-09-09 13:18 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (1.38 KB, patch)
2011-09-09 13:48 PDT, Rob Buis
ap: review-
Details | Formatted Diff | Diff
Patch (3.95 KB, patch)
2011-09-09 18:55 PDT, Jacky Jiang
no flags Details | Formatted Diff | Diff
Patch (3.92 KB, patch)
2011-09-12 13:50 PDT, Jacky Jiang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Martin 2011-03-31 03:41:06 PDT
This snippet has different behavior in HTML vs XHTML documents:

<head><title id='t'>test</title></head>
<body>
<script>document.title = 'foo';
console.log(document.getElementById('t').innerText);</script>

In HTML, we update the <title> tag.  In XHTML, we don't.
Comment 1 Evan Martin 2011-03-31 03:43:01 PDT
The problem is in Document::setTitle(), which tests isHTMLDocument, which isn't true for XHTML documents.
Comment 2 Jacky Jiang 2011-09-08 17:29:50 PDT
This in XHTML document works fine on Opera.
Is there a specific reason that we are not taking XHTML documents into account in Document::setTitle()? It looks that should be the same with HTML documents.
Comment 3 Jacky Jiang 2011-09-09 12:53:38 PDT
Created attachment 106907 [details]
Patch
Comment 4 Rob Buis 2011-09-09 13:18:29 PDT
Created attachment 106910 [details]
Patch
Comment 5 Rob Buis 2011-09-09 13:35:20 PDT
(In reply to comment #4)
> Created an attachment (id=106910) [details]
> Patch

For anyone curious, Jacky is not a committer so I uploaded the patch for him, trying to get EWS results.... Ignore for now.
Cheers,

Rob.
Comment 6 Rob Buis 2011-09-09 13:48:24 PDT
Created attachment 106916 [details]
Patch
Comment 7 Alexey Proskuryakov 2011-09-09 15:28:35 PDT
Comment on attachment 106916 [details]
Patch

Please add a a test.
Comment 8 Jacky Jiang 2011-09-09 15:29:50 PDT
(In reply to comment #7)
> (From update of attachment 106916 [details])
> Please add a a test.

I'll add it.
Comment 9 Jacky Jiang 2011-09-09 18:55:23 PDT
Created attachment 106953 [details]
Patch
Comment 10 Jacky Jiang 2011-09-09 18:57:15 PDT
Patch with a test case.
Comment 11 Darin Adler 2011-09-09 20:16:19 PDT
This patch makes me curious about how many of the 66 call sites of isHTMLDocument() are mishandling XHTML documents.
Comment 12 Alexey Proskuryakov 2011-09-09 20:57:20 PDT
Comment on attachment 106953 [details]
Patch

What is the reason why you didn't use regular WebKit script test machinery? That would make the test much shorter and more consistent with others. See e.g. <http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/allowed-children.html>.
Comment 13 Jacky Jiang 2011-09-12 08:05:05 PDT
(In reply to comment #12)
> (From update of attachment 106953 [details])
> What is the reason why you didn't use regular WebKit script test machinery? That would make the test much shorter and more consistent with others. See e.g. <http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/allowed-children.html>.

Ah, I see, I followed some other tests under dom directory. Thanks for your advice, I will take the advice and update the test case then.
Comment 14 Jacky Jiang 2011-09-12 13:50:36 PDT
Created attachment 107078 [details]
Patch
Comment 15 Alexey Proskuryakov 2011-09-12 14:58:53 PDT
Comment on attachment 107078 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107078&action=review

Looks good, thank you!

As Darin said, there can be a lot of interesting finds around other places that call isHTMLDocument().

> LayoutTests/fast/dom/title-content-set-innerText-get.xhtml:12
> +<p id="description"></p>
> +<div id="console"></div> 
> +<script>
> +description("This test ensures that we can update the contents of the title tag of the XHTML document when setting document.title");

This is how it's done it the test I gave a link to, but there is really no reason to set the description via a JavaScript function - one can just put it directly in a <p> element.
Comment 16 Jacky Jiang 2011-09-12 15:18:02 PDT
(In reply to comment #15)
> (From update of attachment 107078 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107078&action=review
> 
> Looks good, thank you!
> 
> As Darin said, there can be a lot of interesting finds around other places that call isHTMLDocument().
> 
> > LayoutTests/fast/dom/title-content-set-innerText-get.xhtml:12
> > +<p id="description"></p>
> > +<div id="console"></div> 
> > +<script>
> > +description("This test ensures that we can update the contents of the title tag of the XHTML document when setting document.title");
> 
> This is how it's done it the test I gave a link to, but there is really no reason to set the description via a JavaScript function - one can just put it directly in a <p> element.

Thanks for your grant!
If I find other places that call isHTMLDocument and mishandling XHTML documents, I'll file bugs and try to fix them.
Comment 17 WebKit Review Bot 2011-09-12 20:49:06 PDT
Comment on attachment 107078 [details]
Patch

Clearing flags on attachment: 107078

Committed r95009: <http://trac.webkit.org/changeset/95009>
Comment 18 WebKit Review Bot 2011-09-12 20:49:13 PDT
All reviewed patches have been landed.  Closing bug.