[web-animations-2] Add hold phase #6508

kevers-google · 2021-08-10T17:06:43Z

[web-animations-2] Add hold phase to an animation (issue #4325)

This PR is an update on #5479, which converts the PR to a delta spec change. The PR introduces addtional factors when calculating the phase of an animation effect. When using a monotonic timeline, the boundaries for the before and after phase can be determined based on the start delay, end time and active duration. The phase of a scroll timeline linked animation needs to factor in scroll offsets. This requirement is achieved by introducing the "current phase" of an animation, which in turn depends on the hold phase when the animation is paused.

flackr · 2021-08-11T20:22:27Z

web-animations-2/Overview.bs

+
+:   Otherwise,
+::  The <a>current phase</a> is the [=timeline phase=] of the associated
+    <a>timeline</a>.


Doesn't this need the current phase algorithm from animation effect? I.e. before if the local time is less than before-active boundary time, after if it's after, etc.

The animation effect handles that calculation, this is just the phase that gets passed to the animation effect to help it with its calculation. It is basically passing through the timeline phase, but adjusting for if the animation is paused.

Consolidated by moving the logic to the effect phase calculation. I think originally this was a bit confusing since when the timeline phase = active case, we still need to determine if the effect phase is before, active or after. So timeline/animation phase is not the same as effect phase. Having said that, I think the benefit of encapsulating hold phase is not worth the potential for confusing in introducing "animation phase".

flackr · 2021-08-11T20:23:55Z

web-animations-2/Overview.bs

+> animation effect's <a>local time</a> is not <a>unresolved</a> and
+> <em>any</em> of the following conditions are met:
+>
+> 1.  the <a>local time</a> is less than the <a>before-active boundary time</a>,


The local time of an animation effect defers to the associated animation anyways, I think we should probably do something similar to what web-animations-1 does for local time where we refer the phase calculation to the associated animation. I would link it but https://drafts.csswg.org/web-animations-1/ seems to be broken :(

I believe I originally wrote it this way to avoid the animation reaching into the effect to get timing information. It should work either way though.

Updated the logic based on removing the need for "animation phase". Please let me know what you think.

I should note that this PR is not ready to land as it builds on top of CSSNumerish current/start times, which should land first. Preliminary push to address the comments.

PR should be ready for (re)review now.

flackr · 2021-09-24T15:41:01Z

web-animations-2/Overview.bs

-                    [=hold time=] be |current time to match|.
+                1.  If |animation|'s [=playback rate=] is zero, let
+                    |animation|'s [=hold time=] be |current time to match| and
+                    let [=hold phase=] be the |animation|'s [=current phase=].


I think this should be the timeline's phase. The current phase of the animation that we are telling to play is not well defined and ultimately the only phase that needs to be held is the timeline phase AFAICT.

Correct, oversight when removing animation current phase.

flackr · 2021-09-24T15:54:20Z

web-animations-2/Overview.bs

+            ::  1.  Set <var>animation</var>'s <a>hold time</a> to
+                    <var>seek time</var>.
+                1.  Set <var>animation</var>'s <a>hold phase</a> to
+                    [=timeline active phase|active=].


I'm not sure I understand why an unresolved seek time should result in an animation in the active phase. Shouldn't we unset the hold phase so that we fall back to calculating the animation phase to be idle?

Indent error right after the abort step. It looks like the error got introduced by the PR that fixed indent as it is also in the main branch and I didn't catch it when adding the hold phase rules. If seek time is unresolved we abort in step 1.2.

flackr · 2021-09-24T15:57:34Z

web-animations-2/Overview.bs

    complete the pause operation by performing the following steps:
    1.  Set <var>animation</var>'s <a>hold time</a> to <var>seek time</var>.
+    1.  Set <var>animation</var>'s <a>hold phase</a> to
+        [=timeline active phase|active=].


The hold phase calculation in this PR overrides (comes before) the normal animation phase calculation, so I think this will result in an incorrect phase if you pause an animation at a time before / after the active time.

I think overrides is the wrong term here since hold phase and effect phase are different things. The effect phase depends on timeline phase and local time. By setting a hold phase when the animation is paused, we are locking down the timeline phase at the time the animation was paused. Since local time will also be locked down via hold time, we should get a consistent result for the effect change.

The hold phase is being set a bit naively here. If offsets are not 0% and 100%, then we could be in the before or after phase.

The problem is a bit tricky to solve in all cases since the pending pause could be triggered before the scroll timeline time has synced to the scroll position (new scroll timeline). We would not pick up the timeline phase until the next animation update cycle. Perhaps it is OK to leave the hold phase as inactive for this particular edge case.

I think overrides was the correct term before your most recent update to calculating the effect phase. The very first rule was that if the hold phase was phase X, effect phase is phase X, and the other rules for determining before/after phase only applied if the hold phase was unset. So effectively if the hold phase was set, the effect phase would match the hold phase. I think with your recent update this has been resolved so that the active hold phase allows the effect phase to still be before / after.

flackr · 2021-09-24T16:06:19Z

web-animations-2/Overview.bs

+> 1.  <a>animation</a>'s <a>hold phase</a> is unresolved, <em>and</em>,
+>     <a>animation</a>'s [=timeline phase=] is [=timeline/before phase|before=],
+>     <em>or</em>
+> 1.  <a>animation</a>'s <a>hold phase</a> is unresolved, <em>and</em>


I think these conditions that the hold phase is unresolved may be causing a lot of issues where you set the hold phase to active which results in the animation phase being unable to resolve to before or after even if the local time is before / after the active duration.

Yes, this last condition should we updated to include hold phase active.

This will imply that the timeline's effective start and end offsets are hard limits on the active region even with negative delays. It'll still be possible for the timeline phase to be active and the effect phase to be before/after (with the fix for hold phase being active).

Added a definition for "effective timeline phase" to simplify and correct the logic.

flackr · 2021-09-29T13:25:27Z

web-animations-2/Overview.bs

+>     <em>and</em> <em>either</em> of the following conditions are met:
+>     1.  the <a>local time</a> is less than the <a>before-active boundary
+>         time</a>, <em>or</em>
+>     1.  [=timeline duration=] is unresolved, the <a>animation direction</a> is


Why does this now require the timeline duration to be unresolved? This seems like it has overlap with #6673 and we should resolve this separately.

I can pull this out into a separate PR.

Pulling it out to a separate PR.

flackr · 2021-09-29T13:27:07Z

web-animations-2/Overview.bs

    complete the pause operation by performing the following steps:
    1.  Set <var>animation</var>'s <a>hold time</a> to <var>seek time</var>.
+    1.  Set <var>animation</var>'s <a>hold phase</a> to
+        [=timeline active phase|active=].


I think overrides was the correct term before your most recent update to calculating the effect phase. The very first rule was that if the hold phase was phase X, effect phase is phase X, and the other rules for determining before/after phase only applied if the hold phase was unset. So effectively if the hold phase was set, the effect phase would match the hold phase. I think with your recent update this has been resolved so that the active hold phase allows the effect phase to still be before / after.

flackr · 2021-09-29T13:42:02Z

web-animations-2/Overview.bs

+
+> In addition to the <a>hold time</a>, an <a>animation</a> maintains a
+> <dfn>hold phase</dfn> which is set along with the <a>hold time</a> to
+> determine the <a>current phase</a> of an <a>animation</a> with a fixed


I think it would be helpful to explain add that the hold phase is used to hold the effective timeline phase.

e.g. how about instead of "to determine the current phase of an animation with a fixed current time", use
"to hold the effective timeline phase of an animation with a fixed current time"

flackr · 2021-09-29T13:47:19Z

web-animations-2/Overview.bs

+        ::  1.  Set <var>animation</var>'s <a>hold time</a> to
                <var>seek time</var>.
+            1.  Set <var>animation</var>'s <a>hold phase</a> to
+                [=timeline phase=].


nit: I don't think the animation has a definition for timeline phase so you probably need to say the associated timeline's [=timeline phase=] (here and elsewhere)

flackr · 2021-09-29T13:56:55Z

web-animations-2/Overview.bs


        <dl class="switch">

+        ::  1.  Set <var>animation</var>'s <a>hold time</a> to


I don't think these extra statements should be here, maybe this was a merge conflict? If we need additional steps they should go in either of the switch conditions for if |has finite timeline|. I think all switches should be immediately followed by a condition optionally containing a list of steps to follow if that condition matches.

Fixed the merge conflict.

Kevin Ellis added 7 commits July 28, 2021 10:32

CSSNumberish times

cb7a666

Address feedback

ece9697

Address feedback

891e9c8

Update link

c469ff1

Add hold phase

0d769e8

Merge branch 'main' into hold-phase

f45dcff

Minor tweak

3a0b498

flackr requested changes Aug 11, 2021

View reviewed changes

Kevin Ellis added 12 commits August 13, 2021 13:24

Update links

c2a5d8b

Fix indent errors

830bb4d

Cleanup indent

af88852

Merge branch 'main' into numberish-current-and-start-time

523a18c

Fix indentation

41b828d

Formatting

393105f

Merge branch 'numberish-current-and-start-time' into hold-phase

473c9bc

Cleanup

c3ce548

Merge branch 'main' into hold-phase

4241620

Cleanup

6504708

Delint

24e54ce

Formatting tweak

95359d0

flackr requested changes Sep 24, 2021

View reviewed changes

Kevin Ellis added 5 commits September 24, 2021 17:09

effective timeline phase

ac8fc81

Fix indent

8f738f2

Update setting current time

9083b34

Merge branch 'main' into hold-phase

c86bc73

Cleanup

566941f

kevers-google requested a review from flackr September 27, 2021 17:28

kevers-google mentioned this pull request Sep 27, 2021

[web-animation-2] Resolution of fill=auto for progress-based timelines #6673

Closed

flackr requested changes Sep 29, 2021

View reviewed changes

Address feedback.

7b34926

flackr approved these changes Sep 29, 2021

View reviewed changes

flackr merged commit 2424c01 into w3c:main Sep 30, 2021


		<dl class="switch">

		:: 1. Set <var>animation</var>'s <a>hold time</a> to

[web-animations-2] Add hold phase #6508

[web-animations-2] Add hold phase #6508

Uh oh!

Conversation

kevers-google commented Aug 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevers-google Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevers-google Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevers-google Sep 24, 2021 •

edited

Loading

kevers-google Sep 24, 2021 •

edited

Loading