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

[scroll-animations][web-animations-2] rewinding a finished scroll-driven animation and playing it again #11270

Open
graouts opened this issue Nov 25, 2024 · 11 comments
Labels

Comments

@graouts
Copy link
Contributor

graouts commented Nov 25, 2024

I'm working on the WebKit implementation of Scroll-driven Animations and am having issues making the "Animation finish event is fired again after replaying from start" subtest of scroll-animations/scroll-timelines/updating-the-finished-state.html work. The goal of that test is to "scrub" a scroll-driven animation to its finished state and then reset it back to its 0% time to play again.

Here's a slightly reduced self-contained test with some logging to query the timeline and animation state in that case:

<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title></title>
</head>
<body style="height: 200vh">
<div></div>
<script>

(async function () {
    const timeline = new ScrollTimeline({ source: document.documentElement });
    const animation = document.querySelector("div").animate({ "marginLeft": "100px" }, { timeline });

    const log = msg => {
        console.log(`${msg}\ntimeline time = ${timeline.currentTime}, animation time = ${animation.currentTime}, animation start time = ${animation.startTime}`);
    };

    await animation.ready;
    log("1. After animation readiness");

    const maxScroll = timeline.source.scrollHeight - timeline.source.clientHeight;
    timeline.source.scrollTop = maxScroll;
    await new Promise(requestAnimationFrame);
    log("2. After scrolling to 100% and waiting one frame");

    timeline.source.scrollTop = 0;
    animation.play();
    log("3. After scrolling back to 0% and calling play()");

    await new Promise(requestAnimationFrame);
    log("4. After animation readiness");
})();

</script>  
</body>
</html>

Chrome Canary

1. After animation readiness
timeline time = 0%, animation time = 0%, animation start time = 0%

2. After scrolling to 100% and waiting one frame
timeline time = 100%, animation time = 100%, animation start time = 0%

3. After scrolling back to 0% and calling play()
timeline time = 100%, animation time = 100%, animation start time = null

4. After animation readiness
timeline time = 0%, animation time = 0%, animation start time = 0%

WebKit

1. After animation readiness
timeline time = 0%, animation time = 0%, animation start time = 0%

2. After scrolling to 100% and waiting one frame
timeline time = 100%, animation time = 100%, animation start time = 0%

3. After scrolling back to 0% and calling play()
timeline time = 100%, animation time = 100%, animation start time = null

4. After animation readiness
timeline time = 0%, animation time = 100%, animation start time = -100%

The logging is identical in Chrome Canary and WebKit up until we rewind the animation and play it again. The result of what happens in Chrome makes complete sense, but I'm reading the spec over and over and failing to see what should be happening to match the Chrome behavior.

The internal state as play() is called is as follows:

hold time = 100%
start time = unresolved

And then as the pending play task is performed:

ready time = 0%
start time = ready time - hold time (100%) = -100%
hold time = unresolved

Can someone read the spec in a way that yields a different result for the values seen during the performance of the pending play task?

@graouts graouts added web-animations-2 Current Work scroll-animations-1 Current Work labels Nov 25, 2024
@graouts
Copy link
Contributor Author

graouts commented Nov 25, 2024

Cc the Chrome folks knowledgeable in this area: @flackr, @kevers-google and @andruud.

@ydaniv
Copy link
Contributor

ydaniv commented Nov 25, 2024

@graouts why do you need to call animation.play() again?

@graouts
Copy link
Contributor Author

graouts commented Nov 25, 2024

This is just how the WPT test that I'm trying to understand works.

@graouts
Copy link
Contributor Author

graouts commented Nov 25, 2024

Does Chrome perform the pending play task prior to updating the current time on the timeline? This would make the ready time be 100% and as a result the start time be 0%.

@kevers-google
Copy link
Contributor

Play is superflous as the playState will become running again after the next animation tick following reseting the scroll position. I suspect the issue is with unpausing an animation. For time-based we want to preserve current-time to avoid a jump, but for scroll-based we preserve the start-time to ensure that we remain anchored to the appropriate scroll range.

@kevers-google
Copy link
Contributor

From looking at the log output, it appears that Chrome recomputed a start time, without the constraint that it needed to either preserve current-time (to match the hold-time and avoid a jump) or to auto-rewind. Both of these constraints only makes sense only for time-based animations.

