finish() only finishes last item of a set being .animate()d.

Reported by:	david.bau@gmail.com	Owned by:	gibson042
Priority:	low	Milestone:	1.10.1/2.0.2
Component:	effects	Version:	1.9.1
Keywords:		Cc:
Blocked by:		Blocking:

Description

When calling finish() after a set of items is animated, only the last item in the set is correctly finished; the rest are just frozen without finishing.

Another symptom of the same bug: if calling finish() to finish a single selected item of a set of items being animated, the selected item is frozen, and the last in the original animating set is finished instead.

Minimal repro:

http://jsfiddle.net/dUGVJ/

Notice: although number 2 gets .finish() called, instead 3 actually finishes and 2 is frozen and unfinished.

The bug is on line 491 of effects.js:

doAnimation.finish = function() {
    anim.stop( true );
  };

The instance of the doAnimation closure is shared across all items being animated, so modifying doAnimation.finish changes shared state, and the last one wins: calling finish() on an animation in progress ends up calling anim.stop for the last anim instance only.

Attachments (0)

Change History (6)

Changed May 25, 2013 10:17AM UTC by david.bau@gmail.com comment:1

Here is a possible way to change of animate() to fix this issue. The idea is to track the anims in a doAnimation.active array, and make finish() find the right matching anim to stop.

  animate: function( prop, speed, easing, callback ) {
    var empty = jQuery.isEmptyObject( prop ),
      optall = jQuery.speed( speed, easing, callback ),
      doAnimation = function() {
        // Operate on a copy of prop so per-property easing won't be lost
        var anim = Animation( this, jQuery.extend( {}, prop ), optall );
        // Empty animations, or finishing resolves immediately
        if ( empty || jQuery._data( this, "finish" ) ) {
          anim.stop( true );
        } else {
          // Track active animations so they can be finished
          doAnimation.active.push( anim );
        }
      };
    doAnimation.active = [];
    doAnimation.finish = function() {
      // Stop the animation for this element if active.
      for ( var index = 0; index < doAnimation.active.length; ++index ) {
        if ( doAnimation.active[ index ].elem === this) {
          doAnimation.active.splice( index, 1 )[ 0 ].stop( true );
          return;
        }
      }
      doAnimation.call(this);
    }
    return empty || optall.queue === false ?
      this.each( doAnimation ) :
      this.queue( optall.queue, doAnimation );
  },

Changed May 25, 2013 12:59PM UTC by gibson042 comment:2

owner:	→ david.bau@gmail.com
status:	new → assigned

Confirmed. Would you like to file a pull request at http://github.com/jquery/jquery and get the contributor credit?

Changed May 25, 2013 04:05PM UTC by david.bau@gmail.com comment:3

Replying to [comment:2 gibson042]:

Would you like to file a pull request at http://github.com/jquery/jquery and get the contributor credit?

I'd prefer that somebody on the jquery team make the fix. Thanks!!

Changed May 26, 2013 02:11PM UTC by gibson042 comment:4

component:	unfiled → effects
owner:	david.bau@gmail.com → gibson042
priority:	undecided → low

https://github.com/jquery/jquery/pull/1279

Changed May 28, 2013 08:51PM UTC by Richard Gibson comment:5

resolution:	→ fixed
status:	assigned → closed

Fix #13937: Correctly scope .finish() following multi-element .animate(). Thanks @gnarf37. Close gh-1279.

(cherry picked from commit ae9e05e9f3cb071232b056005755acb5926e403e)

Changeset: 6fd5e480c1cd3e481e7097763ee281b65bc74306

Changed May 30, 2013 06:23PM UTC by dmethvin comment:6

milestone:	None → 1.10.1/2.0.2

Side navigation

#13937 closed bug (fixed)