Bug Tracker

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#13937 closed bug (fixed)

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

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


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.

Change History (6)

comment:1 Changed 10 years ago by david.bau@…

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 empty || optall.queue === false ?
      this.each( doAnimation ) :
      this.queue( optall.queue, doAnimation );

comment:2 Changed 10 years ago by gibson042

Owner: set to david.bau@…
Status: newassigned

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

comment:3 in reply to:  2 Changed 10 years ago by david.bau@…

Replying to 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!!

comment:4 Changed 10 years ago by gibson042

Component: unfiledeffects
Owner: changed from david.bau@… to gibson042
Priority: undecidedlow

comment:5 Changed 10 years ago by Richard Gibson

Resolution: fixed
Status: assignedclosed

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

Changeset: 6fd5e480c1cd3e481e7097763ee281b65bc74306

comment:6 Changed 10 years ago by dmethvin

Milestone: None1.10.1/2.0.2
Note: See TracTickets for help on using tickets.