Opened 6 years ago

Last modified 18 months ago

#16309 assigned defect

dom-class::add() does not work on SVG elements

Reported by: Simon Speich Owned by: dylan
Priority: high Milestone: 1.14
Component: Core Version: 1.8.1
Keywords: Cc:
Blocked By: Blocking:

Description

Using dom-class::add() on a SVG node such as <g> does not work, because add() uses node[className] instead of node[className].baseVal on line 163.

I'm not sure if this is a bug or an enhancement, but if I use element.setAttribute() instead it works fine, so I would expect add() to work too, since add() is expected to replace setAttribute().

Attachments (2)

addSVGElementSupport.patch (3.1 KB) - added by Simon Speich 6 years ago.
patch which adds support for SVG
dom-class.html (3.9 KB) - added by Simon Speich 6 years ago.
doh for all methods

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by dylan

Component: GeneralCore
Owner: set to Simon Speich
Status: newpending

dom-class works with the HTML DOM, but doesn't currently support the SVG DOM, though that would be nice.

Is this the only change we would need to make to get this working with the SVG DOM? Care to submit a patch with a test?

comment:2 Changed 6 years ago by Simon Speich

Status: pendingnew

I'll look into it and see if I can provide a patch, but it might take a while.

comment:3 Changed 6 years ago by Simon Speich

Unfortunately I can't use node.setAttribute('class') since that is not supported in IE < 8. I guess I just do var cls = node[className].baseVal || node[className]; and see how it goes.

comment:4 Changed 6 years ago by Simon Speich

Furthermore, in SVG node[className] is a read only attribute, which means I would have to use node.setAttribute('class'). Since SVG is not available in IE < 9 I'm not sure what to do now. I would need to check for SVG support and then use setAttribute(). If that's ok for what's worth, then I'll happily will go ahead with (trying to) providing a patch.

Last edited 6 years ago by Simon Speich (previous) (diff)

comment:5 Changed 6 years ago by dylan

I would be ok if that test was done as a has test (that way it could be stripped out for an app with zero SVG dependencies, or no need for old IE, etc.).

comment:6 Changed 6 years ago by Simon Speich

Just a note: If the node is a SVG element with no or an empty class attribute, the expression node[className].baseVal || node[className] returns an SVGAnimatedString and not an empty string, because the empty string evaluates to false and node[className] is returned instead.

comment:7 Changed 6 years ago by Simon Speich

Another explanatory note: To keep changes as small as possible, I just call node.setAttribute('class', cls) in addition to node[className] = cls, which allows setting the class on SVGElements and silently fails on IE (e.g. just sets an unused attribute class). Since the module dojo/dom-class uses a plain object as a fake node for efficient replacement, I also had to add a fake method setAttribute() to the plain object.

Changed 6 years ago by Simon Speich

Attachment: addSVGElementSupport.patch added

patch which adds support for SVG

Changed 6 years ago by Simon Speich

Attachment: dom-class.html added

doh for all methods

comment:8 Changed 6 years ago by Simon Speich

Even if you are not going to use my patch, at least you have group of tests for all the methods. Just remove the SVG tests and keep the DOM tests.

comment:9 Changed 4 years ago by dylan

Milestone: tbd1.11
Owner: changed from Simon Speich to dylan
Status: newassigned

comment:10 Changed 3 years ago by dylan

Milestone: 1.111.12

comment:11 Changed 2 years ago by dylan

Milestone: 1.131.15

Ticket planning... move current 1.13 tickets out to 1.15 to make it easier to move tickets into the 1.13 milestone.

comment:12 Changed 2 years ago by Simon Speich

For dom-class::contains() to work with svg:

contains: function containsClass(/*DomNode|String*/ node, /*String*/ classStr){
	// summary:
	//		Returns whether or not the specified classes are a portion of the
	//		class list currently applied to the node.
	// node: String|DOMNode
	//		String ID or DomNode reference to check the class for.
	// classStr: String
	//		A string class name to look for.
	// example:
	//		Do something if a node with id="someNode" has class="aSillyClassName" present
	//	|	if(domClass.contains("someNode","aSillyClassName")){ ... }
	var cl = dom.byId(node).getAttribute('class');
		return ((" " + cl + " ").indexOf(" " + classStr + " ") >= 0); // Boolean
},

Is there a reason it's >= 0 instead of > -1 ?

comment:13 Changed 2 years ago by dylan

Milestone: 1.151.13
Priority: undecidedhigh

Simon: It's been that way for more than six years (pre-dates dom-class and AMD). No particular reason that I can think of off the top of my head.

comment:14 Changed 18 months ago by dylan

Milestone: 1.131.14
Note: See TracTickets for help on using tickets.