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] Should setting iterations to Infinity via a call to updatingTiming() throw? #11343

Open
graouts opened this issue Dec 10, 2024 · 4 comments

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants