newline in XML attributes should be serialized as 

RESOLVED FIXED in Firefox 60



17 years ago
5 months ago


(Reporter: hs, Assigned: ashkulz)


Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)



(1 attachment)



17 years ago
An attribute value that would be written as "asdf\nqwer" in C or JavaScript
should be serialized to "asdf
qwer" in XML so that an XML parser that
conforms to section 3.3.3 of the XML-1.0 spec
( will normalize it to "asdf\nqwer"
again. Currently the XML serializer writes out the newline character without
escaping, so a conforming parser normalizes the attribute value to "asdf qwer".

The same applies to tab and carriage-return.

I think it should suffice to set kAttrEntities[10] to "
" in

Comment 1

17 years ago
Unfortunately the work-around to convert the newline to "
" already in the
DOM before serializing does not work. Here the serializer is smart and replaces
the ampersand by "&".


17 years ago
QA Contact: petersen → rakeshmishra
Ever confirmed: true


16 years ago
QA Contact: rakeshmishra → ashishbhatt
QA Contact: ashshbhatt → xml

Comment 2

5 years ago
Nice, a 12-year old bug! And still unresolved.

Added a testcase to show what is happening:

Turns out that a literal newline in attribute value is parsed okay *as long as the parser operates in text/html mode*. For application/xml documents, the literal newline is converted and normalized to a space, as per the original post (and spec).

So clearly we need to explain to the XMLSerializer that it should escape newlines, because the result it not text/html, but rather application/xml.

Comment 3

5 years ago
This is indeed a bug. U+000A and U+0009 should be escaped in XML. (I think it's fine to not escape U+000D, since HTML serializer doesn't escape it and nobody complains about that.)
Comment hidden (mozreview-request)


a year ago
Attachment #8947762 - Flags: review?(bzbarsky)

Comment 5

a year ago
Comment on attachment 8947762 [details]
Bug 169521: fix XML attribute serialization for proper roundtripping

This needs tests.  Incidentally, if we had those, we could test what other browsers do, and see what the compat situation looks like before and after this change...
Attachment #8947762 - Flags: review?(bzbarsky) → review-

Comment 6

a year ago
Comment on attachment 8947762 [details]
Bug 169521: fix XML attribute serialization for proper roundtripping

The compat situation is mentioned in the linked bug for Chromium (Chromium, Edge have the same behavior).

Will change `testing/web-platform/tests/domparsing/XMLSerializer-serializeToString.html` to include this.
Fwiw, there's no obvious linked bug for Chromium here.  ;)

I just tested on (from comment 2) and Safari has the same behavior too.

While I agree this behavior is on its face buggy, the fact that it's interoperably buggy across everyone makes the web compat here a lot higher than I'd like.  :(  Have we reached out to the other UAs about their plans for shipping this?  You should probably send an actual intent to ship per (though the template there doesn't fit this case well) so people who might know of issues around this might have a chance to comment....
Comment hidden (mozreview-request)

Comment 9

a year ago
Sorry, it was linked in the commit message.

Currently, only Safari and Firefox seem to have the buggy behavior -- Chromium was fixed in 2015, while Edge and Presto (no longer relevant now, I guess) have the correct behavior.
Ah, I see.  I totally misread comment 6.  Thank you for explaoining!

Comment 11

a year ago
Comment on attachment 8947762 [details]
Bug 169521: fix XML attribute serialization for proper roundtripping

r=me with the test tweak below.  Thank you for the fix!

::: testing/web-platform/tests/domparsing/XMLSerializer-serializeToString.html:46
(Diff revision 2)
>  }, 'Check if there is no redundant empty namespace declaration.');
> +
> +test(function() {
> +  var serializer = new XMLSerializer();
> +  var root = createXmlDoc().documentElement;
> +  root.firstChild.setAttribute('attr1', 'value1\tvalue2\n');

Should test \r too.
Attachment #8947762 - Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request)

Comment 13

a year ago
The X5 failure is a known issue, fixed in bug 1435644.
Assignee: hjtoi-bugzilla → kulkarni.ashish
Comment hidden (mozreview-request)

Comment 17

a year ago
Pushed by
fix XML attribute serialization for proper roundtripping r=bz
Ashish, thank you again for the fix!

Comment 19

a year ago
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60

Comment 20

a year ago
Thanks also from my side!  (Although I have to admit that I am meanwhile using JSON rather than XML in situations like the one that revealed the bug.)

Comment 21

a year ago
Thanks, but to be honest the patch is just a small variation on what you suggested in comment #1 :)
Created web-platform-tests PR for changes under testing/web-platform/tests

Comment 23

a year ago
Are these tests used by other browsers? If so, there might be differences as Chrome uses 10 while the current patch uses xA, which are functionally equivalent but not handled in the test.
> Are these tests used by other browsers?

Yes.  We should probably allow all valid encodings, which means we should probably test separate strings for the three chars so we don't have to test 8 or more different possible result strings.

Comment 25

a year ago
I will work on it tomorrow, should a new tracking bug be created or this one reopened?
New bug, please.
Can't merge web-platform-tests PR due to failing upstream tests
Upstream PR merged

Comment 29

a year ago
Created bug 1438833 and review request, sorry for the delay.

Comment 30

6 months ago
Backed out part of changeset fe4514c84e7a (bug 169521 for causing bug 1509102) to build Thunderbird 60.3.2. a=jorgk

Comment 31

5 months ago
Backed out test of changeset fe4514c84e7a (bug 169521 / bug 1438833) to build Thunderbird 60.5.0. a=jorgk
I've backed out the test part as well to avoid test failures on the THUNDERBIRD_60_VERBRANCH. Usually I push to the branch with DONTBUILD, but today after a major merge I wanted to get a build.
You need to log in before you can comment on or make changes to this bug.