Bug 106491 - Template element should parse in XHTML just as it does in HTML
Summary: Template element should parse in XHTML just as it does in HTML
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords: WebExposed
: 106599 (view as bug list)
Depends on:
Blocks: 103547
  Show dependency treegraph
 
Reported: 2013-01-09 14:44 PST by Adam Klein
Modified: 2013-01-23 18:33 PST (History)
8 users (show)

See Also:


Attachments
Work in progress (4.32 KB, patch)
2013-01-09 14:45 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (6.44 KB, patch)
2013-01-10 14:01 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (14.54 KB, patch)
2013-01-11 16:09 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (18.00 KB, patch)
2013-01-14 17:42 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (19.23 KB, patch)
2013-01-16 16:15 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch for landing (19.47 KB, patch)
2013-01-22 16:38 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch for landing (19.51 KB, patch)
2013-01-23 16:49 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2013-01-09 14:44:09 PST
See https://www.w3.org/Bugs/Public/show_bug.cgi?id=19889.
Comment 1 Adam Klein 2013-01-09 14:45:40 PST
Created attachment 181986 [details]
Work in progress
Comment 2 Rafael Weinstein 2013-01-10 14:01:40 PST
Created attachment 182202 [details]
Patch
Comment 3 Adam Klein 2013-01-10 15:14:27 PST
(In reply to comment #2)
> Created an attachment (id=182202) [details]
> Patch

Seems like you probably want to merge this with my patch?
Comment 4 Rafael Weinstein 2013-01-11 12:16:14 PST
*** Bug 106599 has been marked as a duplicate of this bug. ***
Comment 5 Rafael Weinstein 2013-01-11 16:09:43 PST
Created attachment 182442 [details]
Patch
Comment 6 Rafael Weinstein 2013-01-11 16:10:13 PST
This is now ready for review
Comment 7 Build Bot 2013-01-11 17:21:06 PST
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
Comment 8 Rafael Weinstein 2013-01-14 17:42:03 PST
Created attachment 182669 [details]
Patch
Comment 9 Ryosuke Niwa 2013-01-15 12:08:26 PST
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.
Comment 10 Rafael Weinstein 2013-01-16 16:14:44 PST
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
Comment 11 Rafael Weinstein 2013-01-16 16:15:22 PST
Created attachment 183056 [details]
Patch
Comment 12 Rafael Weinstein 2013-01-18 11:32:37 PST
ping?
Comment 13 Ryosuke Niwa 2013-01-18 12:04:48 PST
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.
Comment 14 Rafael Weinstein 2013-01-22 16:37:33 PST
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
Comment 15 Rafael Weinstein 2013-01-22 16:38:40 PST
Created attachment 184073 [details]
Patch for landing
Comment 16 WebKit Review Bot 2013-01-22 16:56:03 PST
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
Comment 17 WebKit Review Bot 2013-01-23 16:18:00 PST
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
Comment 18 Rafael Weinstein 2013-01-23 16:49:59 PST
Created attachment 184348 [details]
Patch for landing
Comment 19 WebKit Review Bot 2013-01-23 18:33:04 PST
Comment on attachment 184348 [details]
Patch for landing

Clearing flags on attachment: 184348

Committed r140631: <http://trac.webkit.org/changeset/140631>
Comment 20 WebKit Review Bot 2013-01-23 18:33:09 PST
All reviewed patches have been landed.  Closing bug.