Bug 16496 - XMLSerializer doesn't include namespaces on nodes in HTML documents
Summary: XMLSerializer doesn't include namespaces on nodes in HTML documents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Rob Buis
URL: http://crschmidt.net/projects/webkit/...
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2007-12-17 20:07 PST by Christopher Schmidt
Modified: 2019-02-06 09:03 PST (History)
9 users (show)

See Also:


Attachments
Patch (4.58 KB, patch)
2012-04-06 09:39 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.61 MB, application/zip)
2012-04-06 11:36 PDT, WebKit Review Bot
no flags Details
Patch (13.30 KB, patch)
2012-04-08 18:49 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (6.23 MB, application/zip)
2012-04-09 07:27 PDT, WebKit Review Bot
no flags Details
Patch (13.85 KB, patch)
2012-04-13 05:38 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (6.19 MB, application/zip)
2012-04-13 06:30 PDT, WebKit Review Bot
no flags Details
Patch (13.89 KB, patch)
2012-04-24 09:34 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.10 MB, application/zip)
2012-04-24 19:07 PDT, WebKit Review Bot
no flags Details
Patch (16.11 KB, patch)
2013-06-22 19:37 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (538.78 KB, application/zip)
2013-06-23 04:46 PDT, Build Bot
no flags Details
Patch (17.67 KB, patch)
2013-06-23 08:04 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (17.95 KB, patch)
2013-06-24 09:21 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (545.51 KB, application/zip)
2013-06-24 10:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (589.71 KB, application/zip)
2013-06-24 11:03 PDT, Build Bot
no flags Details
Patch (19.75 KB, patch)
2013-06-24 19:14 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (499.35 KB, application/zip)
2013-06-24 21:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (514.89 KB, application/zip)
2013-06-24 21:18 PDT, Build Bot
no flags Details
Patch (19.94 KB, patch)
2013-06-25 07:16 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (20.19 KB, patch)
2013-06-25 14:16 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (20.19 KB, patch)
2013-06-26 07:06 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (20.44 KB, patch)
2013-06-27 15:23 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (647.24 KB, application/zip)
2013-06-27 16:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (632.89 KB, application/zip)
2013-06-28 00:06 PDT, Build Bot
no flags Details
Patch (21.15 KB, patch)
2013-06-28 12:21 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (483.64 KB, application/zip)
2013-06-28 14:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (646.40 KB, application/zip)
2013-06-28 14:27 PDT, Build Bot
no flags Details
Patch (28.34 KB, patch)
2013-06-28 14:36 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (26.96 KB, patch)
2013-07-15 14:08 PDT, Rob Buis
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Schmidt 2007-12-17 20:07:17 PST
The XMLSerializer serializeToString method does not include namespaces on bare nodes. as a workaround, it is possible to wrap the node in a document, and then serialize the document instead, but this behavior does not match the behavior of Firefox 2.0 or 3.0, nor Opera.
Comment 1 David Kilzer (:ddkilzer) 2007-12-18 05:39:12 PST
Confirmed with a local debug build of WebKit r28774 with Safari 3.0.4 (523.12) on Mac OS X 10.4.11 (8S165).

Comment 2 Alexey Proskuryakov 2007-12-25 07:44:08 PST
The issue here is that the node in question belongs to an HTML document, so we serialize it accordingly.

Firefox and Opera use XML serialization for XMLSerializer, and HTML one for innerHTML. WebKit uses the same code for both, but checks document type.
Comment 3 Alexey Proskuryakov 2010-10-16 17:17:25 PDT
See also: bug 47768.
Comment 4 Ryosuke Niwa 2012-04-03 12:02:44 PDT
This is a bug in markup.cpp.
Comment 5 Rob Buis 2012-04-06 09:39:32 PDT
Created attachment 136035 [details]
Patch
Comment 6 WebKit Review Bot 2012-04-06 11:36:36 PDT
Comment on attachment 136035 [details]
Patch

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

New failing tests:
fast/innerHTML/innerHTML-custom-tag.html
editing/pasteboard/paste-noscript-svg.html
Comment 7 WebKit Review Bot 2012-04-06 11:36:42 PDT
Created attachment 136045 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 8 Rob Buis 2012-04-08 18:49:37 PDT
Created attachment 136169 [details]
Patch
Comment 9 WebKit Review Bot 2012-04-09 07:27:07 PDT
Comment on attachment 136169 [details]
Patch

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

