Bug 1461070: Skip starting other transitions based on specified, not started transitions. r?birtles draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sat, 12 May 2018 10:51:48 +0200
changeset 794536 fc067c0fb7dbb7f46fca42a1fcd93680bb509294
parent 794329 16426b39676d0a8a55210299c6c947ca2dee19ea
child 794675 1d93fe096228543f8387617d3d0a9562fcbb74aa
child 794678 1b100cf702db66f032640d3f4a4316d81c8dc0ab
push id109704
push userbmo:emilio@crisal.io
push dateSat, 12 May 2018 16:45:12 +0000
reviewersbirtles
bugs1461070
milestone62.0a1
Bug 1461070: Skip starting other transitions based on specified, not started transitions. r?birtles Needs a test and all that. Also I wonder if it'd be worth to rewrite that transition-start / stop loop in Rust code, and put it in the style crate. It'd help Servo to fix its completely broken transitions code, WDYT? MozReview-Commit-ID: 3D5elrj2Ypi
layout/style/nsTransitionManager.cpp
layout/style/nsTransitionManager.h
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/css/css-transitions/zero-duration-multiple-transition.html
--- a/layout/style/nsTransitionManager.cpp
+++ b/layout/style/nsTransitionManager.cpp
@@ -457,57 +457,68 @@ nsTransitionManager::DoUpdateTransitions
   const ComputedStyle& aNewStyle)
 {
   MOZ_ASSERT(!aElementTransitions ||
              aElementTransitions->mElement == aElement, "Element mismatch");
 
   // Per http://lists.w3.org/Archives/Public/www-style/2009Aug/0109.html
   // I'll consider only the transitions from the number of items in
   // 'transition-property' on down, and later ones will override earlier
-  // ones (tracked using |whichStarted|).
+  // ones (tracked using |propertiesChecked|).
   bool startedAny = false;
-  nsCSSPropertyIDSet whichStarted;
-  for (uint32_t i = aDisp.mTransitionPropertyCount; i-- != 0; ) {
+  nsCSSPropertyIDSet propertiesChecked;
+  for (uint32_t i = aDisp.mTransitionPropertyCount; i--; ) {
+    // We're not going to look at any further transitions, so we can just avoid
+    // looking at this if we know it will not start any transition.
+    if (i == 0 && aDisp.GetTransitionCombinedDuration(i) <= 0.0f) {
+      continue;
+    }
+
     // Check the combined duration (combination of delay and duration)
     // first, since it defaults to zero, which means we can ignore the
     // transition.
-    if (aDisp.GetTransitionCombinedDuration(i) > 0.0f) {
-      // We might have something to transition.  See if any of the
-      // properties in question changed and are animatable.
-      // FIXME: Would be good to find a way to share code between this
-      // interpretation of transition-property and the one below.
-      nsCSSPropertyID property = aDisp.GetTransitionProperty(i);
-      if (property == eCSSPropertyExtra_no_properties ||
-          property == eCSSPropertyExtra_variable ||
-          property == eCSSProperty_UNKNOWN) {
-        // Nothing to do, but need to exclude this from cases below.
-      } else if (property == eCSSPropertyExtra_all_properties) {
-        for (nsCSSPropertyID p = nsCSSPropertyID(0);
-             p < eCSSProperty_COUNT_no_shorthands;
-             p = nsCSSPropertyID(p + 1)) {
+    nsCSSPropertyID property = aDisp.GetTransitionProperty(i);
+    if (property == eCSSPropertyExtra_no_properties ||
+        property == eCSSPropertyExtra_variable ||
+        property == eCSSProperty_UNKNOWN) {
+      // Nothing to do.
+      continue;
+    }
+    // We might have something to transition.  See if any of the
+    // properties in question changed and are animatable.
+    // FIXME: Would be good to find a way to share code between this
+    // interpretation of transition-property and the one below.
+    // FIXME(emilio): This should probably just use the "all" shorthand id, and
+    // we should probably remove eCSSPropertyExtra_all_properties.
+    if (property == eCSSPropertyExtra_all_properties) {
+      for (nsCSSPropertyID p = nsCSSPropertyID(0);
+           p < eCSSProperty_COUNT_no_shorthands;
+           p = nsCSSPropertyID(p + 1)) {
+        startedAny |=
           ConsiderInitiatingTransition(p, aDisp, i, aElement, aPseudoType,
                                        aElementTransitions,
                                        aOldStyle, aNewStyle,
-                                       &startedAny, &whichStarted);
-        }
-      } else if (nsCSSProps::IsShorthand(property)) {
-        CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(subprop, property,
-                                             CSSEnabledState::eForAllContent)
-        {
+                                       propertiesChecked);
+      }
+    } else if (nsCSSProps::IsShorthand(property)) {
+      CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(subprop, property,
+                                           CSSEnabledState::eForAllContent)
+      {
+        startedAny |=
           ConsiderInitiatingTransition(*subprop, aDisp, i, aElement, aPseudoType,
                                        aElementTransitions,
                                        aOldStyle, aNewStyle,
-                                       &startedAny, &whichStarted);
-        }
-      } else {
+                                       propertiesChecked);
+      }
+    } else {
+      startedAny |=
         ConsiderInitiatingTransition(property, aDisp, i, aElement, aPseudoType,
                                      aElementTransitions,
                                      aOldStyle, aNewStyle,
-                                     &startedAny, &whichStarted);
-      }
+                                     propertiesChecked);
     }
   }
 
   // Stop any transitions for properties that are no longer in
   // 'transition-property', including finished transitions.
   // Also stop any transitions (and remove any finished transitions)
   // for properties that just changed (and are still in the set of
   // properties to transition), but for which we didn't just start the
