Bug 15276 – setAttribute fails to respect SVG attribute case

• Component: SVG
• Status: RESOLVED DUPLICATE

[Eugene Lazutkin]

I couldn't reproduce it with the static code. but it is 100% reproducible with Dojo. 4 files will be attached:

1) safari_bug.html. Check out the DOjo trunk (svn co http://svn.dojotoolkit.org/dojo/view/anon/all/trunk) and place this file in dojox/gfx/tests/. This file creates 2 rectangles with diagonal linear gradients (red-green-blue, and black-white) then dumps the innerHTML to a textarea for inspection.

2) generated_svg.xml is a sample of generated SVG, which appears valid.

3) generated_pic.jpg is a screenshot, which demonstrates a bug (two solid color rectangles).

4) reference.jpg is a screenshot taken with Firefox and/or Opera, which demonstrates how it should look.

[Eugene Lazutkin]

Created attachment 16375 [details]
The test file (see the bug for description).

Check out the DOjo trunk (svn co http://svn.dojotoolkit.org/dojo/view/anon/all/trunk) and place this file in dojox/gfx/tests/. This file creates 2 rectangles with diagonal linear gradients (red-green-blue, and black-white) then dumps the innerHTML to a textarea for inspection.

[Eugene Lazutkin]

Created attachment 16376 [details]
The generated SVG.

This is a sample of generated SVG, which appears valid.

[Eugene Lazutkin]

Created attachment 16377 [details]
A picture of the bug.

It is a screenshot, which demonstrates a bug (two solid color rectangles instead of two gradients).

[Eugene Lazutkin]

Created attachment 16378 [details]
A reference picture.

This image shows how the gradient should look like.

[Eric Seidel (no email)]

The generated SVG has no namespace information.  I'm not sure if that's a WebKit bug, or normal expected behavior for .innerHTML.  Either way, when you add the necessary namespaces, the SVG displays "the bug".  Haven't looked at it enough yet to see if there is anything wrong in our rendering of this content yet.

[Eric Seidel (no email)]

Created attachment 16384 [details]
The generated SVG (now with proper namespace information)

[Eric Seidel (no email)]

Ok, now I see the bug.  You used "gradientunits" instead of "gradientUnits".  Attributes are case sensitive in SVG.  So we ignore "gradientunits" (and should probably write something to the console when we do this).  This code was defining a gradient vector which was started at 0%, 0% and went to 10000% width, 10000% height of the bounding box of whatever it was rendered in.  So only the starting color was ever displayed.

Does that make sense?

[Eugene Lazutkin]

Webkit's .innerHTML skips all xmlns definitions --- I don't know if this is another bug, or not. Obviously  the file was generated with all proper namespaces. BTW, with your changes it still shows single color squares instead of gradients --- this is the bug I've reported.

In order to make your example work, you have to modify it a little bit more: namely you have to change "gradientunits" to "gradientUnits". The same goes for "patternUnits" (not used in this example). Probably this is the root of the bug --- an attribute's name is lowercased somewhere before assignment, but relevant pieces of code expect the proper camelcase.

[Eugene Lazutkin]

(In reply to comment #7)
> Ok, now I see the bug.  You used "gradientunits" instead of "gradientUnits". 
> Attributes are case sensitive in SVG.  So we ignore "gradientunits" (and should
> probably write something to the console when we do this).  This code was
> defining a gradient vector which was started at 0%, 0% and went to 10000%
> width, 10000% height of the bounding box of whatever it was rendered in.  So
> only the starting color was ever displayed.
> 
> Does that make sense?

No it doesn't make any sense. I create all attributes with proper names. The relevan part from dojox/gfx/svg.js (method _setFill):

=======================================
fill.setAttribute("gradientUnits", "userSpaceOnUse");
=======================================

This is the only place, which creates this attribute.

This is why I am filing it as a bug. Obviously the best solution to me is to modify my code, because in this case it would be fixed long time ago without bothering other people, but unfortunately you have to do something on your side. I apologize for that, but I don't see any other solution to fix the problem.

[Eric Seidel (no email)]

(In reply to comment #9)

Thanks for the further clarification.  I'll take a peak to see if I can figure out where the case is getting lost.

[Eric Seidel (no email)]

I think this has to do with the fact that there are XML elements being inserted into an html document, then an html-only serialization method (which has been made to work correctly for XHTML, btw) is being using to serialize the content.  The serlializer is assuming everything is html and lowercasing since the document is an html document (is my guess).  If this is an innerHTML problem (as I suspect) the only way to fix it would be to ask each element if it was a known xml element or not, and only if it's in an xml document, or in an html document but is itself a known xml element, respect attribute case.

[Eugene Lazutkin]

Eric, I think the direction of this bug took the wrong turn. I don't care about .innerHTML and I don't use it anywhere in my code. I used it in this particular example (in read-only fashion) just to show that the structure I generate (svg, defs, two linear gradients, rects, which reference those gradients) is sound. I don't know any other easy way to visualize a tree in a way suitable for attaching to a bug report.

My code produces this bug using standard DOM methods:

document.createElementNS(svg, elemName)
node.setAttribute(attrName, value)
node.setAttributeNS(xlink, "href", url) // for <image>

and some mundane DOM manipulations: mostly appendChild, some insertBefore.

Let me repeat: no innerHTML is used anywhere. Everything works beautifully except linear gradient. If you want to investigate innerHTML and possible XHTML problems please open new bug, do not hijack this one.

Curiously enough long time ago somebody modified my code to make it work with the Webkit by setting x1, y1, x2, and y2 attributes of linearGradient using setAttributeNS(svg, attrName, value) setting the rest with setAttribute() --- the whole idea is totally wrong. Amazingly it started to produce a gradient, but it was always horizontal no matter how you define it. I told you this story to give you a possible insight in what's broken.

[Eric Seidel (no email)]

Ok, I've learned a few things.

By adding this to your test:

var svgNs = "http://www.w3.org/2000/svg"
var gradient = document.createElementNS(svgNs, "linearGradient");
var gradientClass = gradient.__proto__;
gradientClass.oldSetAttribute = gradientClass.setAttribute;
gradientClass.oldSetAttributeNS = gradientClass.setAttributeNS;

gradientClass.setAttribute = function(a, b, c) { alert("setAttribute: " + a + " " + b); this.oldSetAttributeNS(null, a, b) }
gradientClass.setAttributeNS = function(a, b, c) { alert("setAttributeNS: " + a + " " + b + " " + c); this.oldSetAttributeNS(null, b, c) }


I have determined two things:

1.  Your code is still setting a bunch of attributes with the SVG namespace.  As you've noted above, this is wrong.  We seem to be ignoring the namespace in many parts of our code (which is allowing your setNamespaceNS calls to work, which they shouldn't).

2.  We have a check:
static inline bool shouldIgnoreAttributeCase(const Element* e)
{
    return e && e->document()->isHTMLDocument() && e->isHTMLElement();
}
before every place that we ignore attribute case.  This code is designed to prevent exactly the problem you're seeing, but something's (obviously) not working.

As you notice in my code above, I'm overriding setAttribute and calling setAttributeNS (with a null namespace) instead.  That fixes the problem.  So a quick fix for dojo is to always use setAttributeNS instead.  Be careful to always use a null namespace instead of the svg namespace, as we'll eventually fix our bug and start ignoring attributes in the SVG namespace.

Still investigating.

[Eric Seidel (no email)]

Created attachment 16399 [details]
Simpler test case

[Eric Seidel (no email)]

Well, it appears that the "simple test case" (the case bug) is actually already fixed on the feature branch (or at least not broken).  You can see for yourself with:
http://nightly.webkit.org/builds/mac-feature-branch/1

The gradients still don't appear quite correctly however, I'm unsure why.  It may be related to stricter attribute namespace checking though, as when I turn on my "use setAttributeNS instead" hack on the feature-branch build, everything works fine.

[Eugene Lazutkin]

Eric, good work! This is something I can check on our side. Let me dive into possible setAttributeNS() calls --- it should be easy to check.

[Eric Seidel (no email)]

Yes, so I believe this is now a dojo bug (relating to the improper use of the svg namespace on attributes.  The fix from the feature branch will be rolled into trunk sometime mid november I expect.  (SVG work has been done on the feature branch since may, due to leopard stabilization concerns.)

Again, this is how I diagnosed the bug:

var svgNs = "http://www.w3.org/2000/svg"
var gradient = document.createElementNS(svgNs, "linearGradient");
var gradientClass = gradient.__proto__;
gradientClass.oldSetAttribute = gradientClass.setAttribute;
gradientClass.oldSetAttributeNS = gradientClass.setAttributeNS;

// this override fixes the case sensitivity problem (already fixed on the feature branch)
gradientClass.setAttribute = function(a, b, c) { alert("setAttribute: " + a + " " + b); this.oldSetAttributeNS(null, a, b) }
// this override fixes the dojo "use svg namespace for certain attributes" bug
gradientClass.setAttributeNS = function(a, b, c) { alert("setAttributeNS: " + a + " " + b + " " + c); this.oldSetAttributeNS(null, b, c) }

Do you agree?

[Eric Seidel (no email)]

In fact, here is a link to my original fix:
http://trac.webkit.org/projects/webkit/changeset/21934

(again, this fix is currently only on the feature branch)

[Eric Seidel (no email)]

Ha.  Now what's really entertaining is that this is actually a dup of bug 12187, which was filed by Alex Russel, also from Dojo. :)

[Eugene Lazutkin]

How fresh is the Dojo trunk you use? Could you "svn up" again? setAttributeNS() is commented out in the latest trunk for the svg namespace. It is called in 3 places with the xlink namespace for "href" attribute --- I believe this is the valid use. Could you update and re-run your test?

My understanding of the fix is that I should convert all setAttribute() calls (on the gradient object) like this:

setAttribute(attr, val) => setAttributeNS(null, attr, val)

Is it right? Is it a permanent solution?

[Eugene Lazutkin]

Thank you, thank you, thank you!

I changed my code like that:

==================================
if(dojo.isSafari){
  fill.setAttributeNS(null, "gradientUnits", "userSpaceOnUse");
}else{
  fill.setAttribute("gradientUnits", "userSpaceOnUse");
}
==================================

instead of: fill.setAttribute(...), and it works now. And I did the same for setting "patternUnits". Now both cases work!

While it looks like a bug anyway, it is a bug with a work-around --- something we can use and promote.

Thank you again for helping to narrow it down. If you need any help from me --- just ask.

Thanks,

Eugene

[Eric Seidel (no email)]

(In reply to comment #21)
> While it looks like a bug anyway, it is a bug with a work-around --- something
> we can use and promote.

The "use setAttributeNS(null, name, value) instead of setAttribute(name, value) trick will *always* work.  Fortunately it won't be necessary after feature-branch merges into trunk.  I expect that Safari 3.0 (and thus leopard) will ship with a broken setAttribute call, so dojo will have to use this work around for a while.

I'm going to mark this as a duplicate of the original bug 12187.

Thanks again for the report.

*** This bug has been marked as a duplicate of 12187 ***