@flackr
Copy link
Contributor

flackr commented Nov 25, 2024

I suspect the following branch of chrome's implementation of play is responsible for the behavior observed here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/animation.cc;l=1647;drc=277f4ab48eb85f7441f78aed191c31068ce89814;bpv=1;bpt=1:

  if (has_finite_timeline && auto_rewind == AutoRewind::kEnabled) {
    auto_align_start_time_ = true;
    hold_time_ = CurrentTimeInternal();
  }

This appears right before what currently seems to be step 8 of playing an animation, however I'm guessing this may not have been properly spec'd.

Alternately, we could probably consider this call to play to be a no-op as part of step 10 but then I think we would expect the log at step 3 to match step 2.

@graouts
Copy link
Contributor Author

graouts commented Nov 25, 2024

Oh, so Chrome seems to expand step 7 in this way (bold italic is the added text):

If has finite timeline and either previous current time is unresolved or the auto-rewind flag is true:

  • Set the flag auto align start time to true.

This change alone makes the reduced test above behave the same in Chrome and WebKit.

@graouts
Copy link
Contributor Author

graouts commented Nov 25, 2024

@kevers-google I think this bit of code in Chrome's Animation::UpdateFinishedState() is also relevant to the WPT test, though not the reduced test I posted:

  // TODO(kevers): Add a new step to the spec.
  // Clear finished state and abort the procedure if play-pending and waiting
  // for a new start time.
  if (timeline_ && timeline_->IsScrollTimeline() && pending_play_ &&
      auto_align_start_time_) {
    finished_ = false;
    pending_finish_notification_ = false;
    committed_finish_notification_ = false;
    return;
  }

@graouts
Copy link
Contributor Author

graouts commented Nov 25, 2024

So, going back to the WPT test in question:

async_test(t => {
  const animation = createScrollLinkedAnimation(t);
  const scroller = animation.timeline.source;
  const maxScroll = scroller.scrollHeight - scroller.clientHeight;

  animation.play();

  animation.onfinish = event => {
    scroller.scrollTop = 0;
    window.requestAnimationFrame(function() {
      animation.play();
      scroller.scrollTop = maxScroll;
      animation.onfinish = event => {
        t.done();
      };
    });
  };
  scroller.scrollTop = maxScroll;
}, 'Animation finish event is fired again after replaying from start');

There's still something fishy here because we never get in a situation where the timeline is seeked back to 0%. I expect the intent of the requestAnimationFrame call is to delay the play() and scroller.scrollTop = maxScroll calls to the next frame, when the animation timeline time would indeed be 0%. But since the finish event is dispatched at the end of the update animation and send events procedure, which happens before requestAnimationFrame callbacks are serviced, this all happens in the same frame.

@kevers-google and @ogerchikov : git blame says you authored parts of this test, what do you make of this?

@kevers-google
Copy link
Contributor

If I recall correctly, this batch of tests was added prior to the introduction of auto-aligned start times, and was essentially a duplicate of equivalent tests with a document time. The Blink implementation has changed considerably since the test was introduced. The intent of the test was to verify that onfinish would fire each time we scroll to the end. I believe the test should read:

animation.onfinish = event => {
  window.requestAnimationFrame(() => {
    scroller.scrollTop = 0;
    window.requestAnimationFrame(() => {
      scroller.scrollTop = maxScroll;
      animation.onfinish = ...
    });
  });
}

The call to play() is not needed if we ensure that we have an animation tick with the reset scroll position.

This test is passing in Blink for another reason (likely requiring a spec tweak). In Blink, explicit calls to play() reset any stickiness of the start time and force a new start time to be evaluated. Stickiness in the timeline arises when explicitly setting the currentTime of a scroll-driven animation. This overrides the auto-aligned start time, at while point the animation progress is no longer strictly aligned with the calculated range. In this example, the start time is not sticky, and it is arguable if play should simply abort as a no-op. In the Blink implementation, any call to play with a scrollTimeline forces a new startTime to be evaluated.

As the test is currently written, play() is indirectly triggering the reset in UpdateFinishedState. The reason for the change to UpdateFinishedState was that we cannot update the finished state while waiting on an deferred start time. We should have a separate test for an explicit call to play resetting stickiness. I believe one already exists, but have not hunted it down.

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

4 participants