Bug 573241. When reparenting a style context, don't include any transition rules the old context might have had in the new one, since that can confuse transitions. r=dbaron
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 23 Jun 2010 00:46:27 -0400
changeset 44166 54d47e708c9cf0e4c8e9b79ff9c381c5a289e90b
parent 44165 5dd887fd9d3e93ab2d0900a99964a3dc4252dd87
child 44167 13a9aecb404f8bd81656649a2de53f7c89f8661e
push id14005
push userbzbarsky@mozilla.com
push dateWed, 23 Jun 2010 04:51:54 +0000
treeherdermozilla-central@13a9aecb404f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdbaron
bugs573241
milestone1.9.3a6pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 573241. When reparenting a style context, don't include any transition rules the old context might have had in the new one, since that can confuse transitions. r=dbaron
layout/base/nsFrameManager.cpp
layout/style/nsStyleSet.cpp
layout/style/nsStyleSet.h
layout/style/test/test_transitions.html
--- a/layout/base/nsFrameManager.cpp
+++ b/layout/base/nsFrameManager.cpp
@@ -113,16 +113,17 @@
     #define NOISY_TRACE_FRAME(_msg,_frame) \
       printf("%s ",_msg); nsFrame::ListTag(stdout,_frame); printf("\n");
   #else
     #define NOISY_TRACE(_msg);
     #define NOISY_TRACE_FRAME(_msg,_frame);
   #endif
 
 using namespace mozilla;
+using namespace mozilla::dom;
 
 //----------------------------------------------------------------------
 
 struct PlaceholderMapEntry : public PLDHashEntryHdr {
   // key (the out of flow frame) can be obtained through placeholder frame
   nsPlaceholderFrame *placeholderFrame;
 };
 
@@ -730,16 +731,47 @@ TryStartingTransition(nsPresContext *aPr
   if (coverRule) {
     nsCOMArray<nsIStyleRule> rules;
     rules.AppendObject(coverRule);
     *aNewStyleContext = aPresContext->StyleSet()->
                           ResolveStyleByAddingRules(*aNewStyleContext, rules);
   }
 }
 
