Bug 79586

Summary:

xmlserializer strips xlink from xlink:html svg image tag

Product:

WebKit

Reporter:

Peter Kemp <peterejkemp>

Component:

SVG

Assignee:

Stephen Chenney <schenney>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, dglazkov, krit, mifenton, peterejkemp, rwlbuis, schenney, webkit.review.bot, zimmermann

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Windows XP

URL:

http://kemputing.com/demo/importImage.html

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from ec2-cr-linux-04	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	webkit.review.bot: commit-queue-

Peter Kemp

Reported 2012-02-25 16:11:48 PST

I am creating SVGs programmatically with other embedded SVG images. They display correctly on the screen when i create them, but when I serialize the SVG to save it, it strips out the xlink namespace information, and subsequent loading of the saved SVG doesn't render the embedded images as the xlink information is missing. Please see the demo URL above. I'm using Webkit: 535.21 running in Chrome: 19.0.1041 and have also seen the error in Webkit: 535.7 Thanks Pete

Attachments
Patch (5.22 KB, patch) 2012-03-24 12:40 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-04 (9.72 MB, application/zip) 2012-03-24 13:32 PDT, WebKit Review Bot	no flags	Details
Patch (6.46 KB, patch) 2012-03-24 16:08 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Patch (6.40 KB, patch) 2012-03-26 05:31 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Patch (5.63 KB, patch) 2012-07-20 14:28 PDT, Stephen Chenney	no flags	Details Formatted Diff Diff
Patch (10.97 KB, patch) 2012-07-23 12:09 PDT, Stephen Chenney	no flags	Details Formatted Diff Diff
Patch (10.31 KB, patch) 2012-07-23 12:55 PDT, Stephen Chenney	no flags	Details Formatted Diff Diff
Patch (13.83 KB, patch) 2012-07-25 11:21 PDT, Stephen Chenney	no flags	Details Formatted Diff Diff
Patch for landing (10.52 KB, patch) 2012-07-27 11:39 PDT, Stephen Chenney	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Rob Buis

Comment 1 2012-03-24 12:40:37 PDT

Created attachment 133649 [details] Patch

WebKit Review Bot

Comment 2 2012-03-24 13:31:53 PDT

Comment on attachment 133649 [details] Patch Attachment 133649 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12127240 New failing tests: fast/xsl/nbsp-in-stylesheet.html editing/pasteboard/paste-noscript-svg.html

WebKit Review Bot

Comment 3 2012-03-24 13:32:00 PDT

Created attachment 133650 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Rob Buis

Comment 4 2012-03-24 16:08:05 PDT

Created attachment 133659 [details] Patch

Adam Barth

Comment 5 2012-03-24 20:32:10 PDT

Comment on attachment 133659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133659&action=review > Source/WebCore/editing/MarkupAccumulator.cpp:385 > - if (documentIsHTML) > + if (documentIsHTML && element->isHTMLElement()) This change seems very reasonable. > Source/WebCore/svg/SVGURIReference.cpp:36 > + attr->setPrefix("xlink"); We must have xlink in an AtomicString somewhere already. It seems strange to mutate attr here. This function is generally about updating |this| to take |attr| into account.

Nikolas Zimmermann

Comment 6 2012-03-25 04:46:39 PDT

Comment on attachment 133659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133659&action=review Looks good, r=me, with comments: >> Source/WebCore/svg/SVGURIReference.cpp:36 >> + attr->setPrefix("xlink"); > > We must have xlink in an AtomicString somewhere already. It seems strange to mutate attr here. This function is generally about updating |this| to take |attr| into account. Correct, it's XLinkNames::hrefAttr. > LayoutTests/svg/custom/xlink-prefix-in-attributes.html:1 > +<html xmlns="http://www.w3.org/1999/xhtml"> I would have constructed a regular .svg testcase for this, and inject code, that serializes the document to a String, and put that as textContent of a <text> element. You can either follow my suggestion, and rework this, or keep your current test and fix the comments below: > LayoutTests/svg/custom/xlink-prefix-in-attributes.html:9 > + var root = document.getElementById("svg"); Quite confusing, I'd name this differently, this suggests its an SVGSVGElement. how about "var target = document.getElementById("target")". > LayoutTests/svg/custom/xlink-prefix-in-attributes.html:15 > + var svgEl = document.createElementNS(svgns, "svg"); s/svgEl/svgElement/ or root, or svg, but no abbreviations. > LayoutTests/svg/custom/xlink-prefix-in-attributes.html:17 > + svgEl.setAttributeNS(null, "svg", svgns); What's the purpose of this? > LayoutTests/svg/custom/xlink-prefix-in-attributes.html:19 > + svgEl.setAttributeNS(null, "width", 200); , "200" > LayoutTests/svg/custom/xlink-prefix-in-attributes.html:22 > + var imgElem = document.createElementNS(svgns, "image"); s/imgElem/image/ > LayoutTests/svg/custom/xlink-prefix-in-attributes.html:23 > + imgElem.setAttributeNS(null, "width" ,20); , "20" (space is misplaced).

