Opened 12 years ago

Closed 12 years ago

#4083 closed defect (fixed)

dojo.fx.chain/combine destructive

Reported by: dante Owned by: Eugene Lazutkin
Priority: high Milestone: 1.1
Component: fx Version: 0.9
Keywords: Cc: Bryan Forbes, alex, tk
Blocked By: Blocking:

Description

dojo.fx.combine mangles passed _Animations:

var anim1 = new dojo._Animation(); ... var anim4 = dojo.fx.combine([anim1,anim2,anim3]); anim1.play(); === anim4.play(); === anim2.play() etc.

the code looks like it only clobbers the first animation, but affects all.

if fx.combine would return a new _Animation of the combined effects, the issue would be resolved?

dojo.fx.chain should probably return a new _Animation as well:

var anim4 = dojo.fx.chain([anim1,anim2,anim3]); anim1.play(); === anim4.play()

this does seem to be entirely intuitive behavior. (seems like anim1 should still be anim1, and anim4 should be the resulting animation of the chain, or even more simple: a 'null' animation that connects to onEnd of each, and plays each other animation in order)

Attachments (1)

chain_combine_fix.diff (4.9 KB) - added by Bryan Forbes 12 years ago.
Fix for chain and combine bugs.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 12 years ago by bill

Owner: changed from alex to Eugene Lazutkin

This should go to Eugene I think. Reassigning...

comment:2 Changed 12 years ago by dante

Cc: Adam Peller removed

as a testcase, you can use dojox/fx/tests/test_easing.html ... there is a commented out test in the addOnLoad code (in rev > [10073])

comment:3 Changed 12 years ago by Adam Peller

Cc: Bryan Forbes alex tk added

Kind of a dup of #3477, though I think this might be a bit clearer. There are related issues, such as the way the onEnd event is fired -- not at all what I expected.

comment:4 Changed 12 years ago by Eugene Lazutkin

Milestone: 1.01.1

comment:5 Changed 12 years ago by Bryan Forbes

I added two tests to the fx tests in [12085]. These fail because of how combine and chain are currently implemented.

Changed 12 years ago by Bryan Forbes

Attachment: chain_combine_fix.diff added

Fix for chain and combine bugs.

comment:6 Changed 12 years ago by Bryan Forbes

I added a patch to fix the chain and combine bugs. The only way I could think to fix these was to create a duck-typed Animation object and return that. The reason being that there's no good way to make either chain or combine work with just the animations given to the functions.

With chain, we're running into the problem that the returned animation is the first animation. This works fine for playing, but not for connecting to the onEnd handler. It also doesn't handle pausing, stopping, etc. the chained animation.

With combine, we were using a "placeholder" animation that doesn't keep track of if the animations it is combining have ended, it just gives a rough guess (and a poor one as you can see by the test I uploaded in [12085]). It may only be milliseconds off, but when you rely on that calculation to do something like destroy a dialog that you're doing an animation on it's critical so you don't get "node doesn't exist" errors. The other problem that combine had was animations with differing lengths. It set up the "placeholder" animation with the duration of the first animation, but if the second animation's duration was longer it would fire "onEnd" before the combine was truly done.

comment:7 Changed 12 years ago by Eugene Lazutkin

Resolution: fixed
Status: newclosed

(In [12098]) dojo.fx: new versions of dojo.fx.combine() and dojo.fx.chain() based on Bryan Forbes' patch. Minor cleanup, new fx test. Fixes #4083. Thank you, Bryan!

Note: See TracTickets for help on using tickets.