New failing tests:
fast/xsl/xslt-processor.html
Comment 10 WebKit Review Bot 2012-04-09 07:27:13 PDT
Created attachment 136220 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 11 Ryosuke Niwa 2012-04-11 13:20:33 PDT
Comment on attachment 136169 [details]
Patch

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

> Source/WebCore/editing/markup.cpp:762
> +String createMarkup(const Node* node, EChildrenOnly childrenOnly, Vector<Node*>* nodes, EAbsoluteURLs shouldResolveURLs, bool xmlFragmentSerialization)

Please use enum instead of bool. r- because of this.

> Source/WebCore/xml/XMLSerializer.cpp:44
> +    return createMarkup(node, IncludeNode, 0, DoNotResolveURLs, true);

With enum, the semantics of this code will be much clearer.
Comment 12 Rob Buis 2012-04-13 05:38:33 PDT
Created attachment 137071 [details]
Patch
Comment 13 WebKit Review Bot 2012-04-13 06:30:38 PDT
Comment on attachment 137071 [details]
Patch

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

New failing tests:
fast/xsl/xslt-processor.html
Comment 14 WebKit Review Bot 2012-04-13 06:30:53 PDT
Created attachment 137078 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 15 Rob Buis 2012-04-13 07:05:40 PDT
(In reply to comment #11)
> (From update of attachment 136169 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136169&action=review
> 
> > Source/WebCore/editing/markup.cpp:762
> > +String createMarkup(const Node* node, EChildrenOnly childrenOnly, Vector<Node*>* nodes, EAbsoluteURLs shouldResolveURLs, bool xmlFragmentSerialization)
> 
> Please use enum instead of bool. r- because of this.
> 
> > Source/WebCore/xml/XMLSerializer.cpp:44
> > +    return createMarkup(node, IncludeNode, 0, DoNotResolveURLs, true);
> 
> With enum, the semantics of this code will be much clearer.

Again a good suggestion with the enum, I tried to implement this in my latest patch.
About the failing testcase, I know the <br /> part is an improvement. We serialize now in xml mode so we end up with <br /> to feed to xslt, same as in FF. The ns addition on <head> is something which I am not sure libxslt or my patch is doing, will check later.
Cheers,

Rob.
Comment 16 Ryosuke Niwa 2012-04-22 23:02:56 PDT
Comment on attachment 137071 [details]
Patch

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

> Source/WebCore/editing/MarkupAccumulator.cpp:491
> +    if (!m_fragmentSerialization == XMLFragmentSerialization && node->document()->isHTMLDocument())

You mean m_fragmentSerialization != XMLFragmentSerialization?
Comment 17 Rob Buis 2012-04-24 09:34:00 PDT
Created attachment 138579 [details]
Patch
Comment 18 Ryosuke Niwa 2012-04-24 10:34:08 PDT
Comment on attachment 138579 [details]
Patch

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

> LayoutTests/fast/dom/dom-serialize-namespace.html:1
> +<html>

You might want DOCTYPE here.
Comment 19 Rob Buis 2012-04-24 13:08:14 PDT
Hi Ryosuke,

(In reply to comment #18)
> (From update of attachment 138579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138579&action=review
> 
> > LayoutTests/fast/dom/dom-serialize-namespace.html:1
> > +<html>
> 
> You might want DOCTYPE here.

Thanks! However, should I rebaseline fast/xsl/xslt-processor.html? I am not sure how the end result should look, also in FF and Opera the test does not seem to work so I can't compare. Should I look into the test result differences more or do you know how it should look?
Cheers,

Rob.
Comment 20 WebKit Review Bot 2012-04-24 19:07:15 PDT
Comment on attachment 138579 [details]
Patch

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

New failing tests:
fast/xsl/xslt-processor.html
Comment 21 WebKit Review Bot 2012-04-24 19:07:21 PDT
Created attachment 138718 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 22 Ryosuke Niwa 2012-07-19 15:42:21 PDT
Has this patch been landed?
Comment 23 Eric Seidel (no email) 2013-01-04 00:49:22 PST
I do not believe this was ever landed.
Comment 24 Rob Buis 2013-06-19 12:52:46 PDT
(In reply to comment #23)
> I do not believe this was ever landed.

Sorry guys, I can't remember exactly what happened but maybe there were still DRT tests failing with the final patch. I can have another look this weekend.
Comment 25 Rob Buis 2013-06-22 19:37:20 PDT
Created attachment 205259 [details]
Patch
Comment 26 Build Bot 2013-06-23 04:46:54 PDT
Comment on attachment 205259 [details]
Patch

Attachment 205259 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/971324

New failing tests:
fast/xsl/xslt-processor.html
svg/custom/xlink-prefix-in-attributes.html
Comment 27 Build Bot 2013-06-23 04:46:56 PDT
Created attachment 205267 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 28 Rob Buis 2013-06-23 08:04:19 PDT
Created attachment 205270 [details]
Patch
Comment 29 Rob Buis 2013-06-24 09:21:09 PDT
Created attachment 205301 [details]
Patch
Comment 30 Build Bot 2013-06-24 10:45:34 PDT
Comment on attachment 205301 [details]
Patch

Attachment 205301 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/961768

New failing tests:
fast/xsl/xslt-processor.html
Comment 31 Build Bot 2013-06-24 10:45:36 PDT
Created attachment 205308 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3
Comment 32 Build Bot 2013-06-24 11:03:41 PDT
Comment on attachment 205301 [details]
Patch

Attachment 205301 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/981235

New failing tests:
fast/xsl/xslt-processor.html
Comment 33 Build Bot 2013-06-24 11:03:45 PDT
Created attachment 205311 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 34 Rob Buis 2013-06-24 19:14:10 PDT
Created attachment 205354 [details]
Patch
Comment 35 Build Bot 2013-06-24 21:01:02 PDT
Comment on attachment 205354 [details]
Patch

Attachment 205354 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/977329

New failing tests:
inspector/elements/set-outer-html-for-xhtml.xhtml
Comment 36 Build Bot 2013-06-24 21:01:05 PDT
Created attachment 205361 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3
Comment 37 Build Bot 2013-06-24 21:18:27 PDT
Comment on attachment 205354 [details]
Patch

Attachment 205354 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/926539

New failing tests:
inspector/elements/set-outer-html-for-xhtml.xhtml
Comment 38 Build Bot 2013-06-24 21:18:30 PDT
Created attachment 205362 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 39 Rob Buis 2013-06-25 07:16:29 PDT
Created attachment 205402 [details]
Patch
Comment 40 Ryosuke Niwa 2013-06-25 10:25:52 PDT
Comment on attachment 205402 [details]
Patch

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

> Source/WebCore/editing/MarkupAccumulator.cpp:107
> +    , m_fragmentSerialization(fragmentSerialization)

This implicit conversion to bool is bogus.

> Source/WebCore/editing/MarkupAccumulator.cpp:436
> +        if (element->namespaceURI() == XMLNames::xmlNamespaceURI)
> +            prefix = xmlAtom;
> +        if (!prefix.isEmpty()) {
> +            result.append(prefix);
> +            result.append(':');
> +        }

prefix is always empty unless we execute the assignment above: prefix = xmlAtom.

> Source/WebCore/editing/MarkupAccumulator.cpp:551
> -    if (node->document()->isHTMLDocument())
> +    if (m_fragmentSerialization != XMLFragmentSerialization && node->document()->isHTMLDocument())

It seems like we want to extract this condition as a helper function.
In fact, we might even want to add a boolean indicating this as a member variable to speed things up.

> Source/WebCore/editing/MarkupAccumulator.h:117
> +    bool m_fragmentSerialization;

Why is this a boolean?
Comment 41 Rob Buis 2013-06-25 13:02:18 PDT
(In reply to comment #40)
> (From update of attachment 205402 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205402&action=review
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:107
> > +    , m_fragmentSerialization(fragmentSerialization)
> 
> This implicit conversion to bool is bogus.

Right, see below.

> > Source/WebCore/editing/MarkupAccumulator.cpp:436
> > +        if (element->namespaceURI() == XMLNames::xmlNamespaceURI)
> > +            prefix = xmlAtom;
> > +        if (!prefix.isEmpty()) {
> > +            result.append(prefix);
> > +            result.append(':');
> > +        }
> 
> prefix is always empty unless we execute the assignment above: prefix = xmlAtom.

Right, I was thinking of adding more cases where we assign prefix for certain namespaces like we do for attributes. To fix this bug this seems to be actually the only one I need. I guess I'll eliminate prefix temp var for now.

> > Source/WebCore/editing/MarkupAccumulator.cpp:551
> > -    if (node->document()->isHTMLDocument())
> > +    if (m_fragmentSerialization != XMLFragmentSerialization && node->document()->isHTMLDocument())
> 
> It seems like we want to extract this condition as a helper function.
> In fact, we might even want to add a boolean indicating this as a member variable to speed things up.

A helper is a good idea, will do that.

> > Source/WebCore/editing/MarkupAccumulator.h:117
> > +    bool m_fragmentSerialization;
> 
> Why is this a boolean?

Seems like a mistake on my behalf, fixing.

Will have a new patch up soon.
Comment 42 Rob Buis 2013-06-25 14:16:28 PDT
Created attachment 205423 [details]
Patch
Comment 43 Rob Buis 2013-06-26 07:06:28 PDT
Created attachment 205487 [details]
Patch
Comment 44 Rob Buis 2013-06-26 07:10:13 PDT
Uploaded one more time to see if it goes green.
Comment 45 Ryosuke Niwa 2013-06-26 14:00:15 PDT
Comment on attachment 205487 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        http://html5.org/specs/dom-parsing.html#xmlserializer. In this mode

Could you refer to a specific revision of http://domparsing.spec.whatwg.org instead?
Since it's a live standard, the content at the given URL is going to change, and
I'd like to be able to identify which version of the specification is implemented in this patch in the future.

> Source/WebCore/editing/MarkupAccumulator.cpp:276
> -    if (attribute.name() == XMLNSNames::xmlnsAttr) {
> +    if (attribute.name() == XMLNSNames::xmlnsAttr || (inXMLFragmentSerialization() && attribute.localName() == XMLNSNames::xmlnsAttr.localName() && attribute.namespaceURI().isEmpty())) {

It seems like this is part of the DOM4 spec here:
http://dom.spec.whatwg.org/#locate-a-namespace
instead of it being specified in the serialization spec.

If that were the case, then we should probably add a new member function to Attribute that implements this locate-a-namespace algorithm.

> Source/WebCore/editing/MarkupAccumulator.cpp:304
> +        // Suppress xmlns: for xml namespace, it is implied
> +        if (inXMLFragmentSerialization() && namespaceURI == XMLNames::xmlNamespaceURI)
> +            return;

Where is this specified? The current specification is extremely vague about this.

> Source/WebCore/editing/MarkupAccumulator.cpp:434
> +    if (inXMLFragmentSerialization() && element->prefix().isEmpty()) {
> +        if (element->namespaceURI() == XMLNames::xmlNamespaceURI) {
> +            result.append(xmlAtom);
> +            result.append(':');
> +        }
> +    }

Ditto.

> Source/WebCore/editing/MarkupAccumulator.cpp:436
> +    if ((inXMLFragmentSerialization() || !element->document()->isHTMLDocument()) && namespaces && shouldAddNamespaceElement(element))

Ditto.

> Source/WebCore/editing/MarkupAccumulator.cpp:491
> +    if ((inXMLFragmentSerialization() || !documentIsHTML) && namespaces && shouldAddNamespaceAttribute(attribute, *namespaces))
> +        appendNamespace(result, prefixedName.prefix(), prefixedName.namespaceURI(), *namespaces);

Ditto.
Comment 46 Rob Buis 2013-06-27 15:23:33 PDT
Created attachment 205641 [details]
Patch
Comment 47 Build Bot 2013-06-27 16:27:12 PDT
Comment on attachment 205641 [details]
Patch

Attachment 205641 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/974242

New failing tests:
fast/xsl/xslt-processor.html
Comment 48 Build Bot 2013-06-27 16:27:15 PDT
Created attachment 205645 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 49 Build Bot 2013-06-28 00:06:44 PDT
Comment on attachment 205641 [details]
Patch

Attachment 205641 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/995333

New failing tests:
fast/xsl/xslt-processor.html
Comment 50 Build Bot 2013-06-28 00:06:49 PDT
Created attachment 205673 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3
Comment 51 Rob Buis 2013-06-28 12:21:15 PDT
Created attachment 205732 [details]
Patch
Comment 52 Build Bot 2013-06-28 14:05:32 PDT
Comment on attachment 205732 [details]
Patch

Attachment 205732 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/981812

New failing tests:
inspector/elements/set-outer-html-for-xhtml.xhtml
Comment 53 Build Bot 2013-06-28 14:05:36 PDT
Created attachment 205742 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3
Comment 54 Build Bot 2013-06-28 14:26:58 PDT
Comment on attachment 205732 [details]
Patch

Attachment 205732 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/866608

New failing tests:
svg/batik/filters/feTile.svg
inspector/elements/set-outer-html-for-xhtml.xhtml
Comment 55 Build Bot 2013-06-28 14:27:02 PDT
Created attachment 205743 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 56 Rob Buis 2013-06-28 14:36:05 PDT
Created attachment 205744 [details]
Patch
Comment 57 Rob Buis 2013-06-28 18:45:18 PDT
Hi Ryosuke,

(In reply to comment #45)
> (From update of attachment 205487 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205487&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        http://html5.org/specs/dom-parsing.html#xmlserializer. In this mode
> 
> Could you refer to a specific revision of http://domparsing.spec.whatwg.org instead?
> Since it's a live standard, the content at the given URL is going to change, and
> I'd like to be able to identify which version of the specification is implemented in this patch in the future.

I tried to do that in my latest patch.

> > Source/WebCore/editing/MarkupAccumulator.cpp:276
> > -    if (attribute.name() == XMLNSNames::xmlnsAttr) {
> > +    if (attribute.name() == XMLNSNames::xmlnsAttr || (inXMLFragmentSerialization() && attribute.localName() == XMLNSNames::xmlnsAttr.localName() && attribute.namespaceURI().isEmpty())) {
> 
> It seems like this is part of the DOM4 spec here:
> http://dom.spec.whatwg.org/#locate-a-namespace
> instead of it being specified in the serialization spec.

It is a bit different, this one does not look at ancestors. I changed this code a bit in my latest patch.

> If that were the case, then we should probably add a new member function to Attribute that implements this locate-a-namespace algorithm.
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:304
> > +        // Suppress xmlns: for xml namespace, it is implied
> > +        if (inXMLFragmentSerialization() && namespaceURI == XMLNames::xmlNamespaceURI)
> > +            return;
> 
> Where is this specified? The current specification is extremely vague about this.

I removed this in my latest patch, but it is covered under Namespace constraint: Reserved Prefixes and Namespace Names in http://www.w3.org/TR/REC-xml-names/.

> > Source/WebCore/editing/MarkupAccumulator.cpp:434
> > +    if (inXMLFragmentSerialization() && element->prefix().isEmpty()) {
> > +        if (element->namespaceURI() == XMLNames::xmlNamespaceURI) {
> > +            result.append(xmlAtom);
> > +            result.append(':');
> > +        }
> > +    }
> 
> Ditto.

This is done in order not to loose the namespace associated with the element, I put a comment for that in my latest patch.

> > Source/WebCore/editing/MarkupAccumulator.cpp:436
> > +    if ((inXMLFragmentSerialization() || !element->document()->isHTMLDocument()) && namespaces && shouldAddNamespaceElement(element))
> 
> Ditto.

I think this actually needs no explanation, for xml serializing we simply may want to add a namespace depending on the element.
 
> > Source/WebCore/editing/MarkupAccumulator.cpp:491
> > +    if ((inXMLFragmentSerialization() || !documentIsHTML) && namespaces && shouldAddNamespaceAttribute(attribute, *namespaces))
> > +        appendNamespace(result, prefixedName.prefix(), prefixedName.namespaceURI(), *namespaces);
> 
> Ditto.

For xml serializing we simply may want to add a namespace depending on the attribute.

Note that the latest patch finally shows up all green!
Comment 58 Rob Buis 2013-07-15 14:08:16 PDT
Created attachment 206684 [details]
Patch
Comment 59 Ryosuke Niwa 2013-07-15 14:28:01 PDT
Comment on attachment 206684 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        http://html5.org/specs/dom-parsing.html#xmlserializer (23 May 2013). In this mode

Could you refer to a specific hash instead?

> Source/WebCore/ChangeLog:27
> +        (WebCore::MarkupAccumulator::serializeNodesWithNamespaces): makes sure xml namespace/prefix is known so it is never used in namespace declarations.
> +        (WebCore::MarkupAccumulator::appendNamespace): Avoid adding namespace declarations that do not differ from current default namespace.
> +        (WebCore::MarkupAccumulator::appendOpenTag): Print xml prefix if the element's namespace is XML to avoid conflicts.
> +        (WebCore::MarkupAccumulator::appendAttribute): Force any calculated prefix/namespace pair into the generated namespace.
> +        (WebCore::MarkupAccumulator::shouldAddNamespaceAttribute): Also take into account xmlns attributes with no namespace.
> +        (WebCore::MarkupAccumulator::shouldSelfClose): Force self-closing to create well-formed XML elements.
> +        * editing/MarkupAccumulator.h: Use EFragmentSerialization.

It's still unclear to me which parts are referring to the spec mentioned about
and which parts are matching Firefox/IE. It's helpful have that information in the change log, not in the code.

> Source/WebCore/editing/MarkupAccumulator.cpp:144
> +        // Make sure xml prefix and namespace are always known to ensure these Namespace constraints:
> +        // The prefix xml MUST NOT be declared as the default namespace.
> +        // The prefix xml MAY, but need not, be declared (XMLNames specification, Namespace constraint: Reserved Prefixes and Namespace Names)

Instead of copying & pasting the spec. text, can we refer to a specific section from which this text is copied?

> Source/WebCore/editing/MarkupAccumulator.cpp:436
> +        // According to DOM3 appendix B Namespace Normalization we now should create a default namespace declaration to make this namespace well-formed.
> +        // However, XMLNames specification, Namespace constraint: Reserved Prefixes and Namespace Names says:
> +        // "The prefix xml MUST NOT be declared as the default namespace.", so use the xml prefix explicitly.

Ditto about referring to sections of the specifications.
Comment 60 Rob Buis 2013-07-15 17:44:28 PDT
Thanks for the review!

(In reply to comment #59)
> (From update of attachment 206684 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206684&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        http://html5.org/specs/dom-parsing.html#xmlserializer (23 May 2013). In this mode
> 
> Could you refer to a specific hash instead?

I added a commit hash before landing.

> > Source/WebCore/ChangeLog:27
> > +        (WebCore::MarkupAccumulator::serializeNodesWithNamespaces): makes sure xml namespace/prefix is known so it is never used in namespace declarations.
> > +        (WebCore::MarkupAccumulator::appendNamespace): Avoid adding namespace declarations that do not differ from current default namespace.
> > +        (WebCore::MarkupAccumulator::appendOpenTag): Print xml prefix if the element's namespace is XML to avoid conflicts.
> > +        (WebCore::MarkupAccumulator::appendAttribute): Force any calculated prefix/namespace pair into the generated namespace.
> > +        (WebCore::MarkupAccumulator::shouldAddNamespaceAttribute): Also take into account xmlns attributes with no namespace.
> > +        (WebCore::MarkupAccumulator::shouldSelfClose): Force self-closing to create well-formed XML elements.
> > +        * editing/MarkupAccumulator.h: Use EFragmentSerialization.
> 
> It's still unclear to me which parts are referring to the spec mentioned about
> and which parts are matching Firefox/IE. It's helpful have that information in the change log, not in the code.

I reworded the commit log, I still thing the per method explanations are useful as well.

> > Source/WebCore/editing/MarkupAccumulator.cpp:144
> > +        // Make sure xml prefix and namespace are always known to ensure these Namespace constraints:
> > +        // The prefix xml MUST NOT be declared as the default namespace.
> > +        // The prefix xml MAY, but need not, be declared (XMLNames specification, Namespace constraint: Reserved Prefixes and Namespace Names)
> 
> Instead of copying & pasting the spec. text, can we refer to a specific section from which this text is copied?

Done.

> > Source/WebCore/editing/MarkupAccumulator.cpp:436
> > +        // According to DOM3 appendix B Namespace Normalization we now should create a default namespace declaration to make this namespace well-formed.
> > +        // However, XMLNames specification, Namespace constraint: Reserved Prefixes and Namespace Names says:
> > +        // "The prefix xml MUST NOT be declared as the default namespace.", so use the xml prefix explicitly.
> 
> Ditto about referring to sections of the specifications.

Done.

I discovered before landing that one change in appendAttribute is not needed to fix this bug, I left it out for now but think I'll include it in a patch for bug 22958.
Comment 61 Rob Buis 2013-07-16 15:44:18 PDT
Committed r152685: <http://trac.webkit.org/changeset/152685>
Comment 62 Lucas Forschler 2019-02-06 09:03:59 PST
Mass moving XML DOM bugs to the "DOM" Component.