Base style should respond to animations modified with setKeyframes

When animating font-affecting properties (e.g. font-size) while the
base style contains font-relative units (e.g. em), we can not use the
base computed style optimization, since the font-relative units in the
base must respond to the font animation.

A has_font_affecting_animation_ flag was recently added to
ElementAnimations to assist in disabling the optimization under these
circumstances. However, that was added with an insufficient
understanding of ElementAnimation's lifetime, and hence this flag
doesn't really work properly.

For example, if we have an animation that initially doesn't affect the
font, but then suddenly affects the font after all via setKeyframes,
we would paint one incorrect frame before discovering that the font
is now affected, and then (for frame #2 and subsequent) we'd be able
to disable the optimization.

This CL instead checks if the EffectStack affects the font when we're
considering the base computed style for use.

Bug: 437689
Change-Id: If07f1e82559673433be0a80d2c3edea1c1a5165a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2139662Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759197}

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

@@ -36,6 +36,31 @@ void SetV8ObjectPropertyAsNumber(v8::Isolate* isolate,
       .ToChecked();
 }
 
+KeyframeEffect* CreateSimpleKeyframeEffectForTest(Element* target,
+                                                  CSSPropertyID property,
+                                                  String value_start,
+                                                  String value_end) {
+  Timing timing;
+  timing.iteration_duration = AnimationTimeDelta::FromSecondsD(1000);
+
+  StringKeyframe* start_keyframe = MakeGarbageCollected<StringKeyframe>();
+  start_keyframe->SetOffset(0.0);
+  start_keyframe->SetCSSPropertyValue(
+      property, value_start, SecureContextMode::kSecureContext, nullptr);
+
+  StringKeyframe* end_keyframe = MakeGarbageCollected<StringKeyframe>();
+  end_keyframe->SetOffset(1.0);
+  end_keyframe->SetCSSPropertyValue(property, value_end,
+                                    SecureContextMode::kSecureContext, nullptr);
+
+  StringKeyframeVector keyframes;
+  keyframes.push_back(start_keyframe);
+  keyframes.push_back(end_keyframe);
+
+  auto* model = MakeGarbageCollected<StringKeyframeEffectModel>(keyframes);
+  return MakeGarbageCollected<KeyframeEffect>(target, model, timing);
+}
+
 void EnsureInterpolatedValueCached(const ActiveInterpolations& interpolations,
                                    Document& document,
                                    Element* element) {

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

@@ -14,6 +14,7 @@ namespace blink {
 
 class Document;
 class Element;
+class KeyframeEffect;
 
 void SetV8ObjectPropertyAsString(v8::Isolate*,
                                  v8::Local<v8::Object>,
@@ -24,6 +25,14 @@ void SetV8ObjectPropertyAsNumber(v8::Isolate*,
                                  const StringView& name,
                                  double value);
 
+// Creates a KeyframeEffect with two keyframes corresponding to
+// value_start (offset 0.0) and value_end (offset 1.0). Default blink::Timing
+// values are used, except for iteration_duration which is set to 1000ms.
+KeyframeEffect* CreateSimpleKeyframeEffectForTest(Element*,
+                                                  CSSPropertyID,
+                                                  String value_start,
+                                                  String value_end);
+
 // Ensures that a set of interpolations actually computes and caches their
 // internal interpolated value, so that tests can retrieve them.
 //

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

@@ -1056,6 +1056,12 @@ bool IsCustomPropertyHandle(const PropertyHandle& property) {
   return property.IsCSSCustomProperty();
 }
 
+bool IsFontAffectingPropertyHandle(const PropertyHandle& property) {
+  if (property.IsCSSCustomProperty() || !property.IsCSSProperty())
+    return false;
+  return property.GetCSSProperty().AffectsFont();
+}
+
 // TODO(alancutter): CSS properties and presentation attributes may have
 // identical effects. By grouping them in the same set we introduce a bug where
 // arbitrary hash iteration will determine the order the apply in and thus which
@@ -1459,6 +1465,13 @@ bool CSSAnimations::IsAnimatingCustomProperties(
              IsCustomPropertyHandle);
 }
 
+bool CSSAnimations::IsAnimatingFontAffectingProperties(
+    const ElementAnimations* element_animations) {
+  return element_animations &&
+         element_animations->GetEffectStack().AffectsProperties(
+             IsFontAffectingPropertyHandle);
+}
+
 bool CSSAnimations::IsAnimatingRevert(
     const ElementAnimations* element_animations) {
   return element_animations && element_animations->GetEffectStack().HasRevert();

third_party/blink/renderer/core/animation/css/css_animations.h

@@ -61,6 +61,7 @@ class CORE_EXPORT CSSAnimations final {
   static bool IsAnimationAffectingProperty(const CSSProperty&);
   static bool IsAffectedByKeyframesFromScope(const Element&, const TreeScope&);
   static bool IsAnimatingCustomProperties(const ElementAnimations*);
+  static bool IsAnimatingFontAffectingProperties(const ElementAnimations*);
   static bool IsAnimatingRevert(const ElementAnimations*);
   static void CalculateAnimationUpdate(CSSAnimationUpdate&,
                                        const Element* animating_element,

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

@@ -114,13 +114,7 @@ void ElementAnimations::Trace(Visitor* visitor) {
 }
 
 bool ElementAnimations::IsBaseComputedStyleUsable() const {
-  if (has_important_overrides_)
-    return false;
-  if (has_font_affecting_animation_ && base_computed_style_ &&
-      base_computed_style_->HasFontRelativeUnits()) {
-    return false;
-  }
-  return true;
+  return !has_important_overrides_;
 }
 
 const ComputedStyle* ElementAnimations::BaseComputedStyle() const {

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

@@ -80,7 +80,6 @@ class CORE_EXPORT ElementAnimations final
     animation_style_change_ = animation_style_change;
   }
   void SetHasImportantOverrides() { has_important_overrides_ = true; }
-  void SetHasFontAffectingAnimation() { has_font_affecting_animation_ = true; }
 
   const ComputedStyle* BaseComputedStyle() const;
   void UpdateBaseComputedStyle(const ComputedStyle*);
@@ -111,10 +110,6 @@ class CORE_EXPORT ElementAnimations final
   // optimization, since we have no way of knowing the cascade origins used
   // to construct the various parts of the base style.
   bool has_important_overrides_ = false;
-  // If a font-affecting property is undergoing an animation, we can't use
-  // the base computed style optimization, because font-relative units
-  // (such as 'em') present in the base should respond to the animation.
-  bool has_font_affecting_animation_ = false;
   scoped_refptr<ComputedStyle> base_computed_style_;
 
   // CSSAnimations checks if a style change is due to animation.

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

@@ -383,9 +383,6 @@ void StyleCascade::ApplyInterpolation(
     To<TransitionInterpolation>(interpolation).Apply(state_);
   }
 
-  if (property.AffectsFont())
-    state_.SetHasFontAffectingAnimation();
-
   // Applying a color property interpolation will also unconditionally apply
   // the -internal-visited- counterpart (see CSSColorInterpolationType::
   // ApplyStandardPropertyValue). To make sure !important rules in :visited

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

@@ -118,8 +118,6 @@ void SetAnimationUpdateIfNeeded(StyleResolverState& state, Element& element) {
         state.AnimationUpdate());
     if (state.HasImportantOverrides())
       element_animations.SetHasImportantOverrides();
-    if (state.HasFontAffectingAnimation())
-      element_animations.SetHasFontAffectingAnimation();
   }
 }
 
@@ -788,6 +786,14 @@ static const ComputedStyle* CachedAnimationBaseComputedStyle(
     return nullptr;
   }
 
+  if (CSSAnimations::IsAnimatingFontAffectingProperties(element_animations)) {
+    state.SetHasFontAffectingAnimation();
+    if (element_animations->BaseComputedStyle() &&
+        element_animations->BaseComputedStyle()->HasFontRelativeUnits()) {
+      return nullptr;
+    }
+  }
+
   return element_animations->BaseComputedStyle();
 }
 
@@ -798,7 +804,9 @@ static void UpdateAnimationBaseComputedStyle(StyleResolverState& state) {
   ElementAnimations* element_animations =
       state.GetAnimatingElement()->GetElementAnimations();
   if (element_animations) {
-    if (state.IsAnimatingCustomProperties() || state.IsAnimatingRevert()) {
+    if (state.IsAnimatingCustomProperties() || state.IsAnimatingRevert() ||
+        (state.HasFontAffectingAnimation() &&
+         state.Style()->HasFontRelativeUnits())) {
       element_animations->ClearBaseComputedStyle();
     } else {
       element_animations->UpdateBaseComputedStyle(state.Style());

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

@@ -5,7 +5,10 @@
 #include "third_party/blink/renderer/core/css/resolver/style_resolver.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/renderer/core/animation/animation_test_helper.h"
+#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/properties/css_property_ref.h"
 #include "third_party/blink/renderer/core/css/style_engine.h"
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/dom/node_computed_style.h"
@@ -27,6 +30,14 @@ class StyleResolverTest : public PageTestBase {
     return style;
   }
 
+  String ComputedValue(String name, const ComputedStyle& style) {
+    CSSPropertyRef ref(name, GetDocument());
+    DCHECK(ref.IsValid());
+    return ref.GetProperty()
+        .CSSValueFromComputedStyle(style, nullptr, false)
+        ->CssText();
+  }
+
  protected:
 };
 
@@ -112,19 +123,20 @@ TEST_F(StyleResolverTest, HasEmUnits) {
 TEST_F(StyleResolverTest, BasePresentIfFontRelativeUnitsAbsent) {
   GetDocument().documentElement()->setInnerHTML("<div id=div>Test</div>");
   UpdateAllLifecyclePhasesForTest();
-
   Element* div = GetDocument().getElementById("div");
-  StyleResolver* resolver = GetStyleEngine().Resolver();
-  ASSERT_TRUE(resolver);
-  ElementAnimations& animations = div->EnsureElementAnimations();
-  animations.SetAnimationStyleChange(true);
-  // We're animating a font affecting property, but we should still be able to
-  // use the base computed style optimization, since no font-relative units
-  // exist in the base.
-  animations.SetHasFontAffectingAnimation();
 
-  EXPECT_TRUE(resolver->StyleForElement(div));
-  EXPECT_TRUE(animations.BaseComputedStyle());
+  auto* effect = CreateSimpleKeyframeEffectForTest(
+      div, CSSPropertyID::kFontSize, "50px", "100px");
+  GetDocument().Timeline().Play(effect);
+  UpdateAllLifecyclePhasesForTest();
+
+  EXPECT_EQ("50px", ComputedValue("font-size", *StyleForId("div")));
+
+  div->SetNeedsAnimationStyleRecalc();
+  StyleForId("div");
+
+  ASSERT_TRUE(div->GetElementAnimations());
+  EXPECT_TRUE(div->GetElementAnimations()->BaseComputedStyle());
 }
 
 TEST_F(StyleResolverTest, NoCrashWhenAnimatingWithoutCascade) {
@@ -155,12 +167,18 @@ TEST_P(StyleResolverFontRelativeUnitTest, NoBaseIfFontRelativeUnitPresent) {
   UpdateAllLifecyclePhasesForTest();
 
   Element* div = GetDocument().getElementById("div");
-  ElementAnimations& animations = div->EnsureElementAnimations();
-  animations.SetAnimationStyleChange(true);
-  animations.SetHasFontAffectingAnimation();
+  auto* effect = CreateSimpleKeyframeEffectForTest(
+      div, CSSPropertyID::kFontSize, "50px", "100px");
+  GetDocument().Timeline().Play(effect);
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_EQ("50px", ComputedValue("font-size", *StyleForId("div")));
+
+  div->SetNeedsAnimationStyleRecalc();
+  auto computed_style = StyleForId("div");
 
-  EXPECT_TRUE(StyleForId("div")->HasFontRelativeUnits());
-  EXPECT_FALSE(animations.BaseComputedStyle());
+  EXPECT_TRUE(computed_style->HasFontRelativeUnits());
+  ASSERT_TRUE(div->GetElementAnimations());
+  EXPECT_FALSE(div->GetElementAnimations()->BaseComputedStyle());
 }
 
 TEST_P(StyleResolverFontRelativeUnitTest,
@@ -170,11 +188,18 @@ TEST_P(StyleResolverFontRelativeUnitTest,
   UpdateAllLifecyclePhasesForTest();
 
   Element* div = GetDocument().getElementById("div");
-  ElementAnimations& animations = div->EnsureElementAnimations();
-  animations.SetAnimationStyleChange(true);
+  auto* effect = CreateSimpleKeyframeEffectForTest(div, CSSPropertyID::kHeight,
+                                                   "50px", "100px");
+  GetDocument().Timeline().Play(effect);
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_EQ("50px", ComputedValue("height", *StyleForId("div")));
+
+  div->SetNeedsAnimationStyleRecalc();
+  auto computed_style = StyleForId("div");
 
-  EXPECT_TRUE(StyleForId("div")->HasFontRelativeUnits());
-  EXPECT_TRUE(animations.BaseComputedStyle());
+  EXPECT_TRUE(computed_style->HasFontRelativeUnits());
+  ASSERT_TRUE(div->GetElementAnimations());
+  EXPECT_TRUE(div->GetElementAnimations()->BaseComputedStyle());
 }
 
 INSTANTIATE_TEST_SUITE_P(All,

third_party/blink/web_tests/external/wpt/css/css-animations/animation-base-response-004.html

+<!DOCTYPE html>
+<title>Tests that base responds to font-affecting properties appearing via setKeyframes</title>
+<link rel="help" href="https://drafts.csswg.org/css-animations/">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+  #target1 {
+    font-size: 10px;
+    height: 1em;
+  }
+</style>
+<div id=target1></div>
+<script>
+  test(function() {
+    getComputedStyle(target1).height;
+
+    let animation = target1.animate([
+      { height: '50px' },
+      { height: '100px' },
+    ], {
+      duration: 1000000,
+      delay: -500000,
+      easing: 'steps(2, end)'
+    });
+
+    assert_equals(getComputedStyle(target1).height, '75px');
+
+    animation.effect.setKeyframes([
+      { fontSize: '10px' },
+      { fontSize: '20px' },
+    ]);
+
+    assert_equals(getComputedStyle(target1).height, '15px');
+  }, 'Base is responsive to font-affecting appearing via setKeyframes');
+</script>