Fix handling of Timing properties due to API and CSS interactions

Web animation API calls can manipulate timing properties. The values
from API calls override their corresponding CSS properties values.
Specifically, calls to effect.updateTiming(timing_options) override any
property that is included in timing_options. Replacing the animation
effect, overrides all timing properties.

Next step: Fix handling of overridden keyframes.

Bug: 1058731
Change-Id: I351ebf535511c11da0d515bf2e24b6435247d892
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2120734
Commit-Queue: Kevin Ellis <kevers@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754749}

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

@@ -839,6 +839,10 @@ void Animation::setEffect(AnimationEffect* new_effect) {
   // Notify of a potential state change.
   NotifyProbe();
 
+  // The effect is no longer associated with CSS properties.
+  if (new_effect)
+    new_effect->SetIgnoreCssTimingProperties();
+
   // The remaining steps are for handling CSS animation and transition events.
   // Both use an event delegate to dispatch events, which must be reattached to
   // the new effect.

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

@@ -297,6 +297,11 @@ class CORE_EXPORT Animation : public EventTargetWithInlineData,
   }
   bool ReplaceStateActive() const { return replace_state_ == kActive; }
 
+  // Overridden for CSS animations to force pending animation properties to be
+  // applied. This step is required before any web animation API calls that
+  // depends on computed values.
+  virtual void FlushPendingUpdates() const {}
+
   // TODO(yigu): This is a reverse dependency between AnimationTimeline and
   // Animation. We should move the update logic once snapshotting is
   // implemented. https://crbug.com/1060578.

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

@@ -51,12 +51,45 @@ AnimationEffect::AnimationEffect(const Timing& timing,
 }
 
 void AnimationEffect::UpdateSpecifiedTiming(const Timing& timing) {
-  // FIXME: Test whether the timing is actually different?
-  timing_ = timing;
+  if (!timing_.HasTimingOverrides()) {
+    timing_ = timing;
+  } else {
+    // Style changes that are overridden due to an explicit call to
+    // AnimationEffect.updateTiming are not applied.
+    if (!timing_.HasTimingOverride(Timing::kOverrideStartDelay))
+      timing_.start_delay = timing.start_delay;
+
+    if (!timing_.HasTimingOverride(Timing::kOverrideDirection))
+      timing_.direction = timing.direction;
+
+    if (!timing_.HasTimingOverride(Timing::kOverrideDuration))
+      timing_.iteration_duration = timing.iteration_duration;
+
+    if (!timing_.HasTimingOverride(Timing::kOverrideEndDelay))
+      timing_.end_delay = timing.end_delay;
+
+    if (!timing_.HasTimingOverride(Timing::kOverideFillMode))
+      timing_.fill_mode = timing.fill_mode;
+
+    if (!timing_.HasTimingOverride(Timing::kOverrideIterationCount))
+      timing_.iteration_count = timing.iteration_count;
+
+    if (!timing_.HasTimingOverride(Timing::kOverrideIterationStart))
+      timing_.iteration_start = timing.iteration_start;
+
+    if (!timing_.HasTimingOverride(Timing::kOverrideTimingFunction))
+      timing_.timing_function = timing.timing_function;
+  }
   InvalidateAndNotifyOwner();
 }
 
+void AnimationEffect::SetIgnoreCssTimingProperties() {
+  timing_.SetTimingOverride(Timing::kOverrideAll);
+}
+
 EffectTiming* AnimationEffect::getTiming() const {
+  if (const Animation* animation = GetAnimation())
+    animation->FlushPendingUpdates();
   return SpecifiedTiming().ConvertToEffectTiming();
 }
 

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

