Closed Bug 169521 Opened 22 years ago Closed 6 years ago

newline in XML attributes should be serialized as 


Categories

(Core :: XML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: hs, Assigned: ashkulz)

References

Details

Attachments

(1 file)

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
(http://www.w3.org/TR/REC-xml#AVNormalize) 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
nsXMLContentSerializer.cpp.
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 "&".
QA Contact: petersen → rakeshmishra
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: rakeshmishra → ashishbhatt
QA Contact: ashshbhatt → xml
Nice, a 12-year old bug! And still unresolved.

Added a testcase to show what is happening: http://jsfiddle.net/u5ka8f36/

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.
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.)
Attachment #8947762 - Flags: review?(bzbarsky)
Comment on attachment 8947762 [details]
Bug 169521: fix XML attribute serialization for proper roundtripping

https://reviewboard.mozilla.org/r/217480/#review223658

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 on attachment 8947762 [details]
Bug 169521: fix XML attribute serialization for proper roundtripping

https://reviewboard.mozilla.org/r/217480/#review223658

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 http://jsfiddle.net/u5ka8f36/3/ (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 https://wiki.mozilla.org/ExposureGuidelines#Intent_to_ship (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....
Sorry, it was linked in the commit message.

https://bugs.chromium.org/p/chromium/issues/detail?id=418531

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 on attachment 8947762 [details]
Bug 169521: fix XML attribute serialization for proper roundtripping

https://reviewboard.mozilla.org/r/217480/#review223678

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+
Done.
The X5 failure is a known issue, fixed in bug 1435644.
Assignee: hjtoi-bugzilla → kulkarni.ashish
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe4514c84e7a
fix XML attribute serialization for proper roundtripping r=bz
Ashish, thank you again for the fix!
https://hg.mozilla.org/mozilla-central/rev/fe4514c84e7a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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.)
Thanks, but to be honest the patch is just a small variation on what you suggested in comment #1 :)
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/9461 for changes under testing/web-platform/tests
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.
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
Created bug 1438833 and review request, sorry for the delay.
Depends on: 1509102
https://hg.mozilla.org/releases/mozilla-esr60/rev/9f4280a52132bd8335d4833304e4bb7ee4d7c07d
Backed out part of changeset fe4514c84e7a (bug 169521 for causing bug 1509102) to build Thunderbird 60.3.2. a=jorgk
https://hg.mozilla.org/releases/mozilla-esr60/rev/7bd4edb13f5ea0c5936dbc224f1c178df8328eb7
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.

Attachment

General

Created:
Updated:
Size: