Tests? Also, have you signed the CLA?

I actually have no idea how to reproduce this bug in any environment
other than WebKit version 536.26 on the iPad. I don't know how to write
that into a good unit test. Possibly a test could be written than
explicitly clears the timer, but that would be really artificial. I'm
afraid your team will have to figure out how best to test this.
Hopefully you have a WebKit expert that can take this to the next level.

I hadn't heard of the CLA before now, but I'm happy to guarantee for
you that my patch doesn't have any encumbrances. I'll talk to my
company's software director and get an official copy of the release. It
should be signed within a few days.

OK, the CLA is submitted.

I kinda feel like doing fxNow = 0; as the first line of jQuery.fx.tick() would be a better and more reliable solution to ensure that fxNow is cleared at least once per timers loop.

One possible approach to a unit test for this issue would be to run approx 100 animations from 0-100 over 100ms. Ensure that all the values match on all 100 elements (to make sure fxNow did it's job). Assuming that at least 1ms happens (which we could actually force/wait in a blocking tight loop) the next call to jQuery.fx.tick() should create a new value for the animation, and all 100 should be the same.

@gibson042: If I understand your suggestion, then it would not fix this bug. The point is that fxNow is only initialized to a timestamp once. Then subsequently the test fxNow || createFxNow() makes sure that we use the existing value if we can, and only pay the cost of calling jQuery.now() if necessary. Moving that optimization into createFxNow() would be fine with me, and it would shorten the code a miniscule amount, and probably even make it more readable. But it wouldn't affect the clearFxNow issue that is at the heart of this bug.

@gnarf37: The fxNow = 0; suggestion would certainly work. (Actually, fxNow = undefined; would be more consistent with the rest of the code). That would mean that jQuery.now() would get called for every invocation of tick(). There are two things that you should check into first, if you want to go that route.

First, if calling jQuery.now() every time is cheaper and safer than using the clearFxNow technique, then why does this file use the clearFxNow optimization at all? If you are going to avoid clearFxNow in tick, then for consistency you should probably remove it from everywhere in the file.

Second, there is a comment on createFxNow that says:

Animations created synchronously will run synchronously

I believe that means that some animations depend on this optimization to work synchronously. I haven't seen which ones depend on this behavior, or why they would. This comment just makes me think that there is a reason things were done this way.

@gnarf37: The unit test you describe would pass, whether or not my patch were committed. This bug is only exercised in a Safari browser on iPad. I don't think there's much value in a unit test that passes even when the bug still manifests on a browser. The unit test would have to simulate a broken WebKit somehow to be worth using.

@chadparry I think you misunderstood. I was suggesting first to do nothing unless this can be reliably reproduced, and second—if it is found to be useful—to replace all fxNow || createFxNow() with createFxNow() and move the conditional into createFxNow as return ( fxNow = fxNow || jQuery.now() ) (or similar two-statement version). It should be functionally equivalent to your clearFxNow, but more concise.

The whole point of the opti was to skip the call to createFxNow() in potentially tight loops.

The whole reason this was added in the first place - Imagine 100 elements being animated, they all have to have the same startTime if the start in the same JS event frame. They all have to use the same "current time" as well for each tick. Setting the value to undefined at the start of tick I think is probably the most direct solution, It might also allow us to just remove the setTimeout to clearFxNow

Which is why I'm loathe to do anything here... but it seems silly to do fxNow = fxNow || createFxNow(); setTimeout( clearFxNow, 0 ); (always calls setTimeout) instead of fxNow = createFxNow(); (always calls createFxNow, which always calls setTimeout) because the hit from that additional call is so minor if we're already doing a setTimeout every time.

But I think you're right that it'd be better to skip the the setTimeout altogether and handle fxNow at a higher level.

@gibson042: This bug is reproducible 100% of the time on an iPad, so that makes it easier to investigate.

Are you suggesting that you would change createFxNow to look like this?

function createFxNow() {
    setTimeout(function() {
        fxNow = undefined;
    }, 0 );
    return ( fxNow = fxNow || jQuery.now() );
}

In that case, lots more clearFxNow events would get produced compared to before. There would be one event fired for every time that any animation read the fxNow value. Originally, there was only one event for every time that fxNow changed. And with my fix, there was only one event every time fxNow changed plus one event every time tick was called.

If that's not exactly how you want to change createFxNow, then please post the code you would use, because it's likely that I did misunderstand you.

@gnarf37: I think that you have a great idea. The fxNow timestamp could be cleared at the start of tick, and the clearFxNow timer could be removed entirely. That would only work if every animation called tick, and didn't try to access fxNow from outside a tick. With a change like that, the code would be more readable in my opinion. It would also be slightly more performant. And also, it would work on the iPad without any WebKit-specific workarounds.

In master right now, every tick uses fxNow if defined, otherwise creates it with createFxNow (setTimeout to clear ASAP, plus assign and return).

Your change adds the setTimeout previously restricted to createFxNow to every tick.

But if tick is always doing a setTimeout anyway, there's little benefit to it ever skipping the call to createFxNow (which is already doing the same setTimeout), so you might as well drop it and just always createFxNow, which then needs to respect a preexisting value (replacing return ( fxNow = jQuery.now() ) with return ( fxNow = fxNow || jQuery.now() )). And since the only other potential call to createFxNow is also preceded by fxNow || (and not in a time-critical path), it's worth looking into rolling that up too (with due dilligence paid to the setTimeout).

OR

Modify this request to implement something like @gnarf37's suggestion, managing fxNow in jQuery.fx.tick.

@gibson042: IIUC, you are suggesting some changes that would make the animation function slightly slower. Before, animation would use the existing fxNow if it could and not send another clearFxNow event. After your suggested changes, it would always send another clearFxNow event, even if fxNow was already initialized. That's what I have been trying to say. Isn't the code snippet in my last comment exactly what you are suggesting? And do you think that change would be desirable?

Anyway, I think @gnarf37 has the best suggestion.

@gnarf37: I tried to start implementing an enhanced fix. But the code is not very amenable to this idea. The tick method actually gets called once per tick per animation. I was hoping that with two concurrently executing animations, tick would only be called once. From reading your comments, that's what I expected, anyway.

Here is one of the requirements you mentioned:

The whole reason this was added in the first place - Imagine 100 elements being animated, they all have to have the same startTime if the start in the same JS event frame. They all have to use the same "current time" as well for each tick.

There are two problems with this requirement. First, if currentTime were reset at the beginning of each tick, then each animation would not share the same timestamp for every frame. However, they would share the same startTime, so they would stay reasonably in sync with each other. Maybe they would even appear better synchronized, because animation events that fire later than others would have progressed further, rather than using a stale fxNow just because they belong to the same animation frame. The second thing about that requirement is that the existing code does not enforce it. Each frame of each animation was already getting fired in a separate event, so fxNow was already possibly being cleared between them. If all setTimeout events were handled in order by the JavaScript engine, then the animations would stay in sync, but JavaScript does not guarantee any execution order for asynchronous events, (cf. http://stackoverflow.com/a/5998338/1342954). In sum, if we clear fxNow at the beginning of each tick, then the code will be slightly easier to read, slightly faster, and will have the same behavior guarantees as the original.

I've updated the pull request with the new code. Instead of assigning fxNow = undefined, it is more efficient (and safer) for tick to skip fxNow references all together. The new version just assigns currentTime = jQuery.now() directly. The animation function continues to use the createFxNow optimization, because that will ensure that the startTime is preserved across animations in the same frame.

I'd like to hear some feedback on this approach before I pursue it further.

There are two tick() funcitons - the one you are editing, and the one on jQuery.fx you want the one on jQuery.fx.tick() not the per Animation scoped tick()

@chadparry https://github.com/jquery/jquery/blob/9346c0ef9938b44f3711d588ebdb6800bc077d8f/src/effects.js#L610-626 -- This is the tick() that @gibson042 and I have been talking about

It might even make sense to set fxNow = jQuery.now() at the start of tick() and put fxNow = undefined at the end of it

http://bugs.jquery.com/ticket/7917 is the original ticket that landed fxNow - https://github.com/gnarf37/jquery/blob/animate_fix/test/animate.html was a little test case I was using when writing it - the jQuery.fx.sync flag is gone tho

It might even make sense to set fxNow = jQuery.now() at the start of tick() and put fxNow = undefined at the end of it

+1

The current createFxNow approach should stil get used though for start time on animation I think. We will stil need the timeout to clear it in the case of a 0 duration animation being queued it would "lock" the fxNow which would cause an animation queued later to get a bad start time. We still need the clearFxNow in the case of queuing, but the tick() can just clear itself.

Yes, that makes sense for matching start times associated with the same synchronous window.

Agreed. I'll take another shot at it.

Start up on a new branch, new pull - Please CC @gibson042 and me when you open a new pull.