Bug 191643 – Setting document.title should have no effect for non SVG/HTML documents

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Rob Buis]

Setting document.title should have no effect for non SVG/HTML documents, see https://html.spec.whatwg.org/multipage/dom.html#document.title.

[Rob Buis]

Created attachment 354840 [details]
Patch

[Rob Buis]

Chromium bug is crbug.com/906038.

[Rob Buis]

Created attachment 355060 [details]
Patch

[Chris Dumez]

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);

[Rob Buis]

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.

[Rob Buis]

Created attachment 355069 [details]
Patch

[Chris Dumez]

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.

[Rob Buis]

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.

[Rob Buis]

Created attachment 355072 [details]
Patch

[Chris Dumez]

Comment on attachment 355072 [details]
Patch

r=me if the bots are happy.

[Rob Buis]

Created attachment 355261 [details]
Patch

[Rob Buis]

(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?

[Chris Dumez]

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)

[Rob Buis]

Created attachment 355286 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 355286 [details]
Patch

Clearing flags on attachment: 355286

Committed r238377: <https://trac.webkit.org/changeset/238377>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46172624>