Skip to content

Commit add5da7

Browse files
graoutsgraouts
committed
REGRESSION (262875@main): animation of rotate property doesn't work if there's a scale
https://bugs.webkit.org/show_bug.cgi?id=260255 rdar://113999490 Reviewed by Dean Jackson. When we fixed bug 255338 in 262875@main, we made it so that adding a GraphicsLayer animation with a given name would remove any existing animation of that same name. That animation's name is the name of the KeyframeEffect that yielded this GraphicsLayer animation. However, in RenderLayerBacking::startAnimation(), effects that animate multiple transform-related properties will yield one GraphicsLayer animation per property, all using the same name. As such, animating both the `rotate` and `scale` properties, for instance, will add first the `rotate` animation, and then add the `scale` animation which will have the unwanted effect of removing the prior `rotate` animation since both animations share the same name. So we now pass an optional property name to `GraphicsLayerCA::removeAnimation()` such that we check both the name and the property before removing a pre-existing animation when adding a new animation under `GraphicsLayerCA::createTransformAnimationsFromKeyframes()`. In the meantime, we make sure to pass `std::nullopt` in `RenderLayerBacking::animationFinished()` in order to remove *all* animations. * LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/scale-and-rotate-both-specified-on-animation-keyframes-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/scale-and-rotate-both-specified-on-animation-keyframes-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/scale-and-rotate-both-specified-on-animation-keyframes.html: Added. * Source/WebCore/platform/graphics/GraphicsLayer.h: (WebCore::GraphicsLayer::removeAnimation): * Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp: (WebCore::GraphicsLayerCA::removeAnimation): (WebCore::GraphicsLayerCA::createTransformAnimationsFromKeyframes): (WebCore::GraphicsLayerCA::createFilterAnimationsFromKeyframes): * Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h: * Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp: (WebCore::GraphicsLayerTextureMapper::removeAnimation): * Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h: * Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp: (WebCore::CoordinatedGraphicsLayer::removeAnimation): * Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h: * Source/WebCore/rendering/RenderLayerBacking.cpp: (WebCore::RenderLayerBacking::animationFinished): Canonical link: https://commits.webkit.org/269453@main
1 parent bfe384e commit add5da7

11 files changed

+105
-10
lines changed

‎LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/scale-and-rotate-both-specified-on-animation-keyframes-expected.html

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<title>Animating both the "scale" and "rotate" property</title>
5+
<link rel="help" href="https://drafts.csswg.org/css-transforms-2/#individual-transforms">
6+
7+
<style>
8+
9+
#target {
10+
position: absolute;
11+
width: 100px;
12+
height: 100px;
13+
background-color: black;
14+
15+
transform-origin: bottom left;
16+
rotate: 90deg;
17+
}
18+
19+
</style>
20+
</head>
21+
<body>
22+
<div id="target"></div>
23+
</body>
24+
</html>

‎LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/scale-and-rotate-both-specified-on-animation-keyframes-ref.html

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<title>Animating both the "scale" and "rotate" property</title>
5+
<link rel="help" href="https://drafts.csswg.org/css-transforms-2/#individual-transforms">
6+
7+
<style>
8+
9+
#target {
10+
position: absolute;
11+
width: 100px;
12+
height: 100px;
13+
background-color: black;
14+
15+
transform-origin: bottom left;
16+
rotate: 90deg;
17+
}
18+
19+
</style>
20+
</head>
21+
<body>
22+
<div id="target"></div>
23+
</body>
24+
</html>

‎LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/scale-and-rotate-both-specified-on-animation-keyframes.html

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
<!DOCTYPE html>
2+
<html class="reftest-wait">
3+
<head>
4+
<title>Animating both the "scale" and "rotate" property</title>
5+
<link rel="help" href="https://drafts.csswg.org/css-transforms-2/#individual-transforms">
6+
<link rel="match" href="scale-and-rotate-both-specified-on-animation-keyframes-ref.html">
7+
8+
<style>
9+
10+
@keyframes scale-and-rotate-animation {
11+
0% { scale: 1.5; rotate: 0deg; }
12+
0.001% { scale: 1; rotate: 90deg; }
13+
100% { scale: 1; rotate: 90deg; }
14+
}
15+
16+
#target {
17+
position: absolute;
18+
width: 100px;
19+
height: 100px;
20+
background-color: black;
21+
22+
transform-origin: bottom left;
23+
animation: scale-and-rotate-animation linear 100s;
24+
}
25+
26+
</style>
27+
</head>
28+
<body>
29+
<div id="target"></div>
30+
31+
<script>
32+
33+
(async function() {
34+
// We need to wait for the animation to have started and progressed for one frame
35+
// before taking the snapshot.
36+
await Promise.all(document.getAnimations().map(animation => animation.ready));
37+
await new Promise(requestAnimationFrame);
38+
document.documentElement.classList.remove("reftest-wait");
39+
})();
40+
41+
</script>
42+
</body>
43+
</html>

‎Source/WebCore/platform/graphics/GraphicsLayer.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -523,7 +523,7 @@ class GraphicsLayer : public RefCounted<GraphicsLayer>, public CanMakeCheckedPtr
523523
// Return true if the animation is handled by the compositing system.
524524
virtual bool addAnimation(const KeyframeValueList&, const FloatSize& /*boxSize*/, const Animation*, const String& /*animationName*/, double /*timeOffset*/) { return false; }
525525
virtual void pauseAnimation(const String& /*animationName*/, double /*timeOffset*/) { }
526-
virtual void removeAnimation(const String& /*animationName*/) { }
526+
virtual void removeAnimation(const String& /*animationName*/, std::optional<AnimatedProperty>) { }
527527
virtual void transformRelatedPropertyDidChange() { }
528528
WEBCORE_EXPORT virtual void suspendAnimations(MonotonicTime);
529529
WEBCORE_EXPORT virtual void resumeAnimations();

