Bug 106491 – Template element should parse in XHTML just as it does in HTML

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Adam Klein]

See https://www.w3.org/Bugs/Public/show_bug.cgi?id=19889.

[Adam Klein]

Created attachment 181986 [details]
Work in progress

[Rafael Weinstein]

Created attachment 182202 [details]
Patch

[Adam Klein]

(In reply to comment #2)
> Created an attachment (id=182202) [details]
> Patch

Seems like you probably want to merge this with my patch?

[Rafael Weinstein]

*** Bug 106599 has been marked as a duplicate of this bug. ***

[Rafael Weinstein]

Created attachment 182442 [details]
Patch

[Rafael Weinstein]

This is now ready for review

[Build Bot]

Comment on attachment 182442 [details]
Patch

Attachment 182442 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15822111

New failing tests:
fast/xsl/xslt-xhtml-template.xml
fast/xpath/xpath-template-element.html
fast/xsl/xslt-processor-template.html

[Rafael Weinstein]

Created attachment 182669 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 182669 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +

Can we have a spec URL?

> Source/WebCore/ChangeLog:20
> +        * xml/parser/XMLDocumentParserLibxml2.cpp:

No changes to Qt's XML parser?

> LayoutTests/fast/xsl/xslt-processor-template-expected.txt:1
> +Body divs: CD, Template content divs: AB

This output doesn't help us understand what this test is testing.
e.g. I can't tell whether this test is passing or failing without having to read the test code.
Please add a description.

> LayoutTests/fast/xsl/xslt-xhtml-template-expected.txt:3
> +C
> +D
> +Body divs: CD, Template content divs: AB

Ditto.

[Rafael Weinstein]

Comment on attachment 182669 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +
> 
> Can we have a spec URL?

done

>> Source/WebCore/ChangeLog:20
>> +        * xml/parser/XMLDocumentParserLibxml2.cpp:
> 
> No changes to Qt's XML parser?

It'd prefer not to mess with Qt's XML parser, especially since this feature is only enabled for the chromium port. I can open a bug against the template element meta bug to fix the Qt XML parser, if you think that's appropriate.

>> LayoutTests/fast/xsl/xslt-processor-template-expected.txt:1
>> +Body divs: CD, Template content divs: AB
> 
> This output doesn't help us understand what this test is testing.
> e.g. I can't tell whether this test is passing or failing without having to read the test code.
> Please add a description.

done

>> LayoutTests/fast/xsl/xslt-xhtml-template-expected.txt:3
>> +Body divs: CD, Template content divs: AB
> 
> Ditto.

done

[Rafael Weinstein]

Created attachment 183056 [details]
Patch

[Rafael Weinstein]

ping?

[Ryosuke Niwa]

Comment on attachment 183056 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/templates/index.html#parsing-xhtml-documents.

It'll be better to refer to a specific revision of the spec. you're implementing so that we know which revision of the spec you implemented in this patch should the spec change in the future.

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:792
> -    RefPtr<Element> newElement = document()->createElement(qName, true);
> +    RefPtr<Element> newElement = m_currentNode->document()->createElement(qName, true);

There are a lot more places where document() is referenced like when we instantiate XMLDocumentParserScope in XMLDocumentParser::doWrite or when we call executeScript in XMLDocumentParser::endElementNs. It know we don't need to access document() through m_currentNode in those cases but it might be better that way for simplicity.

Also, I suppose you can't have DOCTYPE inside a template element?

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:826
> +        pushCurrentNode(newElement.get());

Why don't we just leave it here instead of repeating the same statement twice.

> LayoutTests/fast/dom/HTMLTemplateElement/xhtml-parsing-and-serialization.xml:15
> +if (window.testRunner) {
> +    testRunner.dumpAsText();
> +}

Nit: No curly brackets around a single statement.

> LayoutTests/fast/xpath/xpath-template-element-expected.txt:3
> +A B

It might be better to hide these once the test had ran in DRT/WTR.

> LayoutTests/fast/xsl/xslt-processor-template-expected.txt:1
> +This tests that XSLT transforms can traverse into XHTML template element content when applying XSL template. If the test succeeds, the transform will have swapped the position of the body spans (A and B) with the template content spans (C and D) and replaced the spans with divs.

This is an interesting behavior. I didn't expect XSLT to work inside the template. It might be worth mentioning it in the change log
and/or refer to the part of the specification where this behavior is mandated.

[Rafael Weinstein]

Comment on attachment 183056 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/templates/index.html#parsing-xhtml-documents.
> 
> It'll be better to refer to a specific revision of the spec. you're implementing so that we know which revision of the spec you implemented in this patch should the spec change in the future.

done

>> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:792
>> +    RefPtr<Element> newElement = m_currentNode->document()->createElement(qName, true);
> 
> There are a lot more places where document() is referenced like when we instantiate XMLDocumentParserScope in XMLDocumentParser::doWrite or when we call executeScript in XMLDocumentParser::endElementNs. It know we don't need to access document() through m_currentNode in those cases but it might be better that way for simplicity.
> 
> Also, I suppose you can't have DOCTYPE inside a template element?

Changing how we access document() everywhere seems wrong. And no, DOCTYPE is ignored inside template.

>> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:826
>> +        pushCurrentNode(newElement.get());
> 
> Why don't we just leave it here instead of repeating the same statement twice.

Done

>> LayoutTests/fast/dom/HTMLTemplateElement/xhtml-parsing-and-serialization.xml:15
>> +}
> 
> Nit: No curly brackets around a single statement.

done

>> LayoutTests/fast/xsl/xslt-processor-template-expected.txt:1
>> +This tests that XSLT transforms can traverse into XHTML template element content when applying XSL template. If the test succeeds, the transform will have swapped the position of the body spans (A and B) with the template content spans (C and D) and replaced the spans with divs.
> 
> This is an interesting behavior. I didn't expect XSLT to work inside the template. It might be worth mentioning it in the change log
> and/or refer to the part of the specification where this behavior is mandated.

done

[Rafael Weinstein]

Created attachment 184073 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 184073 [details]
Patch for landing

Rejecting attachment 184073 [details] from commit-queue.

New failing tests:
css2.1/20110323/background-intrinsic-004.htm
css2.1/20110323/background-intrinsic-006.htm
css2.1/20110323/replaced-intrinsic-ratio-001.htm
css3/selectors3/xhtml/css3-modsel-5.xml
css2.1/20110323/background-intrinsic-005.htm
compositing/images/direct-svg-image.html
css3/images/cross-fade-overflow-position.html
css2.1/20110323/replaced-intrinsic-002.htm
css3/filters/effect-reference-external.html
css3/selectors3/xhtml/css3-modsel-7.xml
css3/selectors3/xhtml/css3-modsel-4.xml
css2.1/20110323/replaced-intrinsic-005.htm
css3/selectors3/xhtml/css3-modsel-1.xml
css2.1/20110323/replaced-intrinsic-004.htm
accessibility/svg-remote-element.html
css2.1/20110323/background-intrinsic-002.htm
css3/selectors3/xhtml/css3-modsel-2.xml
css2.1/20110323/background-intrinsic-007.htm
animations/suspend-resume-animation-events.html
css2.1/20110323/background-intrinsic-009.htm
css2.1/20110323/replaced-intrinsic-003.htm
css2.1/20110323/background-intrinsic-001.htm
css3/zoom-coords.xhtml
css2.1/20110323/replaced-intrinsic-001.htm
css2.1/20110323/background-intrinsic-008.htm
css3/selectors3/xhtml/css3-modsel-3a.xml
css3/selectors3/xhtml/css3-modsel-3.xml
css3/selectors3/xhtml/css3-modsel-6.xml
http/tests/inspector/resource-parameters.html
css2.1/20110323/background-intrinsic-003.htm
Full output: http://queues.webkit.org/results/16033918

[WebKit Review Bot]

Comment on attachment 184073 [details]
Patch for landing

Attachment 184073 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16079389

New failing tests:
css2.1/20110323/background-intrinsic-004.htm
css3/selectors3/xhtml/css3-modsel-7.xml
css2.1/20110323/replaced-intrinsic-ratio-001.htm
css3/selectors3/xhtml/css3-modsel-5.xml
css2.1/20110323/background-intrinsic-005.htm
compositing/images/direct-svg-image.html
http/tests/eventsource/eventsource-cors-with-credentials.html
css3/images/cross-fade-overflow-position.html
css2.1/20110323/replaced-intrinsic-002.htm
css3/filters/effect-reference-external.html
css3/selectors3/xhtml/css3-modsel-4.xml
css2.1/20110323/replaced-intrinsic-005.htm
css3/selectors3/xhtml/css3-modsel-1.xml
css2.1/20110323/replaced-intrinsic-004.htm
accessibility/svg-remote-element.html
css2.1/20110323/background-intrinsic-002.htm
css3/selectors3/xhtml/css3-modsel-2.xml
css2.1/20110323/background-intrinsic-007.htm
css2.1/20110323/background-intrinsic-009.htm
css2.1/20110323/background-intrinsic-001.htm
css2.1/20110323/background-intrinsic-006.htm
css3/zoom-coords.xhtml
css2.1/20110323/replaced-intrinsic-001.htm
css2.1/20110323/replaced-intrinsic-003.htm
css2.1/20110323/background-intrinsic-008.htm
css3/selectors3/xhtml/css3-modsel-3a.xml
css3/selectors3/xhtml/css3-modsel-3.xml
css3/selectors3/xhtml/css3-modsel-6.xml
http/tests/inspector/resource-parameters.html
css2.1/20110323/background-intrinsic-003.htm

[Rafael Weinstein]

Created attachment 184348 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 184348 [details]
Patch for landing

Clearing flags on attachment: 184348

Committed r140631: <http://trac.webkit.org/changeset/140631>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.