@@ -626,52 +637,65 @@ GetTransitionKeyframes(nsCSSPropertyID a
 }
 
 static bool
 IsTransitionable(nsCSSPropertyID aProperty)
 {
   return Servo_Property_IsTransitionable(aProperty);
 }
 
-void
+bool
 nsTransitionManager::ConsiderInitiatingTransition(
   nsCSSPropertyID aProperty,
   const nsStyleDisplay& aStyleDisplay,
   uint32_t transitionIdx,
   dom::Element* aElement,
   CSSPseudoElementType aPseudoType,
   CSSTransitionCollection*& aElementTransitions,
   const ComputedStyle& aOldStyle,
   const ComputedStyle& aNewStyle,
-  bool* aStartedAny,
-  nsCSSPropertyIDSet* aWhichStarted)
+  nsCSSPropertyIDSet& propertiesChecked)
 {
   // IsShorthand itself will assert if aProperty is not a property.
   MOZ_ASSERT(!nsCSSProps::IsShorthand(aProperty),
              "property out of range");
   NS_ASSERTION(!aElementTransitions ||
                aElementTransitions->mElement == aElement, "Element mismatch");
 
+  if (propertiesChecked.HasProperty(aProperty)) {
+    return false;
+  }
+
+  // A later item in transition-property already specified a transition for this
+  // property, so we ignore this one.
+  // See comment above and
+  // http://lists.w3.org/Archives/Public/www-style/2009Aug/0109.html .
+  propertiesChecked.AddProperty(aProperty);
+
   // Ignore disabled properties. We can arrive here if the transition-property
   // is 'all' and the disabled property has a default value which derives value
   // from another property, e.g. color.
   if (!nsCSSProps::IsEnabled(aProperty, CSSEnabledState::eForAllContent)) {
-    return;
-  }
-
-  if (aWhichStarted->HasProperty(aProperty)) {
-    // A later item in transition-property already started a
-    // transition for this property, so we ignore this one.
-    // See comment above and
-    // http://lists.w3.org/Archives/Public/www-style/2009Aug/0109.html .
-    return;
+    return false;
   }
 
   if (!IsTransitionable(aProperty)) {
-    return;
+    return false;
+  }
+
+  float delay = aStyleDisplay.GetTransitionDelay(transitionIdx);
+
+  // The spec says a negative duration is treated as zero.
+  float duration =
+    std::max(aStyleDisplay.GetTransitionDuration(transitionIdx), 0.0f);
+
+  // The combined duration of this transition is 0 or less, so don't start a
+  // transition.
+  if (delay + duration <= 0.0f) {
+    return false;
   }
 
   dom::DocumentTimeline* timeline = aElement->OwnerDoc()->Timeline();
 
   AnimationValue startValue, endValue;
   bool haveValues =
     ExtractNonDiscreteComputedValue(aProperty, aOldStyle, startValue) &&
     ExtractNonDiscreteComputedValue(aProperty, aNewStyle, endValue);
@@ -712,17 +736,17 @@ nsTransitionManager::ConsiderInitiatingT
   //
   // Likewise, if we got a style change that changed the value to the
   // endpoint of our finished transition, we also don't want to start
   // a new transition for the reasons described in
   // https://lists.w3.org/Archives/Public/www-style/2015Jan/0444.html .
   if (haveCurrentTransition && haveValues &&
       aElementTransitions->mAnimations[currentIndex]->ToValue() == endValue) {
     // GetAnimationRule already called RestyleForAnimation.
-    return;
+    return false;
   }
 
   if (!shouldAnimate) {
     if (haveCurrentTransition) {
       // We're in the middle of a transition, and just got a non-transition
       // style change to something that we can't animate.  This might happen
       // because we got a non-transition style change changing to the current
       // in-progress value (which is particularly easy to cause when we're
@@ -740,27 +764,21 @@ nsTransitionManager::ConsiderInitiatingT
 
       if (animations.IsEmpty()) {
         aElementTransitions->Destroy();
         // |aElementTransitions| is now a dangling pointer!
         aElementTransitions = nullptr;
       }
       // GetAnimationRule already called RestyleForAnimation.
     }
-    return;
+    return false;
   }
 
   const nsTimingFunction &tf =
     aStyleDisplay.GetTransitionTimingFunction(transitionIdx);
-  float delay = aStyleDisplay.GetTransitionDelay(transitionIdx);
-  float duration = aStyleDisplay.GetTransitionDuration(transitionIdx);
-  if (duration < 0.0) {
-    // The spec says a negative duration is treated as zero.
-    duration = 0.0;
-  }
 
   AnimationValue startForReversingTest = startValue;
   double reversePortion = 1.0;
 
   // If the new transition reverses an existing one, we'll need to
   // handle the timing differently.
   // FIXME: Move mStartForReversingTest, mReversePortion to CSSTransition,
   //        and set the timing function on transitions as an effect-level
@@ -834,17 +852,17 @@ nsTransitionManager::ConsiderInitiatingT
   if (!aElementTransitions) {
     bool createdCollection = false;
     aElementTransitions =
       CSSTransitionCollection::GetOrCreateAnimationCollection(
         aElement, aPseudoType, &createdCollection);
     if (!aElementTransitions) {
       MOZ_ASSERT(!createdCollection, "outparam should agree with return value");
       NS_WARNING("allocating collection failed");
-      return;
+      return false;
     }
 
     if (createdCollection) {
       AddElementCollection(aElementTransitions);
     }
   }
 
   OwningCSSTransitionPtrArray& animations = aElementTransitions->mAnimations;
@@ -881,21 +899,20 @@ nsTransitionManager::ConsiderInitiatingT
       );
     }
     animations[currentIndex]->CancelFromStyle();
     oldPT = nullptr; // Clear pointer so it doesn't dangle
     animations[currentIndex] = animation;
   } else {
     if (!animations.AppendElement(animation)) {
       NS_WARNING("out of memory");
-      return;
+      return false;
     }
   }
 
   EffectSet* effectSet = EffectSet::GetEffectSet(aElement, aPseudoType);
   if (effectSet) {
     effectSet->UpdateAnimationGeneration(mPresContext);
   }
 
