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: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Evan Martin
2011-03-31 03:41:06 PDT
The problem is in Document::setTitle(), which tests isHTMLDocument, which isn't true for XHTML documents. 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. Created attachment 106907 [details]
Patch
Created attachment 106910 [details]
Patch
(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. Created attachment 106916 [details]
Patch
Comment on attachment 106916 [details]
Patch
Please add a a test.
(In reply to comment #7) > (From update of attachment 106916 [details]) > Please add a a test. I'll add it. Created attachment 106953 [details]
Patch
Patch with a test case. This patch makes me curious about how many of the 66 call sites of isHTMLDocument() are mishandling XHTML documents. 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>. (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. Created attachment 107078 [details]
Patch
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. (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 on attachment 107078 [details] Patch Clearing flags on attachment: 107078 Committed r95009: <http://trac.webkit.org/changeset/95009> All reviewed patches have been landed. Closing bug. |