Skip to content

[scroll-animations] Should setting iterations to Infinity via a call to updatingTiming() throw? #11343

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

Open
graouts opened this issue Dec 10, 2024 · 5 comments
Labels

Comments

@graouts
Copy link
Contributor

graouts commented Dec 10, 2024

I'm looking at the WPT test for updateTiming() at scroll-timelines/effect-updateTiming.html and noticed that it has this subtest:

test(t => {
  const anim = createScrollLinkedAnimationWithTiming(t, { duration: 2000 });
  anim.play();
  assert_throws_js(TypeError, () => {
    anim.effect.updateTiming({ iterations: Infinity });
  }, "test");
}, `Throws when setting iterations to Infinity`);

I couldn't find any spec text that would support this, although it seems reasonable.

@graouts
Copy link
Contributor Author

graouts commented Dec 10, 2024

Cc @kevers-google @flackr @andruud.

@ydaniv
Copy link
Contributor

ydaniv commented Dec 10, 2024

The web-animations-1 spec says this:

The number of times an animation effect repeats is called its iteration count. The iteration count is a real number greater than or equal to zero. The iteration count may also be positive infinity to represent that the animation effect repeats indefinitely.

Constructing effects with Infinity works everywhere. Seems wrong to be failing on update.

@flackr
Copy link
Contributor

flackr commented Dec 10, 2024

Constructing effects with Infinity works everywhere. Seems wrong to be failing on update.

We have many cases in the web animations spec where trying to do something nonsensical throws (e.g. negative iterations, negative duration, passing unsorted keyframes to setKeyframes, trying to play an infinite duration or iteration animation in reverse).

In this test case, having infinite iterations results in a total time of infinity when converting to a proportional animation, meaning that naively start delay, iteration duration and end delay would all be 0 I suppose. This can't match the expected result of the animation filling the specified range, so it seems like throwing an error might be reasonable in the same way you can't start a reverse direction infinite iteration animation.

@ydaniv
Copy link
Contributor

ydaniv commented Dec 10, 2024

Ohh, progress-based 🤦 missed that somehow.

graouts added a commit to graouts/WebKit that referenced this issue Dec 11, 2024
…or effects associated with a progress-based animation

https://bugs.webkit.org/show_bug.cgi?id=284446
rdar://141271158

Reviewed by NOBODY (OOPS!).

It does not make sense to have an infinite duration for a progress-based animation, so we
should throw in that situation. Since the relevant specifications do not call this out
specifically yet, even though it is tested that way in WPT and Chrome implements this
behavior, the following spec issue was filed: w3c/csswg-drafts#11343.

* LayoutTests/imported/w3c/web-platform-tests/scroll-animations/scroll-timelines/effect-updateTiming-expected.txt:
* Source/WebCore/animation/AnimationEffect.cpp:
(WebCore::AnimationEffect::updateTiming):
graouts added a commit to graouts/WebKit that referenced this issue Dec 11, 2024
…or effects associated with a progress-based animation

https://bugs.webkit.org/show_bug.cgi?id=284446
rdar://141271158

Reviewed by NOBODY (OOPS!).

It does not make sense to have an infinite duration for a progress-based animation, so we
should throw in that situation. Since the relevant specifications do not call this out
specifically yet, even though it is tested that way in WPT and Chrome implements this
behavior, the following spec issue was filed: w3c/csswg-drafts#11343.

* LayoutTests/imported/w3c/web-platform-tests/scroll-animations/scroll-timelines/effect-updateTiming-expected.txt:
* Source/WebCore/animation/AnimationEffect.cpp:
(WebCore::AnimationEffect::updateTiming):
webkit-commit-queue pushed a commit to graouts/WebKit that referenced this issue Dec 11, 2024
…or effects associated with a progress-based animation

https://bugs.webkit.org/show_bug.cgi?id=284446
rdar://141271158

Reviewed by Tim Nguyen.

It does not make sense to have an infinite duration for a progress-based animation, so we
should throw in that situation. Since the relevant specifications do not call this out
specifically yet, even though it is tested that way in WPT and Chrome implements this
behavior, the following spec issue was filed: w3c/csswg-drafts#11343.

* LayoutTests/imported/w3c/web-platform-tests/scroll-animations/scroll-timelines/effect-updateTiming-expected.txt:
* Source/WebCore/animation/AnimationEffect.cpp:
(WebCore::AnimationEffect::updateTiming):

Canonical link: https://commits.webkit.org/287663@main
@birtles birtles added web-animations-2 Current Work and removed web-animations-1 labels May 1, 2025
@birtles
Copy link
Contributor

birtles commented May 1, 2025

Seems like this is related to converting to a proportional animation which is part of level 2. Adjusted the tags accordingly.

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