Fix detection of size changes for a composited transform.

A width / height mismatch is what triggered the problem in
crbug.com/1149012 which led to the revert of enabling compositing of
relative transforms; however it exposed an even deeper problem. We were
unconditionally restarting the animation when we noticed there was a
size dependency. This will cause the start of the composited animation
to lag one frame behind a main frame animation.  If the composited
animation has a size dependency  on a value that is continuously
changing on the main thread then were unable to start the animation and
it remained locked in a play pending state.  We only need to restart the
animation if it has already started running and the size dependency
changes.  Note that with the fix, the animation will still be restarting
as the value changes in main, but the next restart won't trigger until
the previous start operation has completed thereby preventing stalling
of the animation.

Bug: 389359
Change-Id: Ifcde6e42089b3f8bced382440de92594de24ba93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2614365
Commit-Queue: Kevin Ellis <kevers@chromium.org>
Reviewed-by: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841524}

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

@@ -231,6 +231,38 @@ class AnimationAnimationTestNoCompositing : public RenderingTest {
 
 class AnimationAnimationTestCompositing
     : public AnimationAnimationTestNoCompositing {
+ public:
+  Animation* CreateAnimation(CSSPropertyID property_id,
+                             String from,
+                             String to) {
+    Timing timing;
+    timing.iteration_duration = AnimationTimeDelta::FromSecondsD(30);
+
+    Persistent<StringKeyframe> start_keyframe =
+        MakeGarbageCollected<StringKeyframe>();
+    start_keyframe->SetCSSPropertyValue(
+        property_id, from, SecureContextMode::kInsecureContext, nullptr);
+    Persistent<StringKeyframe> end_keyframe =
+        MakeGarbageCollected<StringKeyframe>();
+    end_keyframe->SetCSSPropertyValue(
+        property_id, to, SecureContextMode::kInsecureContext, nullptr);
+
+    StringKeyframeVector keyframes;
+    keyframes.push_back(start_keyframe);
+    keyframes.push_back(end_keyframe);
+
+    Element* element = GetElementById("target");
+    auto* model = MakeGarbageCollected<StringKeyframeEffectModel>(keyframes);
+
+    NonThrowableExceptionState exception_state;
+    DocumentTimeline* timeline =
+        MakeGarbageCollected<DocumentTimeline>(&GetDocument());
+    return Animation::Create(
+        MakeGarbageCollected<KeyframeEffect>(element, model, timing), timeline,
+        exception_state);
+  }
+
+ private:
   void SetUp() override {
     EnableCompositing();
     AnimationAnimationTestNoCompositing::SetUp();
@@ -1411,39 +1443,150 @@ TEST_F(AnimationAnimationTestCompositing, BackgroundColorComposited) {
     </div>
   )HTML");
 
-  // Create KeyframeEffect
-  Timing timing;
-  timing.iteration_duration = AnimationTimeDelta::FromSecondsD(30);
+  Animation* animation =
+      CreateAnimation(CSSPropertyID::kBackgroundColor, "red", "green");
 
-  Persistent<StringKeyframe> start_keyframe =
-      MakeGarbageCollected<StringKeyframe>();
-  start_keyframe->SetCSSPropertyValue(CSSPropertyID::kBackgroundColor, "red",
-                                      SecureContextMode::kInsecureContext,
-                                      nullptr);
-  Persistent<StringKeyframe> end_keyframe =
-      MakeGarbageCollected<StringKeyframe>();
-  end_keyframe->SetCSSPropertyValue(CSSPropertyID::kBackgroundColor, "green",
-                                    SecureContextMode::kInsecureContext,
-                                    nullptr);
+  UpdateAllLifecyclePhasesForTest();
+  animation->play();
+  EXPECT_EQ(animation->CheckCanStartAnimationOnCompositor(nullptr),
+            CompositorAnimations::kNoFailure);
+}
 
-  StringKeyframeVector keyframes;
-  keyframes.push_back(start_keyframe);
-  keyframes.push_back(end_keyframe);
+// crbug.com/1149012
+// Regression test to ensure proper restart logic for composited animations on
+// relative transforms after a size change. In this test, the transform depends
+// on the width and height of the box and a change to either triggers a restart
+// of the animation if running.
+TEST_F(AnimationAnimationTestCompositing,
+       RestartCompositedAnimationOnSizeChange) {
+  // TODO(crbug.com/389359): Remove forced feature enabling once on by
+  // default.
+  ScopedCompositeRelativeKeyframesForTest composite_relative_keyframes(true);
+  SetBodyInnerHTML(R"HTML(
+    <div id ="target"
+         style="width: 100px; height: 200px; will-change: transform">
+    </div>
+  )HTML");
 
-  Element* element = GetElementById("target");
-  auto* model = MakeGarbageCollected<StringKeyframeEffectModel>(keyframes);
-
-  NonThrowableExceptionState exception_state;
-  DocumentTimeline* timeline =
-      MakeGarbageCollected<DocumentTimeline>(&GetDocument());
-  Animation* animation = Animation::Create(
-      MakeGarbageCollected<KeyframeEffect>(element, model, timing), timeline,
-      exception_state);
+  Animation* animation = CreateAnimation(
+      CSSPropertyID::kTransform, "translate(100%, 100%)", "translate(0%, 0%)");
 
   UpdateAllLifecyclePhasesForTest();
   animation->play();
+  KeyframeEffect* keyframe_effect =
+      DynamicTo<KeyframeEffect>(animation->effect());
+  ASSERT_TRUE(keyframe_effect);
+
   EXPECT_EQ(animation->CheckCanStartAnimationOnCompositor(nullptr),
             CompositorAnimations::kNoFailure);
+
+  GetDocument().GetPendingAnimations().Update(nullptr, true);
+  EXPECT_TRUE(animation->HasActiveAnimationsOnCompositor());
+
+  // Kick the animation out of the play-pending state.
+  animation->setStartTime(0);
+
+  // No size change and animation does not require a restart.
+  keyframe_effect->UpdateBoxSizeAndCheckTransformAxisAlignment(
+      FloatSize(100, 200));
+  EXPECT_TRUE(animation->HasActiveAnimationsOnCompositor());
+
+  // Restart animation on a width change.
+  keyframe_effect->UpdateBoxSizeAndCheckTransformAxisAlignment(
+      FloatSize(200, 200));
+  EXPECT_FALSE(animation->HasActiveAnimationsOnCompositor());
+
+  GetDocument().GetPendingAnimations().Update(nullptr, true);
+  EXPECT_TRUE(animation->HasActiveAnimationsOnCompositor());
+
+  // Restart animation on a height change.
+  keyframe_effect->UpdateBoxSizeAndCheckTransformAxisAlignment(
+      FloatSize(200, 300));
+  EXPECT_FALSE(animation->HasActiveAnimationsOnCompositor());
+}
+
+// crbug.com/1149012
+// Regression test to ensure proper restart logic for composited animations on
+// relative transforms after a size change. In this test, the transform only
+// depends on width and a change to the height does not trigger a restart.
+TEST_F(AnimationAnimationTestCompositing,
+       RestartCompositedAnimationOnWidthChange) {
+  // TODO(crbug.com/389359): Remove forced feature enabling once on by
+  // default.
+  ScopedCompositeRelativeKeyframesForTest composite_relative_keyframes(true);
+  SetBodyInnerHTML(R"HTML(
+    <div id ="target"
+         style="width: 100px; height: 200px; will-change: transform">
+    </div>
+  )HTML");
+
+  animation = CreateAnimation(CSSPropertyID::kTransform, "translateX(100%)",
+                              "translateX(0%)");
+
+  UpdateAllLifecyclePhasesForTest();
+  animation->play();
+  KeyframeEffect* keyframe_effect =
+      DynamicTo<KeyframeEffect>(animation->effect());
+  ASSERT_TRUE(keyframe_effect);
+
+  GetDocument().GetPendingAnimations().Update(nullptr, true);
+  EXPECT_TRUE(animation->HasActiveAnimationsOnCompositor());
+  keyframe_effect->UpdateBoxSizeAndCheckTransformAxisAlignment(
+      FloatSize(100, 200));
+  animation->setStartTime(0);
+
+  // Transform is not height dependent and a change to the height does not force
+  // an animation restart.
+  keyframe_effect->UpdateBoxSizeAndCheckTransformAxisAlignment(
+      FloatSize(100, 300));
+  EXPECT_TRUE(animation->HasActiveAnimationsOnCompositor());
+
+  // Width change forces a restart.
+  keyframe_effect->UpdateBoxSizeAndCheckTransformAxisAlignment(
+      FloatSize(200, 300));
+  EXPECT_FALSE(animation->HasActiveAnimationsOnCompositor());
+}
+
+// crbug.com/1149012
+// Regression test to ensure proper restart logic for composited animations on
+// relative transforms after a size change.  In this test, the transition only
+// affects height and a change to the width does not trigger a restart.
+TEST_F(AnimationAnimationTestCompositing,
+       RestartCompositedAnimationOnHeightChange) {
+  // TODO(crbug.com/389359): Remove forced feature enabling once on by
+  // default.
+  ScopedCompositeRelativeKeyframesForTest composite_relative_keyframes(true);
+  SetBodyInnerHTML(R"HTML(
+    <div id ="target"
+         style="width: 100px; height: 200px; will-change: transform">
+    </div>
+  )HTML");
+
+  animation = CreateAnimation(CSSPropertyID::kTransform, "translateY(100%)",
+                              "translateY(0%)");
+
+  UpdateAllLifecyclePhasesForTest();
+  animation->play();
+  KeyframeEffect* keyframe_effect =
+      DynamicTo<KeyframeEffect>(animation->effect());
+  ASSERT_TRUE(keyframe_effect);
+
+  GetDocument().GetPendingAnimations().Update(nullptr, true);
+  EXPECT_TRUE(animation->HasActiveAnimationsOnCompositor());
+  keyframe_effect->UpdateBoxSizeAndCheckTransformAxisAlignment(
+      FloatSize(100, 200));
+  animation->setStartTime(0);
+
+  // Transform is not width dependent and a change to the width does not force
+  // an animation restart.
+  keyframe_effect->UpdateBoxSizeAndCheckTransformAxisAlignment(
+      FloatSize(300, 200));
+  EXPECT_TRUE(animation->HasActiveAnimationsOnCompositor());
+
+  // Height change forces a restart.
+  keyframe_effect->UpdateBoxSizeAndCheckTransformAxisAlignment(
+      FloatSize(300, 400));
+  EXPECT_FALSE(animation->HasActiveAnimationsOnCompositor());
 }
 
 TEST_F(AnimationAnimationTestCompositing,

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

@@ -503,12 +503,10 @@ bool KeyframeEffect::UpdateBoxSizeAndCheckTransformAxisAlignment(
     if (effect_target_size_) {
       if ((size_dependencies & TransformOperation::kDependsWidth) &&
           (effect_target_size_->Width() != box_size.Width()))
-        GetAnimation()->RestartAnimationOnCompositor();
+        RestartRunningAnimationOnCompositor();
       else if ((size_dependencies & TransformOperation::kDependsHeight) &&
-               (effect_target_size_->Width() != box_size.Height()))
-        GetAnimation()->RestartAnimationOnCompositor();
-    } else if (size_dependencies) {
-      GetAnimation()->RestartAnimationOnCompositor();
+               (effect_target_size_->Height() != box_size.Height()))
+        RestartRunningAnimationOnCompositor();
     }
   }
 
@@ -517,6 +515,19 @@ bool KeyframeEffect::UpdateBoxSizeAndCheckTransformAxisAlignment(
   return preserves_axis_alignment;
 }
 
+void KeyframeEffect::RestartRunningAnimationOnCompositor() {
+  Animation* animation = GetAnimation();
+  if (!animation)
+    return;
+
+  // No need to to restart an animation that is in the process of starting up,
+  // paused or idle.
+  if (!animation->startTime())
+    return;
+
+  animation->RestartAnimationOnCompositor();
+}
+
 bool KeyframeEffect::IsIdentityOrTranslation() const {
   static const auto** properties = TransformProperties();
   for (size_t i = 0; i < num_transform_properties; i++) {

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

@@ -166,6 +166,7 @@ class CORE_EXPORT KeyframeEffect final : public AnimationEffect {
       AnimationTimeDelta time_to_next_iteration) const override;
   bool HasIncompatibleStyle() const;
   bool HasMultipleTransformProperties() const;
+  void RestartRunningAnimationOnCompositor();
 
   Member<Element> effect_target_;
   Member<Element> target_element_;