Effects: Resolve issues revealed by recent Callbacks fix

gibson042 · web-flow · commit 3c89329cb24c · 2017-01-16T10:18:49.000-08:00
Notify full progress before resolving empty animations Register animation callbacks before their ticker Remove the right timer when immediately-done animations spawn more Ref 9d822bc Fixes gh-3502 Fixes gh-3503 Closes gh-3496
diff --git a/src/effects.js b/src/effects.js
@@ -315,12 +315,19 @@ function Animation( elem, properties, options ) {
 
 			deferred.notifyWith( elem, [ animation, percent, remaining ] );
 
+			// If there's more to do, yield
 			if ( percent < 1 && length ) {
 				return remaining;
-			} else {
-				deferred.resolveWith( elem, [ animation ] );
-				return false;
 			}
+
+			// If this was an empty animation, synthesize a final progress notification
+			if ( !length ) {
+				deferred.notifyWith( elem, [ animation, 1, 0 ] );
+			}
+
+			// Resolve the animation and report its conclusion
+			deferred.resolveWith( elem, [ animation ] );
+			return false;
 		},
 		animation = deferred.promise( {
 			elem: elem,
@@ -385,6 +392,13 @@ function Animation( elem, properties, options ) {
 		animation.opts.start.call( elem, animation );
 	}
 
+	// Attach callbacks from options
+	animation
+		.progress( animation.opts.progress )
+		.done( animation.opts.done, animation.opts.complete )
+		.fail( animation.opts.fail )
+		.always( animation.opts.always );
+
 	jQuery.fx.timer(
 		jQuery.extend( tick, {
 			elem: elem,
@@ -393,11 +407,7 @@ function Animation( elem, properties, options ) {
 		} )
 	);
 
-	// attach callbacks from options
-	return animation.progress( animation.opts.progress )
-		.done( animation.opts.done, animation.opts.complete )
-		.fail( animation.opts.fail )
-		.always( animation.opts.always );
+	return animation;
 }
 
 jQuery.Animation = jQuery.extend( Animation, {
@@ -641,7 +651,7 @@ jQuery.fx.tick = function() {
 	for ( ; i < timers.length; i++ ) {
 		timer = timers[ i ];
 
-		// Checks the timer has not already been removed
+		// Run the timer and safely remove it when done (allowing for external removal)
 		if ( !timer() && timers[ i ] === timer ) {
 			timers.splice( i--, 1 );
 		}
@@ -654,11 +664,16 @@ jQuery.fx.tick = function() {
 };
 
 jQuery.fx.timer = function( timer ) {
-	jQuery.timers.push( timer );
+	var i = jQuery.timers.push( timer ) - 1,
+		timers = jQuery.timers;
+
 	if ( timer() ) {
 		jQuery.fx.start();
-	} else {
-		jQuery.timers.pop();
+
+	// If the timer finished immediately, safely remove it (allowing for external removal)
+	// Use a superfluous post-decrement for better compressibility w.r.t. jQuery.fx.tick above
+	} else if ( timers[ i ] === timer ) {
+		timers.splice( i--, 1 );
 	}
 };
 
diff --git a/test/unit/effects.js b/test/unit/effects.js
@@ -1256,17 +1256,18 @@ QUnit.test( "animate with CSS shorthand properties", function( assert ) {
 } );
 
 QUnit.test( "hide hidden elements, with animation (bug #7141)", function( assert ) {
-	assert.expect( 3 );
+	assert.expect( 4 );
 
-	var div = jQuery( "<div style='display:none'></div>" ).appendTo( "#qunit-fixture" );
-	assert.equal( div.css( "display" ), "none", "Element is hidden by default" );
-	div.hide( 1, function() {
-		assert.ok( !jQuery._data( div, "olddisplay" ), "olddisplay is undefined after hiding an already-hidden element" );
-		div.show( 1, function() {
-			assert.equal( div.css( "display" ), "block", "Show a double-hidden element" );
+	var div = jQuery( "<div id='bug7141' style='display:none'/>" ).appendTo( "#qunit-fixture" );
+	assert.equal( div.css( "display" ), "none", "Element is initially hidden" );
+	div.hide( 10, function() {
+		assert.equal( div.css( "display" ), "none", "Element is hidden in .hide() callback" );
+		div.show( 11, function() {
+			assert.equal( div.css( "display" ), "block", "Element is visible in .show() callback" );
 		} );
 	} );
-	this.clock.tick( 10 );
+	this.clock.tick( 50 );
+	assert.equal( div.css( "display" ), "block", "Element is visible after animations" );
 } );
 
 QUnit.test( "animate unit-less properties (#4966)", function( assert ) {
