newline in XML attributes should be serialized as 


RESOLVED FIXED in Firefox 60

Status

()

RESOLVED FIXED
17 years ago
3 months ago

People

(Reporter: hs, Assigned: ashkulz)

Tracking

Trunk
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

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
(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

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 "&".

Updated

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

Updated

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: 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

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)
(Assignee)

Updated

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

Comment 5

a year 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

a year 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.
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

a year 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.
Ah, I see.  I totally misread comment 6.  Thank you for explaoining!

Comment 11

a year 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

a year ago
Done.
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 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!

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fe4514c84e7a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(Reporter)

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.)
(Assignee)

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 https://github.com/w3c/web-platform-tests/pull/9461 for changes under testing/web-platform/tests
(Assignee)

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.
(Assignee)

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
(Assignee)

Comment 29

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

Comment 30

4 months 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

3 months 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.