[csswg-drafts] [web-animations-1][css-animations-2] Bug in handling of auto-rewind state when playing new CSS animations (#7145)

birtles has just created a new issue for https://github.com/w3c/csswg-drafts:

== [web-animations-1][css-animations-2] Bug in handling of auto-rewind state when playing new CSS animations ==
[CSS Animations Level 2](https://drafts.csswg.org/css-animations-2/#animation-play-state) describes the handling of `animation-play-state` as follows:

> If at any time, including when the animation is first generated, the resolved value of [animation-play-state](https://drafts.csswg.org/css-animations-1/#propdef-animation-play-state) corresponding to an animation is newly [running](https://drafts.csswg.org/css-animations-1/#valdef-animation-play-state-running), the implementation must run the procedure to [play an animation](https://drafts.csswg.org/web-animations-1/#play-an-animation) for the given animation with the auto-rewind flag set to false.

That is, whether or not a new CSS animation is created _or_ updated, the play procedure is run with the auto-rewind flag set to **false**.

However, if we follow [that procedure](https://drafts.csswg.org/web-animations-1/#play-an-animation) through for a _new_ CSS animation, it won't work:

> ...
> 
> 5. Perform the steps corresponding to the first matching condition from the following, if any:
>   - If animation’s [effective playback rate](https://drafts.csswg.org/web-animations-1/#effective-playback-rate) > 0, the auto-rewind flag is true and either animation’s:
>     - [current time](https://drafts.csswg.org/web-animations-1/#animation-current-time) is [unresolved](https://drafts.csswg.org/web-animations-1/#unresolved), or
>     - [current time](https://drafts.csswg.org/web-animations-1/#animation-current-time) < zero, or
>     - [current time](https://drafts.csswg.org/web-animations-1/#animation-current-time) ≥ [associated effect end](https://drafts.csswg.org/web-animations-1/#associated-effect-end),
>       - Set seek time to zero.

At this point _auto-rewind_ is false so we skip this step.

>   - If animation’s [effective playback rate](https://drafts.csswg.org/web-animations-1/#effective-playback-rate) < 0, the auto-rewind flag is true and either animation’s:
>     - [current time](https://drafts.csswg.org/web-animations-1/#animation-current-time) is [unresolved](https://drafts.csswg.org/web-animations-1/#unresolved), or
>     - [current time](https://drafts.csswg.org/web-animations-1/#animation-current-time) ≤ zero, or
>     - [current time](https://drafts.csswg.org/web-animations-1/#animation-current-time) > [associated effect end](https://drafts.csswg.org/web-animations-1/#associated-effect-end),
>       - If [associated effect end](https://drafts.csswg.org/web-animations-1/#associated-effect-end) is positive infinity,
>         - [throw](https://heycam.github.io/webidl/#dfn-throw) an "[InvalidStateError](https://webidl.spec.whatwg.org/#invalidstateerror)" [DOMException](https://webidl.spec.whatwg.org/#idl-DOMException) and abort these steps.
>       - Otherwise,
>         - Set seek time to animation’s [associated effect end](https://drafts.csswg.org/web-animations-1/#associated-effect-end).
>   - If animation’s [effective playback rate](https://drafts.csswg.org/web-animations-1/#effective-playback-rate) = 0 and animation’s [current time](https://drafts.csswg.org/web-animations-1/#animation-current-time) is [unresolved](https://drafts.csswg.org/web-animations-1/#unresolved),
>     - Set seek time to zero.

In fact, none of the steps above match so _seek time_ is still unresolved.

> 6. If seek time is [resolved](https://drafts.csswg.org/web-animations-1/#unresolved),
>   - If has finite timeline is true,
>     1. Set animation’s [start time](https://drafts.csswg.org/web-animations-1/#animation-start-time) to seek time.
>     2. Let animation’s [hold time](https://drafts.csswg.org/web-animations-1/#animation-hold-time) be [unresolved](https://drafts.csswg.org/web-animations-1/#unresolved).
>     3. [Apply any pending playback rate](https://drafts.csswg.org/web-animations-1/#apply-any-pending-playback-rate) on animation.
>   - Otherwise,
>     - Set animation’s [hold time](https://drafts.csswg.org/web-animations-1/#animation-hold-time) to seek time.

Again, _seek time_ is unresolved so we don't set the hold time or start time.

> 7. If animation’s [hold time](https://drafts.csswg.org/web-animations-1/#animation-hold-time) is [resolved](https://drafts.csswg.org/web-animations-1/#unresolved), let its [start time](https://drafts.csswg.org/web-animations-1/#animation-start-time) be unresolved.

_Hold time_ is unresolved so we don't touch the _start time_.

> ...
> 
> 9. If the following four conditions are all satisfied:
>     - animation’s [hold time](https://drafts.csswg.org/web-animations-1/#animation-hold-time) is [unresolved](https://drafts.csswg.org/web-animations-1/#unresolved), and
>     - seek time is [unresolved](https://drafts.csswg.org/web-animations-1/#unresolved), and
>     - aborted pause is false, and
>     - animation does not have a [pending playback rate](https://drafts.csswg.org/web-animations-1/#pending-playback-rate),
> abort this procedure.

Since all these conditions hold, we end up aborting the procedure.

### Why do browsers work at all then?

For Blink, it looks like it uses a different code path for a _new_ animation vs updating an _existing_ animation

- For a _new_ animation it uses the [`play` method](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/css/css_animations.cc;l=1089;drc=1dff2af6ff82513b605efc7ee00d16c23051fc3d) which does _not_ set _auto-rewind_ to false.
- For an _existing_ animation it uses the [`Unpause` method](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/animation.cc;l=1425;drc=ca12161acfe9f64e1b0d96a6836f07c51d0546b9) which _does_ set _auto-rewind_ to false.

Hence it doesn't hit this issue. I believe @BorisChiou looked into WebKit and found something similar.

Gecko doesn't quite follow the spec correctly for step 5 above and so far hasn't hit this issue but in trying to fix step 5 we ran into it.

### Possible fixes

I think there are at least two possible approaches to fixing this:

1. Make the procedure to play an animation ignore the _auto-rewind_ flag if the current time is unresolved
2. Make CSS Animations level 2 distinguish between playing a new animation vs unpausing an existing animation

I think the first option is cleaner and more intuitive -- if you try to play an animation without a current time, it should play from the start, regardless of whether or not you set _auto-rewind_ to false since it's not rewinding.

I think that would be equivalent to changing the following text from:

> If animation’s [effective playback rate](https://drafts.csswg.org/web-animations-1/#effective-playback-rate) = 0 and animation’s [current time](https://drafts.csswg.org/web-animations-1/#animation-current-time) is [unresolved](https://drafts.csswg.org/web-animations-1/#unresolved),
> - Set seek time to zero.

to just:

> If animation’s [current time](https://drafts.csswg.org/web-animations-1/#animation-current-time) is [unresolved](https://drafts.csswg.org/web-animations-1/#unresolved),
> - Set seek time to zero.

I wonder what else that might break, though?

Please view or discuss this issue at https://github.com/w3c/csswg-drafts/issues/7145 using your GitHub account


-- 
Sent via github-notify-ml as configured in https://github.com/w3c/github-notify-ml-config

Received on Wednesday, 16 March 2022 09:41:07 UTC