Report computed property values for CSS animation keyframes

CSS animations are to return computed values for the keyframe properties
rather than values directly from the keyframes rule. In particular,
variable references are to be resolved. The spec calls for resolving at
the time the animation is generated but acknowledges that the extraction
can be done lazily as an optimization.

https://drafts.csswg.org/css-animations-2/#keyframes

New tests failures are the result of not using the correct canonical
form during property serialization.

Bug: 1070627
Change-Id: If6ad836147c15feaf4d0374f8798e75fc390878b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2194279
Commit-Queue: Kevin Ellis <kevers@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812803}

third_party/blink/renderer/core/animation/css/css_keyframe_effect_model.cc

@@ -8,7 +8,10 @@
 #include "third_party/blink/renderer/core/animation/animation_utils.h"
 #include "third_party/blink/renderer/core/animation/property_handle.h"
 #include "third_party/blink/renderer/core/animation/string_keyframe.h"
+#include "third_party/blink/renderer/core/css/properties/computed_style_utils.h"
 #include "third_party/blink/renderer/core/css/properties/css_property.h"
+#include "third_party/blink/renderer/core/css/resolver/style_resolver.h"
+#include "third_party/blink/renderer/core/dom/node_computed_style.h"
 
 namespace blink {
 
@@ -41,6 +44,11 @@ void AddMissingProperties(const MissingPropertyValueMap& property_map,
                           const PropertyHandleSet& keyframe_properties,
                           StringKeyframe* keyframe) {
   for (const auto& property : all_properties) {
+    // At present, custom properties are to be excluded from the keyframes.
+    // https://github.com/w3c/csswg-drafts/issues/5126.
+    if (property.IsCSSCustomProperty())
+      continue;
+
     if (keyframe_properties.Contains(property))
       continue;
 
@@ -48,15 +56,35 @@ void AddMissingProperties(const MissingPropertyValueMap& property_map,
         AnimationInputHelpers::PropertyHandleToKeyframeAttribute(property);
     if (property_map.Contains(property_name)) {
       const String& value = property_map.at(property_name);
-      if (property.IsCSSCustomProperty()) {
-        keyframe->SetCSSPropertyValue(property.CustomPropertyName(), value,
-                                      SecureContextMode::kInsecureContext,
-                                      nullptr);
-      } else {
-        keyframe->SetCSSPropertyValue(
-            property.GetCSSProperty().PropertyID(), value,
-            SecureContextMode::kInsecureContext, nullptr);
-      }
+      keyframe->SetCSSPropertyValue(property.GetCSSProperty().PropertyID(),
+                                    value, SecureContextMode::kInsecureContext,
+                                    nullptr);
+    }
+  }
+}
+
+void ResolveComputedValues(Element* element, StringKeyframe* keyframe) {
+  DCHECK(element);
+  // Styles are flushed when getKeyframes is called on a CSS animation.
+  DCHECK(element->GetComputedStyle());
+  for (const auto& property : keyframe->Properties()) {
+    if (property.IsCSSCustomProperty()) {
+      // At present, custom properties are to be excluded from the keyframes.
+      // https://github.com/w3c/csswg-drafts/issues/5126.
+      // TODO(csswg/issues/5126): Revisit once issue regarding inclusion of
+      // custom properties is resolved. Perhaps registered should likely be
+      // included since they can be animated in Blink. Pruning unregistered
+      // variables seems justifiable.
+      keyframe->RemoveCustomCSSProperty(property);
+    } else if (property.IsCSSProperty()) {
+      const CSSValue& value = keyframe->CssPropertyValue(property);
+      const CSSPropertyName property_name =
+          property.IsCSSCustomProperty()
+              ? CSSPropertyName(property.CustomPropertyName())
+              : CSSPropertyName(property.GetCSSProperty().PropertyID());
+      const CSSValue* computed_value =
+          StyleResolver::ComputeValue(element, property_name, value);
+      keyframe->SetCSSPropertyValue(property.GetCSSProperty(), *computed_value);
     }
   }
 }
@@ -83,14 +111,16 @@ CssKeyframeEffectModel::GetComputedKeyframes(Element* element) {
     Keyframe* keyframe = keyframes[i];
     // TODO(crbug.com/1070627): Use computed values, prune variable references,
     // and convert logical properties to physical properties.
-    computed_keyframes.push_back(keyframe->Clone());
+    StringKeyframe* computed_keyframe = To<StringKeyframe>(keyframe->Clone());
+    ResolveComputedValues(element, computed_keyframe);
+    computed_keyframes.push_back(computed_keyframe);
     double offset = computed_offsets[i];
     if (offset == 0) {
-      for (const auto& property : keyframe->Properties()) {
+      for (const auto& property : computed_keyframe->Properties()) {
         from_properties.insert(property);
       }
     } else if (offset == 1) {
-      for (const auto& property : keyframe->Properties()) {
+      for (const auto& property : computed_keyframe->Properties()) {
         to_properties.insert(property);
       }
     }

third_party/blink/renderer/core/animation/string_keyframe.cc

@@ -145,6 +145,13 @@ void StringKeyframe::SetCSSPropertyValue(const CSSProperty& property,
   InvalidateCssPropertyMap();
 }
 
+void StringKeyframe::RemoveCustomCSSProperty(const PropertyHandle& property) {
+  DCHECK(property.IsCSSCustomProperty());
+  if (css_property_map_)
+    css_property_map_->RemoveProperty(property.CustomPropertyName());
+  input_properties_.erase(property);
+}
+
 void StringKeyframe::SetPresentationAttributeValue(
     const CSSProperty& property,
     const String& value,

third_party/blink/renderer/core/animation/string_keyframe.h

@@ -42,6 +42,7 @@ class CORE_EXPORT StringKeyframe : public Keyframe {
       SecureContextMode,
       StyleSheetContents*);
   void SetCSSPropertyValue(const CSSProperty&, const CSSValue&);
+  void RemoveCustomCSSProperty(const PropertyHandle& property);
 
   void SetPresentationAttributeValue(const CSSProperty&,
                                      const String& value,

third_party/blink/renderer/core/css/resolver/style_resolver.cc

@@ -50,6 +50,7 @@
 #include "third_party/blink/renderer/core/css/font_face.h"
 #include "third_party/blink/renderer/core/css/page_rule_collector.h"
 #include "third_party/blink/renderer/core/css/part_names.h"
+#include "third_party/blink/renderer/core/css/properties/computed_style_utils.h"
 #include "third_party/blink/renderer/core/css/properties/css_property.h"
 #include "third_party/blink/renderer/core/css/properties/css_property_ref.h"
 #include "third_party/blink/renderer/core/css/resolver/match_result.h"
@@ -1614,6 +1615,31 @@ bool StyleResolver::CanReuseBaseComputedStyle(const StyleResolverState& state) {
   return true;
 }
 
+const CSSValue* StyleResolver::ComputeValue(
+    Element* element,
+    const CSSPropertyName& property_name,
+    const CSSValue& value) {
+  const ComputedStyle* base_style = element->GetComputedStyle();
+  StyleResolverState state(element->GetDocument(), *element);
+  STACK_UNINITIALIZED StyleCascade cascade(state);
+  state.SetStyle(ComputedStyle::Clone(*base_style));
+  auto* set =
+      MakeGarbageCollected<MutableCSSPropertyValueSet>(state.GetParserMode());
+  if (property_name.IsCustomProperty()) {
+    set->SetProperty(CSSPropertyValue(property_name, value));
+  } else {
+    set->SetProperty(property_name.Id(), value);
+  }
+  cascade.MutableMatchResult().FinishAddingUARules();
+  cascade.MutableMatchResult().FinishAddingUserRules();
+  cascade.MutableMatchResult().AddMatchedProperties(set);
+  cascade.Apply();
+
+  CSSPropertyRef property_ref(property_name, element->GetDocument());
+  return ComputedStyleUtils::ComputedPropertyValue(property_ref.GetProperty(),
+                                                   *state.Style());
+}
+
 scoped_refptr<ComputedStyle> StyleResolver::StyleForInterpolations(
     Element& element,
     ActiveInterpolationsMap& interpolations) {

third_party/blink/renderer/core/css/resolver/style_resolver.h

@@ -139,6 +139,10 @@ class CORE_EXPORT StyleResolver final : public GarbageCollected<StyleResolver> {
 
   static bool CanReuseBaseComputedStyle(const StyleResolverState& state);
 
+  static const CSSValue* ComputeValue(Element* element,
+                                      const CSSPropertyName&,
+                                      const CSSValue&);
+
   scoped_refptr<ComputedStyle> StyleForInterpolations(
       Element& element,
       ActiveInterpolationsMap& animations);

third_party/blink/renderer/core/css/resolver/style_resolver_test.cc

@@ -9,6 +9,7 @@
 #include "third_party/blink/renderer/core/animation/document_timeline.h"
 #include "third_party/blink/renderer/core/animation/element_animations.h"
 #include "third_party/blink/renderer/core/css/css_image_value.h"
+#include "third_party/blink/renderer/core/css/css_test_helpers.h"
 #include "third_party/blink/renderer/core/css/css_value_list.h"
 #include "third_party/blink/renderer/core/css/properties/computed_style_utils.h"
 #include "third_party/blink/renderer/core/css/properties/css_property_ref.h"
@@ -812,4 +813,55 @@ TEST_F(StyleResolverTest, EnsureComputedStyleSlotFallback) {
             fallback_style->VisitedDependentColor(GetCSSPropertyColor()));
 }
 
+TEST_F(StyleResolverTest, ComputeValueStandardProperty) {
+  GetDocument().body()->setInnerHTML(R"HTML(
+    <style>
+      #target { --color: green }
+    </style>
+    <div id="target"></div>
+  )HTML");
+  UpdateAllLifecyclePhasesForTest();
+
+  Element* target = GetDocument().getElementById("target");
+  ASSERT_TRUE(target);
+
+  // Unable to parse a variable reference with css_test_helpers::ParseLonghand.
+  CSSPropertyID property_id = CSSPropertyID::kColor;
+  auto* set =
+      MakeGarbageCollected<MutableCSSPropertyValueSet>(kHTMLStandardMode);
+  MutableCSSPropertyValueSet::SetResult result = set->SetProperty(
+      property_id, "var(--color)", false, SecureContextMode::kInsecureContext,
+      /*style_sheet_contents=*/nullptr);
+  ASSERT_TRUE(result.did_parse);
+  const CSSValue* parsed_value = set->GetPropertyCSSValue(property_id);
+  ASSERT_TRUE(parsed_value);
+  const CSSValue* computed_value = StyleResolver::ComputeValue(
+      target, CSSPropertyName(property_id), *parsed_value);
+  ASSERT_TRUE(computed_value);
+  EXPECT_EQ("rgb(0, 128, 0)", computed_value->CssText());
+}
+
+TEST_F(StyleResolverTest, ComputeValueCustomProperty) {
+  GetDocument().body()->setInnerHTML(R"HTML(
+    <style>
+      #target { --color: green }
+    </style>
+    <div id="target"></div>
+  )HTML");
+  UpdateAllLifecyclePhasesForTest();
+
+  Element* target = GetDocument().getElementById("target");
+  ASSERT_TRUE(target);
+
+  AtomicString custom_property_name = "--color";
+  const CSSValue* parsed_value = css_test_helpers::ParseLonghand(
+      GetDocument(), CustomProperty(custom_property_name, GetDocument()),
+      "blue");
+  ASSERT_TRUE(parsed_value);
+  const CSSValue* computed_value = StyleResolver::ComputeValue(
+      target, CSSPropertyName(custom_property_name), *parsed_value);
+  ASSERT_TRUE(computed_value);
+  EXPECT_EQ("blue", computed_value->CssText());
+}
+
 }  // namespace blink

third_party/blink/web_tests/external/wpt/css/css-animations/KeyframeEffect-getKeyframes.tentative-expected.txt

@@ -15,13 +15,13 @@ PASS KeyframeEffect.getKeyframes() returns expected frames for an animation with
 PASS KeyframeEffect.getKeyframes() returns expected frames for an animation with multiple keyframes for the same time and with different easing functions
 PASS KeyframeEffect.getKeyframes() returns expected frames for an animation with multiple keyframes for the same time and with different but equivalent easing functions
 PASS KeyframeEffect.getKeyframes() returns expected frames for overlapping keyframes
-PASS KeyframeEffect.getKeyframes() returns expected values for animations with filter properties and missing keyframes
+FAIL KeyframeEffect.getKeyframes() returns expected values for animations with filter properties and missing keyframes assert_equals: value for 'filter' on Keyframe #1 should match expected "blur(5px) sepia(60%) saturate(30%)" but got "blur(5px) sepia(0.6) saturate(0.3)"
 PASS KeyframeEffect.getKeyframes() returns expected values for animation with drop-shadow of filter property
 PASS KeyframeEffect.getKeyframes() returns expected values for animations with text-shadow properties and missing keyframes
 PASS KeyframeEffect.getKeyframes() returns expected values for animations with background-size properties and missing keyframes
-FAIL KeyframeEffect.getKeyframes() returns expected values for animations with CSS variables as keyframe values assert_equals: value for 'transform' on Keyframe #1 should match expected "translate(100px)" but got "translate(var(--var-100px), 0) "
-FAIL KeyframeEffect.getKeyframes() returns expected values for animations with CSS variables as keyframe values in a shorthand property assert_equals: value for 'marginBottom' on Keyframe #1 should match expected "100px" but got ""
-FAIL KeyframeEffect.getKeyframes() returns expected values for animations with a CSS variable which is overriden by the value in keyframe assert_array_equals: properties on Keyframe #0 should match lengths differ, expected array ["color", "composite", "computedOffset", "easing", "offset"] length 5, got ["--end-color", "color", "composite", "computedOffset", "easing", "offset"] length 6
+FAIL KeyframeEffect.getKeyframes() returns expected values for animations with CSS variables as keyframe values assert_equals: value for 'transform' on Keyframe #1 should match expected "translate(100px)" but got "translate(100px, 0px)"
+PASS KeyframeEffect.getKeyframes() returns expected values for animations with CSS variables as keyframe values in a shorthand property
+PASS KeyframeEffect.getKeyframes() returns expected values for animations with a CSS variable which is overriden by the value in keyframe
 FAIL KeyframeEffect.getKeyframes() returns expected values for animations with only custom property in a keyframe assert_equals: value for 'transform' on Keyframe #0 should match expected "translate(100px)" but got "translate(100px, 0px)"
 PASS KeyframeEffect.getKeyframes() reflects changes to @keyframes rules
 Harness: the test ran to completion.