Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Refactor timer initialization, ticket #12837 #1021

Closed
wants to merge 1 commit into from

2 participants

@chadparry

Each step of each animation should not need to check fxNow. Instead, the time can be queried once at the beginning of jQuery.fx.tick. This also solves an iPad bug where an event for clearing fxNow was being dropped, resulting in a never-ending animation.

Note that the new unit test passes both before and after my patch is applied. The only way to exercise the bug is if the user is in Safari and pinches to zoom a colorbox before the animation starts. The test is probably still useful for verifying that synchronized animations work as advertized.

This is a continuation of pull request #1015.

/cc @gibson042 @gnarf37

@gnarf gnarf commented on the diff
src/effects.js
@@ -1,4 +1,4 @@
-var fxNow, timerId,
+var fxNow, fxTickNow, timerId,
@gnarf Owner
gnarf added a note

Why not just share the current fxNow and keep that logic around?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gnarf
Owner

Check out that branch I just sent a PR from @chadparry - let me know if it fixes your issue. Also - that unit test fails against current build but passes in new build.

@chadparry

It wouldn't be the worst thing to consolidate fxNow and fxTickNow. I was worried that when jQuery.fx.tick clobbers the old timestamp, it may break animations that were supposed to be synchronized. Suppose that one animation started, then jQuery.fx.tick was called, then in the same frame another animation was started. The second animation would end up with a different start time from the first. My use of two timestamp variables prevents that confusion.

Although, now I don't see how jQuery.fx.tick could ever be called in the same JS frame as anything else. It is always called from its own setInterval. Is it considered part of the public API, so users could potentially request a tick on demand? I don't see it documented anywhere. If it is considered private, then I can remove fxTickNow and we'll have nothing to worry about.

@gnarf
Owner

@chadparry If someone were to call jQuery.fx.tick() directly, I would think this should start a new "frame" of animation regardless of it it was in the same JavaScript event loop. See gh-1022 :smiley_cat: - I wrote that to show the cleaner test case that actually fails in the old code, but passes in the new.

@gnarf gnarf closed this in 781a5c0
@gnarf gnarf referenced this pull request from a commit
@gnarf gnarf Ensure each tick gets it's own fxNow - Fixes #12837 - Thanks @chadparry
Closes gh-1022
Closes gh-1021
(cherry picked from commit 781a5c0)
c56732f
@chadparry

You're right. Your patch is simpler. Sorry I wasn't able to be more helpful.

Thanks for the quick turnaround!

@mescoda mescoda referenced this pull request from a commit in mescoda/jquery
@gnarf gnarf Ensure each tick gets it's own fxNow - Fixes #12837 - Thanks @chadparry
Closes gh-1022
Closes gh-1021
876076f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 7, 2012
  1. @chadparry

    Refactor timer initialization so the animation makes forward progress…

    chadparry authored
    … even if WebKit drops events, ticket #12837
This page is out of date. Refresh to see the latest.
Showing with 74 additions and 3 deletions.
  1. +4 −3 src/effects.js
  2. +70 −0 test/unit/effects.js
View
7 src/effects.js
@@ -1,4 +1,4 @@
-var fxNow, timerId,
+var fxNow, fxTickNow, timerId,
@gnarf Owner
gnarf added a note

