dojo.html.getClass fails on an SVG document node, and probably many other document nodes.

[guest]

dojo.html.getClass fails on a non-HTML domNode.

The getClass call assumes that if className exists, it is typeof == "string". That isn't true for at least an SVG document root node. So, the function fails with an exception because the replace function doesn't exist on the className.

addClass also fails because getClass fails.

[guest]

A patch that fixes the bug.

[guest]

Here's the patch that fixes the bug. It's a one line fix that copies a similar fix in setClass that generalizes to node.setAttribute if the attribute is not a simple string.

[sjmiles]

I admit we use the term node loosely, but the input to this function is supposed to be an object of type HTMLElement.

Objects of that type should always have a string property className.

So I'm not convinced this is a bug per se. If it sounds like I'm being pedantic, it's because precision here is important with respect to 0.9.

Before changing any code, I'd like to see a rationale why this function needs to protect itself from this particular bad input.

[guest]

Well, personally, I'd like to see the patch incorporated because it will save me from implementing a specific addClass for svg and vml document nodes. This change is the only blocker that I've found. The other functions in style.js have been generalized to use getAttribute/setAttribute if trivial access would fail.

We can reclassify this as an "enhancement" request instead, if you'd rather. I classified it as a bug because it looked like the style routines were generalized with this exception.

If this introduces a performance penalty, we could implement a try/catch/generalized retry approach that would also achieve generality.

[guest]

To be more specific, with this change, addClass works on dojo.gfx.Surface.rawNode elements, and the browsers I've tested respect the CSS. That's dead useful.

[sjmiles]

Sorry, I'm not being clear.

The classification of the bug is not what I'm on about. In not an expert in SVG, and I want to understand as much as I can about this use case because it's (seemingly) outside of the w3c html specs I generally work from.

Once I understand this better, I don't imagine I will have any trouble getting this patch in.

In particular, can you define "SVG document node" for me?

[guest]

I'm not really an expert in SVG either. But, from tracking this down, it appears SVGElement supports a className attribute which is intended to be very similar to the HTMLElement::className element. It is modified in parallel with the "class" attribute when element.setAttribute ("class", "foo") is called. A javascript string version of the value is returned by element.getAttribute ("class").

However, at least in the Firefox's SVG implementation, the className attribute itself is an SVGAnimatedString instead of a normal javascript string. This probably represents a bug in the Firefox's SVG implementation. But, I'm not an expert enough to prove it. If W3C DOMString == native javascript string, it is definitely a bug in the SVG implementation in Firefox 1.5-2.0

​http://www.w3.org/TR/2000/03/WD-SVG-20000303/struct.html ​http://www.w3.org/TR/SVG11/styling.html#ClassAttribute

Unfortunately, the SVGAnimatedString does not implement the replace method. So, calling addClass on an SVGElement fails with a "cs.replace is not a function" type of error. Getting the attribute as a string with getAttribute, then doing the replace, and then calling setAttribute fixes the problem.

[guest]

Ah, and, addClass fails because it calls getClass, which uses the cs.replace function on the className before returning it.

[sjmiles]

Beautiful, thank you very much for doing that follow up work. That's exactly the kind of documentation I wanted to have here.

I'll get your patch committed.

[guest]

Thanks!

[guest]

A test case for this fix.

[guest]

Abort.

I was looking at the dojo/trunk code instead of dojo/dojo/trunk. The code in dojo/dojo/trunk does not work at all with non HTML nodes, and there are already suggested improvements that will fix these issues in dojox/gfx/_base.js, dojox.gfx._hasClass, _addClass, _removeClass.

[sjmiles]

(In [10067]) Fix getClass/setClass to be SVG friendly (0.4 trunk), fixes #3929, fixes #3878.