[web-animations-1] Add hold phase to animation #4325

[JTensai]

[web-animations-1] Add hold phase to animation #4325

Added the concept of hold phase to animations. This will allow them to pass the correct phase to associated effects under such conditions as pausing or any other time the existing hold time is used.

Added conditions to the animation effect phase calculation to account for animation current phase.

[kevers-google]

Overall, it looks good. Would like input from those more familiar with timeline phase.

[birtles]

Thanks for doing this. The formatting and everything looks great.

I'm just a little concerned that this is quite invasive and increases the essential complexity of the model in the sense that there is now yet another state variable we need to check/update in each procedure.

Is there any other possible way of fixing #4325? Perhaps this is the best/only way but I'd like to be sure so if you have any comments about why this is necessary that would be most helpful!

(Incidentally, this is the sort of thing I would like to put in level 2 if possible, but I'll wait to hear how others feel about doing that.)

[JTensai]

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: Add hold_phase which will hold timeline phase data (same as how hold_time hold timeline time data) and is updated anytime hold_time is updated. 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 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.

[birtles]

Thanks Jordan! The example really helps.

I don't suppose this has any affect on #5423 by the way?

I've printed this out and put it on my desk so I'll look at it tomorrow morning.

(For my own notes, I believe this will be fine when we come to nested effects since we'll only apply the behavior to the rootmost effect.)

[birtles]

This looks good with the suggested changes.

Ideally we want to move this and timeline phase to level 2 but that's going to make for a pretty hard-to-read delta spec I guess.

[JTensai]

The current plan is to have this PR moved to level 2, but will do so by waiting for level 1 to get finalized. As I understand it, that will make it much easier than trying to modify this into a delta spec change.

[birtles]

Looks good with these comments addressed.

[birtles]

Nit for future reference as much as anything, in specs we try to use semantic line breaking where possible. This makes diffs smaller and easier to read.

There are a few articles about this like:
https://rhodesmill.org/brandon/2012/one-sentence-per-line/
https://ramshankar.org/blog/posts/2019/semantic-line-breaks/

We haven't done this very consistently in this spec so far, but going forward we should try to at least start each sentence on a new line (and preferably also after each comma, clause etc.).

[JTensai]

I will try to remember that going forward. Thanks!

[JTensai]

This PR will be moved to web-animations-2 after web-animations-1 has been merged into web-animations-2.

This PR should not be merged while modifications are in web-animations-1.

[JTensai]

I have moved all the changes over to Web-Animations-2.

Please take a look and make sure I did this the right way. First time moving from one level to another :)

[birtles]

I have moved all the changes over to Web-Animations-2.

Please take a look and make sure I did this the right way. First time moving from one level to another :)

Hi Jordan, thanks for doing this. I'm afraid I haven't looked through the whole patch, but just looking at the start it seems to be duplicating the text from level 1?

I believe the idea was to keep the level 2 spec a delta spec until level 1 is stabilized? That is, it should be more like a diff? I think the existing text does that be saying things like "This paragraph is added", "This sentence is changed" etc.

Is it possible to do the same for this patch? That way if we update the duplicated text in level 1 we will be sure to pick up those changes in level 2 later.

[birtles]

It looks like #4325 has been marked as fixed. @JTensai is this PR still needed?