Skip to content

Callback doesn't fire when doing simultaneous animations (SC1.9) #861

Closed
artgon opened this Issue Oct 30, 2012 · 6 comments

3 participants

@artgon
artgon commented Oct 30, 2012

It's hard to describe the exact problem, but it's something along these lines:

var callback = function() {...}
var self = this;
this.animate('scale', 0.2, {
    duration: 0.2,
    callback: function() {
        callback.call(self)
    }
});
this.animate('opacity', 0, 0.2);

The problem I was having is the callback in the first animation wasn't firing.

Workarounds:

  • When I commented out the second animation, the callback worked fine in Chrome (not FF)
  • When I put the callback in an "invokeLater" block that fired after the animation, everything worked more or less the same (in Chrome/FF/IE)

Cross Browser:

  • FF/Chrome experienced this issue
  • IE9 did not experience this issue
@publickeating
SproutCore member

Hi, any change based off of very latest master? Otherwise, I'll try to reproduce and look at this more closely soon. Thanks.

@dcporter dcporter closed this Nov 26, 2012
@dcporter dcporter reopened this Nov 26, 2012
@dcporter
SproutCore member

(Good morning, wrong button.)

With a few minutes to kill, I ran the following code from the console on read.kobobooks.com (not the latest master but should be fairly up to date):

self = SC.View.views.sc1189; // a label view

function abc() {var callback = function() {console.log('hi')}
  self.animate('scale', 0.2, {
      duration: 0.2,
      callback: function() {
          callback.call(self)
      }
  });
  self.animate('opacity', 0, 0.2);
};

SC.run(function() { abc() });

The label successfully faded down and out, and the callback successfully printed "hi" to the console.

@publickeating
SproutCore member

The callback fails to fire, because it should be an extra argument, not a property of the options hash. I was going to write that I don't know why that would have ever worked, but I just realized that the options hash gets passed through as is and if the callback as an argument just got added to it.

In any case, that's not the best behaviour and SC.View:animate() has been updated with the proper documentation and now accepts the callback function as an argument or a target and method as arguments (which is standard SproutCore). I'll add a warning if callback is found on the options and provide backwards compatibility for 1.10 (maybe need to patch 1.9?)

@dcporter
SproutCore member
dcporter commented Jan 3, 2013

Why in the world would we prevent passing call back in the object?

@publickeating publickeating added a commit that closed this issue Jan 3, 2013
@publickeating publickeating Provides backwards compatibility for providing the callback inside of…
… the options object and adds warnings that only the standard arguments should be used. Fixes #861 for 1.10.
600ad3a
@publickeating
SproutCore member

This is the current jsdocs for animate on master:

@param {object|string} properties Hash of property names with new layout values or a single property name.
@param {number} [value] The new layout value for a single property (only provide if the first parameter is a string).
@param {object} options Hash of transition options.
@param {number} options.duration The duration of the transition in seconds.
@param {string|array} options.timing The transition timing function.  This may be a predefined CSS timing
  function (e.g. 'linear', 'ease', 'ease-in', 'ease-out', 'ease-in-out') or
  it may be an array of values to make a cubic bezier (e.g. [0, 0, 0.58, 1.0]).
@param {number} options.delay The transition delay in seconds.
@param {object} [target=this] The target for the method.
@param {animateCallback|string} [method] The method to run when the transition completes.  May be a function or a property path.

The amount of argument overloading is already high, which results in a bunch of normalizing code required to massage everything into the actual variables and doesn't help the developer one bit. Extra options that accomplish the same thing just means extra work to remember the proper options, extra chances to get the proper options wrong and extra time spent trying to debug other people's code if they happen to use different options.

As well, the options hash should be made of primitives that are to be applied to the animation. The callback function does not alter the animation and doesn't qualify as an animation "option".

Finally, target + method as arguments is already a paradigm in SproutCore and should be the case here.

@publickeating publickeating added a commit that referenced this issue Jan 3, 2013
@publickeating publickeating Provides backwards compatibility for providing the callback inside of…
… the options object and adds warnings that only the standard arguments should be used. Fixes #861 for 1.9.2.
fa2cb3b
@dcporter
SproutCore member
dcporter commented Jan 3, 2013

Had a hunch as soon as I sent the question that the answer was going to involve keeping non-animation options out of the animation options object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.