Opened 14 years ago
Closed 14 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)
Change History (14)
Changed 14 years ago by
Attachment: | getClass.patch added |
---|
comment:1 Changed 14 years ago by
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 follow-up: 3 Changed 14 years ago by
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 Changed 14 years ago by
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 14 years ago by
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 14 years ago by
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 14 years ago by
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 14 years ago by
Ah, and, addClass fails because it calls getClass, which uses the cs.replace function on the className before returning it.
comment:8 Changed 14 years ago by
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:10 follow-up: 11 Changed 14 years ago by
Cc: | alex added |
---|
comment:11 Changed 14 years ago by
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 14 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
A patch that fixes the bug.