Serialization algorithm never preserves attribute prefixes

[bwrrp]

If implemented as described in the specification, the algorithm to serialize an element's attributes (https://w3c.github.io/DOM-Parsing/#serializing-an-element-s-attributes) never preserves the prefix for any namespaced attribute that is not a namespace declaration:

• Step 3.5: attributeNamespace is not null, run the sub-steps
• Step 3.5.1: a candidate prefix is determined(*)
• Step 3.5.2: attributeNamespace is not the XMLNS namespace, skip the sub-steps and continue with 3.5.3.
• Step 3.5.3: interpreting the typo in "the attribute namespace in not the XMLNS namespace." as an assertion of the condition in 3.5.2 being false, run the sub-steps.
• Step 3.5.3.1: the candidate prefix is overwritten by a generated prefix

It seems that the candidate prefix generated in step 3.5.1 (marked (*)) is used in neither of the two branches.

Am I correct in assuming that this is not the intended behavior? I would expect a new prefix to be generated only if the existing prefix conflicts with one associated with a different namespace in the local prefix map. This would require an extra check for candidate prefix being null afterwards, as the prefix may need to be redeclared (not regenerated) in that case.

[bwrrp]

The following changes would make the algorithm match the expected behavior. I'll try to prepare a PR later.

• 3.5.3: Change to "Otherwise, the attribute namespace is not the XMLNS namespace. If candidate prefix is null, run these steps:"
• 3.5.3.1: Change to "Let new prefix be attr's prefix attribute if the local prefixes map does not contain a key matching attr's prefix, or the result of generating a prefix providing map, attribute namespace, and prefix index as input otherwise.
• 3.5.3.2.3: Change to "The value of new prefix;"

[domenic]

@Sebmaster your help again would be appreciated here. Did you do the same thing in https://github.com/jsdom/w3c-xmlserializer as @bwrrp did in slimdon.js?

[bwrrp]

Please note that this needs another small change to make sure the selected, newly declared prefix is added to the maps (so it may be reused for similarly namespaced attributes). I'll see if I can update the PR next week to match the slimdom implementation.

[cscott]

Ran across this bug as well. Seems like a reference to this issue should be added to the WPT test "Check if an attribute with namespace and no prefix is serialized with the nearest-declared prefix"; an implementation following the spec as written will currently fail this assertion: https://github.com/web-platform-tests/wpt/blame/master/domparsing/XMLSerializer-serializeToString.html#L86 (since it will generate a new prefix instead of reusing one).

[cscott]

Adding the condition at https://github.com/jsdom/w3c-xmlserializer/blob/master/lib/attributes.js#L92 is the solution by @Sebmaster. That matches the current state of WPT.

@bwrrp has a slightly better complicated proposal in bwrrp@97e7bb3 which also preserves the prefix name in the case where the attribute has a prefix but it hasn't (yet) been properly declared. The "'Check if the prefix of an attribute is NOT preserved in a case where neither its prefix nor its namespace URI is not already used." test at https://github.com/web-platform-tests/wpt/blame/master/domparsing/XMLSerializer-serializeToString.html#L126 would have to be updated if this change were made to the spec. Firefox appears to already implement this change, though:

> doc = (new DOMParser()).parseFromString("<r xmlns:xx=\"uri\"></r>", "text/xml");
> root = doc.firstChild;
> root.setAttributeNS('uri2', 'p:name', 'value');
> (new XMLSerializer()).serializeToString($doc)
"<r xmlns:xx=\"uri\" p:name=\"value\" xmlns:p=\"uri2\"/>"

(The WPT test case states the output should be <r xmlns:xx="uri" xmlns:ns1="uri2" ns1:name="value"/> which is what https://github.com/jsdom/w3c-xmlserializer/blob/master/lib/attributes.js would emit.

[cscott]

Note also that the "Check if no special handling for XLink namespace unlike HTML serializer." test case in https://github.com/web-platform-tests/wpt/blame/master/domparsing/XMLSerializer-serializeToString.html#L218 also implicitly assumes the "firefox" behavior, not the spec behavior:

const root2 = (new Document()).createElement('root');
root2.setAttributeNS('http://www.w3.org/1999/xlink', 'xl:type', 'v');
assert_equals(serialize(root2), '<root xmlns:xl="http://www.w3.org/1999/xlink" xl:type="v"/>');

The spec currently says the output should be <root <root xmlns:ns1="http://www.w3.org/1999/xlink" ns1:type="v"/> since the xl prefix on the attribute wouldn't be preserved.