Re: [csswg-drafts] [web-animations-1] Make animations become idle when they have an inactive timeline (#2066)

Sorry for my delay in reviewing this. It's looking good but there are a few parts I'm unsure about.

> 2. https://drafts.csswg.org/scroll-animations-1/#current-time-algorithm - update ScrollTimeline.currentTime calculation as follows:
> 
>     * If current scroll offset is less than startScrollOffset, return (0 - epsilon) or -infinity.
> 
>     * If current scroll offset is greater than or equal to endScrollOffset, return ( effective time range + epsilon) or +infinity.
> 
>     * Consider: do we need explicitly address a case when startScrollOffset equals to endScrollOffset?

I want to avoid returning any epsilon values. That still leaves open the question of what is the current time of a timeline when it is in the new `BeforeStart` or `AfterEnd` states.

My intuition here is simply that it should be `0` or `effective time range` respectively. That can lead to bugs if script fails to check the timeline state, but given that it is the current time that will be used by those animations that are deemed to be active in those states, I think it's the most correct behavior.

> 3. https://drafts.csswg.org/web-animations-1/#animationtimeline -  add timeline state readonly attribute: TimelineState {Inactive, BeforeStart, InRange, AfterEnd}

This table looks good.

> ## Web Animations Spec Changes
> 
> For the purpose of this discussion null timeline is handled the same as inactive timeline.
> 
> 1. https://drafts.csswg.org/web-animations/#responding-to-a-newly-inactive-timeline - drop this section. Instead clarify animation state when responding to inactive timeline as follows:
>     * Start time is preserved.
>     * Hold time is unresolved.
>     * Play state is ‘idle’.
> 
> 2. Add a section “Responding to a newly active timeline”:
>     * If hold_time is resolved, calculate start_time based on the hold_time and timeline current time.

I'm a little unsure about this. It would mean that if we have an animation that is paused, then if the timeline temporarily became inactive, it would become idle. An animation that is running, however, would resume running after the timeline became active again.

By comparison, if we set the timeline to null and then back again, the paused animation would continue being paused. That's because the procedure for setting a timeline only clears the hold time if the start time is set.

(Also, note that we wouldn't specify that "Play state is `idle`" since play state is a calculated field.)

I suspect the procedure for responding to a newly inactive timeline might need to cancel pending play/pause tasks. Or perhaps those tasks would need to be updated since the play pending task asserts that either start time or hold time is set but that will no longer be the case if we unconditionally clear the hold time. I'm not sure though--perhaps we'll never queue the task in the case when the hold time is cleared. But we should probably decide what the desired behavior is for a play-pending animation whose timeline becomes inactive.

> 3. https://drafts.csswg.org/web-animations/#playing-an-animation-section - update to handle inactive timelines as follows:
>     * If the timeline is inactive and both animation start and current time are unresolved, set start time to zero (or timeline.getInitialCurrentTime())

What is the reason for the "... and both animation start and current time are unresolved" requirement? 

> 4. https://drafts.csswg.org/web-animations/#pausing-an-animation-section - update to handle inactive timelines. Differentiate between states:
> 
>     * Start and hold time are unresolved, play state is ‘idle’ (animation.play was not called).
>     * Start time is resolved, hold time is unresolved, play state is ‘idle’ (play was called).
>     * Start and hold time are resolved, play stay is ‘paused’ (play was called, timeline became inactive, animation.currentTime was set)

I'm afraid I don't quite follow what the proposed change here is.

> 6. https://drafts.csswg.org/web-animations/#play-states
>     1. Include the following condition into the “paused” state definition:
>        * If both start and hold time are resolved and timeline is inactive (or null).
>     2. Include the following condition into the “idle” state definition:
>        * The current time of animation is unresolved, the timeline is inactive and has pending play state.

I'm not sure I follow the first point, since it seems like if the timeline becomes inactive we will clear the hold time so we would no longer be paused in that case anyway.

But it's the second point I'm more concerned about. Typically if we have a pending task, the play state tells you what state the animation _will_ be in when it finishes pending. However, this change would make that no longer true. This relates to the previous question about what should happen if the timeline of a play-pending animation becomes inactive.

> ## Use Cases
> 
> 1. An animation that is idle until its timeline starts to play
>    ```js
>    scroller.style.overflow="none";
>    scroll_timeline = new ScrollTimeline({scrollSource: scroller, ...});
>    animation = new Animation(new KeyframeEffect(...}], { duration: 2000}), scroll_timeline);
>    animation.play();
>    console.log(animation.startTime); // 0
>    console.log(animation.currentTime); // null
>    console.log(animation.playState); // idle
>    ```

I think it would actually be preferable if the `playState` were `'running'` here--assuming that the animation will be running once `scroller` becomes available.

The other use cases look good but I didn't go into them in details since my comments above might affect the behavior there.

Thank you for doing all this work. This is definitely getting closer.

Unfortunately, there are a lot of states to consider here. Ultimately we should really draw up a complete state chart of all the possible states and inputs (including the odd states like when a pending-pause is interrupted by a `play()` etc.) and reason from that, but that's quite a bit of work.

-- 
GitHub Notification of comment by birtles
Please view or discuss this issue at https://github.com/w3c/csswg-drafts/issues/2066#issuecomment-576967673 using your GitHub account

Received on Wednesday, 22 January 2020 01:35:41 UTC