Bug 1524480 - Add a version of KeyframeEffect::GetTargetComputedStyle that does not flush style and use it; r=hiro

A forthcoming spec change will require that Animatable.animate() and other
methods that mutate animations do not flush style:

  https://github.com/w3c/csswg-drafts/issues/3613

Bug 1525809 will add web-platform-tests for this change once it is made (and
tweak the behavior introduced in this patch if necessary).

Currently Firefox and WebKit will flush styles when calling
Animatable.animate(). This is undesirable since this method will _also_
invalidate style. As a result, if content triggers multiple animations in
a single animation frame, it will restyle every time it creates an animation.

This patch removes the style flush from a number of these methods.

In general the style flush is not necessary. For example, we don't need to flush
style before getting the computed style to pass to UpdateProperties. That's
because if there are pending style changes, then UpdateProperties will be called
with the updated style once they are applied anyway. Flushing style first means
that we may end up resolving style twice, when once would be sufficient.

For GetKeyframes() however, when used on a CSS animation, it should return
the most up-to-date style so for that call site we *do* want to flush style.

The test case added in this patch will fail without the code changes in the
patch. Specifically, we will observe 10 non-animation restyles (from the
5 animations) if we flush styles from SetKeyframes.

Differential Revision: https://phabricator.services.mozilla.com/D18916

--HG--
extra : moz-landing-system : lando

dom/animation/KeyframeEffect.cpp

@@ -112,7 +112,7 @@ void KeyframeEffect::SetComposite(const CompositeOperation& aComposite) {
   }
 
   if (mTarget) {
-    RefPtr<ComputedStyle> computedStyle = GetTargetComputedStyle();
+    RefPtr<ComputedStyle> computedStyle = GetTargetComputedStyle(Flush::None);
     if (computedStyle) {
       UpdateProperties(computedStyle);
     }
@@ -206,7 +206,7 @@ void KeyframeEffect::SetKeyframes(JSContext* aContext,
     return;
   }
 
-  RefPtr<ComputedStyle> style = GetTargetComputedStyle();
+  RefPtr<ComputedStyle> style = GetTargetComputedStyle(Flush::None);
   SetKeyframes(std::move(keyframes), style);
 }
 
@@ -768,7 +768,8 @@ void KeyframeEffect::RequestRestyle(
   }
 }
 
-already_AddRefed<ComputedStyle> KeyframeEffect::GetTargetComputedStyle() const {
+already_AddRefed<ComputedStyle> KeyframeEffect::GetTargetComputedStyle(
+    Flush aFlushType) const {
   if (!GetRenderedDocument()) {
     return nullptr;
   }
@@ -784,7 +785,10 @@ already_AddRefed<ComputedStyle> KeyframeEffect::GetTargetComputedStyle() const {
   OwningAnimationTarget kungfuDeathGrip(mTarget->mElement,
                                         mTarget->mPseudoType);
 
-  return nsComputedDOMStyle::GetComputedStyle(mTarget->mElement, pseudo);
+  return aFlushType == Flush::Style
+             ? nsComputedDOMStyle::GetComputedStyle(mTarget->mElement, pseudo)
+             : nsComputedDOMStyle::GetComputedStyleNoFlush(mTarget->mElement,
+                                                           pseudo);
 }
 
 #ifdef DEBUG
@@ -900,7 +904,7 @@ void KeyframeEffect::SetTarget(
 
   if (mTarget) {
     UpdateTargetRegistration();
-    RefPtr<ComputedStyle> computedStyle = GetTargetComputedStyle();
+    RefPtr<ComputedStyle> computedStyle = GetTargetComputedStyle(Flush::None);
     if (computedStyle) {
       UpdateProperties(computedStyle);
     }
@@ -1032,7 +1036,7 @@ void KeyframeEffect::GetKeyframes(JSContext*& aCx, nsTArray<JSObject*>& aResult,
     // we might end up returning variables as-is or empty string. That should be
     // acceptable however, since such a case is rare and this is only
     // short-term (and unshipped) behavior until bug 1391537 is fixed.
-    computedStyle = GetTargetComputedStyle();
+    computedStyle = GetTargetComputedStyle(Flush::Style);
   }
 
   for (const Keyframe& keyframe : mKeyframes) {

dom/animation/KeyframeEffect.h

@@ -361,7 +361,12 @@ class KeyframeEffect : public AnimationEffect {
   // context. That's because calling GetComputedStyle when we are in the process
   // of building a ComputedStyle may trigger various forms of infinite
   // recursion.
-  already_AddRefed<ComputedStyle> GetTargetComputedStyle() const;
+  enum class Flush {
+    Style,
+    None,
+  };
+  already_AddRefed<ComputedStyle> GetTargetComputedStyle(
+      Flush aFlushType) const;
 
   // A wrapper for marking cascade update according to the current
   // target and its effectSet.

dom/animation/test/mozilla/file_restyles.html

@@ -1869,6 +1869,37 @@ waitForAllPaints(() => {
     await ensureElementRemoval(div);
   });
 
+  add_task(async function restyling_on_create_animation() {
+    const div = addDiv();
+    const docShell = getDocShellForObservingRestylesForWindow(window);
+
+    const animationA = div.animate(
+      { transform: ['none', 'rotate(360deg)'] },
+      100 * MS_PER_SEC
+    );
+    const animationB = div.animate({ opacity: [0, 1] }, 100 * MS_PER_SEC);
+    const animationC = div.animate(
+      { color: ['blue', 'green'] },
+      100 * MS_PER_SEC
+    );
+    const animationD = div.animate(
+      { width: ['100px', '200px'] },
+      100 * MS_PER_SEC
+    );
+    const animationE = div.animate(
+      { height: ['100px', '200px'] },
+      100 * MS_PER_SEC
+    );
+
+    const markers = docShell
+      .popProfileTimelineMarkers()
+      .filter(marker => marker.name === 'Styles' && !marker.isAnimationOnly);
+    docShell.recordProfileTimelineMarkers = false;
+
+    is(markers.length, 0, 'Creating animations should not flush styles');
+
+    await ensureElementRemoval(div);
+  });
 });
 
 </script>