[webkit-changes] [WebKit/WebKit] 494bf3: [web-animations] setting currentTime=0 when animat...

[Antoine Quint]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 494bf388fd775f7a80dc95337a3ba8c58e29bf21
      https://github.com/WebKit/WebKit/commit/494bf388fd775f7a80dc95337a3ba8c58e29bf21
  Author: Antoine Quint <graouts at webkit.org>
  Date:   2023-12-11 (Mon, 11 Dec 2023)

  Changed paths:
    A LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-restarted-after-changing-iteration-count-after-completion-expected.txt
    A LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-restarted-after-changing-iteration-count-after-completion.html
    M Source/WebCore/animation/KeyframeEffect.cpp
    M Source/WebCore/animation/KeyframeEffect.h
    M Source/WebCore/animation/KeyframeEffectStack.cpp

  Log Message:
  -----------
  [web-animations] setting currentTime=0 when animation-play-state=paused, doesn't restart animation after unpausing it
https://bugs.webkit.org/show_bug.cgi?id=265280
rdar://118826588

Reviewed by Antti Koivisto.

The test attached to the bug report did the following:

- started a CSS Animation with the default single iteration
- when that animation ended would set `animation-iteration-count: infinite`
- called getAnimations() on the target element on which the CSS Animation was applied

We failed to return an animation in that case because, even though we did everything right to update
the timing properties of the CSS Animation and mark it as "relevant" (in Web Animations parlance) again,
we did not successfully add it back to the target's keyframe effect stack.

This was due to some shoddy engineering (on this patch author's part) which almost guaranteed a bug like
this would crop up. Indeed, KeyframeEffect keeps an instance variable `m_inTargetEffectStack` around to
cache whether it is in the associated target's keyframe effect stack. While we would correctly update
this instance variable when calling `addEffect()` and `removeEffect()` within KeyframeEffect on the
effect stack, we completely neglected to have this instance variable updated when other parts of the
code would manipulate the effect stack in which this effect was found, such as under
`AnimationTimeline::removeAnimation()`.

Arguably, we could have change the call in `AnimationTimeline::removeAnimation()` to go through
`KeyframeEffectStack`, but instead we chose to guarantee `m_inTargetEffectStack` is now reflective
of whether the effect is found in its associated target's effect stack by ensuring that, when
calls to `addEffect()` and `removeEffect()` successfully add or remove the provided effect to or
from the stack, the instance variable is updated.

To that end, we add a new `KeyframeEffect::wasAddedToEffectStack()` method and rename `wasRemovedFromStack()`
to be more specific that this is an effect stack we're dealing with.

* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-restarted-after-changing-iteration-count-after-completion-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-restarted-after-changing-iteration-count-after-completion.html: Added.
* Source/WebCore/animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::animationTimelineDidChange):
(WebCore::KeyframeEffect::updateEffectStackMembership):
(WebCore::KeyframeEffect::didChangeTargetStyleable):
(WebCore::KeyframeEffect::wasAddedToEffectStack):
(WebCore::KeyframeEffect::wasRemovedFromEffectStack):
(WebCore::KeyframeEffect::wasRemovedFromStack): Deleted.
* Source/WebCore/animation/KeyframeEffect.h:
* Source/WebCore/animation/KeyframeEffectStack.cpp:
(WebCore::KeyframeEffectStack::addEffect):
(WebCore::KeyframeEffectStack::removeEffect):

Canonical link: https://commits.webkit.org/271872@main