- Revision
- 287827
- Author
- grao...@webkit.org
- Date
- 2022-01-09 13:19:45 -0800 (Sun, 09 Jan 2022)
Log Message
Implicit keyframe for a CSS Animation should always use the underlying style
https://bugs.webkit.org/show_bug.cgi?id=235014
Reviewed by Antti Koivisto.
LayoutTests/imported/w3c:
Add a new WPT which changes the underlying style for a property that is being
animated with an @keyframes rule that doesn't explicitly specify this value
on all keyframes.
* web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight-expected.txt: Added.
* web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight.html: Added.
Source/WebCore:
When resolving keyframes for a CSS Animation in Style::Resolver::keyframeStylesForAnimation(),
we would fill implicit keyframes based on the style at the time the animation was created.
However, the underlying style may change at any point while the CSS Animation is in flight,
so we should not be filling those values.
The code that actually resolves animated styles already knows how to handle this case, which
we already supported correctly for JS-originated animations. The only thing we need to do is
to ensure we fill implicit keyframes when calling getKeyframes().
This surfaced an issue with KeyframeList::copyKeyframes() where we only copied the styles
from one set of keyframes to another, but not the timing function, which would cause a
getKeyframes() test to regress. We should also make sure to copy the composite operation
for when we support the animation-composition property (see bug 232086).
Test: imported/w3c/web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight.html
* animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::getKeyframes):
* rendering/style/KeyframeList.cpp:
(WebCore::KeyframeList::copyKeyframes):
* style/StyleResolver.cpp:
(WebCore::Style::Resolver::keyframeStylesForAnimation):
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (287826 => 287827)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2022-01-09 19:51:57 UTC (rev 287826)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2022-01-09 21:19:45 UTC (rev 287827)
@@ -1,5 +1,19 @@
2022-01-09 Antoine Quint <grao...@webkit.org>
+ Implicit keyframe for a CSS Animation should always use the underlying style
+ https://bugs.webkit.org/show_bug.cgi?id=235014
+
+ Reviewed by Antti Koivisto.
+
+ Add a new WPT which changes the underlying style for a property that is being
+ animated with an @keyframes rule that doesn't explicitly specify this value
+ on all keyframes.
+
+ * web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight-expected.txt: Added.
+ * web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight.html: Added.
+
+2022-01-09 Antoine Quint <grao...@webkit.org>
+
Interpolation for the "filter" property fails with a single keyframe
https://bugs.webkit.org/show_bug.cgi?id=235019
Added: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight-expected.txt (0 => 287827)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight-expected.txt (rev 0)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight-expected.txt 2022-01-09 21:19:45 UTC (rev 287827)
@@ -0,0 +1,4 @@
+
+PASS Changing the underlying value of an animated property with an implicit 0% keyframe
+PASS Changing the underlying value of an animated property with an implicit 100% keyframe
+
Added: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight.html (0 => 287827)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight.html (rev 0)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight.html 2022-01-09 21:19:45 UTC (rev 287827)
@@ -0,0 +1,42 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>Changing the underlying value of an animated property with implicit keyframes</title>
+<link rel="help" href=""
+<script src=""
+<script src=""
+<script src=""
+<style>
+
+@keyframes implicit-from {
+ to { margin-left: 100px }
+}
+
+@keyframes implicit-to {
+ from { margin-left: 100px }
+}
+
+</style>
+<div id="log"></div>
+<script>
+'use strict';
+
+const implicit_keyframe_test = (animationName, offset) => {
+ test(t => {
+ const div = addDiv(t);
+
+ // Set up animation to be paused and be at its mid-way point through easing.
+ div.style.animation = `${animationName} 10s paused steps(2, start)`;
+ const animation = div.getAnimations()[0];
+
+ assert_equals(getComputedStyle(div).marginLeft, "50px", "Computed style before changing the underlying style");
+
+ // Change the underlying value.
+ div.style.marginLeft = "200px";
+ assert_equals(getComputedStyle(div).marginLeft, "150px", "Computed style after changing the underlying style");
+ }, `Changing the underlying value of an animated property with an implicit ${offset}% keyframe`);
+};
+
+implicit_keyframe_test("implicit-from", "0");
+implicit_keyframe_test("implicit-to", "100");
+
+</script>
Modified: trunk/Source/WebCore/ChangeLog (287826 => 287827)
--- trunk/Source/WebCore/ChangeLog 2022-01-09 19:51:57 UTC (rev 287826)
+++ trunk/Source/WebCore/ChangeLog 2022-01-09 21:19:45 UTC (rev 287827)
@@ -1,5 +1,35 @@
2022-01-09 Antoine Quint <grao...@webkit.org>
+ Implicit keyframe for a CSS Animation should always use the underlying style
+ https://bugs.webkit.org/show_bug.cgi?id=235014
+
+ Reviewed by Antti Koivisto.
+
+ When resolving keyframes for a CSS Animation in Style::Resolver::keyframeStylesForAnimation(),
+ we would fill implicit keyframes based on the style at the time the animation was created.
+ However, the underlying style may change at any point while the CSS Animation is in flight,
+ so we should not be filling those values.
+
+ The code that actually resolves animated styles already knows how to handle this case, which
+ we already supported correctly for JS-originated animations. The only thing we need to do is
+ to ensure we fill implicit keyframes when calling getKeyframes().
+
+ This surfaced an issue with KeyframeList::copyKeyframes() where we only copied the styles
+ from one set of keyframes to another, but not the timing function, which would cause a
+ getKeyframes() test to regress. We should also make sure to copy the composite operation
+ for when we support the animation-composition property (see bug 232086).
+
+ Test: imported/w3c/web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight.html
+
+ * animation/KeyframeEffect.cpp:
+ (WebCore::KeyframeEffect::getKeyframes):
+ * rendering/style/KeyframeList.cpp:
+ (WebCore::KeyframeList::copyKeyframes):
+ * style/StyleResolver.cpp:
+ (WebCore::Style::Resolver::keyframeStylesForAnimation):
+
+2022-01-09 Antoine Quint <grao...@webkit.org>
+
Interpolation for the "filter" property fails with a single keyframe
https://bugs.webkit.org/show_bug.cgi?id=235019
Modified: trunk/Source/WebCore/animation/KeyframeEffect.cpp (287826 => 287827)
--- trunk/Source/WebCore/animation/KeyframeEffect.cpp 2022-01-09 19:51:57 UTC (rev 287826)
+++ trunk/Source/WebCore/animation/KeyframeEffect.cpp 2022-01-09 21:19:45 UTC (rev 287827)
@@ -639,6 +639,10 @@
auto computedStyleExtractor = ComputedStyleExtractor(target, false, m_pseudoId);
+ KeyframeList computedKeyframes(m_blendingKeyframes.animationName());
+ computedKeyframes.copyKeyframes(m_blendingKeyframes);
+ computedKeyframes.fillImplicitKeyframes(*m_target, m_target->styleResolver(), lastStyleChangeEventStyle, nullptr);
+
auto keyframeRules = [&]() -> const Vector<Ref<StyleRuleKeyframe>> {
if (!is<CSSAnimation>(animation()))
return { };
@@ -648,7 +652,7 @@
if (!styleScope)
return { };
- return styleScope->resolver().keyframeRulesForName(m_blendingKeyframes.animationName());
+ return styleScope->resolver().keyframeRulesForName(computedKeyframes.animationName());
}();
auto keyframeRuleForKey = [&](double key) -> StyleRuleKeyframe* {
@@ -662,12 +666,12 @@
};
// We need to establish which properties are implicit for 0% and 100%.
- HashSet<CSSPropertyID> zeroKeyframeProperties = m_blendingKeyframes.properties();
- HashSet<CSSPropertyID> _oneKeyframeProperties_ = m_blendingKeyframes.properties();
+ HashSet<CSSPropertyID> zeroKeyframeProperties = computedKeyframes.properties();
+ HashSet<CSSPropertyID> _oneKeyframeProperties_ = computedKeyframes.properties();
zeroKeyframeProperties.remove(CSSPropertyCustom);
oneKeyframeProperties.remove(CSSPropertyCustom);
- for (size_t i = 0; i < m_blendingKeyframes.size(); ++i) {
- auto& keyframe = m_blendingKeyframes[i];
+ for (size_t i = 0; i < computedKeyframes.size(); ++i) {
+ auto& keyframe = computedKeyframes[i];
if (!keyframe.key()) {
for (auto cssPropertyId : keyframe.properties())
zeroKeyframeProperties.remove(cssPropertyId);
@@ -677,7 +681,7 @@
}
}
- for (size_t i = 0; i < m_blendingKeyframes.size(); ++i) {
+ for (size_t i = 0; i < computedKeyframes.size(); ++i) {
// 1. Initialize a dictionary object, output keyframe, using the following definition:
//
// dictionary BaseComputedKeyframe {
@@ -687,7 +691,7 @@
// CompositeOperationOrAuto composite = "auto";
// };
- auto& keyframe = m_blendingKeyframes[i];
+ auto& keyframe = computedKeyframes[i];
// 2. Set offset, computedOffset, easing members of output keyframe to the respective values keyframe offset, computed keyframe offset,
// and keyframe-specific timing function of keyframe.
@@ -695,7 +699,7 @@
computedKeyframe.offset = keyframe.key();
computedKeyframe.computedOffset = keyframe.key();
// For CSS transitions, all keyframes should return "linear" since the effect's global timing function applies.
- computedKeyframe.easing = is<CSSTransition>(animation()) ? "linear" : timingFunctionForKeyframeAtIndex(i)->cssText();
+ computedKeyframe.easing = is<CSSTransition>(animation()) ? "linear" : timingFunctionForBlendingKeyframe(keyframe)->cssText();
auto outputKeyframe = convertDictionaryToJS(lexicalGlobalObject, *jsCast<JSDOMGlobalObject*>(&lexicalGlobalObject), computedKeyframe);
Modified: trunk/Source/WebCore/rendering/style/KeyframeList.cpp (287826 => 287827)
--- trunk/Source/WebCore/rendering/style/KeyframeList.cpp 2022-01-09 19:51:57 UTC (rev 287826)
+++ trunk/Source/WebCore/rendering/style/KeyframeList.cpp 2022-01-09 21:19:45 UTC (rev 287827)
@@ -88,6 +88,8 @@
KeyframeValue keyframeValue(keyframe.key(), RenderStyle::clonePtr(*keyframe.style()));
for (auto propertyId : keyframe.properties())
keyframeValue.addProperty(propertyId);
+ keyframeValue.setTimingFunction(keyframe.timingFunction());
+ keyframeValue.setCompositeOperation(keyframe.compositeOperation());
insert(WTFMove(keyframeValue));
}
}
Modified: trunk/Source/WebCore/style/StyleResolver.cpp (287826 => 287827)
--- trunk/Source/WebCore/style/StyleResolver.cpp 2022-01-09 19:51:57 UTC (rev 287826)
+++ trunk/Source/WebCore/style/StyleResolver.cpp 2022-01-09 21:19:45 UTC (rev 287827)
@@ -399,8 +399,6 @@
list.insert(WTFMove(keyframeValue));
}
}
-
- list.fillImplicitKeyframes(element, *this, elementStyle, context.parentStyle);
}
std::unique_ptr<RenderStyle> Resolver::pseudoStyleForElement(const Element& element, const PseudoElementRequest& pseudoElementRequest, const ResolutionContext& context)
