Skip to content
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

[web-animations-2] moving from a progress-based timeline to a monotonic timeline can yield incompatible timeline time and animation current time #11761

Open
graouts opened this issue Feb 21, 2025 · 2 comments
Labels

Comments

@graouts
Copy link
Contributor

graouts commented Feb 21, 2025

I'm working on a failure in the WPT subtest "Playback rate affects whether active phase boundary is inclusive" in scroll-animations/scroll-timelines/scroll-animation-effect-phases.tentative.html and encountered what appears to be a spec issue. I've simplified the test in this particular reduction:

<style>

.scroller {
    overflow: scroll;
    height: 200px;
    width: 200px;
}

.contents {
    height: 1200px;
    width: 100%;
    background-color: green;
}

</style>

<div class="scroller">
    <div class="contents"></div>
</div>

<script>

(async function () {
    // create an animation associated with a progress-based timeline
    const timeline = new ScrollTimeline({ source: document.querySelector(".scroller") });
    const animation = document.querySelector(".contents").animate({ opacity: [0.1, 1] }, { timeline });

    await Promise.all([animation.ready, new Promise(requestAnimationFrame)]);

    // set the animation to the "after" phase
    const scroller = animation.timeline.source;
    const maxScroll = scroller.scrollHeight - scroller.clientHeight;
    scroller.scrollTop = 1.5 * maxScroll;

    // wait a frame and set the timeline to a monotonic timeline
    await new Promise(requestAnimationFrame);
    animation.timeline = document.timeline;
})();

</script>

In WebKit – at the time of writing (290776@main) – this particular test yields a debug assertion because we end up attempting an operation with a time value that is expressed in seconds and one expressed as a percentage. Here's how we get there, and I believe in accordance with the current specifications:

Step 1

Under the call to Element.animate(), the procedure to set the timeline of an animation takes us inside the "if to finite timeline" branch and we set both the start time and the hold time to be unresolved.

Step 2

As the update animations and send events procedure is triggered and the animation becomes ready, we calculate an auto-aligned start time and set the start time to 0% as the hold time remains unresolved.

Step 3

After the animation timeline's source changes scroll position, and during the next occurence of the update animations and send events procedure, we update the animation's finished state and match this condition:

If all three of the following conditions are true,

  • the unconstrained current time is resolved, and
  • animation’s start time is resolved, and
  • animation does not have a pending play task or a pending pause task,

then update animation’s hold time based on the first matching condition for animation from below, if any:

… and then match this condition:

If playback rate > 0 and unconstrained current time is greater than or equal to associated effect end

… and finally this sub-condition:

If did seek is false, let the hold time be the maximum value of previous current time and associated effect end.

As a result, the hold time is set to 100% and the start time remains set to 0%.

Step 4

As we set the timeline to the document timeline, and as such move from a progress-based timeline to a monotonic timeline, we enter this condition:

If from finite timeline and previous progress is resolved,
Run the procedure to set the current time to previous progress * end time.

As such we immediately run the procedure to silently set the current time and enter this condition:

If any of the following conditions are true:

  • animation’s hold time is resolved, or
  • animation’s start time is unresolved, or
  • animation has no associated timeline or the associated timeline is inactive, or
  • animation’s playback rate is 0,

Set animation’s hold time to seek time.

As a result, the hold time becomes 0s and the start time remains 0%. Note that these two times are now using different time units.

As the setting the timeline procedure continues, we additionally run this step:

If the start time of animation is resolved, make animation’s hold time unresolved.

As a result, the hold time becomes unresolved and the start time remains 0%.

To finish setting the timeline we update the animation's finished state and attempt to compute the unconstrained current time at which point we attempt to compute:

current time = (timeline time - start time) * playback rate

The timeline time is a resolved time expressed in seconds since the timeline is monotonic while the start time has remained 0%. This is when WebKit hits a debug assertion.

The WPT in question test immediately sets the current time to 0 but I don't think we should be in the situation described above as we set the timeline.

@graouts graouts added web-animations-2 Current Work scroll-animations-1 Current Work labels Feb 21, 2025
@graouts
Copy link
Contributor Author

graouts commented Feb 21, 2025

graouts added a commit to graouts/WebKit that referenced this issue Feb 21, 2025
…l-animation-effect-phases.tentative.html has failures

https://bugs.webkit.org/show_bug.cgi?id=284545
rdar://problem/141356963

Reviewed by NOBODY (OOPS!).

The technique used by this test to determine the phase [0] in which an animation
is, which is not explicitly exposed via the Web Animations API, was not up-to-date
with the fact that progress-based animations remain active at the 100% boundary.

So we update the supporting `assert_phase()` function to deal with such animations
and pass the previously failing assertion.

But going further into this test, we would hit a debug assertion when trying to compute
the unconstrained current time as we get to the end of the procedure to set the timeline
as we move from the scroll timeline to the document timeline.

I filed a spec issue [1] to deal with this and came up with a temporary fix to restore
the timing incompatibility and pass this test.

[0] https://drafts.csswg.org/web-animations-1/#animation-effect-phases-and-states
[1] w3c/csswg-drafts#11761

* LayoutTests/imported/w3c/web-platform-tests/scroll-animations/scroll-timelines/scroll-animation-effect-phases.tentative-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/testcommon.js:
(assert_phase):
* Source/WebCore/animation/WebAnimation.cpp:
(WebCore::WebAnimation::setTimeline):
@flackr
Copy link
Contributor

flackr commented Feb 21, 2025

The purpose of step 9 in setting the timeline is to continue the animation from its previous progress on the new timeline. It seems that we didn't expect to have both start time and hold time set. Given the animation is holding in a finished state I guess we should make the start time unresolved in this case?

webkit-commit-queue pushed a commit to graouts/WebKit that referenced this issue Feb 21, 2025
…l-animation-effect-phases.tentative.html has failures

https://bugs.webkit.org/show_bug.cgi?id=284545
rdar://problem/141356963

Reviewed by Anne van Kesteren.

The technique used by this test to determine the phase [0] in which an animation
is, which is not explicitly exposed via the Web Animations API, was not up-to-date
with the fact that progress-based animations remain active at the 100% boundary.

So we update the supporting `assert_phase()` function to deal with such animations
and pass the previously failing assertion.

But going further into this test, we would hit a debug assertion when trying to compute
the unconstrained current time as we get to the end of the procedure to set the timeline
as we move from the scroll timeline to the document timeline.

I filed a spec issue [1] to deal with this and came up with a temporary fix to restore
the timing incompatibility and pass this test.

[0] https://drafts.csswg.org/web-animations-1/#animation-effect-phases-and-states
[1] w3c/csswg-drafts#11761

* LayoutTests/imported/w3c/web-platform-tests/scroll-animations/scroll-timelines/scroll-animation-effect-phases.tentative-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/testcommon.js:
(assert_phase):
* Source/WebCore/animation/WebAnimation.cpp:
(WebCore::WebAnimation::setTimeline):

Canonical link: https://commits.webkit.org/290787@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants