Skip to content
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

[web-animations-1] Setting the target effect to null on a play-pending animation should probably not make it paused #2077

Closed
birtles opened this issue Dec 5, 2017 · 8 comments

Comments

Projects
None yet
1 participant
@birtles
Copy link
Contributor

commented Dec 5, 2017

From @birtles on November 20, 2017 7:7

const anim = div.animate({ marginLeft: [ '0px', '100px' ] }, 1000);
// Currently play-pending. Now, drop the effect...
anim.effect = null;
console.log(anim.playState);
// "paused"?!?

That's because in the procedure to update the target effect we have this step:

If new effect is null and old effect is not null, run the procedure to reset an animation’s pending tasks on animation.

So, whereas we would previously have had a pending task and reported either "pending" (prior to recent spec changes) or "running" we now find ourselves without a pending task, with a current time, and without a start time, i.e. paused.

Intuitively though if we drop the effect we should either remain pending (i.e. "running") or go to the "finished" state -- probably depending on what happens when we subsequently attach a valid effect.

Copied from original issue: w3c/web-animations#207

@birtles

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

See also #2048 which seems related.

@birtles

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2018

We should probably make the procedure to update the target effect clear the current time in this case.

@birtles

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2018

Considering a few cases:

a) Animation is running, set target effect to something non-null:
→ Animation continues running or finishes, depending on length of target effect.

b) Animation is running, set target effect to null:
→ Animation becomes finished.

However, importantly in the above cases, even if we finish, if we were to subsequently set the target effect to something long enough, we'd resume playback. That is, we can recover from this change.

c) Animation is play-pending, set target effect to something non-null:
→ Animation remains play-pending, will play once ready.

d) Animation is play-pending, set target effect to null:
→ The question for this issue.

I have no idea why I suggested clearing the current time (presumably I meant to say hold time). That would make this idle which doesn't match the behavior for any of the other cases.

What if we were to calculate the start time from the hold time (e.g. start time = timeline time - hold time / playback rate though possibly with some complexity to handle pending playback rates) such that we end up being finished?

Making it become finished is consistent with (a) and (b). If we later set the target effect to something non-null I think we'd end up playing it--i.e. we can recover from the situation. We'd have effectively skipped the asynchronous playback however. (We could make the behavior when setting a non-null target effect on an animation with a null target effect trigger the async play procedure but generally we try to make setting properties not produce async behavior.)

The other option that comes to mind is to simply let the animation remain play-pending until a new target effect is associated with it.

I need to check what happens for pause-pending too.

@birtles

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2018

On further thought, maybe we should just not cancel pending tasks and let the animation start playing normally (i.e. let it progress to finish)? If you play an animation without a null target effect that's what would happen anyway.

@birtles

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2018

I started experimenting with just not canceling pending tasks in Firefox for this case and so far it seems to work well. (Code/test changes)

@birtles

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

Discussed this yesterday with @graouts, @stephenmcgruer, and @flackr and decided this was probably ok (i.e. dropping the cancelation step), provided that the promises resolve as expected.

Given the following test:

const anim = div.animate({ marginLeft: [ '0px', '100px' ] }, 1000);

console.group('Initial state');
console.log(`playState: ${anim.playState}`);
console.log(`pending: ${anim.pending}`);
console.groupEnd();

anim.ready.then(() => {
  console.log('Ready promise resolved');
}).catch(() => {
  console.log('Ready promise rejected');
});
anim.finished.then(() => {
  console.log('Finished promise resolved');
}).catch(() => {
  console.log('Finished promise rejected');
});

console.group('After setting effect to null');
anim.effect = null;
console.log(`playState: ${anim.playState}`);
console.log(`pending: ${anim.pending}`);
console.groupEnd();

requestAnimationFrame(() => {
  console.log('Next frame');
});

(Codepen)

Current Firefox gives:

+ Initial state
  playState: running
  pending: true
+ After setting effect to null
  playState: paused
  pending: false
Ready promise rejected
Next frame

With these changes we get:

+ Initial state
  playState: running
  pending: true
+ After setting effect to null
  playState: finished
  pending: true
Finished promise resolved
Ready promise resolved
Next frame

The reason for the finished promise being resolved first is that when we update the effect, the spec says to:

Run the procedure to update an animation’s finished state for animation with the did seek flag set to false, and the synchronously notify flag set to false.

which will bring us to:

if synchronously notify is false, queue a microtask to run finish notification steps for animation

So we'll end up with a microtask on the queue to resolve the finish promise that will be processed before the next frame where we resolve the ready promise.

That's a little unexpected perhaps, but I think it kind of makes sense too (and trying to fix it would probably only complicate the model further).

The other question we had was whether this arrangement would allow you to recover from the finished-pending state and go back to play-pending.

For example, if you do:

const anim = div.animate({ marginLeft: [ '0px', '100px' ] }, 1000);
anim.effect = null;
anim.effect = new KeyframeEffect(...);

Are you play-pending at the end?

The answer is yes. Adding the following to the above test:

console.group('After setting effect to something else');
anim.effect = new KeyframeEffect(div, { marginLeft: [ '0px', '100px' ] }, 1000);
console.log(`playState: ${anim.playState}`);
console.log(`pending: ${anim.pending}`);
console.groupEnd();

I get:

+ Initial state
  playState: running
  pending: true
+ After setting effect to null
  playState: finished
  pending: true
+ After setting effect to something else
  playState: running
  pending: true
Ready promise resolved
Next frame
(1s later)
Finished promise resolved

Which seems like the behavior we want.

Unless anyone has any objections, I'm going to go ahead and make this change.

@birtles birtles closed this in 2e56cd7 Jul 26, 2018

@birtles

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

Tests will be updated/added upstream in Mozilla bug 1478266.

@birtles

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

kisg pushed a commit to paul99/webkit-mips that referenced this issue Nov 6, 2018

graouts@webkit.org
[Web Animations] Don't reset pending tasks when setting a null effect
https://bugs.webkit.org/show_bug.cgi?id=191301
<rdar://problem/45838422>

Reviewed by Dean Jackson.

The issue w3c/csswg-drafts#2077 has changed the Web Animations API such that
we no longer reset pending tasks when setting a null effect on an animation.

* animation/WebAnimation.cpp:
(WebCore::WebAnimation::setEffect):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@237856 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.