@@ -104,6 +104,8 @@ class CORE_EXPORT AnimationEffect : public ScriptWrappable {
 
   const Timing& SpecifiedTiming() const { return timing_; }
   void UpdateSpecifiedTiming(const Timing&);
+  void SetIgnoreCssTimingProperties();
+
   EventDelegate* GetEventDelegate() { return event_delegate_; }
   void SetEventDelegate(EventDelegate* delegate) { event_delegate_ = delegate; }
 

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

@@ -57,6 +57,11 @@ class CORE_EXPORT CSSAnimation : public Animation {
     visitor->Trace(owning_element_);
   }
 
+  // Force pending animation properties to be applied, as these may alter the
+  // animation. This step is required before any web animation API calls that
+  // depends on computed values.
+  void FlushPendingUpdates() const override { FlushStyles(); }
+
  protected:
   AnimationEffect::EventDelegate* CreateEventDelegate(
       Element* target,

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

@@ -63,6 +63,21 @@ struct CORE_EXPORT Timing {
     kBackwards,
   };
 
+  // Timing properties set via AnimationEffect.updateTiming override their
+  // corresponding CSS properties.
+  enum AnimationTimingOverride {
+    kOverrideNode = 0,
+    kOverrideDirection = 1,
+    kOverrideDuration = 1 << 1,
+    kOverrideEndDelay = 1 << 2,
+    kOverideFillMode = 1 << 3,
+    kOverrideIterationCount = 1 << 4,
+    kOverrideIterationStart = 1 << 5,
+    kOverrideStartDelay = 1 << 6,
+    kOverrideTimingFunction = 1 << 7,
+    kOverrideAll = (1 << 8) - 1
+  };
+
   using FillMode = CompositorKeyframeModel::FillMode;
   using PlaybackDirection = CompositorKeyframeModel::Direction;
 
@@ -81,7 +96,8 @@ struct CORE_EXPORT Timing {
         iteration_count(1),
         iteration_duration(base::nullopt),
         direction(PlaybackDirection::NORMAL),
-        timing_function(LinearTimingFunction::Shared()) {}
+        timing_function(LinearTimingFunction::Shared()),
+        timing_overrides(kOverrideNode) {}
 
   void AssertValid() const {
     DCHECK(std::isfinite(start_delay));
@@ -116,6 +132,16 @@ struct CORE_EXPORT Timing {
 
   bool operator!=(const Timing& other) const { return !(*this == other); }
 
+  // Explicit changes to animation timing through the web animations API,
+  // override timing changes due to CSS style.
+  void SetTimingOverride(AnimationTimingOverride override) {
+    timing_overrides |= override;
+  }
+  bool HasTimingOverride(AnimationTimingOverride override) {
+    return timing_overrides & override;
+  }
+  bool HasTimingOverrides() { return timing_overrides != kOverrideNode; }
+
   double start_delay;
   double end_delay;
   FillMode fill_mode;
@@ -126,6 +152,10 @@ struct CORE_EXPORT Timing {
 
   PlaybackDirection direction;
   scoped_refptr<TimingFunction> timing_function;
+  // Mask of timing attributes that are set by calls to
+  // AnimationEffect.updateTiming. Once set, these attributes ignore changes
+  // based on the CSS style.
+  uint16_t timing_overrides;
 
   struct CalculatedTiming {
     DISALLOW_NEW();

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

@@ -164,30 +164,37 @@ bool TimingInput::Update(Timing& timing,
   if (input->hasDelay()) {
     DCHECK(std::isfinite(input->delay()));
     changed |= UpdateValueIfChanged(timing.start_delay, input->delay() / 1000);
+    timing.SetTimingOverride(Timing::kOverrideStartDelay);
   }
   if (input->hasEndDelay()) {
     DCHECK(std::isfinite(input->endDelay()));
     changed |= UpdateValueIfChanged(timing.end_delay, input->endDelay() / 1000);
+    timing.SetTimingOverride(Timing::kOverrideEndDelay);
   }
   if (input->hasFill()) {
     changed |= UpdateValueIfChanged(timing.fill_mode,
                                     Timing::StringToFillMode(input->fill()));
+    timing.SetTimingOverride(Timing::kOverideFillMode);
   }
   if (input->hasIterationStart()) {
     changed |=
         UpdateValueIfChanged(timing.iteration_start, input->iterationStart());
+    timing.SetTimingOverride(Timing::kOverrideIterationStart);
   }
   if (input->hasIterations()) {
     changed |=
         UpdateValueIfChanged(timing.iteration_count, input->iterations());
+    timing.SetTimingOverride(Timing::kOverrideIterationCount);
   }
   if (input->hasDuration()) {
     changed |= UpdateValueIfChanged(
         timing.iteration_duration, ConvertIterationDuration(input->duration()));
+    timing.SetTimingOverride(Timing::kOverrideDuration);
   }
   if (input->hasDirection()) {
     changed |= UpdateValueIfChanged(
         timing.direction, ConvertPlaybackDirection(input->direction()));
+    timing.SetTimingOverride(Timing::kOverrideDirection);
   }
   if (timing_function) {
     // We need to compare the timing functions by underlying value to see if
@@ -195,8 +202,8 @@ bool TimingInput::Update(Timing& timing,
     // UpdateValueIfChanged.
     changed |= (*timing.timing_function != *timing_function);
     timing.timing_function = timing_function;
+    timing.SetTimingOverride(Timing::kOverrideTimingFunction);
   }
-
   return changed;
 }
 

third_party/blink/web_tests/external/wpt/css/css-animations/AnimationEffect-updateTiming.tentative-expected.txt

-This is a testharness.js-based test.
-FAIL AnimationEffect.updateTiming({ duration }) causes changes to the animation-duration to be ignored assert_equals: Delay should be the value set by style expected 6000 but got 0
-FAIL AnimationEffect.updateTiming({ iterations, direction }) causes changes to the animation-iteration-count and animation-direction to be ignored assert_equals: Delay should be the value set by style expected 6000 but got 0
-FAIL AnimationEffect.updateTiming({ delay, fill }) causes changes to the animation-delay and animation-fill-mode to be ignored assert_equals: Iterations should be the value set by style expected 6 but got 1
-FAIL AnimationEffect.updateTiming() does override to changes from animation-* properties if there is an error assert_equals: Duration should be the value set by style expected 4000 but got 100000
-PASS AnimationEffect properties that do not map to animation-* properties should not be changed when animation-* style is updated
-Harness: the test ran to completion.
-

third_party/blink/web_tests/external/wpt/css/css-animations/CSSAnimation-effect.tentative-expected.txt

+This is a testharness.js-based test.
+PASS Setting a null effect on a running animation fires an animationend event
+PASS Replacing an animation's effect with an effect that targets a different property should update both properties
+PASS Replacing an animation's effect with a shorter one that should have already finished, the animation finishes immediately
+PASS A play-pending animation's effect whose effect is replaced still exits the pending state
+PASS CSS animation events are dispatched at the original element even after setting an effect with a different target element
+PASS After replacing a finished animation's effect with a longer one it fires an animationstart event
+FAIL Replacing the effect of a CSSAnimation causes subsequent changes to corresponding animation-* properties to be ignored assert_equals: keyframes should be the value set by the API expected (string) "200px" but got (undefined) undefined
+Harness: the test ran to completion.
+