Rob Buis

Comment 7 2012-03-26 05:31:57 PDT

Created attachment 133783 [details] Patch

Rob Buis

Comment 8 2012-03-27 07:22:05 PDT

Hi Niko, (In reply to comment #6) > (From update of attachment 133659 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133659&action=review > > Looks good, r=me, with comments: > > >> Source/WebCore/svg/SVGURIReference.cpp:36 > >> + attr->setPrefix("xlink"); > > > > We must have xlink in an AtomicString somewhere already. It seems strange to mutate attr here. This function is generally about updating |this| to take |attr| into account. > > Correct, it's XLinkNames::hrefAttr. So "xlink" is nowhere AFAICS, we could introduce it if needed. I agree it is a strange place, but I found no better one. I am not sure about the "it's XLinkNames::hrefAttr" comment. The attribute qname is not the same object as XLinkNames::hrefAttr AFAIK, but matches its localName and namespace. So I thought about adding the xlink prefix to XLinkNames::hrefAttr but in this case it would not help. Note that Mozilla always seems to add the xlink prefix when serializing this. Let me know if anybody sees a better way, I'd like to clear it up before landing. > > LayoutTests/svg/custom/xlink-prefix-in-attributes.html:1 > > +<html xmlns="http://www.w3.org/1999/xhtml"> > > I would have constructed a regular .svg testcase for this, and inject code, that serializes the document to a String, and put that as textContent of a <text> element. > You can either follow my suggestion, and rework this, or keep your current test and fix the comments below: > > > LayoutTests/svg/custom/xlink-prefix-in-attributes.html:9 > > + var root = document.getElementById("svg"); > > Quite confusing, I'd name this differently, this suggests its an SVGSVGElement. > how about "var target = document.getElementById("target")". > > > LayoutTests/svg/custom/xlink-prefix-in-attributes.html:15 > > + var svgEl = document.createElementNS(svgns, "svg"); > > s/svgEl/svgElement/ or root, or svg, but no abbreviations. > > > LayoutTests/svg/custom/xlink-prefix-in-attributes.html:17 > > + svgEl.setAttributeNS(null, "svg", svgns); > > What's the purpose of this? Hmm, maybe it is not needed, I didnt want to change the original testcase too much. > > LayoutTests/svg/custom/xlink-prefix-in-attributes.html:19 > > + svgEl.setAttributeNS(null, "width", 200); > > , "200" > > > LayoutTests/svg/custom/xlink-prefix-in-attributes.html:22 > > + var imgElem = document.createElementNS(svgns, "image"); > > s/imgElem/image/ > > > LayoutTests/svg/custom/xlink-prefix-in-attributes.html:23 > > + imgElem.setAttributeNS(null, "width" ,20); > > , "20" (space is misplaced). I have corrected the testcase problems in my latest patch. Cheers, Rob.

Ryosuke Niwa

Comment 9 2012-04-22 22:49:30 PDT

Comment on attachment 133783 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133783&action=review > Source/WebCore/ChangeLog:8 > + Always write out xlink prefix on href attribute for SVG elements. This description isn't quite accurate. We're writing out qualified names for all non-HTML elements. > Source/WebCore/editing/MarkupAccumulator.cpp:385 > - if (documentIsHTML) > + if (documentIsHTML && element->isHTMLElement()) According to http://www.whatwg.org/specs/web-apps/current-work/multipage/the-end.html#serializing-html-fragments, we should be checking attribute's namespace, not element's namespace. So this won't match the spec if e.g. xml:lang is specified.

Rob Buis

Comment 10 2012-04-24 21:04:36 PDT

(In reply to comment #9) > (From update of attachment 133783 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133783&action=review > > > Source/WebCore/ChangeLog:8 > > + Always write out xlink prefix on href attribute for SVG elements. > > This description isn't quite accurate. We're writing out qualified names for all non-HTML elements. > > > Source/WebCore/editing/MarkupAccumulator.cpp:385 > > - if (documentIsHTML) > > + if (documentIsHTML && element->isHTMLElement()) > > According to http://www.whatwg.org/specs/web-apps/current-work/multipage/the-end.html#serializing-html-fragments, > we should be checking attribute's namespace, not element's namespace. So this won't match the spec if e.g. xml:lang is specified. http://html5.org/specs/dom-parsing.html#the-xmlserializer-interface seems to indicate an XML serialization algorithm, not HTML, so I don't think the above link applies, can you check? Cheers, Rob.

Alexey Proskuryakov

Comment 11 2012-04-24 23:29:14 PDT

Both specs are new, and given massive differences in behavior between Gecko and Firefox, I'm not sure if they are worth much unless we agree with them.

Alexey Proskuryakov

Comment 12 2012-04-24 23:29:38 PDT

Gecko and _WebKit_!

Stephen Chenney

Comment 13 2012-07-20 11:45:25 PDT

This has become a high Google priority. I'll sort out the minor issues and take it from here. Credit will go to Rob when I'm done.

Stephen Chenney

Comment 14 2012-07-20 14:28:31 PDT

Created attachment 153602 [details] Patch

Stephen Chenney

Comment 15 2012-07-20 14:33:01 PDT

The new patch removes unnecessary and now impossible change to the attribute when in SVGURIReference. And it does as Ryosuke suggested; it only applies the prefix for attributes with namespace information. I think Ryosuke was the only person with outstanding questions on this, so he should review.

Stephen Chenney

Comment 16 2012-07-20 15:05:51 PDT

Comment on attachment 153602 [details] Patch Not quite there. We do need to output the xlink: in front of href if not already present. The test case needs to be updated to test this.

Stephen Chenney

Comment 17 2012-07-23 12:09:39 PDT

Created attachment 153831 [details] Patch

Ryosuke Niwa

Comment 18 2012-07-23 12:16:45 PDT

Comment on attachment 153831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153831&action=review > Source/WebCore/editing/MarkupAccumulator.cpp:426 > +static inline bool shouldPrefixAttributeWithXLinkNamespace(const Attribute& attribute) > +{ > + return attribute.namespaceURI() == XLinkNames::xlinkNamespaceURI && !attribute.name().toString().startsWith("xlink:"); > +} I would have inlined these functions in the if. These added abstractions make the code less self-evident. > Source/WebCore/editing/MarkupAccumulator.cpp:430 > + return attribute.namespaceURI() == XMLNames::xmlNamespaceURI && !attribute.name().toString().startsWith("xml:"); Why can't we do attribute.name().prefix() == "xml"? > Source/WebCore/editing/MarkupAccumulator.cpp:461 > + if (documentIsHTML && !attributeIsInSerializedNamespace(attribute)) > result.append(attribute.name().localName()); > - else > + else { > + if (shouldPrefixAttributeWithXLinkNamespace(attribute)) > + result.append("xlink:"); > + else if (shouldPrefixAttributeWithXMLNamespace(attribute)) > + result.append("xml:"); > + else if (shouldPrefixAttributeWithXMLNSNamespace(attribute)) > + result.append("xmlns:"); It's probably better to allocate a local QualifiedName and then call setPrefix on that according to the rules here.

Stephen Chenney

Comment 19 2012-07-23 12:55:20 PDT

Created attachment 153844 [details] Patch

Stephen Chenney

Comment 20 2012-07-23 12:58:39 PDT

(In reply to comment #18) > (From update of attachment 153831 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153831&action=review > > > Source/WebCore/editing/MarkupAccumulator.cpp:426 > > +static inline bool shouldPrefixAttributeWithXLinkNamespace(const Attribute& attribute) > > +{ > > + return attribute.namespaceURI() == XLinkNames::xlinkNamespaceURI && !attribute.name().toString().startsWith("xlink:"); > > +} > > I would have inlined these functions in the if. These added abstractions make the code less self-evident. Done. I wasn't sure what reviewers would prefer. :-) > > Source/WebCore/editing/MarkupAccumulator.cpp:430 > > + return attribute.namespaceURI() == XMLNames::xmlNamespaceURI && !attribute.name().toString().startsWith("xml:"); > > Why can't we do attribute.name().prefix() == "xml"? Because if you write in JS something like this: image.setAttributeNS(xlinkns, "href", "resources/green-checker.png"); there is no prefix at all on the href attribute, and we need to add one. The namespaceURI is the only reliable indicator, because you must provide that with any of the JS namespace-dependent creation method. > > Source/WebCore/editing/MarkupAccumulator.cpp:461 > > + if (documentIsHTML && !attributeIsInSerializedNamespace(attribute)) > > result.append(attribute.name().localName()); > > - else > > + else { > > + if (shouldPrefixAttributeWithXLinkNamespace(attribute)) > > + result.append("xlink:"); > > + else if (shouldPrefixAttributeWithXMLNamespace(attribute)) > > + result.append("xml:"); > > + else if (shouldPrefixAttributeWithXMLNSNamespace(attribute)) > > + result.append("xmlns:"); > > It's probably better to allocate a local QualifiedName and then call setPrefix on that according to the rules here. Done. And I'm using some pre-defined AtomicStrings where they exist.

Ryosuke Niwa

Comment 21 2012-07-24 13:33:10 PDT

Comment on attachment 153844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153844&action=review > Source/WebCore/editing/MarkupAccumulator.cpp:441 > + if (!attribute.name().toString().startsWith("xlink:")) Per IRC discussion, please replace this by attribute.name().prefix() != "xlink" or get rid of it altogether since there's no harm in overriding prefix here. > Source/WebCore/editing/MarkupAccumulator.cpp:447 > + if (!attribute.name().toString().startsWith("xml:")) > + prefixedName.setPrefix(WTF::xmlAtom); > + } else if (attribute.namespaceURI() == XMLNSNames::xmlnsNamespaceURI) { > + if (attribute.name() != XMLNSNames::xmlnsAttr && !attribute.name().toString().startsWith("xmlns:")) Ditto. Here, they'll be simple AtomicString comparisons.

Stephen Chenney

Comment 22 2012-07-25 11:21:33 PDT

Created attachment 154400 [details] Patch

WebKit Review Bot

Comment 23 2012-07-25 11:24:57 PDT

Attachment 154400 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 Source/WTF/wtf/text/StringStatics.cpp:62: DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Stephen Chenney

Comment 24 2012-07-25 11:44:24 PDT

Style failure is bogus. I'm doing exactly what the existing code does, using a well established macro.

Nikolas Zimmermann

Comment 25 2012-07-26 00:40:39 PDT

Comment on attachment 154400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154400&action=review Looks great! r=me, with some nitpicks: > Source/WTF/wtf/text/AtomicString.h:199 > +extern const WTF_EXPORTDATA AtomicString xlinkAtom; You should add "using WTF::xllinkAtom;" some lines below as well, so we can make use of this w/o using the WTF:: prefix. > Source/WebCore/editing/MarkupAccumulator.cpp:441 > + if (attribute.prefix() != WTF::xlinkAtom) WTF:: prefix is unncessary, no? Or is ATOMICSTRING_HIDE_GLOBALS on by default?

Stephen Chenney

Comment 26 2012-07-27 11:07:08 PDT

(In reply to comment #25) > (From update of attachment 154400 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154400&action=review > > Looks great! r=me, with some nitpicks: > > > Source/WTF/wtf/text/AtomicString.h:199 > > +extern const WTF_EXPORTDATA AtomicString xlinkAtom; > > You should add "using WTF::xllinkAtom;" some lines below as well, so we can make use of this w/o using the WTF:: prefix. Yes. Done. > > Source/WebCore/editing/MarkupAccumulator.cpp:441 > > + if (attribute.prefix() != WTF::xlinkAtom) > > WTF:: prefix is unncessary, no? Or is ATOMICSTRING_HIDE_GLOBALS on by default? There is no explicit namespace when xmlAtom is used in the code, so it seems very safe to assume that ATOMICSTRING_HIDE_GLOBALS is turned only only in cases when the globals are not needed at all, and that it is safe to use xlinkAtom without a namespace.

Stephen Chenney

Comment 27 2012-07-27 11:39:46 PDT

Created attachment 154997 [details] Patch for landing

WebKit Review Bot

Comment 28 2012-07-27 13:57:00 PDT

Comment on attachment 154997 [details] Patch for landing Rejecting attachment 154997 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: bkit-commit-queue/Source/WebKit/chromium/sdch/open-vcdiff --revision 42 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 19>At revision 42. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/13370714

Ryosuke Niwa

Comment 29 2012-07-30 10:52:40 PDT

Comment on attachment 154997 [details] Patch for landing Let's try again.

WebKit Review Bot

Comment 30 2012-07-30 12:58:52 PDT

Comment on attachment 154997 [details] Patch for landing Rejecting attachment 154997 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: conflict in LayoutTests/platform/chromium/TestExpectations Auto-merging Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Unreviewed GTK gardening. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. Full output: http://queues.webkit.org/results/13384675

Stephen Chenney

Comment 31 2012-07-30 13:07:23 PDT

I'll land manually.

Dirk Schulze

Comment 32 2012-07-30 14:00:30 PDT

I may did not get the full context. But the SVG WG decided to get rid of XLink namespace where ever possible. Wouldn't it be better to just accept href without namespace directly? That is what SVG 2.0 will promote anyway.

Stephen Chenney

Comment 33 2012-07-31 08:47:16 PDT

> I may did not get the full context. But the SVG WG decided to get rid of XLink namespace where ever possible. Wouldn't it be better to just accept href without namespace directly? That is what SVG 2.0 will promote anyway. We accept href without namespace when parsing code, but we do need the prefix on xmlns:xlink in order to handle href's correctly.

Stephen Chenney

Comment 34 2012-07-31 09:02:38 PDT

Committed r124210: <http://trac.webkit.org/changeset/124210>

Note You need to log in before you can comment on or make changes to this bug.