Bug 57537

Summary:

Setting document.title doesn't affect contents of title tag of XHTML documents

Product:

WebKit

Reporter:

Evan Martin <evan>

Component:

DOM

Assignee:

Rob Buis <rwlbuis>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, darin, jkjiang, rwlbuis, shinyak, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	ap: review-
Patch	none
Patch	none

Evan Martin

Reported 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.

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Evan Martin

Comment 1 2011-03-31 03:43:01 PDT

The problem is in Document::setTitle(), which tests isHTMLDocument, which isn't true for XHTML documents.

Jacky Jiang

Comment 2 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.

Jacky Jiang

Comment 3 2011-09-09 12:53:38 PDT

Created attachment 106907 [details] Patch

Rob Buis

Comment 4 2011-09-09 13:18:29 PDT

Created attachment 106910 [details] Patch

Rob Buis

Comment 5 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.

Rob Buis

Comment 6 2011-09-09 13:48:24 PDT

Created attachment 106916 [details] Patch

Alexey Proskuryakov

Comment 7 2011-09-09 15:28:35 PDT

Comment on attachment 106916 [details] Patch Please add a a test.

Jacky Jiang

Comment 8 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.

Jacky Jiang

Comment 9 2011-09-09 18:55:23 PDT

Created attachment 106953 [details] Patch

Jacky Jiang

Comment 10 2011-09-09 18:57:15 PDT

Patch with a test case.

Darin Adler

Comment 11 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.

Alexey Proskuryakov

Comment 12 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>.

Jacky Jiang

Comment 13 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.

Jacky Jiang

Comment 14 2011-09-12 13:50:36 PDT

Created attachment 107078 [details] Patch

Alexey Proskuryakov

Comment 15 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.

Jacky Jiang

Comment 16 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.

WebKit Review Bot

Comment 17 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>

WebKit Review Bot

Comment 18 2011-09-12 20:49:13 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.