‎Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1135,14 +1135,18 @@ void GraphicsLayerCA::pauseAnimation(const String& animationName, double timeOff
11351135
}
11361136
}
11371137

1138-
void GraphicsLayerCA::removeAnimation(const String& animationName)
1138+
void GraphicsLayerCA::removeAnimation(const String& animationName, std::optional<AnimatedProperty> property)
11391139
{
11401140
LOG_WITH_STREAM(Animations, stream << "GraphicsLayerCA " << this << " id " << primaryLayerID() << " removeAnimation " << animationName << " (is running " << animationIsRunning(animationName) << ")");
11411141

11421142
for (auto& animation : m_animations) {
11431143
// There may be several animations with the same name in the case of transform animations
11441144
// animating multiple components as individual animations.
11451145
if (animation.m_name == animationName && !animation.m_pendingRemoval) {
1146+
// If a specific property is provided, we must check we only remove the animations
1147+
// for this specific property.
1148+
if (property && animation.m_property != *property)
1149+
continue;
11461150
animation.m_pendingRemoval = true;
11471151
noteLayerPropertyChanged(AnimationChanged | CoverageRectChanged);
11481152
}
@@ -3584,7 +3588,7 @@ bool GraphicsLayerCA::createTransformAnimationsFromKeyframes(const KeyframeValue
35843588
const auto& primitives = prefix.primitives();
35853589
unsigned numberOfSharedPrimitives = valueList.size() > 1 ? primitives.size() : 0;
35863590

3587-
removeAnimation(animationName);
3591+
removeAnimation(animationName, valueList.property());
35883592

35893593
for (unsigned animationIndex = 0; animationIndex < numberOfSharedPrimitives; ++animationIndex) {
35903594
if (!appendToUncommittedAnimations(valueList, primitives[animationIndex], animation, animationName, boxSize, animationIndex, timeOffset, false /* isMatrixAnimation */, keyframesShouldUseAnimationWideTimingFunction))
@@ -3649,7 +3653,7 @@ bool GraphicsLayerCA::createFilterAnimationsFromKeyframes(const KeyframeValueLis
36493653
return false;
36503654
}
36513655

3652-
removeAnimation(animationName);
3656+
removeAnimation(animationName, valueList.property());
36533657

36543658
for (int animationIndex = 0; animationIndex < numAnimations; ++animationIndex) {
36553659
if (!appendToUncommittedAnimations(valueList, operations.operations().at(animationIndex).get(), animation, animationName, animationIndex, timeOffset, keyframesShouldUseAnimationWideTimingFunction))

‎Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ class GraphicsLayerCA : public GraphicsLayer, public PlatformCALayerClient {
146146

147147
WEBCORE_EXPORT bool addAnimation(const KeyframeValueList&, const FloatSize& boxSize, const Animation*, const String& animationName, double timeOffset) override;
148148
WEBCORE_EXPORT void pauseAnimation(const String& animationName, double timeOffset) override;
149-
WEBCORE_EXPORT void removeAnimation(const String& animationName) override;
149+
WEBCORE_EXPORT void removeAnimation(const String& animationName, std::optional<AnimatedProperty>) override;
150150
WEBCORE_EXPORT void transformRelatedPropertyDidChange() override;
151151
WEBCORE_EXPORT void setContentsToImage(Image*) override;
152152
#if PLATFORM(IOS_FAMILY)

‎Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -638,7 +638,7 @@ void GraphicsLayerTextureMapper::pauseAnimation(const String& animationName, dou
638638
m_animations.pause(animationName, Seconds(timeOffset));
639639
}
640640

641-
void GraphicsLayerTextureMapper::removeAnimation(const String& animationName)
641+
void GraphicsLayerTextureMapper::removeAnimation(const String& animationName, std::optional<AnimatedProperty>)
642642
{
643643
m_animations.remove(animationName);
644644
}

‎Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ class GraphicsLayerTextureMapper final : public GraphicsLayer, TextureMapperPlat
7373

7474
bool addAnimation(const KeyframeValueList&, const FloatSize&, const Animation*, const String&, double) override;
7575
void pauseAnimation(const String&, double) override;
76-
void removeAnimation(const String&) override;
76+
void removeAnimation(const String&, std::optional<AnimatedProperty>) override;
7777

7878
void setContentsToImage(Image*) override;
7979
void setContentsToSolidColor(const Color&) override;

‎Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1462,7 +1462,7 @@ void CoordinatedGraphicsLayer::pauseAnimation(const String& animationName, doubl
14621462
didChangeAnimations();
14631463
}
14641464

1465-
void CoordinatedGraphicsLayer::removeAnimation(const String& animationName)
1465+
void CoordinatedGraphicsLayer::removeAnimation(const String& animationName, std::optional<AnimatedProperty>)
14661466
{
14671467
m_animations.remove(animationName);
14681468
didChangeAnimations();

‎Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ class WEBCORE_EXPORT CoordinatedGraphicsLayer : public GraphicsLayer {
117117
void setBackdropFiltersRect(const FloatRoundedRect&) override;
118118
bool addAnimation(const KeyframeValueList&, const FloatSize&, const Animation*, const String&, double) override;
119119
void pauseAnimation(const String&, double) override;
120-
void removeAnimation(const String&) override;
120+
void removeAnimation(const String&, std::optional<AnimatedProperty>) override;
121121
void suspendAnimations(MonotonicTime) override;
122122
void resumeAnimations() override;
123123
bool usesContentsLayer() const override;

0 commit comments

Comments
 (0)