-  *aStartedAny = true;
-  aWhichStarted->AddProperty(aProperty);
+  return true;
 }
 
--- a/layout/style/nsTransitionManager.h
+++ b/layout/style/nsTransitionManager.h
@@ -334,22 +334,21 @@ protected:
   // could be a nullptr if we don't have any transitions.
   bool DoUpdateTransitions(const nsStyleDisplay& aDisp,
                            mozilla::dom::Element* aElement,
                            mozilla::CSSPseudoElementType aPseudoType,
                            CSSTransitionCollection*& aElementTransitions,
                            const mozilla::ComputedStyle& aOldStyle,
                            const mozilla::ComputedStyle& aNewStyle);
 
-  void ConsiderInitiatingTransition(nsCSSPropertyID aProperty,
+  // Returns whether the transition actually started.
+  bool ConsiderInitiatingTransition(nsCSSPropertyID aProperty,
                                     const nsStyleDisplay& aStyleDisplay,
                                     uint32_t transitionIdx,
                                     mozilla::dom::Element* aElement,
                                     mozilla::CSSPseudoElementType aPseudoType,
                                     CSSTransitionCollection*& aElementTransitions,
                                     const mozilla::ComputedStyle& aOldStyle,
                                     const mozilla::ComputedStyle& aNewStyle,
-                                    bool* aStartedAny,
-                                    nsCSSPropertyIDSet* aWhichStarted);
-
+                                    nsCSSPropertyIDSet& propertiesChecked);
 };
 
 #endif /* !defined(nsTransitionManager_h_) */
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -320573,16 +320573,22 @@
     ]
    ],
    "css/css-transitions/transitions-animatable-properties-01.html": [
     [
      "/css/css-transitions/transitions-animatable-properties-01.html",
      {}
     ]
    ],
