Closed
Bug 169521
Opened 22 years ago
Closed 7 years ago
newline in XML attributes should be serialized as 

Categories
(Core :: XML, defect)
Core
XML
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.
Reporter | ||
Comment 1•22 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 "&".
Updated•22 years ago
|
QA Contact: petersen → rakeshmishra
Updated•22 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•21 years ago
|
QA Contact: rakeshmishra → ashishbhatt
Updated•15 years ago
|
QA Contact: ashshbhatt → xml
Comment 2•10 years ago
|
||
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.
Comment 3•10 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) |
Assignee | ||
Updated•7 years ago
|
Attachment #8947762 -
Flags: review?(bzbarsky)
Comment 5•7 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
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.
Comment 7•7 years ago
|
||
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....
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
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.
Comment 10•7 years ago
|
||
Ah, I see. I totally misread comment 6. Thank you for explaoining!
Comment 11•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
Done.
Comment 15•7 years ago
|
||
The X5 failure is a known issue, fixed in bug 1435644.
Updated•7 years ago
|
Assignee: hjtoi-bugzilla → kulkarni.ashish
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fe4514c84e7a fix XML attribute serialization for proper roundtripping r=bz
Comment 18•7 years ago
|
||
Ashish, thank you again for the fix!
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fe4514c84e7a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Reporter | ||
Comment 20•7 years 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.)
Assignee | ||
Comment 21•7 years 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 https://github.com/w3c/web-platform-tests/pull/9461 for changes under testing/web-platform/tests
Assignee | ||
Comment 23•7 years 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.
Comment 24•7 years ago
|
||
> 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.
Assignee | ||
Comment 25•7 years ago
|
||
I will work on it tomorrow, should a new tracking bug be created or this one reopened?
Comment 26•7 years ago
|
||
New bug, please.
Can't merge web-platform-tests PR due to failing upstream tests
Upstream PR merged
Assignee | ||
Comment 29•7 years ago
|
||
Created bug 1438833 and review request, sorry for the delay.
Comment 30•6 years ago
|
||
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
Comment 31•6 years ago
|
||
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.
Description
•