From b4797974ce93ef9ce31d73e303a4f849b7ddd0b2 Mon Sep 17 00:00:00 2001
From: "L. David Baron" <dbaron@dbaron.org>
Date: Fri, 11 Dec 2009 08:13:19 -0800
Subject: [PATCH] Stop transitions when -moz-transition-property changes to a
 value that no longer includes the transitioning property.  (Bug 525530) 
 r=bzbarsky

---
 layout/style/nsStyleStruct.cpp                | 11 ++-
 layout/style/nsTransitionManager.cpp          | 92 +++++++++++++++----
 layout/style/test/Makefile.in                 |  1 +
 .../test_transitions_dynamic_changes.html     | 46 ++++++++++
 4 files changed, 128 insertions(+), 22 deletions(-)
 create mode 100644 layout/style/test/test_transitions_dynamic_changes.html

diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp
index 023a09ef3e9..c76ec964ce0 100644
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -1918,15 +1918,16 @@ nsChangeHint nsStyleDisplay::CalcDifference(const nsStyleDisplay& aOther) const
         break;
       }
   }
-  
-  // Note:  Our current behavior for handling changes to transition
+
+  // Note:  Our current behavior for handling changes to the
+  // transition-duration, transition-delay, and transition-timing-function
   // properties is to do nothing.  In other words, the transition
   // property that matters is what it is when the transition begins, and
   // we don't stop a transition later because the transition property
   // changed.
-  // FIXME (Bug 522599):  Need to test for this and write it in the
-  // spec, if it's compatible with other browsers.  Test for behavior at
-  // http://dbaron.org/css/test/2009/transitions/dynamic-transition-change
+  // We do handle changes to transition-property, but we don't need to
+  // bother with anything here, since the transition manager is notified
+  // of any style context change anyway.
   
   return hint;
 }
diff --git a/layout/style/nsTransitionManager.cpp b/layout/style/nsTransitionManager.cpp
index c041dc97432..48bb078ac7b 100644
--- a/layout/style/nsTransitionManager.cpp
+++ b/layout/style/nsTransitionManager.cpp
@@ -358,12 +358,20 @@ nsTransitionManager::StyleContextChanged(nsIContent *aElement,
                       aNewStyleContext->HasPseudoElementData(),
                   "pseudo type mismatch");
 
+  // NOTE: Things in this function (and ConsiderStartingTransition)
+  // should never call PeekStyleData because we don't preserve gotten
+  // structs across reframes.
+
   // Return sooner (before the startedAny check below) for the most
   // common case: no transitions specified.
   const nsStyleDisplay *disp = aNewStyleContext->GetStyleDisplay();
+  const nsStyleDisplay *oldDisp = aOldStyleContext->GetStyleDisplay();
   if (disp->mTransitionPropertyCount == 1 &&
+      oldDisp->mTransitionPropertyCount == 1 &&
       disp->mTransitions[0].GetDelay() == 0.0f &&
-      disp->mTransitions[0].GetDuration() == 0.0f) {
+      disp->mTransitions[0].GetDuration() == 0.0f &&
+      oldDisp->mTransitions[0].GetProperty() ==
+        disp->mTransitions[0].GetProperty()) {
     return nsnull;
   }      
 
@@ -392,16 +400,17 @@ nsTransitionManager::StyleContextChanged(nsIContent *aElement,
   // ones (tracked using |whichStarted|).
   PRBool startedAny = PR_FALSE;
   nsCSSPropertySet whichStarted;
-  ElementTransitions *et = nsnull;
+  ElementTransitions *et =
+    GetElementTransitions(aElement, pseudoType, PR_FALSE);
   for (PRUint32 i = disp->mTransitionPropertyCount; i-- != 0; ) {
     const nsTransition& t = disp->mTransitions[i];
     // Check delay and duration first, since they default to zero, and
     // when they're both zero, we can ignore the transition.
     if (t.GetDelay() != 0.0f || t.GetDuration() != 0.0f) {
-      et = GetElementTransitions(aElement, pseudoType, PR_FALSE);
-
       // 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.
       nsCSSProperty property = t.GetProperty();
       if (property == eCSSPropertyExtra_no_properties ||
           property == eCSSProperty_UNKNOWN) {
@@ -428,6 +437,51 @@ nsTransitionManager::StyleContextChanged(nsIContent *aElement,
     }
   }
 
+  // Stop any transitions for properties that are no longer in
+  // 'transition-property'.
+  if (et && disp->mTransitions[0].GetProperty() !=
+            eCSSPropertyExtra_all_properties) {
+    nsCSSPropertySet allTransitionProperties;
+    for (PRUint32 i = disp->mTransitionPropertyCount; i-- != 0; ) {
+      const nsTransition& t = disp->mTransitions[i];
+      // FIXME: Would be good to find a way to share code between this
+      // interpretation of transition-property and the one above.
+      nsCSSProperty property = t.GetProperty();
+      if (property == eCSSPropertyExtra_no_properties ||
+          property == eCSSProperty_UNKNOWN) {
+        // Nothing to do, but need to exclude this from cases below.
+      } else if (property == eCSSPropertyExtra_all_properties) {
+        for (nsCSSProperty p = nsCSSProperty(0); 
+             p < eCSSProperty_COUNT_no_shorthands;
+             p = nsCSSProperty(p + 1)) {
+          allTransitionProperties.AddProperty(p);
+        }
+      } else if (nsCSSProps::IsShorthand(property)) {
+        CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(subprop, property) {
+          allTransitionProperties.AddProperty(*subprop);
+        }
+      } else {
+        allTransitionProperties.AddProperty(property);
+      }
+    }
+
+    nsTArray<ElementPropertyTransition> &pts = et->mPropertyTransitions;
+    PRUint32 i = pts.Length();
+    NS_ABORT_IF_FALSE(i != 0, "empty transitions list?");
+    do {
+      --i;
+      ElementPropertyTransition &pt = pts[i];
+      if (!allTransitionProperties.HasProperty(pt.mProperty)) {
+        pts.RemoveElementAt(i);
+      }
+    } while (i != 0);
+
+    if (pts.IsEmpty()) {
+      et->Destroy();
+      et = nsnull;
+    }
+  }
+
   if (!startedAny) {
     return nsnull;
   }
@@ -542,7 +596,7 @@ nsTransitionManager::ConsiderStartingTransition(nsCSSProperty aProperty,
         // |aElementTransitions| is now a dangling pointer!
         aElementTransitions = nsnull;
       }
-      presContext->PresShell()->RestyleForAnimation(aElement);
+      // WalkTransitionRule already called RestyleForAnimation.
     }
     return;
   }
@@ -564,9 +618,7 @@ nsTransitionManager::ConsiderStartingTransition(nsCSSProperty aProperty,
       // If we got a style change that changed the value to the endpoint
       // of the currently running transition, we don't want to interrupt
       // its timing function.
-      // But don't forget to restyle with animation so we show the
-      // current transition.
-      presContext->PresShell()->RestyleForAnimation(aElement);
+      // WalkTransitionRule already called RestyleForAnimation.
       return;
     }
 
@@ -712,15 +764,7 @@ nsresult
 nsTransitionManager::WalkTransitionRule(RuleProcessorData* aData,
                                         nsCSSPseudoElements::Type aPseudoType)
 {
-  if (!aData->mPresContext->IsProcessingAnimationStyleChange()) {
-    // If we're processing a normal style change rather than one from
-    // animation, don't add the transition rule.  This allows us to
-    // compute the new style value rather than having the transition
-    // override it, so that we can start transitioning differently.
-
-    // In most cases, we need to immediately restyle with animation
-    // after doing this.  However, ConsiderStartingTransition takes care
-    // of that for us.
+  if (!aData->mContent) {
     return NS_OK;
   }
 
@@ -730,6 +774,20 @@ nsTransitionManager::WalkTransitionRule(RuleProcessorData* aData,
     return NS_OK;
   }
 
+  if (!aData->mPresContext->IsProcessingAnimationStyleChange()) {
+    // If we're processing a normal style change rather than one from
+    // animation, don't add the transition rule.  This allows us to
+    // compute the new style value rather than having the transition
+    // override it, so that we can start transitioning differently.
+
+    // We need to immediately restyle with animation
+    // after doing this.
+    if (et) {
+      mPresContext->PresShell()->RestyleForAnimation(aData->mContent);
+    }
+    return NS_OK;
+  }
+
   if (!et->EnsureStyleRuleFor(
         aData->mPresContext->RefreshDriver()->MostRecentRefresh())) {
     return NS_ERROR_OUT_OF_MEMORY;
diff --git a/layout/style/test/Makefile.in b/layout/style/test/Makefile.in
index 7f43232edda..6734f9acae1 100644
--- a/layout/style/test/Makefile.in
+++ b/layout/style/test/Makefile.in
@@ -140,6 +140,7 @@ _TEST_FILES =	test_acid3_test46.html \
 		test_transitions_computed_value_combinations.html \
 		test_transitions.html \
 		test_transitions_per_property.html \
+		test_transitions_dynamic_changes.html \
 		test_units_angle.html \
 		test_units_frequency.html \
 		test_units_length.html \
diff --git a/layout/style/test/test_transitions_dynamic_changes.html b/layout/style/test/test_transitions_dynamic_changes.html
new file mode 100644
index 00000000000..f5a5ad022db
--- /dev/null
+++ b/layout/style/test/test_transitions_dynamic_changes.html
@@ -0,0 +1,46 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=525530
+-->
+<head>
+  <title>Test for Bug 525530</title>
+  <script type="application/javascript" src="/MochiKit/packed.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=525530">Mozilla Bug 525530</a>
+<p id="display" style="text-indent: 100px"></p>
+<pre id="test">
+<script type="application/javascript">
+
+/** Test for Bug 525530 **/
+
+var p = document.getElementById("display");
+var cs = getComputedStyle(p, "");
+
+p.style.MozTransitionProperty = "all";
+p.style.MozTransitionDuration = "4s";
+p.style.MozTransitionDelay = "-2s";
+p.style.MozTransitionTimingFunction = "linear";
+
+is(cs.textIndent, "100px", "initial value");
+
+p.style.textIndent = "0";
+is(cs.textIndent, "50px", "transition is halfway");
+p.style.MozTransitionDuration = "0s";
+is(cs.textIndent, "50px", "changing duration doesn't change transitioning");
+p.style.MozTransitionDelay = "0s";
+is(cs.textIndent, "50px", "changing delay doesn't change transitioning");
+p.style.MozTransitionProperty = "text-indent";
+is(cs.textIndent, "50px",
+   "irrelevant change to transition property doesn't change transitioning");
+p.style.MozTransitionProperty = "font";
+is(cs.textIndent, "0px",
+   "relevant change to transition property does change transitioning");
+
+</script>
+</pre>
+</body>
+</html>