Opened 12 years ago

Closed 12 years ago

#3878 closed defect (fixed)

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

Reported by: guest Owned by: sjmiles
Priority: high Milestone:
Component: HTML Version: 0.9
Keywords: html getClass Cc: alex
Blocked By: Blocking:

Description

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.

Attachments (2)

getClass.patch (438 bytes) - added by guest 12 years ago.
A patch that fixes the bug.
test_gfxclass.html (1.9 KB) - added by guest 12 years ago.
A test case for this fix.

Download all attachments as: .zip

Change History (14)

Changed 12 years ago by guest

Attachment: getClass.patch added

A patch that fixes the bug.

comment:1 Changed 12 years ago by 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.

comment:2 Changed 12 years ago by 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.

comment:3 in reply to:  2 Changed 12 years ago by 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.

comment:4 Changed 12 years ago by 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.

comment:5 Changed 12 years ago by 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?

comment:6 Changed 12 years ago by 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.

comment:7 Changed 12 years ago by guest

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

comment:8 Changed 12 years ago by 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.

comment:9 Changed 12 years ago by guest

Thanks!

Changed 12 years ago by guest

Attachment: test_gfxclass.html added

A test case for this fix.

comment:10 Changed 12 years ago by alex

Cc: alex added

comment:11 in reply to:  10 Changed 12 years ago by 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.

comment:12 Changed 12 years ago by sjmiles

Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.