Bug Tracker

Ticket #13937 (closed bug: fixed)

Opened 18 months ago

Last modified 17 months ago

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:
Blocking: Blocked by:

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.

Change History

comment:1 Changed 17 months 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;
        }
      }
      doAnimation.call(this);
    }
    return empty || optall.queue === false ?
      this.each( doAnimation ) :
      this.queue( optall.queue, doAnimation );
  },

comment:2 follow-up: ↓ 3 Changed 17 months ago by gibson042

  • Owner set to david.bau@…
  • Status changed from new to assigned

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 17 months 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 17 months ago by gibson042

  • Owner changed from david.bau@… to gibson042
  • Priority changed from undecided to low
  • Component changed from unfiled to effects

comment:5 Changed 17 months ago by Richard Gibson

  • Status changed from assigned to closed
  • Resolution set to fixed

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

Changeset: 6fd5e480c1cd3e481e7097763ee281b65bc74306

comment:6 Changed 17 months ago by dmethvin

  • Milestone changed from None to 1.10.1/2.0.2
Note: See TracTickets for help on using tickets.