[scroll-animations-1][web-animations-2] Deferred start time - #9181
Conversation
There was a problem hiding this comment.
Hi @kevers-google, when you land this PR can you flatten all the clean-up commits into one commit (with an appropriate commit message) and keep the substantive changes separate in their own commit?
(And in the future, feel free to land markup/linking fixes etc. directly on main.)
|
Link / markup fixes pulled out into separate PR and merged. |
fantasai
left a comment
There was a problem hiding this comment.
Still have a few stray markup changes, look for "switch" (x2) and "constructors".
|
Believe I've reverted the remaining formatting changes (constructor typo and 2 div / dl format). Moved to separate formatting PR. |
| @@ -318,7 +322,6 @@ follows: | |||
|
|
|||
There was a problem hiding this comment.
I'm not sure if setting the start time to 0 / 100% makes sense here or here (couldn't comment on the actual lines since they're too far from the diff).
If auto align start time
- is true, then we expect it to be updated later and should probably set it to unresolved in the interim.
- is false, then would we expect to keep the previously set start time?
There was a problem hiding this comment.
I cannot see where your links are pointing to.
There was a problem hiding this comment.
Only one of the two links is working for me.
Referring to the handling of start time in the set timeline procedure, we have a few options:
- When switching timelines, we always set auto align start time. This means that setting current time than then switching timelines won't preserve current time. Though as the units of time are changing, all we would be able to preserve is progress.
- When switching timelines, we preserve progress if auto-align is false, or the new timeline is time-based, otherwise hands off. This means that we should either make auto-alignment 3 state (true|false|unset) and only set to true if in the unset state, or always set auto-align to true when calling play regardless of the timeline type (it simply won't get used while time-based). Then when we switch to a scroll-based animation, the flag will already be set if current time or start time have not been called.
There was a problem hiding this comment.
Cleaned up the procedure for setting the timeline based on offline discussion.
| > </dl> | ||
| > | ||
| > If the animation's [=hold time=] and [=start time=] are unresolved, | ||
| > 1. If |has finite timeline| = true, |
| |previous progress| is resolved: | ||
|
|
||
| <dl class="switch"> | ||
| 1. Set [=animation/start time=] to unresolved. |
There was a problem hiding this comment.
Shouldn't we set the start time to unresolved when we set auto align start time to true regardless of the previous play state, as once we switch timelines, deciding to auto align the start time we don't know what the start time will be, right?
There was a problem hiding this comment.
Doing so would affect the value of anim.currentTime. Not that currentTime is that relevant if in the process of switching timelines and needing a new start time. My initial gut feeling was that rather than having a currentTime that can potentially become null, we should hold the last known value of currentTime until we've established a new deferred start time. Having said that a play-pending animation will have a null value of currentTime until start time is resolved rather than 0 or effect-end that it would have if time-based. In short, I can be convinced either way.
There was a problem hiding this comment.
A finished animation would also briefly appear to be paused if we unconditionally reset the start time if auto-aligning the start time unless we also clear the hold time. Note that if paused we will set the hold time based on the previous progress so likely OK to clear both start and hold time. Then we are in a consistent state with playing an animation.
There was a problem hiding this comment.
Fixed.
Made resetting the start time unconditional after setting auto-aligned start time.
Also clear hold time to avoid changing the play state to paused and added scheduling a pending play task if "running" or "finished".
flackr
left a comment
There was a problem hiding this comment.
Thanks for all the changes. I think this is looking good.
[scroll-animations-1][web-animations-2] Deferred start time
When using a scroll-driven animation the start time of the animation is auto-aligned with the animation range boundary is not explicitly set.