8000 [scroll-animations-1][web-animations-2] Deferred start time by kevers-google · Pull Request #9181 · w3c/csswg-drafts · GitHub
[go: up one dir, main page]

Skip to content

Conversation

kevers-google
Copy link
Contributor

[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.

@kevers-google kevers-google requested a review from flackr August 11, 2023 16:42
Copy link
Collaborator
@fantasai fantasai left a 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.)

@kevers-google
Copy link
Contributor Author

Link / markup fixes pulled out into separate PR and merged.

Copy link
Collaborator
@fantasai fantasai left a 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".

@kevers-google
Copy link
Contributor Author

Believe I've reverted the remaining formatting changes (constructor typo and 2 div / dl format). Moved to separate formatting PR.

Copy link
Collaborator
@fantasai fantasai left a 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.

:: Set |animation|'s [=animation/start time=] to |seek time|.

: If |previous play state| is "paused":

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

< 67DE details-collapsible>
> </dl>
>
> If the animation's [=hold time=] and [=start time=] are unresolved,
> 1. If |has finite timeline| = true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/=/is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

|previous progress| is resolved:

<dl class="switch">
1. Set [=animation/start time=] to unresolved.
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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".

Cleanup < A3E2 /code>
Copy link
Contributor
@flackr flackr left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0