Bug 1336772 - Request any restyles required by changes to the cascade result. r=birtles, a=lizzard
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Tue, 05 Sep 2017 16:34:24 +0900
changeset 423965 4f6fcd8f33c7df235b23a94f7599accf11fd3950
parent 423964 9055f0524b2de6d2b076ca2068e59fcecb85cd1b
child 423966 64b7ee7060ed09d4376dc7587001984f368e0b4d
push id1517
push userjlorenzo@mozilla.com
push dateThu, 14 Sep 2017 16:50:54 +0000
treeherdermozilla-release@3b41fd564418 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbirtles, lizzard
bugs1336772
milestone56.0
Bug 1336772 - Request any restyles required by changes to the cascade result. r=birtles, a=lizzard When an animation is newly created while the same property transition is running, the transition style rule persists until we call RequestRestyle() for transitions level. That means if user calls getComputedStyle for the property right after creating animation, the style obtained by getComputedStyle still included the transitions level rule. As a result, the transitions level style overrides newly created animation style until the next normal restyling process happens (i.e. process transition level restyle request). Vice versa, in the case where an animation is removed, transitions level style does not appear until the next normal restyling. This patch fixes this problem by trigerring a resyle of the transitions level when an animation is created or removed. MozReview-Commit-ID: HY6amLmDHTi
dom/animation/EffectCompositor.cpp
dom/animation/EffectCompositor.h
dom/animation/test/mochitest.ini
dom/animation/test/mozilla/test_cascade.html
layout/style/nsCSSPropertyIDSet.h
--- a/dom/animation/EffectCompositor.cpp
+++ b/dom/animation/EffectCompositor.cpp
@@ -876,42 +876,42 @@ EffectCompositor::UpdateCascadeResults(S
   nsCSSPropertyIDSet& propertiesForAnimationsLevel =
     aEffectSet.PropertiesForAnimationsLevel();
 
   // Record which compositor-animatable properties were originally set so we can
   // compare for changes later.
   std::bitset<LayerAnimationInfo::kRecords>
     prevCompositorPropertiesWithImportantRules =
       compositorPropertiesInSet(propertiesWithImportantRules);
-  std::bitset<LayerAnimationInfo::kRecords>
-    prevCompositorPropertiesForAnimationsLevel =
-      compositorPropertiesInSet(propertiesForAnimationsLevel);
+
+  nsCSSPropertyIDSet prevPropertiesForAnimationsLevel =
+    propertiesForAnimationsLevel;
 
   propertiesWithImportantRules.Empty();
   propertiesForAnimationsLevel.Empty();
 
-  bool hasCompositorPropertiesForTransition = false;
+  nsCSSPropertyIDSet propertiesForTransitionsLevel;
 
   for (const KeyframeEffectReadOnly* effect : sortedEffectList) {
     MOZ_ASSERT(effect->GetAnimation(),
                "Effects on a target element should have an Animation");
     CascadeLevel cascadeLevel = effect->GetAnimation()->CascadeLevel();
 
     for (const AnimationProperty& prop : effect->Properties()) {
       if (overriddenProperties.HasProperty(prop.mProperty)) {
         propertiesWithImportantRules.AddProperty(prop.mProperty);
       }
-      if (cascadeLevel == EffectCompositor::CascadeLevel::Animations) {
-        propertiesForAnimationsLevel.AddProperty(prop.mProperty);
-      }
 
-      if (nsCSSProps::PropHasFlags(prop.mProperty,
-                                   CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR) &&
-          cascadeLevel == EffectCompositor::CascadeLevel::Transitions) {
-        hasCompositorPropertiesForTransition = true;
+      switch (cascadeLevel) {
+        case EffectCompositor::CascadeLevel::Animations:
+          propertiesForAnimationsLevel.AddProperty(prop.mProperty);
+          break;
+        case EffectCompositor::CascadeLevel::Transitions:
+          propertiesForTransitionsLevel.AddProperty(prop.mProperty);
+          break;
       }
     }
   }
 
   aEffectSet.MarkCascadeUpdated();
 
   nsPresContext* presContext = nsContentUtils::GetContextForContent(aElement);
   if (!presContext) {
@@ -924,25 +924,33 @@ EffectCompositor::UpdateCascadeResults(S
   // the compositor or pull animations back from the compositor.
   if (prevCompositorPropertiesWithImportantRules !=
         compositorPropertiesInSet(propertiesWithImportantRules)) {
     presContext->EffectCompositor()->
       RequestRestyle(aElement, aPseudoType,
                      EffectCompositor::RestyleType::Layer,
                      EffectCompositor::CascadeLevel::Animations);
   }
-  // If we have transition properties for compositor and if the same propery
-  // for animations level is newly added or removed, we need to update layers
-  // for transitions level because composite order has been changed now.
-  if (hasCompositorPropertiesForTransition &&
-      prevCompositorPropertiesForAnimationsLevel !=
-        compositorPropertiesInSet(propertiesForAnimationsLevel)) {
+
+  // If we have transition properties and if the same propery for animations
+  // level is newly added or removed, we need to update the transition level
+  // rule since the it will be added/removed from the rule tree.
+  nsCSSPropertyIDSet changedPropertiesForAnimationLevel =
+    prevPropertiesForAnimationsLevel.Xor(propertiesForAnimationsLevel);
+  nsCSSPropertyIDSet commonProperties =
+    propertiesForTransitionsLevel.Intersect(
+      changedPropertiesForAnimationLevel);
+  if (!commonProperties.IsEmpty()) {
+    EffectCompositor::RestyleType restyleType =
+      compositorPropertiesInSet(changedPropertiesForAnimationLevel).none()
+      ? EffectCompositor::RestyleType::Standard
+      : EffectCompositor::RestyleType::Layer;
     presContext->EffectCompositor()->
       RequestRestyle(aElement, aPseudoType,
-                     EffectCompositor::RestyleType::Layer,
+                     restyleType,
                      EffectCompositor::CascadeLevel::Transitions);
   }
 }
 
 /* static */ void
 EffectCompositor::SetPerformanceWarning(
   const nsIFrame *aFrame,
   nsCSSPropertyID aProperty,
--- a/dom/animation/EffectCompositor.h
+++ b/dom/animation/EffectCompositor.h
@@ -275,17 +275,18 @@ private:
   static nsCSSPropertyIDSet
   GetOverriddenProperties(StyleBackendType aBackendType,
                           EffectSet& aEffectSet,
                           dom::Element* aElement,
                           CSSPseudoElementType aPseudoType,
                           nsStyleContext* aStyleContext);
 
   // Update the mPropertiesWithImportantRules and
-  // mPropertiesForAnimationsLevel members of the given EffectSet.
+  // mPropertiesForAnimationsLevel members of the given EffectSet, and also
+  // request any restyles required by changes to the cascade result.
   //
   // This can be expensive so we should only call it if styles that apply
   // above the animation level of the cascade might have changed. For all
   // other cases we should call MaybeUpdateCascadeResults.
   //
   // As with MaybeUpdateCascadeResults, |aStyleContext| is only used
   // when |aBackendType| is StyleBackendType::Gecko. When |aBackendType| is
   // StyleBackendType::Servo, it is ignored.
--- a/dom/animation/test/mochitest.ini
+++ b/dom/animation/test/mochitest.ini
@@ -96,16 +96,17 @@ skip-if = stylo
 [css-transitions/test_effect-target.html]
 [css-transitions/test_element-get-animations.html]
 [css-transitions/test_event-dispatch.html]
 [css-transitions/test_keyframeeffect-getkeyframes.html]
 [css-transitions/test_pseudoElement-get-animations.html]
 [css-transitions/test_setting-effect.html]
 [document-timeline/test_document-timeline.html]
 [document-timeline/test_request_animation_frame.html]
+[mozilla/test_cascade.html]
 [mozilla/test_cubic_bezier_limits.html]
 [mozilla/test_deferred_start.html]
 [mozilla/test_disable_animations_api_core.html]
 [mozilla/test_disabled_properties.html]
 [mozilla/test_discrete-animations.html]
 skip-if = stylo
 [mozilla/test_document-timeline-origin-time-range.html]
 [mozilla/test_hide_and_show.html]
new file mode 100644
--- /dev/null
+++ b/dom/animation/test/mozilla/test_cascade.html
@@ -0,0 +1,37 @@
+<!doctype html>
+<meta charset=utf-8>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="../testcommon.js"></script>
+<style>
+@keyframes margin-left {
+  from { margin-left: 20px; }
+  to   { margin-left: 80px; }
+}
+</style>
+<body>
+<div id="log"></div>
+<script>
+'use strict';
+
+test(function(t) {
+  var div = addDiv(t, { style: 'transition: margin-left 100s; ' +
+                               'margin-left: 80px' });
+  var cs = getComputedStyle(div);
+
+  assert_equals(cs.marginLeft, '80px', 'initial margin-left');
+
+  div.style.marginLeft = "20px";
+  assert_equals(cs.marginLeft, '80px', 'margin-left transition at 0s');
+
+  div.style.animation = "margin-left 2s";
+  assert_equals(cs.marginLeft, '20px',
+                'margin-left animation overrides transition at 0s');
+
+  div.style.animation = "none";
+  assert_equals(cs.marginLeft, '80px',
+                'margin-left animation stops overriding transition at 0s');
+}, 'Animation overrides/stops overriding transition immediately');
+
+</script>
+</body>
--- a/layout/style/nsCSSPropertyIDSet.h
+++ b/layout/style/nsCSSPropertyIDSet.h
@@ -62,25 +62,54 @@ public:
             NS_ASSERTION(mProperties[i] == 0, aText);
         }
     }
 
     bool Equals(const nsCSSPropertyIDSet& aOther) const {
       return mozilla::PodEqual(mProperties, aOther.mProperties);
     }
 
+    bool IsEmpty() const {
+      for (size_t i = 0; i < mozilla::ArrayLength(mProperties); ++i) {
+          if (mProperties[i] != 0) {
+            return false;
+          }
+      }
+      return true;
+    }
+
     // Return a new nsCSSPropertyIDSet which is the inverse of this set.
     nsCSSPropertyIDSet Inverse() const {
       nsCSSPropertyIDSet result;
       for (size_t i = 0; i < mozilla::ArrayLength(mProperties); ++i) {
         result.mProperties[i] = ~mProperties[i];
       }
       return result;
     }
 
+    // Returns a new nsCSSPropertyIDSet with all properties that are both in
+    // this set and |aOther|.
+    nsCSSPropertyIDSet Intersect(const nsCSSPropertyIDSet& aOther) const {
+      nsCSSPropertyIDSet result;
+      for (size_t i = 0; i < mozilla::ArrayLength(mProperties); ++i) {
+        result.mProperties[i] = mProperties[i] & aOther.mProperties[i];
+      }
+      return result;
+    }
+
+    // Return a new nsCSSPropertyIDSet with all properties that are in either
+    // this set or |aOther| but not both.
+    nsCSSPropertyIDSet Xor(const nsCSSPropertyIDSet& aOther) const {
+      nsCSSPropertyIDSet result;
+      for (size_t i = 0; i < mozilla::ArrayLength(mProperties); ++i) {
+        result.mProperties[i] = mProperties[i] ^ aOther.mProperties[i];
+      }
+      return result;
+    }
+
 private:
     typedef unsigned long property_set_type;
 public:
     // number of bits in |property_set_type|.
     static const size_t kBitsInChunk = sizeof(property_set_type)*CHAR_BIT;
     // number of |property_set_type|s in the set
     static const size_t kChunkCount =
         (eCSSProperty_COUNT_no_shorthands + kBitsInChunk - 1) / kBitsInChunk;