+   "css/css-transitions/zero-duration-multiple-transition.html": [
+    [
+     "/css/css-transitions/zero-duration-multiple-transition.html",
+     {}
+    ]
+   ],
    "css/css-typed-om/CSSMatrixComponent-DOMMatrix-mutable.html": [
     [
      "/css/css-typed-om/CSSMatrixComponent-DOMMatrix-mutable.html",
      {}
     ]
    ],
    "css/css-typed-om/declared-styleMap-accepts-inherit.html": [
     [
@@ -516325,17 +516331,17 @@
    "04a6a67837127ba47e757e5cc3a9ff91c1e01d9a",
    "reftest"
   ],
   "css/css-shapes/shape-outside/supported-shapes/inset/shape-outside-inset-028.html": [
    "cafd680e210a6d677bde00ae4c5f9263c1e7b48e",
    "reftest"
   ],
   "css/css-shapes/shape-outside/supported-shapes/inset/shape-outside-inset-029.html": [
-   "67161f08b8aec3fe9bd94e14cbe865dd4c9d3664",
+   "763518bc498fa2560e91eb6dfd13803a5dab2e17",
    "reftest"
   ],
   "css/css-shapes/shape-outside/supported-shapes/inset/shape-outside-inset-030.html": [
    "e5b82f3b41a856a34152af46c9cc659202c61843",
    "reftest"
   ],
   "css/css-shapes/shape-outside/supported-shapes/polygon/reference/shape-outside-polygon-007-ref.html": [
    "3ee945e85e3c2a9b502e93c04ed53966551e7df1",
@@ -529972,16 +529978,20 @@
   "css/css-transitions/transitioncancel-001.html": [
    "4db1d84665ccf75d993d877eb08574b1fa7d0203",
    "testharness"
   ],
   "css/css-transitions/transitions-animatable-properties-01.html": [
    "538b95863c061da60e95c1a61ef9dc93da007aa4",
    "testharness"
   ],
+  "css/css-transitions/zero-duration-multiple-transition.html": [
+   "7db6bc8641a6a2eb5f1ee1fdbc9b6215a0462bbc",
+   "testharness"
+  ],
   "css/css-typed-om/CSSMatrixComponent-DOMMatrix-mutable.html": [
    "f6056e2480829c7aa9885673d332496faf7777b5",
    "testharness"
   ],
   "css/css-typed-om/OWNERS": [
    "f5f0861ac3382b3b12008133c1334f812a5a2caa",
    "support"
   ],
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-transitions/zero-duration-multiple-transition.html
@@ -0,0 +1,29 @@
+<!doctype html>
+<meta charset="utf-8">
+<link rel="help" href="https://drafts.csswg.org/css-transitions/#starting">
+<link rel="author" title="Emilio Cobos Álvarez" href="emilio@crisal.io">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+  #t {
+    color: red;
+    transition-property: color, color;
+    transition-duration: 1s, 0s;
+  }
+</style>
+<div id="t"></div>
+<script>
+test(function() {
+  let div = document.getElementById("t");
+  assert_equals(getComputedStyle(div).color, "rgb(255, 0, 0)");
+
+  div.style.color = "green";
+  assert_equals(
+    div.getAnimations().length,
+    0,
+    "No transition should've started"
+  );
+
+  assert_equals(getComputedStyle(div).color, "rgb(0, 128, 0)");
+}, "transition-duration of 0 prevents earlier transitions with the same property from starting.");
+</script>