Skip to content

Conversation

@markelog
Copy link
Member

@markelog markelog commented May 9, 2014

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 raf has 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 requestAnimationFrame but has PVAPI – in there, last animation will finish all it's frames but new animations would not be accepted, iOS < 7 has requestAnimationFrame and hasn't PVAPI (we could workaround that with pagehide/pageshow events but i don't think it's worth it), but in there requestAnimationFrame is prefixed, in iOS 7 raf is 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?

src/effects.js Outdated
Copy link
Contributor

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?

@markelog
Copy link
Member Author

markelog commented May 9, 2014

@dcherman flying blind without the tests, fixed, thank you

src/effects.js Outdated
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

cool, done

Copy link
Member Author

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

Copy link
Member Author

Choose a reason for hiding this comment

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

pushed that commit again

@dmethvin dmethvin added this to the 1.12/2.2 milestone May 28, 2014
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
@markelog
Copy link
Member Author

@dmethvin, @dcherman done

* Make animation behave as if jQuery.fx.off = true if document is hidden

* Use cancelAnimationFrame in jQuery.fx.stop
Copy link
Contributor

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

Copy link
Member Author

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

Copy link
Member

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?

Copy link
Member Author

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.

@markelog markelog closed this in 708764f Jun 15, 2014
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

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.

@markelog markelog mentioned this pull request Nov 16, 2015
markelog added a commit to markelog/jquery that referenced this pull request Mar 6, 2017
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
markelog added a commit to markelog/jquery that referenced this pull request Mar 6, 2017
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
markelog added a commit to markelog/jquery that referenced this pull request Mar 6, 2017
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
markelog added a commit to markelog/jquery that referenced this pull request Mar 6, 2017
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
markelog added a commit that referenced this pull request Mar 6, 2017
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
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Development

Successfully merging this pull request may close these issues.

5 participants