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
Created attachment 133649 [details] Patch
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
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
Created attachment 133659 [details] Patch
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.
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).
Created attachment 133783 [details] Patch
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.
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.
(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.
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.
Gecko and _WebKit_!
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.
Created attachment 153602 [details] Patch
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.
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.
Created attachment 153831 [details] Patch
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.
Created attachment 153844 [details] Patch
(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.
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.
Created attachment 154400 [details] Patch
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.
Style failure is bogus. I'm doing exactly what the existing code does, using a well established macro.
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?
(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.
Created attachment 154997 [details] Patch for landing
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
Comment on attachment 154997 [details] Patch for landing Let's try again.
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
I'll land manually.
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.
> 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.
Committed r124210: <http://trac.webkit.org/changeset/124210>