Title: [291526] trunk
- Revision
- 291526
- Author
- [email protected]
- Date
- 2022-03-19 04:41:44 -0700 (Sat, 19 Mar 2022)
Log Message
REGRESSION (r285529): Flash of missing text when content on https://ahumanfuture.co animates
https://bugs.webkit.org/show_bug.cgi?id=237972
rdar://90385586
Reviewed by Simon Fraser.
Source/WebCore:
Accelerated animations are added to a CALayer in two different ways.
Transform-related animations are added in a CAAnimationGroup so that we can have control of the
way they interact with each other. We added this system when we added support for individual CSS
transform properties on top of the existing "transform" property. All CAAnimationGroup animations
are timed to start at 1s and thus we subtract that same 1s value from all animations contained
within such groups.
All other animations, such as "opacity" animations, are added as simple leaf animations and their
begin time is not subject to that 1s subtraction.
In r285529, when we introduced leaf animations for transform properties other than transform-related
properties, we took an unfortunate approach to deal with that 1s subtraction. We would *always* subtract
1s for all animations, and for leaf animation add that 1s back when we added them in the
addLeafAnimation() lambda.
However, we *did not* check whether that animation was newly started or if we were merely adding it
again. This meant that every time GraphicsLayerCA::updateAnimations() was called, any leaf animation
would have 1s added to their begin time.
So, if for instance a page ran on a given element a "scale" animation for 1s and an "opacity" animation
for 2s, when the "scale" animation would complete, GraphicsLayerCA::updateAnimations() would be called
to remove that animation, and add 1s to the "opacity" animation begin time.
We now set the begin time, if not already set, in two places: in the addLeafAnimation() lambda for
leaf animations, and in the addAnimationsForProperty() lambda for animations added to groups. We
only ever set the begin time if not already set during a previous update and only subtract the
parent group begin time for animations contained within a group.
Additionally, this means we set the begin times when we iterate over remaining animations after culling
finished animations. As such, we no longer need to iterate over *all* known animations to set their
begin time if needed prior to adding them.
Test: webanimations/accelerated-animation-opacity-animation-begin-time-after-scale-animation-ends.html
* platform/graphics/ca/GraphicsLayerCA.cpp:
(WebCore::GraphicsLayerCA::updateAnimations):
LayoutTests:
Add a new test which would fail prior to the source change. In this test we start two animations: a "scale"
animation which lasts for 100ms, and an "opacity" animation which lasts much longer but only visually updates
for the duration of the "scale" animation. When the "scale" animation completes, we stop the test and ensure
that the reference test shows that the "opacity" animation is at the state it should be in at at that time,
and was not rewound due to the bug being fixed.
* webanimations/accelerated-animation-opacity-animation-begin-time-after-scale-animation-ends-expected.html: Added.
* webanimations/accelerated-animation-opacity-animation-begin-time-after-scale-animation-ends.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (291525 => 291526)
--- trunk/LayoutTests/ChangeLog 2022-03-19 10:36:04 UTC (rev 291525)
+++ trunk/LayoutTests/ChangeLog 2022-03-19 11:41:44 UTC (rev 291526)
@@ -1,3 +1,20 @@
+2022-03-19 Antoine Quint <[email protected]>
+
+ REGRESSION (r285529): Flash of missing text when content on https://ahumanfuture.co animates
+ https://bugs.webkit.org/show_bug.cgi?id=237972
+ rdar://90385586
+
+ Reviewed by Simon Fraser.
+
+ Add a new test which would fail prior to the source change. In this test we start two animations: a "scale"
+ animation which lasts for 100ms, and an "opacity" animation which lasts much longer but only visually updates
+ for the duration of the "scale" animation. When the "scale" animation completes, we stop the test and ensure
+ that the reference test shows that the "opacity" animation is at the state it should be in at at that time,
+ and was not rewound due to the bug being fixed.
+
+ * webanimations/accelerated-animation-opacity-animation-begin-time-after-scale-animation-ends-expected.html: Added.
+ * webanimations/accelerated-animation-opacity-animation-begin-time-after-scale-animation-ends.html: Added.
+
2022-03-18 Devin Rousso <[email protected]>
Web Inspector: REGRESSION(?): Emulate User Gesture doesn't work
Added: trunk/LayoutTests/webanimations/accelerated-animation-opacity-animation-begin-time-after-scale-animation-ends-expected.html (0 => 291526)
--- trunk/LayoutTests/webanimations/accelerated-animation-opacity-animation-begin-time-after-scale-animation-ends-expected.html (rev 0)
+++ trunk/LayoutTests/webanimations/accelerated-animation-opacity-animation-begin-time-after-scale-animation-ends-expected.html 2022-03-19 11:41:44 UTC (rev 291526)
@@ -0,0 +1,13 @@
+<style>
+
+div {
+ position: absolute;
+ top: 0;
+ left: 0;
+ width: 100px;
+ height: 100px;
+ background-color: green;
+}
+
+</style>
+<div></div>
Added: trunk/LayoutTests/webanimations/accelerated-animation-opacity-animation-begin-time-after-scale-animation-ends.html (0 => 291526)
--- trunk/LayoutTests/webanimations/accelerated-animation-opacity-animation-begin-time-after-scale-animation-ends.html (rev 0)
+++ trunk/LayoutTests/webanimations/accelerated-animation-opacity-animation-begin-time-after-scale-animation-ends.html 2022-03-19 11:41:44 UTC (rev 291526)
@@ -0,0 +1,47 @@
+<style>
+
+div {
+ position: absolute;
+ top: 0;
+ left: 0;
+ width: 100px;
+ height: 100px;
+ background-color: green;
+ opacity: 0;
+}
+
+</style>
+
+<div></div>
+
+<script src=""
+<script>
+
+(async () => {
+ window.testRunner?.waitUntilDone();
+
+ const target = document.querySelector("div");
+
+ // Create a scale animation for 0.1s.
+ const scale = target.animate({ scale: [0.5, 1] }, 100);
+
+ // And a fade animation for 10s but which visually completes
+ // at the same time as the scale animation.
+ const fade = target.animate([
+ { offset: 0, opacity: 0.5 },
+ { offset: 0.01, opacity: 1 },
+ { offset: 1, opacity: 1 },
+ ], { duration: 10_000, fill: "both" });
+
+ // Wait until the scale animation ends and for animations to be updated
+ // to check that the opacity animation is in the expected visual state.
+ await scale.finished;
+
+ await UIHelper.animationFrame();
+ await UIHelper.animationFrame();
+ await UIHelper.animationFrame();
+
+ window.testRunner?.notifyDone();
+})();
+
+</script>
Modified: trunk/Source/WebCore/ChangeLog (291525 => 291526)
--- trunk/Source/WebCore/ChangeLog 2022-03-19 10:36:04 UTC (rev 291525)
+++ trunk/Source/WebCore/ChangeLog 2022-03-19 11:41:44 UTC (rev 291526)
@@ -1,3 +1,49 @@
+2022-03-19 Antoine Quint <[email protected]>
+
+ REGRESSION (r285529): Flash of missing text when content on https://ahumanfuture.co animates
+ https://bugs.webkit.org/show_bug.cgi?id=237972
+ rdar://90385586
+
+ Reviewed by Simon Fraser.
+
+ Accelerated animations are added to a CALayer in two different ways.
+
+ Transform-related animations are added in a CAAnimationGroup so that we can have control of the
+ way they interact with each other. We added this system when we added support for individual CSS
+ transform properties on top of the existing "transform" property. All CAAnimationGroup animations
+ are timed to start at 1s and thus we subtract that same 1s value from all animations contained
+ within such groups.
+
+ All other animations, such as "opacity" animations, are added as simple leaf animations and their
+ begin time is not subject to that 1s subtraction.
+
+ In r285529, when we introduced leaf animations for transform properties other than transform-related
+ properties, we took an unfortunate approach to deal with that 1s subtraction. We would *always* subtract
+ 1s for all animations, and for leaf animation add that 1s back when we added them in the
+ addLeafAnimation() lambda.
+
+ However, we *did not* check whether that animation was newly started or if we were merely adding it
+ again. This meant that every time GraphicsLayerCA::updateAnimations() was called, any leaf animation
+ would have 1s added to their begin time.
+
+ So, if for instance a page ran on a given element a "scale" animation for 1s and an "opacity" animation
+ for 2s, when the "scale" animation would complete, GraphicsLayerCA::updateAnimations() would be called
+ to remove that animation, and add 1s to the "opacity" animation begin time.
+
+ We now set the begin time, if not already set, in two places: in the addLeafAnimation() lambda for
+ leaf animations, and in the addAnimationsForProperty() lambda for animations added to groups. We
+ only ever set the begin time if not already set during a previous update and only subtract the
+ parent group begin time for animations contained within a group.
+
+ Additionally, this means we set the begin times when we iterate over remaining animations after culling
+ finished animations. As such, we no longer need to iterate over *all* known animations to set their
+ begin time if needed prior to adding them.
+
+ Test: webanimations/accelerated-animation-opacity-animation-begin-time-after-scale-animation-ends.html
+
+ * platform/graphics/ca/GraphicsLayerCA.cpp:
+ (WebCore::GraphicsLayerCA::updateAnimations):
+
2022-03-19 Philippe Normand <[email protected]>
Unreviewed, WPE clang build fix after r291343.
Modified: trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp (291525 => 291526)
--- trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp 2022-03-19 10:36:04 UTC (rev 291525)
+++ trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp 2022-03-19 11:41:44 UTC (rev 291526)
@@ -3129,8 +3129,8 @@
};
auto addLeafAnimation = [&](LayerPropertyAnimation& animation) {
- ASSERT(animation.m_beginTime);
- *animation.m_beginTime += animationGroupBeginTime;
+ if (!animation.m_beginTime)
+ animation.m_beginTime = currentTime;
prepareAnimationForAddition(animation, Additive::No);
setAnimationOnLayer(animation);
};
@@ -3173,12 +3173,6 @@
}
};
- // Iterate through all animations to set the begin time of any new animations.
- for (auto& animation : m_animations) {
- if (!animation.m_pendingRemoval && !animation.m_beginTime)
- animation.m_beginTime = currentTime - animationGroupBeginTime;
- }
-
// Now, remove all animation groups and leaf animations from the layer so that
// we no longer have any layer animations.
for (auto& animation : m_animations)
@@ -3268,6 +3262,8 @@
LayerPropertyAnimation* earliestAnimation = nullptr;
Vector<RefPtr<PlatformCAAnimation>> caAnimations;
for (auto* animation : makeReversedRange(animations)) {
+ if (!animation->m_beginTime)
+ animation->m_beginTime = currentTime - animationGroupBeginTime;
if (auto beginTime = animation->computedBeginTime()) {
if (!earliestAnimation || *earliestAnimation->computedBeginTime() > *beginTime)
earliestAnimation = animation;
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
