[web-animations-1] Play state of an animation with an unresolved timeline

[kevers-google]

Recently the rules for determining the play state of an animation were updated to add the condition of having an unresolved start time for the animation to be idle. This constraint was added to accommodate non-monotonically-increasing (i.e. scroll) timelines which should not be flagged as idle if they have a start time but an inactive timeline; however, it does break the play state for cases where the timeline is unresolved. Explicitly setting the start time of an animation with an unresolved timeline should leave the animation in an idle state. This issue can be resolved by relaxing the constraint to require that either the start time or the timeline be unresolved.

https://drafts.csswg.org/web-animations-1/#play-states

[kevers-google]

cc @birtles @ogerchikov @majido @flackr

Related to issue: #2066

Changes to how play state is calculated for supporting scroll timelines introduced changes in the behavior of set timeline. These side effects would not have been visible in WPT tests in Blink since setting the timeline was not supported.

Suggesting the following change to play state:

All of the following conditions are true:
The current time of animation is unresolved, and
the start time of animation is unresolved or the timeline is unresolved, and
animation does not have either a pending play task or a pending pause task,

→ idle

If the timeline is explicitly set to null, should the play state be allowed to be in a running state?

With this change, the changes in support of scroll timelines do not introduce inconsistencies between browsers for null timelines.

[flackr]

Just for clarification, by unresolved timeline I assume you mean there is no associated timeline for the animation?

[kevers-google]

Correct

[flackr]

This makes sense to me. Can you make a PR?

We have some testing of setting timelines in web-animations/timing-model/animations/setting-the-timeline-of-an-animation.html and Firefox and Safari are passing these tests - do we have test coverage which was broken by this change and/or will we after we make it?

[kevers-google]

Firefox is passing due to not having the test for startTime being unresolved in the idle check.
https://searchfox.org/mozilla-central/source/dom/animation/Animation.cpp#480

The following test was updated with the algorithm change and currently fails on Firefox and Safari:
wpt/web-animations/timing-model/animations/setting-the-timeline-of-an-animation.html

Once setTimeline is supported in Blink the following tests will have new failure reasons based on the plate state:
wpt/css/css-animations/event-dispatch.tentative.html
wpt/css/css-transitions/event-dispatch.tentative.html

See the tests titled "Active -> Idle, setting Animation.timeline = null"

Gladly make the PR

[birtles]

however, it does break the play state for cases where the timeline is unresolved. Explicitly setting the start time of an animation with an unresolved timeline should leave the animation in an idle state.

I'm not sure I quite follow this part. I think it actually makes sense to me that the animation is finished/running at this point?

Suggesting the following change to play state:

All of the following conditions are true:
The current time of animation is unresolved, and
the start time of animation is unresolved or the timeline is unresolved, and
animation does not have either a pending play task or a pending pause task,

→ idle

Is there precedent for distinguishing between the lack of an associated timeline and an associated but inactive timeline? I thought we always treated those two as the same? (i.e. the lack of an associated timeline can be thought of as being associated with a "null timeline" that is always inactive).

If the timeline is explicitly set to null, should the play state be allowed to be in a running state?

I think so?

[kevers-google]

We have precedence for the play state being restricted to the idle or paused when the timeline is explicitly null. This was captured in the following WPT test:

• wpt/web-animations/timing-model/animations/setting-the-timeline-of-an-animation.html
  • After clearing timeline on finished animation it is idle
  • After clearing timeline on running animation it is idle

The expectations were updated in https://chromium-review.googlesource.com/c/chromium/src/+/2150687; however, the expectations no longer align with the test description. The text associated with these tests could be updated if the new behavior is in fact what we want.

Though one can think of an animation with a null timeline as one that is always inactive, the distinction is that the animation will never tick until the timeline is changed. In the case of scroll timelines, the inactive state is transient.

A benefit of restoring the previous behavior for play state of a null timelines is that we then have consistent behavior across browsers. Mutable timelines is likely to have cross browser support before scroll timelines. Having said that, I'm OK with going either way on this, but would like to see consistency with the test expectations and descriptions.

[kevers-google]

Missed two other relevant tests (again from setting-the-timeline-of-animation.html):
* After setting timeline on an idle animation with a start time it is running
* After setting timeline on an idle animation with a sufficiently ancient start time it is finished

Both of these tests assert that an animation with a null timeline remains idle after the start time is set. Again, the tests could be updated if we are content with the implication of recent spec changes on the play state.

[birtles]

We have precedence for the play state being restricted to the idle or paused when the timeline is explicitly null. This was captured in the following WPT test:

• wpt/web-animations/timing-model/animations/setting-the-timeline-of-an-animation.html
  • After clearing timeline on finished animation it is idle
  • After clearing timeline on running animation it is idle

The expectations were updated in https://chromium-review.googlesource.com/c/chromium/src/+/2150687; however, the expectations no longer align with the test description. The text associated with these tests could be updated if the new behavior is in fact what we want.

I think the tests and descriptions are somewhat irrelevant to determining the specified behavior. They should simply be capturing what the spec text says so what I'm curious about is if the spec ever differentiates between the lack of an associated timeline and an associated but inactive timeline for the purposes of timing calculations. If it doesn't (and as best I recall it doesn't), it seems like we probably shouldn't introduce that distinction here.

Though one can think of an animation with a null timeline as one that is always inactive, the distinction is that the animation will never tick until the timeline is changed. In the case of scroll timelines, the inactive state is transient.

Yes, but I wonder if that should matter? With regards to the determining the play state, it seems like it shouldn't depend on whether the timeline could become active or not?

Having said that, I'm OK with going either way on this, but would like to see consistency with the test expectations and descriptions.

Yes, agreed!

I find these changes very difficult to reason about (which is really my fault for failing to get spec in a better shape before it was shipped) and was very concerned about the original change to the play state algorithm (which added the condition for an unresolved start time). Assuming we stick with that, however, I think we should not add the condition regarding an unresolved timeline because:

• We typically treat the lack of an associated timeline and an inactive timeline as the same for the purpose of timing calculations
• It would be odd to further extend this condition to include inactive timelines because then the play state would suddenly change when the timeline became active and it probably shouldn't do that (except with regards to the finished state)