Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

SVGAnimatedString causes Slick error #2262

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

mcfedr commented Jan 27, 2012

When i have an animated svg embeded in the page, the '*=' attribute matching crashes because the value className on SVGSVGElement is not a string but SVGAnimatedString

I am not sure this is the best fix, but it works, this might have implications else where.

I considered just skipping all svg's but is seems more helpful to be able to find them and be able to script them.

i found this error matching [class*="tipOptions:"] on the document.

@arian arian and 2 others commented on an outdated diff Jan 27, 2012

Source/Slick/Slick.Finder.js
@@ -860,7 +860,7 @@ for (var p in pseudos) local['pseudo:' + p] = pseudos[p];
var attributeGetters = local.attributeGetters = {
'class': function(){
- return this.getAttribute('class') || this.className;
+ return this.getAttribute('class') || typeOf(this.className) == 'string' ? this.className : null;
@arian

arian Jan 27, 2012

Owner

Maybe

var className = this.getAttribute('class') || this.className;
return className ? ('' + className) : null;

or maybe

return this.getAttribute('class') || ('' + this.className);

but that depends on if className is null or an empty string when it's not set...

@ibolmo

ibolmo Jan 29, 2012

Owner

instead of '' + try String(this.className) or String.prototype.toString.call(this.className)

Also we could do:

var className = this.className;
if (className && className.baseVal) className = className.baseVal;
return className || this.getAttribute('class');
@mcfedr

mcfedr Jan 30, 2012

I think its good to get the baseval, i have had to change it slightly because of "" being false

var className = this.className;
if (className && className.baseVal !== undefined) className = className.baseVal;
return className || this.getAttribute('class');

mcfedr commented Jan 27, 2012

I have created a fiddle that shows the bug, http://jsfiddle.net/mcfedr/RVuFW/

As you can see the suggestion by arian will still cause a problem.

I have found it takes quite a specific selector to get the bug, maybe I am not always triggering Slick. Also it seems only to be for className, not any other attributes.

Owner

ibolmo commented Jan 30, 2012

That's fine can you check that this works:

return (this.className && this.className.baseVal) || this.getAttribute('class');

This takes advantage of && properties.

See:

console.log((true && 1) || 'default');
console.log((false && 1) || 'default');
console.log((false && 0) || 'default');
console.log((true && 0) || 'default');

mcfedr commented Jan 31, 2012

that will return the wrong value for a normal element with a normal string className

var ts = [{className: "hi"},
{className: ""},
{className: {baseVal: "hi"}},
{className: {baseVal: ""}}];
for(t in ts) {
    console.log((ts[t].className && ts[t].className.baseVal) || 'default'); 
}

prints

default
default
hi
default

we need the first case to be hi as well

Owner

arian commented Feb 5, 2012

This is actually something that should be fixed in the https://github.com/mootools/slick repository. @subtleGradient / @fabiomcosta / @ibolmo any good places to put the tests?

Member

sebmarkbage commented Feb 5, 2012

There may be a much simpler solution to it. Simply remove the attributeGetter all together.

I opened a pull request on the slick repository to continue the discussion about whether that's safe or not.

mootools/slick#66

Owner

arian commented Feb 5, 2012

@calyptus fix was merged, so it's fixed in Slick now. Just need to update Slick for mootools 1.4.4 and we're done :)

@arian arian closed this Feb 5, 2012

Owner

ibolmo commented Feb 6, 2012

Committed: a9c73ce

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment