Re: [csswg-drafts] [web-animations-1] Add hold phase to animation #4325 (#5479)

Thanks for looking at it Brian! I'm finally getting the hang of formatting :)

I will try to explain the thoughts behind hold_phase while also providing a potential alternative.

First some setup:
Similarly to how an effect is given a current time from the animation, the effect also needs to be given a phase so that it can correctly calculate it's own phase (e.g. if provided phase is active, calculate phase, otherwise use the given phase).

To accomplish this, we have a function that returns the current phase, similar to existing function that returns current time (https://drafts.csswg.org/web-animations/#the-current-time-of-an-animation). 

Here is where we can have 2 different implementations of CurrentPhase():

**Option 1**: Use hold_phase to maintain state after pausing or when timeline goes inactive. This mirrors hold_time in that it holds data from the associated timeline. They should both always be updated or cleared together. Then we can do this:

`return hold_phase ? hold_phase : timeline.phase`

**Option 2**: If we decide adding hold_phase is too much overhead, we could have a simpler, although not as correct, implementation:

`return hold_time ? kActive : timeline.phase`

This will work as expected as long as the timeline is active and playing, but starts to misbehave when paused or the timeline is inactive or any other situation that causes hold_time to be in use.

Take for example a timeline with `scroll_offset: 20%` and `current_offset: 0`. The effect is `opacity: [0.3, 0.7]`.

Now pause the animation.
 
For Option 1: When the animation is paused, hold_phase is set to `kBefore` which is then provided to the effect. Which should result in the effect not being applied since it doesn't have `fill: backwards`. This is correct behavior. 

For Option 2: When the animation is paused, hold_time is set to 0. This causes `kActive` to be provided to the effect. Which results in the effect calculating its own phase as `Active` since it has no concept of scroll offset and the first keyframe of the effect (opacity: 0.3) is applied. This is incorrect behavior.

I suggest we add hold_phase for correctness and because it updates at the same time as hold_time, I think maintainability will be manageable. 

-- 
GitHub Notification of comment by JTensai
Please view or discuss this issue at https://github.com/w3c/csswg-drafts/pull/5479#issuecomment-685100859 using your GitHub account


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

Received on Tuesday, 1 September 2020 20:02:28 UTC