+static inline Element*
+ElementForStyleContext(nsIContent* aParentContent,
+                       nsIFrame* aFrame,
+                       nsCSSPseudoElements::Type aPseudoType)
+{
+  // We don't expect XUL tree stuff here.
+  NS_PRECONDITION(aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement ||
+                  aPseudoType == nsCSSPseudoElements::ePseudo_AnonBox ||
+                  aPseudoType < nsCSSPseudoElements::ePseudo_PseudoElementCount,
+                  "Unexpected pseudo");
+  // XXX see the comments about the various element confusion in
+  // ReResolveStyleContext.
+  if (aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement) {
+    return aFrame->GetContent()->AsElement();
+  }
+
+  if (aPseudoType == nsCSSPseudoElements::ePseudo_AnonBox) {
+    return nsnull;
+  }
+
+  if (aPseudoType == nsCSSPseudoElements::ePseudo_firstLetter) {
+    NS_ASSERTION(aFrame->GetType() == nsGkAtoms::letterFrame,
+                 "firstLetter pseudoTag without a nsFirstLetterFrame");
+    nsBlockFrame* block = nsBlockFrame::GetNearestAncestorBlock(aFrame);
+    return block->GetContent()->AsElement();
+  }
+
+  nsIContent* content = aParentContent ? aParentContent : aFrame->GetContent();
+  return content->AsElement();
+}
+
 nsresult
 nsFrameManager::ReparentStyleContext(nsIFrame* aFrame)
 {
   if (nsGkAtoms::placeholderFrame == aFrame->GetType()) {
     // Also reparent the out-of-flow
     nsIFrame* outOfFlow =
       nsPlaceholderFrame::GetRealFrameForPlaceholder(aFrame);
     NS_ASSERTION(outOfFlow, "no out-of-flow frame");
@@ -808,18 +840,24 @@ nsFrameManager::ReparentStyleContext(nsI
        prevContinuationContext->GetParent() == newParentContext;
     if (copyFromContinuation) {
       // Just use the style context from the frame's previous
       // continuation (see assertion about aFrame->GetNextContinuation()
       // above, which we would have previously hit for aFrame's previous
       // continuation).
       newContext = prevContinuationContext;
     } else {
+      nsIFrame* parentFrame = aFrame->GetParent();
+      Element* element =
+        ElementForStyleContext(parentFrame ? parentFrame->GetContent() : nsnull,
+                               aFrame,
+                               oldContext->GetPseudoType());
       newContext = mStyleSet->ReparentStyleContext(oldContext,
-                                                   newParentContext);
+                                                   newParentContext,
+                                                   element);
     }
 
     if (newContext) {
       if (newContext != oldContext) {
         // We probably don't want to initiate transitions from
         // ReparentStyleContext, since we call it during frame
         // construction rather than in response to dynamic changes.
         // Also see the comment at the start of
@@ -892,17 +930,18 @@ nsFrameManager::ReparentStyleContext(nsI
         // do additional contexts 
         PRInt32 contextIndex = -1;
         while (1) {
           nsStyleContext* oldExtraContext =
             aFrame->GetAdditionalStyleContext(++contextIndex);
           if (oldExtraContext) {
             nsRefPtr<nsStyleContext> newExtraContext;
             newExtraContext = mStyleSet->ReparentStyleContext(oldExtraContext,
-                                                              newContext);
+                                                              newContext,
+                                                              nsnull);
             if (newExtraContext) {
               if (newExtraContext != oldExtraContext) {
                 // Make sure to call CalcStyleDifference so that the new
                 // context ends up resolving all the structs the old context
                 // resolved.
                 styleChange =
                   oldExtraContext->CalcStyleDifference(newExtraContext);
                 // The style change is always 0 because we have the same
@@ -1114,69 +1153,66 @@ nsFrameManager::ReResolveStyleContext(ns
        prevContinuationContext->GetParent() == parentContext;
     if (copyFromContinuation) {
       // Just use the style context from the frame's previous
       // continuation (see assertion about aFrame->GetNextContinuation()
       // above, which we would have previously hit for aFrame's previous
       // continuation).
       newContext = prevContinuationContext;
     }
-    else if (!aRestyleHint) {
-      newContext = styleSet->ReparentStyleContext(oldContext, parentContext);
-    }
     else if (pseudoTag == nsCSSAnonBoxes::mozNonElement) {
       NS_ASSERTION(localContent,
                    "non pseudo-element frame without content node");
       newContext = styleSet->ResolveStyleForNonElement(parentContext);
     }
-    else if (pseudoTag) {
-      // XXXldb This choice of pseudoContent seems incorrect for anon
-      // boxes and perhaps other cases.
-      // See also the comment above the assertion at the start of this
-      // function.
-      nsIContent* pseudoContent =
-          aParentContent ? aParentContent : localContent;
-      if (pseudoTag == nsCSSPseudoElements::before ||
-          pseudoTag == nsCSSPseudoElements::after) {
-        // XXX what other pseudos do we need to treat like this?
-        newContext = styleSet->ProbePseudoElementStyle(pseudoContent->AsElement(),
-                                                       pseudoType,
-                                                       parentContext);
-        if (!newContext) {
-          // This pseudo should no longer exist; gotta reframe
-          NS_UpdateHint(aMinChange, nsChangeHint_ReconstructFrame);
-          aChangeList->AppendChange(aFrame, pseudoContent,
-                                    nsChangeHint_ReconstructFrame);
-          // We're reframing anyway; just keep the same context
-          newContext = oldContext;
-        }
-      } else if (pseudoType == nsCSSPseudoElements::ePseudo_AnonBox) {
-        newContext = styleSet->ResolveAnonymousBoxStyle(pseudoTag,
-                                                        parentContext);
-      } else {
-        // Don't expect XUL tree stuff here, since it needs a comparator and
-        // all.
-        NS_ASSERTION(pseudoType <
-                       nsCSSPseudoElements::ePseudo_PseudoElementCount,
-                     "Unexpected pseudo type");
-        if (pseudoTag == nsCSSPseudoElements::firstLetter) {
-          NS_ASSERTION(aFrame->GetType() == nsGkAtoms::letterFrame,
-                       "firstLetter pseudoTag without a nsFirstLetterFrame");
-          nsBlockFrame* block = nsBlockFrame::GetNearestAncestorBlock(aFrame);
-          pseudoContent = block->GetContent();
-        }
-        newContext = styleSet->ResolvePseudoElementStyle(pseudoContent->AsElement(),
+    else if (!aRestyleHint) {
+      newContext =
+        styleSet->ReparentStyleContext(oldContext, parentContext,
+                                       ElementForStyleContext(aParentContent,
+                                                              aFrame,
+                                                              pseudoType));
+    } else if (pseudoType == nsCSSPseudoElements::ePseudo_AnonBox) {
+      newContext = styleSet->ResolveAnonymousBoxStyle(pseudoTag,
+                                                      parentContext);
+    }
+    else {
+      Element* element = ElementForStyleContext(aParentContent,
+                                                aFrame,
+                                                pseudoType);
+      if (pseudoTag) {
+        if (pseudoTag == nsCSSPseudoElements::before ||
+            pseudoTag == nsCSSPseudoElements::after) {
+          // XXX what other pseudos do we need to treat like this?
+          newContext = styleSet->ProbePseudoElementStyle(element,
                                                          pseudoType,
                                                          parentContext);
+          if (!newContext) {
+            // This pseudo should no longer exist; gotta reframe
+            NS_UpdateHint(aMinChange, nsChangeHint_ReconstructFrame);
+            aChangeList->AppendChange(aFrame, element,
+                                      nsChangeHint_ReconstructFrame);
+            // We're reframing anyway; just keep the same context
+            newContext = oldContext;
+          }
+        } else {
+          // Don't expect XUL tree stuff here, since it needs a comparator and
+          // all.
+          NS_ASSERTION(pseudoType <
+                         nsCSSPseudoElements::ePseudo_PseudoElementCount,
+                       "Unexpected pseudo type");
+          newContext = styleSet->ResolvePseudoElementStyle(element,
+                                                           pseudoType,
+                                                           parentContext);
+        }
       }
-    }
-    else {
-      NS_ASSERTION(localContent,
-                   "non pseudo-element frame without content node");
-      newContext = styleSet->ResolveStyleFor(content->AsElement(), parentContext);
+      else {
+        NS_ASSERTION(localContent,
+                     "non pseudo-element frame without content node");
+        newContext = styleSet->ResolveStyleFor(element, parentContext);
+      }
     }
 
     NS_ASSERTION(newContext, "failed to get new style context");
     if (newContext) {
       if (!parentContext) {
         if (oldContext->GetRuleNode() == newContext->GetRuleNode()) {
           // We're the root of the style context tree and the new style
           // context returned has the same rule node.  This means that
@@ -1288,17 +1324,18 @@ nsFrameManager::ReResolveStyleContext(ns
         }
         nsRefPtr<nsStyleContext> undisplayedContext;
         if (thisChildHint) {
           undisplayedContext =
             styleSet->ResolveStyleFor(undisplayed->mContent->AsElement(),
                                       newContext);
         } else {
           undisplayedContext =
-            styleSet->ReparentStyleContext(undisplayed->mStyle, newContext);
+            styleSet->ReparentStyleContext(undisplayed->mStyle, newContext,
+                                           undisplayed->mContent->AsElement());
         }
         if (undisplayedContext) {
           const nsStyleDisplay* display = undisplayedContext->GetStyleDisplay();
           if (display->mDisplay != NS_STYLE_DISPLAY_NONE) {
             NS_ASSERTION(undisplayed->mContent,
                          "Must have undisplayed content");
             aChangeList->AppendChange(nsnull, undisplayed->mContent, 
                                       NS_STYLE_HINT_FRAMECHANGE);
--- a/layout/style/nsStyleSet.cpp
+++ b/layout/style/nsStyleSet.cpp
@@ -1129,41 +1129,76 @@ nsStyleSet::GCRuleTrees()
       // It was deleted, as it should be.
       mOldRuleTrees.RemoveElementAt(i);
     } else {
       NS_NOTREACHED("old rule tree still referenced");
     }
   }
 }
 
+static inline nsRuleNode*
+SkipTransitionRules(nsRuleNode* aRuleNode, Element* aElement)
+{
+  nsRuleNode* ruleNode = aRuleNode;
+  while (!ruleNode->IsRoot() &&
+         ruleNode->GetLevel() == nsStyleSet::eTransitionSheet) {
+    ruleNode = ruleNode->GetParent();
+  }
+  if (ruleNode != aRuleNode) {
+    NS_ASSERTION(aElement, "How can we have transition rules but no element?");
+    // Need to do an animation restyle, just like
+    // nsTransitionManager::WalkTransitionRule would.
+    aRuleNode->GetPresContext()->PresShell()->RestyleForAnimation(aElement);
+  }
+  return ruleNode;
+}
+
 already_AddRefed<nsStyleContext>
 nsStyleSet::ReparentStyleContext(nsStyleContext* aStyleContext,
-                                 nsStyleContext* aNewParentContext)
+                                 nsStyleContext* aNewParentContext,
+                                 Element* aElement)
 {
   if (!aStyleContext) {
     NS_NOTREACHED("must have style context");
     return nsnull;
   }
 
+  // This short-circuit is OK because we don't call TryStartingTransition
+  // during style reresolution if the style context pointer hasn't changed.
   if (aStyleContext->GetParent() == aNewParentContext) {
     aStyleContext->AddRef();
     return aStyleContext;
   }
 
   nsIAtom* pseudoTag = aStyleContext->GetPseudo();
   nsCSSPseudoElements::Type pseudoType = aStyleContext->GetPseudoType();
   nsRuleNode* ruleNode = aStyleContext->GetRuleNode();
+
+  // Skip transition rules as needed just like
+  // nsTransitionManager::WalkTransitionRule would.
+  PRBool skipTransitionRules = PresContext()->IsProcessingRestyles() &&
+    !PresContext()->IsProcessingAnimationStyleChange();
+  if (skipTransitionRules) {
+    // Make sure that we're not using transition rules for our new style
+    // context.  If we need them, an animation restyle will provide.
+    ruleNode = SkipTransitionRules(ruleNode, aElement);
+  }
+
   nsRuleNode* visitedRuleNode = nsnull;
   nsStyleContext* visitedContext = aStyleContext->GetStyleIfVisited();
   // Reparenting a style context just changes where we inherit from,
   // not what rules we match or what our DOM looks like.  In
   // particular, it doesn't change whether this is a style context for
   // a link.
   if (visitedContext) {
      visitedRuleNode = visitedContext->GetRuleNode();
+     // Again, skip transition rules as needed
+     if (skipTransitionRules) {
+       visitedRuleNode = SkipTransitionRules(visitedRuleNode, aElement);
+     }
   }
 
   return GetContext(aNewParentContext, ruleNode, visitedRuleNode,
                     aStyleContext->IsLinkContext(),
                     aStyleContext->RelevantLinkVisited(),
                     pseudoTag, pseudoType);
 }
 
--- a/layout/style/nsStyleSet.h
+++ b/layout/style/nsStyleSet.h
@@ -169,19 +169,23 @@ class nsStyleSet
 
   // Notification that a style context is being destroyed.
   void NotifyStyleContextDestroyed(nsPresContext* aPresContext,
                                    nsStyleContext* aStyleContext);
 
   // Get a new style context that lives in a different parent
   // The new context will be the same as the old if the new parent is the
   // same as the old parent.
+  // aElement should be non-null if this is a style context for an
+  // element or pseudo-element; in the latter case it should be the
+  // real element the pseudo-element is for.
   already_AddRefed<nsStyleContext>
   ReparentStyleContext(nsStyleContext* aStyleContext,
-                       nsStyleContext* aNewParentContext);
+                       nsStyleContext* aNewParentContext,
+                       mozilla::dom::Element* aElement);
 
   // Test if style is dependent on a document state.
   PRBool HasDocumentStateDependentStyle(nsPresContext* aPresContext,
                                         nsIContent*    aContent,
                                         PRInt32        aStateMask);
 
   // Test if style is dependent on content state
   nsRestyleHint HasStateDependentStyle(nsPresContext* aPresContext,
--- a/layout/style/test/test_transitions.html
+++ b/layout/style/test/test_transitions.html
@@ -160,34 +160,41 @@ for (var tf in timingFunctions) {
     is(getComputedStyle(p, "").textIndent, "0px",
        "should be zero before changing value");
     tftests.push([ p, tf ]);
 }
 
 // Check that the timing function continues even when we restyle in the
 // middle.
 var interrupt_tests = [];
-for (var itime = 2; itime < 8; itime += 2) {
-    var p = document.createElement("p");
-    var t = document.createTextNode("interrupt at " + itime + "s");
-    p.appendChild(t);
-    p.style.textIndent = "0px";
-    p.style.MozTransition = "8s text-indent cubic-bezier(0, 1, 1, 0)";
-    div.appendChild(p);
-    is(getComputedStyle(p, "").textIndent, "0px",
-       "should be zero before changing value");
-    interrupt_tests.push([ p, itime ]);
-    
+for each (var restyleParent in [true, false]) {
+    for (var itime = 2; itime < 8; itime += 2) {
+        var p = document.createElement("p");
+        var t = document.createTextNode("interrupt on " +
+                                        (restyleParent ? "parent" : "node itself") +
+                                        " at " + itime + "s");
+        p.appendChild(t);
+        p.style.textIndent = "0px";
+        p.style.MozTransition = "8s text-indent cubic-bezier(0, 1, 1, 0)";
+        if (restyleParent) {
+          var d = document.createElement("div");
+          d.appendChild(p);
+          div.appendChild(d);
+        } else {
+          div.appendChild(p);
+        }
+        is(getComputedStyle(p, "").textIndent, "0px",
+           "should be zero before changing value");
+        setTimeout("interrupt_tests[" + interrupt_tests.length + "]" +
+                   "[0]" + (restyleParent ? ".parentNode" : "") +
+                   ".style.color = 'blue';" +
+                   "check_interrupt_tests()", itime*1000);
+        interrupt_tests.push([ p, itime ]);
+    }
 }
-setTimeout("interrupt_tests[0][0].style.color = 'blue';" +
-           "check_interrupt_tests()", 2000);
-setTimeout("interrupt_tests[1][0].style.color = 'blue';" +
-           "check_interrupt_tests()", 4000);
-setTimeout("interrupt_tests[2][0].style.color = 'blue';" +
-           "check_interrupt_tests()", 6000);
 
 // Test transition-delay values of -4s through 4s on a 4s transition
 // with 'ease-out' timing function.
 var delay_tests = {};
 for (var d = -4; d <= 4; ++d) {
     var p = document.createElement("p");
     var delay = d + "s";
     var t = document.createTextNode("transition-delay: " + delay);
@@ -509,17 +516,19 @@ function check_interrupt_tests()
 {
     for (var test in interrupt_tests) {
         var p = interrupt_tests[test][0];
         var itime = interrupt_tests[test][1];
 
         check_transition_value(timingFunctions["cubic-bezier(0, 1, 1, 0)"],
                                0, 8, 0, 100,
                                getComputedStyle(p, "").textIndent,
-                               "interrupt test for time " + itime + "s");
+                               "interrupt " +
+                               (p.parentNode == div ? "" : "on parent ") +
+                               "test for time " + itime + "s");
     }
 }
 
 // check_interrupt_tests is called from check_tf_test and from
 // where we reset the interrupts
 
 function check_delay_test(time)
 {