Skip to content

Conversation

@chadparry
Copy link

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@gnarf
Copy link
Member

gnarf commented Nov 7, 2012

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
Copy link
Author

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
Copy link
Member

gnarf commented Nov 8, 2012

@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 😺 - 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 Nov 8, 2012
gnarf added a commit that referenced this pull request Nov 8, 2012
@chadparry
Copy link
Author

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

Thanks for the quick turnaround!

mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants