fix(jqLite): use get/setAttribute so that jqLite works on SVG #4129
Conversation
|
@btford you are both fragrant and a talented developer. |
| @@ -279,17 +279,17 @@ function JQLiteData(element, key, value) { | |||
| } | |||
|
|
|||
| function JQLiteHasClass(element, selector) { | |||
| return ((" " + element.className + " ").replace(/[\n\t]/g, " "). | |||
| return ((" " + (element.getAttribute('class') || '') + " ").replace(/[\n\t]/g, " "). | |||
There was a problem hiding this comment.
this is not necessary, reading className property is not an issue for us and the property is updated with every setAttribute call.
There was a problem hiding this comment.
Yes it is; SVG Nodes are special. className is an object for them :(
There was a problem hiding this comment.
@btford is right here. In this case, the className is of type SVGAnimatedString. This has a property baseVal that you can read if you like but writing to it has no affect on the actual CSS class attribute of the element. getAttribute and setAttribute seem like the only reasonable solution.
|
Shouldn't the spec/test be in jqLiteSpec.js rather than animateSpec.js? |
jqLite previously used `elt.className` to add and remove classes from a DOM Node, but because the className property is not writable on SVG elements, it doesn't work with them. This patch replaces accesses to `className` with `get/setAttribute`. `classList` was also considered as a solution, but because only IE10+ supports it, we have to wait. :'( Closes angular#3858
|
@petebacondarwin I don't even know anymore. We need to test that |
|
The work around for JQuery is to use the 3rd party plugin I linked to in the original issue. |
| @@ -156,7 +156,9 @@ var $AnimateProvider = ['$provide', function($provide) { | |||
| className = isString(className) ? | |||
| className : | |||
| isArray(className) ? className.join(' ') : ''; | |||
| element.addClass(className); | |||
| forEach(element, function (element) { | |||
There was a problem hiding this comment.
wouldn't forEach try to iterate over element properties if element is not an array? I suspect that we need an isArray check here
There was a problem hiding this comment.
The element passed to addClass should be a jQuery/jqLite element, so it's always an ArrayLike object, making forEach treat is as an array instead of an object
So forEach'ing it allows you to iterate over each DOM node inside the collection.
Maybe a change in the naming of the variable would make this more clear?
That's related to style more than anything else
There was a problem hiding this comment.
what @rodyhaddad said.
There was a problem hiding this comment.
oh. ok. that makes sense.
|
ok, this looks good to me. just check the forEach and make sure it handles single elements correctly. then merge. |
|
Landed as c785267. |
|
But this doesn't work in IE7. |
jqLite previously used
elt.classNameto add and remove classes from a DOM Node, but because the className property is not writable on SVG elements, it doesn't work with them. This patch replaces accesses toclassNamewithget/setAttribute.Note that jQuery still uses
className, so developers who include jQuery will have this functionality broken. The solution for them is to have jQuery fix it. :)classListwas also considered as a solution, but because only IE10+ supports it, we have to wait.Closes #3858