-
Notifications
You must be signed in to change notification settings - Fork 746
[scroll-animations-1][web-animations-2] Deferred start time #9181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defer to @flackr on the content of this PR, but thanks for cleaning up the commits. :)
At some point we'll want to update the WD also, so I suggest thinking about what remains before you're ready to publish.
web-animations-2/Overview.bs
Outdated
:: Set |animation|'s [=animation/start time=] to |seek time|. | ||
|
||
: If |previous play state| is "paused": | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot see where your links are pointing to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned up the procedure for setting the timeline based on offline discussion.
web-animations-2/Overview.bs
Outdated
> </dl> | ||
> | ||
> If the animation's [=hold time=] and [=start time=] are unresolved, | ||
> 1. If |has finite timeline| = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/=/is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
web-animations-2/Overview.bs
Outdated
|previous progress| is resolved: | ||
|
||
<dl class="switch"> | ||
1. Set [=animation/start time=] to unresolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.