CSS animation via JavaScript

Question

I call this animateElements function as part of an animation loop (inspired via Dave Gamache). Everything works as expected, but I would love to try and improve/optimize if possible.

var animation, translate, translateY, translateX, scale, rotate, opacity, display; animateElements = function() { //var animation, translateY, translateX, scale, rotate, opacity, display; //do{ //No performance improvement in chrome with neg-loop + ...havent tested others //var i = keyframesConverted[currentKeyframe].animations.length; //while(i--) { for (var i = 0; i < keyframesConverted[currentKeyframe].animations.length; i++) { animation = keyframesConverted[currentKeyframe].animations[i]; //NOT MUCH IN IT: this "if" vs ternary, but fps was better on ternary. display = (typeof animation['display'] === 'undefined') ? null : animation['display'][1] || null; if (display !== 'none') { translateY = (typeof animation['translateY'] === 'undefined') ? 0 : calcPropValue(animation, 'translateY', 2); translateX = (typeof animation['translateX'] === 'undefined') ? 0 : calcPropValue(animation, 'translateX', 2); scale = (typeof animation['scale'] === 'undefined') ? "" : " scale(" + calcPropValue(animation, 'scale', 2) + ")"; rotate = (typeof animation['rotate'] === 'undefined') ? "" : " rotateX(" + calcPropValue(animation, 'rotate', 1) + "deg)"; opacity = (typeof animation['opacity'] === 'undefined') ? 1 : calcPropValue(animation, 'opacity', 2); translate = 'translate3d(' + translateX + 'px, ' + translateY + 'px, 0.01px)'; $(animation.selector).css({ 'transform': translate + scale + rotate, 'opacity': opacity, 'display': display //= (opacity < 0) ? 'none' : display//Todo: Potential for performance Improvement but would require major recode? }); } else { $(animation.selector).css({ 'display': 'none' }); } }//while(i--) } calcPropValue = function (animation, property, deciPlaces) { var value = animation[property]; return (tweenEase(relativeScrollTop, value[0], (value[1] - value[0]), keyframesConverted[currentKeyframe].duration, (typeof animation['tween'] === 'undefined') ? null : animation['tween'][1] || null)).toFixed(deciPlaces); } 

Concerns:

• The use of typeof animation[...] as most likely the ternary will reference it twice a loop, for each animation property; hence, should I place it in a var?
• When calling calcPropValue(), would there be a performance gain by just passing animation['...type'] in one go, rather than the 'animation' and then its 'type'?
• As this function has no further use - except from where it is called - would it be an improvement just to move this function block into that animation loop directly, as I assume 'fewer calls to functions, the better'?

Answer 1

Your priority is to reduce dependency on expensive strings :

• Change all associative syntax, animation['x'] >>> dot notation, animation.x .
• Change 'undefined' >>>undefined.
• Avoid passing/returning strings to/from functions. They are passed by value (a function uses its own copy).
• Pass back a number from calcPropValue() and do the toFixed() back in the caller statement.

Also :

• The tween setting is the same for every call in the same turn of the for loop, therefore calculate it once and pass to calcPropValue().
• Simplify testing of tween and display values.

Inside the for loop, assign :

display = (animation.display && animation.display[1]) || null; tween = (animation.tween && animation.tween[1]) || null; 

Don't forget to add tween to the var declaration.

Modify calcPropValue() as follows :

calcPropValue = function (value, tween) { return tweenEase(relativeScrollTop, value[0], (value[1] - value[0]), keyframesConverted[currentKeyframe].duration, tween); } 

Ternery's will become, eg :

translateX = animation.translateX ? calcPropValue(animation.translateX, tween).toFixed(2) : '0.00'; 

Then consider sacrificing readability for performance by not assigning what the terneries return. Instead write them directly into the expressions where the resultant strings get used, eg :

'transform': 'translate3d(' + (animation.translateX ? calcPropValue(animation.translateX, tween).toFixed(2) : '0.00') + 'px, ' + (animation.translateY ? calcPropValue(animation.translateY, tween).toFixed(2) : '0.00') + 'px, 0.01px)' + (animation.scale ? ' scale(' + calcPropValue(animation.scale, tween).toFixed(2) + ')' : '') + (animation.rotate ? ' rotateX(' + calcPropValue(animation.rotate, tween).toFixed(1) + "deg)" : ''), 

When you're done, pass the code through jslint or jshint to help spot any bloomers.

Answer 2

First I would say remove the dead code, like the ones in the comments. They're useless. If I encountered commented-out code in a project, I wouldn't even read it nor give value to it. It's dead code regardless of what people put in the comments. Besides, if you use version control, there's a history for that code. One can simply roll back.

var fName = function(...){...} // vs function fName(...){...} 

Next would be to define your functions as actual function declarations, and not function expressions assigned to variables. One advantage to it is "hoisting". This means, regardless of where in the scope the function is declared, they're always available.

Next, you'd need actual variables when using function expressions. Look, you've forgotten to declare animateElements and calcPropValue with var. This means they go straight up in the global scope, potentially clobbering any other global with the same name. If wrote them as function declarations, they'd stick inside the scope where they're defined.

Next is your variables. I don't see the reason why they are outside animateElements when all of them are only being used in animateElements. Move them in. Saves the engine time to locate them outside the scope of the function.

I see you're checking against undefined. undefined is falsy, which means you can use loose comparison. Additionally, it's usually simpler if you put the truthy result first. That way, you don't have to deal with negations. You can simplify the ternary into:

value = animation['property'] ? calcPropValue(...) : 0; 

Next, calcPropValue is totally unreadable. I suggest you pull out every property into a variable that's properly named. Then call tweenEase with these variables. Pulling stuff into variables have negligible effect on performance. If you're worried that they're affecting performance, then compile them with something like Closure compiler which inlines stuff when it can.

Now your biggest enemy isn't how you're structuring code. The above review is simply semantics and readability. The performance problem lies on your excessive use of $(animation.selector).css({ ... }). Internally, what this call does is:

• Parse the selector. This is slow.
• Locate potential matches in the DOM. This is very slow.
• Wrap the result set as a jQuery object instance. You're creating an object, thus eating memory.
• Alter the CSS. This causes the browser to not just calculate the layout for your element, but it also needs to recalculate how the surrounding elements affect your element as well as how your element affects them.
• You're actually creating an object on each call of .css (the {} part of .css).

But this is a black box in terms of your code, and you can't do much about it's implementation. What I can suggest is to reduce their impact by lessening their use.

For the elements, I suggest you fetch these selectors once, store them in something (like in your data structure). That way, JS doesn't have to parse the selector, look for the element and wrap it in jQuery.

As for the .css, you can do nothing with that. But we can save you from creating that object every call. You can simply create one object, and on each iteration, just update its values and send it to .css().

var obj = {}; for(...){ obj[something] = value; $(...).css(obj); } 

Answer 3

My first concern is the use of typeof animation[...]? As most likely the ternary will reference it twice a loop, for each animation property; hence, should I place it in a var?

I don't quite understand what this question is asking so I'm going to address this:

animation[...] 

In JavaScript, bracket notation is usually only used for arrays or for objects when the key is in a variable. Since most of the time you are just putting an immediate string literal in the brackets, there is no point and you should use dot notation.

That would look like this, for example:

animation.display 

---

And when calling calcPropValue() would there be a performance gain by just passing "animation['...type']" in one go, rather than the 'animation' and then its 'type'?

I'm not sure how much performance is to be gained from this (not much, I don't think, but I'm not sure), but yes; this can be simplified.

Both of the functions that you have presented share the same scope, so there is no need for an animation parameter since you are always passing in that animation variable.

Basically, all you need to do is remove that parameter.

---

Also, as this function has no further use - except from where it is called - would it be an improvement just to move this function block into that animation loop directly, as I assume 'less calls to functions, the better'?

I don't know the rest of your code, but I believe that it is fine keeping this as its own function; it is good to separate logic in code.

You will not have much of a performance impact with function calls, and it's okay to use as many as you need as long as you are following good practices (and you are for this, as far as I can tell).

---

(typeof animation['display'] === 'undefined') 

You do something like this a lot in your code.

In JavaScript, undefined is a false-y value. This means that it can be treated as though it were just false.

That repeated expression can simply become:

!animation.display 

(with dot notation)

---

//NOT MUCH IN IT: this "if" vs ternary, but fps was better on ternary. 

What? That doesn't make any sense. There should not be any change in speed based on whether or not something is an if or something is a ternary. Even if there was, the difference would not be that great.

Now, for the most part, you are doing okay with the ternaries that you are using. However, for the most of your ternaries, I think you should switch the order. For example, right now you are doing this:

translateY = (typeof animation['translateY'] === 'undefined') ? 0 : calcPropValue(animation, 'translateY', 2); 

This makes it seem like you are almost expecting the value to be undefined. Instead, I think you should make it like this:

translateY = (typeof animation['translateY'] !== 'undefined') ? calcPropValue(animation, 'translateY', 2) : 0; 

Now it reads a bit better.

However, not all of your ternaries are all that great:

return (tweenEase(relativeScrollTop, value[0], (value[1] - value[0]), keyframesConverted[currentKeyframe].duration, (typeof animation['tween'] === 'undefined') ? null : animation['tween'][1] || null)).toFixed(deciPlaces); 

What is this supposed to be? I can't even read this.

In this situation, this should definitely be an if statement. That plus some indentation would make this a whole lot more readable.

---

Again, I had a lot of trouble reading this section, but this:

... || null)).toFixed(deciPlaces); 

