Setting document.title should have no effect for non SVG/HTML documents, see https://html.spec.whatwg.org/multipage/dom.html#document.title.
Created attachment 354840 [details] Patch
Chromium bug is crbug.com/906038.
Created attachment 355060 [details] Patch
Comment on attachment 355060 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355060&action=review > Source/WebCore/ChangeLog:11 > + Behavior matches Firefox. What's Chrome behavior? > Source/WebCore/dom/Document.cpp:1583 > + ASSERT(!m_titleElement || isHTMLDocument() || isXHTMLDocument() || isSVGDocument()); Not sure how useful this assertion is. > Source/WebCore/dom/Document.cpp:1597 > } else return; > Source/WebCore/dom/Document.cpp:1600 > if (is<HTMLTitleElement>(m_titleElement.get())) I think we can get rid of all these if checks and simply do: m_titleElement->setTextContent(title);
Comment on attachment 355060 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355060&action=review >> Source/WebCore/ChangeLog:11 >> + Behavior matches Firefox. > > What's Chrome behavior? Same as WebKit. I am preparing a patch on chromium side to implement the new behavior (https://chromium-review.googlesource.com/c/chromium/src/+/1339959). >> Source/WebCore/dom/Document.cpp:1583 >> + ASSERT(!m_titleElement || isHTMLDocument() || isXHTMLDocument() || isSVGDocument()); > > Not sure how useful this assertion is. This is maybe the controversial (and optional) part of the patch. If you read the FIXME, it is clear we suspect the old code is not useful anymore (setting m_titleElement to nullptr). I think m_titleElement will only be set for html/svg/xhtml case, so no need to set it to nullptr for other document types (like xml). So to play it safe I wanted to keep the check as an ASSERT, but fine to leave it out of the patch as well. Or we can just keep setting m_titleElement to nullptr to play it safe, but it does look a bit silly. My feeling is to go for removing it. >> Source/WebCore/dom/Document.cpp:1597 >> } > > else > return; Done. >> Source/WebCore/dom/Document.cpp:1600 >> if (is<HTMLTitleElement>(m_titleElement.get())) > > I think we can get rid of all these if checks and simply do: > m_titleElement->setTextContent(title); I like it! Done.
Created attachment 355069 [details] Patch
Comment on attachment 355069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355069&action=review > LayoutTests/imported/w3c/ChangeLog:8 > + * web-platform-tests/html/dom/documents/dom-tree-accessors/document.title-10.html: Added. Aren't you missing the -expected.txt ? Unlike Blink, I believe WebKit still relies on them.
Comment on attachment 355069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355069&action=review >> LayoutTests/imported/w3c/ChangeLog:8 >> + * web-platform-tests/html/dom/documents/dom-tree-accessors/document.title-10.html: Added. > > Aren't you missing the -expected.txt ? Unlike Blink, I believe WebKit still relies on them. Well spotted, will fix.
Created attachment 355072 [details] Patch
Comment on attachment 355072 [details] Patch r=me if the bots are happy.
Created attachment 355261 [details] Patch
(In reply to Chris Dumez from comment #10) > Comment on attachment 355072 [details] > Patch > > r=me if the bots are happy. Thanks Chris! Since your review, tkent has suggested another subtest and I had to change setTitle to make it work: https://chromium-review.googlesource.com/c/chromium/src/+/1339959 Basically I made setTitle exactly match the specification steps, which I think is not a bad thing. Can you have a quick check that the r=me still stands?
Comment on attachment 355261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355261&action=review > Source/WebCore/dom/Document.cpp:1590 > + } else if (element && element->isHTMLElement()) { is<HTMLElement>(element)
Created attachment 355286 [details] Patch
Comment on attachment 355286 [details] Patch Clearing flags on attachment: 355286 Committed r238377: <https://trac.webkit.org/changeset/238377>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46172624>