Why not just share the current fxNow and keep that logic around?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
rfxtypes = /^(?:toggle|show|hide)$/,
rfxnum = new RegExp( "^(?:([-+])=|)(" + core_pnum + ")([a-z%]*)$", "i" ),
rrun = /queueHooks$/,
@@ -79,8 +79,7 @@ function Animation( elem, properties, options ) {
delete tick.elem;
}),
tick = function() {
- var currentTime = fxNow || createFxNow(),
- remaining = Math.max( 0, animation.startTime + animation.duration - currentTime ),
+ var remaining = Math.max( 0, animation.startTime + animation.duration - fxTickNow ),
percent = 1 - ( remaining / animation.duration || 0 ),
index = 0,
length = animation.tweens.length;
@@ -612,6 +611,7 @@ jQuery.fx.tick = function() {
timers = jQuery.timers,
i = 0;
+ fxTickNow = jQuery.now();
for ( ; i < timers.length; i++ ) {
timer = timers[ i ];
// Checks the timer has not already been removed
@@ -626,6 +626,7 @@ jQuery.fx.tick = function() {
};
jQuery.fx.timer = function( timer ) {
+ fxTickNow = fxNow || createFxNow();
if ( timer() && jQuery.timers.push( timer ) && !timerId ) {
timerId = setInterval( jQuery.fx.tick, jQuery.fx.interval );
}
View
70 test/unit/effects.js
@@ -1836,4 +1836,74 @@ test( "Animations with 0 duration don't ease (#12273)", 1, function() {
delete jQuery.easing.test;
});
+asyncTest( "Animations started in the same JS frame stay in sync (#12837)", 2, function() {
+ var TIMER_COUNT = 20,
+ tickLog = [],
+ completed = 0,
+ nextFrameIdx = 0,
+ currentFrameIdx,
+ // This handler gets called when each animation completes.
+ onComplete = function() {
+ // Check whether all timers have completed.
+ if (++completed === TIMER_COUNT) {
+ // The animations are all over, so perform all the test's assertions.
+ var referenceTimer = tickLog[0],
+ timerIdx = 1;
+ // Make sure the animations didn't try to complete synchronously in a single tick.
+ ok( referenceTimer.length > 1, "Animations should span multple ticks" );
+ // Compare all other timer logs to the reference timer log.
+ // Break out of the timer loop on the first observed discrepancy.
+ timerLoop:
+ for ( ; timerIdx < TIMER_COUNT; timerIdx++ ) {
+ if ( tickLog[timerIdx].length !== referenceTimer.length ) {
+ strictEqual( tickLog[timerIdx].length, referenceTimer.length, "Timers should synchronize the number of ticks" );
+ break timerLoop;
+ }
+ for ( var stepIdx = 0; stepIdx < referenceTimer.length; stepIdx++ ) {
+ var referenceStep = referenceTimer[stepIdx],
+ compareStep = tickLog[timerIdx][stepIdx];
+ if ( compareStep.value !== referenceStep.value ||
+ compareStep.frameIdx !== referenceStep.frameIdx) {
+ strictEqual( compareStep, referenceStep, "Timers should fire in sync" );
+ break timerLoop;
+ }
+ }
+ }
+ // If the timer loop made it through all the timers, then the comparisons must have succeeded.
+ if (timerIdx === TIMER_COUNT) {
+ ok( true, "All timer ticks fired in sync" );
+ }
+ start();
+ }
+ },
+ // This creates a handler for each step of each animation.
+ createStepHandler = function(timerLog) {
+ return function(now, fx) {
+ // At each step, check whether the frame index was initialized.
+ if (currentFrameIdx === undefined) {
+ // Initialize the frame index to a new value, and also make sure it gets
+ // cleared as soon as possible after this event callback completes. If
+ // everything is working, then every timer should tick during the same frame.
+ setTimeout(function() {
+ currentFrameIdx = undefined;
+ }, 0 );
+ currentFrameIdx = nextFrameIdx++;
+ }
+ // Record how far and when the animation had progressed for this step.
+ timerLog.push({ value: now, frameIdx: currentFrameIdx });
+ };
+ };
+
+ // Initialize lots of different timers.
+ for ( var timerIdx = 0; timerIdx < TIMER_COUNT; timerIdx++ ) {
+ tickLog[timerIdx] = [];
+ // Each timer will animate the property of a different object.
+ jQuery({ test: 0 }).animate({ test: 1 }, {
+ duration: 200,
+ complete: onComplete,
+ step: createStepHandler(tickLog[timerIdx])
+ });
+ }
+});
+
} // if ( jQuery.fx )
Something went wrong with that request. Please try again.