concerns me. What if this conditional returns null? Calling toFixed on it would result in a TypeError.

Are you handling this somewhere else in your code?

---

animateElements = function() { 

Where is the var? Without this, you are making this function "global" and that is bad practice. If you want to intentionally make this global, do:

window.animateElements = function() { 

This is much better practice.

---

Remove all the extra comments in your code (the ones with old code in them). They are just cluttering up your code.

---

Now your code, after following (most) of the above recommendations, would look like this:

var animation, translate, translateY, translateX, scale, rotate, opacity, display; var animateElements = function () { for (var i = 0; i < keyframesConverted[currentKeyframe].animations.length; i++) { animation = keyframesConverted[currentKeyframe].animations[i]; display = animation.display ? animation.display[1] || null : null if (display !== 'none') { translateY = animation.translateX ? calcPropValue('translateY', 2) : 0; translateX = animation.translateY ? calcPropValue('translateX', 2) : 0; scale = animation.scale ? " scale(" + calcPropValue('scale', 2) + ")" : ""; rotate = animation.rotate ? " rotateX(" + calcPropValue('rotate', 1) + "deg)" : ""; opacity = animation.opacity ? calcPropValue('opacity', 2) : 1; translate = 'translate3d(' + translateX + 'px, ' + translateY + 'px, 0.01px)'; $(animation.selector).css({ 'transform': translate + scale + rotate, 'opacity': opacity, 'display': display }); } else { $(animation.selector).css({ 'display': 'none' }); } } } calcPropValue = function (property, deciPlaces) { var value = animation[property]; return (tweenEase(relativeScrollTop, value[0], (value[1] - value[0]), keyframesConverted[currentKeyframe].duration, (typeof animation['tween'] === 'undefined') ? null : animation['tween'][1] || null)).toFixed(deciPlaces); }