-
Notifications
You must be signed in to change notification settings - Fork 20.5k
Effects: Reintroduce use of requestAnimationFrame #1578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/effects.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would break chainability, wouldn't it?
|
@dcherman flying blind without the tests, fixed, thank you |
src/effects.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, wouldn't doing this break expectations that the style was applied? If you wanted the size to animate to something smaller for example, it'd be surprising that it wasn't applied if the document was hidden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the point of raf method, in order to preserve battery life and CPU time, memory etc; if you can't see something to do something - it shouldn't happen, besides, this hack doesn't prevent that, raf itself does it, this only an attempt to fix 9381 issue.
See previous discussions about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @dcherman was pointing out that this is becoming a no-op, though you probably intended for this to become a synchronous change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @dcherman was pointing out that this is becoming a no-op
Yes... so concern here is that if animation is triggered when document is hidden, animation would never happen, right?
If so, we could workaround that like we do here - if animation is scheduled when document is hidden, make a note in the jQuery _data object about it, if it scheduled again when document is hidden - do the nop, when (if) document becomes visible again - trigger that first animation on the element(s). But it would require more size for that and i wonder if that really a real world problem, it might be, but i'm not sure - like when some ajax request is finished, when document is hidden, it would not trigger animation to show some box with new info?
Or the concern here that paused animation would not reach it's final state after tabs switching?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes... so concern here is that if animation is triggered when document is hidden, animation would never happen, right?
It's not about whether the animation happens, just whether the end state is reached. If you run elem.animate({ height: 500 }) and the document is hidden, when the document becomes visible again, the element should have a height of 500. The no-op prevents that from happening; instead it should jump to the final state and invoke the callbacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially, you should treat this scenario the same as when $.fx.off is set to true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, there is some problems with github right now, since commit is shown here, but not in this pull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pushed that commit again
Same as before, just use don't use prefixes, since they pretty match useless now and use page visibility API to determine if animation should start. Also null the requestAnimationFrame attribute in window for tests since sinon does not provide fake method for it. Fixes #9381
* Make animation behave as if jQuery.fx.off = true if document is hidden * Use cancelAnimationFrame in jQuery.fx.stop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be window.clearInterval, or should window.cancelAnimationFrame just be cancelAnimationFrame? Just curious why in one place the implicit global is used, and in others the explicit one is used.
Ditto for the setInterval and window.requestAnimationFrame calls above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would want to wait until jshint/jshint@0050e43 commit is released, then remove the window prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just ignore the rule before that via sth like:
if ( hasRaf ) {
/* jshint undef: false */
window.cancelAnimationFrame( timerId );
/* jshint undef: true */
} else {(not tested) instead of using window.cancelAnimationFrame directly. At least the comments will go away after minification.
But maybe the release can happen soon... @rwaldron, can you ask @valueof to release a new patch version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just ignore the rule before that via sth like
sure, or we could add it to globals prop, not that important i think. Still a long road ahead until we hit 2.2/1.12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, will http://bugs.jquery.com/ticket/13494 be relevant here? Wonder if the suite will still run on older Android devices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a "weekly" (it runs 3 times a week) run on Android devices: http://swarm.jquery.org/project/jqueryweekly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If http://caniuse.com/#feat=pagevisibility is correct, the branch that sets document.hidden = true should take care of the unit test, since Android 2.x doesn't support Page Visibility API.
rAF logic was introduced almost three years ago relative to this commit, as a primary method for scheduling animation (see jquerygh-1578 pull). With it there was two substantial changes - one was explicitly mentioned and the other was not. First, if browser window was hidden aka `document.hidden === true` it would immediately execute all scheduled animation without waiting for time pass i.e. tick time become `0` instead of 13 ms of a default value. Which created possibility for circular executions in case if `complete` method executed the same animation (see jquerygh-3434 issue). And the second one - since then there was two ways of scheduling animation: with `setInterval` and `requestAnimationFrame`, but there was a difference in their execution. In case of `setInterval` it waited default `jQuery.fx.interval` value before actually starting the new tick, not counting the first step which wasn't set to be executed through tick method (aka `jQuery.fx.tick`). Whereas `requestAnimationFrame` first scheduled the call and executed the `step` method right after that, counting the first call of `jQuery.fx.timer`, `tick` was happening twice in one frame. But since tests explicitly disabled rAF method i.e. `requestAnimationFrame = null` and checking only `setInterval` logic, since it's impossible to do it otherwise - we missed that change. Faulty logic also was presented with `cancelAnimationFrame`, which couldn't clear any timers since `raf` scheduler didn't define new `timerId` value. Because that change was so subtle, apparently no user noticed it proving that both `cancelAnimationFrame` and `clearInterval` code paths are redundant. Since `cancelAnimationFrame` didn't work properly and rAF is and was a primary used code path, plus the same approach is used in other popular animation libs. Therefore those code paths were removed. These changes also replace two different functions which schedule the animation with one, which checks what type of logic should be used and executes it appropriatley, but for secondary path it now uses `setTimeout` making it more consistent with rAF path. Since ticks are happening globally we also don't require to listen `visibilitychange` event. It also changes the way how first call is scheduled so execution of animation will not happen twice in one frame. No new tests were not introduced, since now `setTimeout` logic should be equivalent to the rAF one, but one test was changed since now we actually execute animation at the first tick. Fixes jquerygh-3434
rAF logic was introduced almost three years ago relative to this commit, as a primary method for scheduling animation (see jquerygh-1578 pull). With it there was two substantial changes - one was explicitly mentioned and the other was not. First, if browser window was hidden aka `document.hidden === true` it would immediately execute all scheduled animation without waiting for time pass i.e. tick time become `0` instead of 13 ms of a default value. Which created possibility for circular executions in case if `complete` method executed the same animation (see jquerygh-3434 issue). And the second one - since then there was two ways of scheduling animation: with `setInterval` and `requestAnimationFrame`, but there was a difference in their execution. In case of `setInterval` it waited default `jQuery.fx.interval` value before actually starting the new tick, not counting the first step which wasn't set to be executed through tick method (aka `jQuery.fx.tick`). Whereas `requestAnimationFrame` first scheduled the call and executed the `step` method right after that, counting the first call of `jQuery.fx.timer`, `tick` was happening twice in one frame. But since tests explicitly disabled rAF method i.e. `requestAnimationFrame = null` and checking only `setInterval` logic, since it's impossible to do it otherwise - we missed that change. Faulty logic also was presented with `cancelAnimationFrame`, which couldn't clear any timers since `raf` scheduler didn't define new `timerId` value. Because that change was so subtle, apparently no user noticed it proving that both `cancelAnimationFrame` and `clearInterval` code paths are redundant. Since `cancelAnimationFrame` didn't work properly and rAF is and was a primary used code path, plus the same approach is used in other popular animation libs. Therefore those code paths were removed. These changes also replace two different functions which schedule the animation with one, which checks what type of logic should be used and executes it appropriatley, but for secondary path it now uses `setTimeout` making it more consistent with rAF path. Since ticks are happening globally we also don't require to listen `visibilitychange` event. It also changes the way how first call is scheduled so execution of animation will not happen twice in one frame. No new tests were not introduced, since now `setTimeout` logic should be equivalent to the rAF one, but one test was changed since now we actually execute animation at the first tick. Fixes jquerygh-3434
rAF logic was introduced almost three years ago relative to this commit, as a primary method for scheduling animation (see jquerygh-1578 pull). With it there was two substantial changes - one was explicitly mentioned and the other was not. First, if browser window was hidden aka `document.hidden === true` it would immediately execute all scheduled animation without waiting for time pass i.e. tick time become `0` instead of 13 ms of a default value. Which created possibility for circular executions in case if `complete` method executed the same animation (see jquerygh-3434 issue). And the second one - since then there was two ways of scheduling animation: with `setInterval` and `requestAnimationFrame`, but there was a difference in their execution. In case of `setInterval` it waited default `jQuery.fx.interval` value before actually starting the new tick, not counting the first step which wasn't set to be executed through tick method (aka `jQuery.fx.tick`). Whereas `requestAnimationFrame` first scheduled the call and executed the `step` method right after that, counting the first call of `jQuery.fx.timer`, `tick` was happening twice in one frame. But since tests explicitly disabled rAF method i.e. `requestAnimationFrame = null` and checking only `setInterval` logic, since it's impossible to do it otherwise - we missed that change. Faulty logic also was presented with `cancelAnimationFrame`, which couldn't clear any timers since `raf` scheduler didn't define new `timerId` value. Because that change was so subtle, apparently no user noticed it proving that both `cancelAnimationFrame` and `clearInterval` code paths are redundant. Since `cancelAnimationFrame` didn't work properly and rAF is and was a primary used code path, plus the same approach is used in other popular animation libs. Therefore those code paths were removed. These changes also replace two different functions which schedule the animation with one, which checks what type of logic should be used and executes it appropriatley, but for secondary path it now uses `setTimeout` making it more consistent with rAF path. Since ticks are happening globally we also don't require to listen `visibilitychange` event. It also changes the way how first call is scheduled so execution of animation will not happen twice in one frame. No new tests were not introduced, since now `setTimeout` logic should be equivalent to the rAF one, but one test was changed since now we actually execute animation at the first tick. Fixes jquerygh-3434
rAF logic was introduced almost three years ago relative to this commit, as a primary method for scheduling animation (see jquerygh-1578 pull). With it there was two substantial changes - one was explicitly mentioned and the other was not. First, if browser window was hidden aka `document.hidden === true` it would immediately execute all scheduled animation without waiting for time pass i.e. tick time become `0` instead of 13 ms of a default value. Which created possibility for circular executions in case if `complete` method executed the same animation (see jquerygh-3434 issue). And the second one - since then there was two ways of scheduling animation: with `setInterval` and `requestAnimationFrame`, but there was a difference in their execution. In case of `setInterval` it waited default `jQuery.fx.interval` value before actually starting the new tick, not counting the first step which wasn't set to be executed through tick method (aka `jQuery.fx.tick`). Whereas `requestAnimationFrame` first scheduled the call and executed the `step` method right after that, counting the first call of `jQuery.fx.timer`, `tick` was happening twice in one frame. But since tests explicitly disabled rAF method i.e. `requestAnimationFrame = null` and checking only `setInterval` logic, since it's impossible to do it otherwise - we missed that change. Faulty logic also was presented with `cancelAnimationFrame`, which couldn't clear any timers since `raf` scheduler didn't define new `timerId` value. Because that change was so subtle, apparently no user noticed it proving that both `cancelAnimationFrame` and `clearInterval` code paths are redundant. Since `cancelAnimationFrame` didn't work properly and rAF is and was a primary used code path, plus the same approach is used in other popular animation libs. Therefore those code paths were removed. These changes also replace two different functions which schedule the animation with one, which checks what type of logic should be used and executes it appropriatley, but for secondary path it now uses `setTimeout` making it more consistent with rAF path. Since ticks are happening globally we also don't require to listen `visibilitychange` event. It also changes the way how first call is scheduled so execution of animation will not happen twice in one frame. No new tests were not introduced, since now `setTimeout` logic should be equivalent to the rAF one, but one test was changed since now we actually execute animation at the first tick. Fixes jquerygh-3434
rAF logic was introduced almost three years ago relative to this commit, as a primary method for scheduling animation (see gh-1578 pull). With it there was two substantial changes - one was explicitly mentioned and the other was not. First, if browser window was hidden aka `document.hidden === true` it would immediately execute all scheduled animation without waiting for time pass i.e. tick time become `0` instead of 13 ms of a default value. Which created possibility for circular executions in case if `complete` method executed the same animation (see gh-3434 issue). And the second one - since then there was two ways of scheduling animation: with `setInterval` and `requestAnimationFrame`, but there was a difference in their execution. In case of `setInterval` it waited default `jQuery.fx.interval` value before actually starting the new tick, not counting the first step which wasn't set to be executed through tick method (aka `jQuery.fx.tick`). Whereas `requestAnimationFrame` first scheduled the call and executed the `step` method right after that, counting the first call of `jQuery.fx.timer`, `tick` was happening twice in one frame. But since tests explicitly disabled rAF method i.e. `requestAnimationFrame = null` and checking only `setInterval` logic, since it's impossible to do it otherwise - we missed that change. Faulty logic also was presented with `cancelAnimationFrame`, which couldn't clear any timers since `raf` scheduler didn't define new `timerId` value. Because that change was so subtle, apparently no user noticed it proving that both `cancelAnimationFrame` and `clearInterval` code paths are redundant. Since `cancelAnimationFrame` didn't work properly and rAF is and was a primary used code path, plus the same approach is used in other popular animation libs. Therefore those code paths were removed. These changes also replace two different functions which schedule the animation with one, which checks what type of logic should be used and executes it appropriatley, but for secondary path it now uses `setTimeout` making it more consistent with rAF path. Since ticks are happening globally we also don't require to listen `visibilitychange` event. It also changes the way how first call is scheduled so execution of animation will not happen twice in one frame. No new tests were not introduced, since now `setTimeout` logic should be equivalent to the rAF one, but one test was changed since now we actually execute animation at the first tick. Fixes gh-3434 Closes gh-3559
Pretty easy, takes advantage of Page Visibility API (PVAPI), provides incompatible change – animations in background tabs get paused, but obviously, in older browsers it would work as it works right now
In environment where PVAPI has not exist but
rafhas it could create issues described in #9381 but such environments live only in the past.It worth to note that Opera 12.16 has not
requestAnimationFramebut has PVAPI – in there, last animation will finish all it's frames but new animations would not be accepted, iOS < 7 hasrequestAnimationFrameand hasn't PVAPI (we could workaround that withpagehide/pageshowevents but i don't think it's worth it), but in thererequestAnimationFrameis prefixed, in iOS 7rafis unprefixed but it also has PVAPI. Android 4.4 version has both API's and none in previous ones.Why do i get a feeling that i'm miss something?