Bug 1285407 - Part 1: Pass a newly created nsStyleContext to GetAnimationRule and MaybeUpdateAnimationRule. r?birtles draft
authorHiroyuki Ikezoe <hiikezoe@mozilla-japan.org>
Mon, 11 Jul 2016 17:28:14 +0900
changeset 386113 710fed6e08324309a73d78e6d5c07f44185b12e9
parent 386060 179f6ebe5d409c0f3285b59219f9c1572de55690
child 386114 8c9daee7dbf9474d7e1940fa9a35491e2d8ad99f
push id22626
push userhiikezoe@mozilla-japan.org
push dateMon, 11 Jul 2016 08:29:02 +0000
reviewersbirtles
bugs1285407
milestone50.0a1
Bug 1285407 - Part 1: Pass a newly created nsStyleContext to GetAnimationRule and MaybeUpdateAnimationRule. r?birtles While resolving style context, the primary frame of the target element has previous style context so if we don't pass the newly created nsStyleContext, UpdateCascadeResults uses the previous style to get overridden properties, it will result unexpected cascading results. MozReview-Commit-ID: osqXQlP43X
dom/animation/EffectCompositor.cpp
dom/animation/EffectCompositor.h
layout/style/nsAnimationManager.cpp
layout/style/nsStyleSet.cpp
layout/style/nsTransitionManager.cpp
--- a/dom/animation/EffectCompositor.cpp
+++ b/dom/animation/EffectCompositor.cpp
@@ -235,21 +235,22 @@ EffectCompositor::UpdateEffectProperties
   for (KeyframeEffectReadOnly* effect : *effectSet) {
     effect->UpdateProperties(aStyleContext);
   }
 }
 
 void
 EffectCompositor::MaybeUpdateAnimationRule(dom::Element* aElement,
                                            CSSPseudoElementType aPseudoType,
-                                           CascadeLevel aCascadeLevel)
+                                           CascadeLevel aCascadeLevel,
+                                           nsStyleContext* aStyleContext)
 {
   // First update cascade results since that may cause some elements to
   // be marked as needing a restyle.
-  MaybeUpdateCascadeResults(aElement, aPseudoType);
+  MaybeUpdateCascadeResults(aElement, aPseudoType, aStyleContext);
 
   auto& elementsToRestyle = mElementsToRestyle[aCascadeLevel];
   PseudoElementHashEntry::KeyType key = { aElement, aPseudoType };
 
   if (!mPresContext || !elementsToRestyle.Contains(key)) {
     return;
   }
 
@@ -257,17 +258,18 @@ EffectCompositor::MaybeUpdateAnimationRu
                        mPresContext->RefreshDriver()->MostRecentRefresh());
 
   elementsToRestyle.Remove(key);
 }
 
 nsIStyleRule*
 EffectCompositor::GetAnimationRule(dom::Element* aElement,
                                    CSSPseudoElementType aPseudoType,
-                                   CascadeLevel aCascadeLevel)
+                                   CascadeLevel aCascadeLevel,
+                                   nsStyleContext* aStyleContext)
 {
   // NOTE: We need to be careful about early returns in this method where
   // we *don't* update mElementsToRestyle. When we get a call to
   // RequestRestyle that results in a call to PostRestyleForAnimation, we
   // will set a bool flag in mElementsToRestyle indicating that we've
   // called PostRestyleForAnimation so we don't need to call it again
   // until that restyle happens. During that restyle, if we arrive here
   // and *don't* update mElementsToRestyle we'll continue to skip calling
@@ -283,17 +285,17 @@ EffectCompositor::GetAnimationRule(dom::
              "EffectCompositor");
   if (mPresContext->RestyleManager()->AsGecko()->SkipAnimationRules()) {
     // We don't need to worry about updating mElementsToRestyle in this case
     // since this is not the animation restyle we requested when we called
     // PostRestyleForAnimation (see comment at start of this method).
     return nullptr;
   }
 
-  MaybeUpdateAnimationRule(aElement, aPseudoType, aCascadeLevel);
+  MaybeUpdateAnimationRule(aElement, aPseudoType, aCascadeLevel, aStyleContext);
 
 #ifdef DEBUG
   {
     auto& elementsToRestyle = mElementsToRestyle[aCascadeLevel];
     PseudoElementHashEntry::KeyType key = { aElement, aPseudoType };
     MOZ_ASSERT(!elementsToRestyle.Contains(key),
                "Element should no longer require a restyle after its "
                "animation rule has been updated");
@@ -450,17 +452,27 @@ EffectCompositor::MaybeUpdateCascadeResu
                                             CSSPseudoElementType aPseudoType,
                                             nsStyleContext* aStyleContext)
 {
   EffectSet* effects = EffectSet::GetEffectSet(aElement, aPseudoType);
   if (!effects || !effects->CascadeNeedsUpdate()) {
     return;
   }
 
-  UpdateCascadeResults(*effects, aElement, aPseudoType, aStyleContext);
+  nsStyleContext* styleContext = aStyleContext;
+  if (!styleContext) {
+    dom::Element* elementToRestyle = GetElementToRestyle(aElement, aPseudoType);
+    if (elementToRestyle) {
+      nsIFrame* frame = elementToRestyle->GetPrimaryFrame();
+      if (frame) {
+        styleContext = frame->StyleContext();
+      }
+    }
+  }
+  UpdateCascadeResults(*effects, aElement, aPseudoType, styleContext);
 
   MOZ_ASSERT(!effects->CascadeNeedsUpdate(), "Failed to update cascade state");
 }
 
 /* static */ void
 EffectCompositor::MaybeUpdateCascadeResults(Element* aElement,
                                             CSSPseudoElementType aPseudoType)
 {
@@ -793,17 +805,18 @@ EffectCompositor::AnimationStyleRuleProc
 
 void
 EffectCompositor::AnimationStyleRuleProcessor::RulesMatching(
   ElementRuleProcessorData* aData)
 {
   nsIStyleRule *rule =
     mCompositor->GetAnimationRule(aData->mElement,
                                   CSSPseudoElementType::NotPseudo,
-                                  mCascadeLevel);
+                                  mCascadeLevel,
+                                  nullptr);
   if (rule) {
     aData->mRuleWalker->Forward(rule);
     aData->mRuleWalker->CurrentNode()->SetIsAnimationRule();
   }
 }
 
 void
 EffectCompositor::AnimationStyleRuleProcessor::RulesMatching(
@@ -812,17 +825,18 @@ EffectCompositor::AnimationStyleRuleProc
   if (aData->mPseudoType != CSSPseudoElementType::before &&
       aData->mPseudoType != CSSPseudoElementType::after) {
     return;
   }
 
   nsIStyleRule *rule =
     mCompositor->GetAnimationRule(aData->mElement,
                                   aData->mPseudoType,
-                                  mCascadeLevel);
+                                  mCascadeLevel,
+                                  nullptr);
   if (rule) {
     aData->mRuleWalker->Forward(rule);
     aData->mRuleWalker->CurrentNode()->SetIsAnimationRule();
   }
 }
 
 void
 EffectCompositor::AnimationStyleRuleProcessor::RulesMatching(
--- a/dom/animation/EffectCompositor.h
+++ b/dom/animation/EffectCompositor.h
@@ -118,23 +118,40 @@ public:
   void UpdateEffectProperties(nsStyleContext* aStyleContext,
                               dom::Element* aElement,
                               CSSPseudoElementType aPseudoType);
 
   // Updates the animation rule stored on the EffectSet for the
   // specified (pseudo-)element for cascade level |aLevel|.
   // If the animation rule is not marked as needing an update,
   // no work is done.
+  // |aStyleContext| is used for UpdateCascadingResults.
+  // |aStyleContext| can be nullptr if style context, which is associated with
+  // the primary frame of the specified (pseudo-)element, is the current style
+  // context.
+  // If we are resolving a new style context, we shoud pass the newly created
+  // style context, otherwise we may use an old style context, it will result
+  // unexpected cascading results.
   void MaybeUpdateAnimationRule(dom::Element* aElement,
                                 CSSPseudoElementType aPseudoType,
-                                CascadeLevel aCascadeLevel);
+                                CascadeLevel aCascadeLevel,
+                                nsStyleContext *aStyleContext);
 
+  // We need to pass the newly resolved style context as |aStyleContext| when
+  // we call this function during resolving style context because this function
+  // calls UpdateCascadingResults with a style context if necessary, at the
+  // time, we end up using the previous style context if we don't pass the new
+  // style context.
+  // When we are not resolving style context, |aStyleContext| can be nullptr, we
+  // will use a style context associated with the primary frame of the specified
+  // (pseudo-)element.
   nsIStyleRule* GetAnimationRule(dom::Element* aElement,
                                  CSSPseudoElementType aPseudoType,
-                                 CascadeLevel aCascadeLevel);
+                                 CascadeLevel aCascadeLevel,
+                                 nsStyleContext* aStyleContext);
 
   bool HasPendingStyleUpdates() const;
   bool HasThrottledStyleUpdates() const;
 
   // Tell the restyle tracker about all the animated styles that have
   // pending updates so that it can update the animation rule for these
   // elements.
   void AddStyleUpdatesTo(RestyleTracker& aTracker);
@@ -153,16 +170,18 @@ public:
 
   static void ClearIsRunningOnCompositor(const nsIFrame* aFrame,
                                          nsCSSProperty aProperty);
 
   // Update animation cascade results for the specified (pseudo-)element
   // but only if we have marked the cascade as needing an update due a
   // the change in the set of effects or a change in one of the effects'
   // "in effect" state.
+  // |aStyleContext| may be nullptr in which case we will use the
+  // nsStyleContext of the primary frame of the specified (pseudo-)element.
   //
   // This method does NOT detect if other styles that apply above the
   // animation level of the cascade have changed.
   static void
   MaybeUpdateCascadeResults(dom::Element* aElement,
                             CSSPseudoElementType aPseudoType,
                             nsStyleContext* aStyleContext);
 
--- a/layout/style/nsAnimationManager.cpp
+++ b/layout/style/nsAnimationManager.cpp
@@ -442,17 +442,18 @@ nsAnimationManager::UpdateAnimations(nsS
 
   EffectCompositor::UpdateCascadeResults(aElement,
                                          aStyleContext->GetPseudoType(),
                                          aStyleContext);
 
   mPresContext->EffectCompositor()->
     MaybeUpdateAnimationRule(aElement,
                              aStyleContext->GetPseudoType(),
-                             EffectCompositor::CascadeLevel::Animations);
+                             EffectCompositor::CascadeLevel::Animations,
+                             aStyleContext);
 
   // We don't actually dispatch the pending events now.  We'll either
   // dispatch them the next time we get a refresh driver notification
   // or the next time somebody calls
   // nsPresShell::FlushPendingNotifications.
   if (mEventDispatcher.HasQueuedEvents()) {
     mPresContext->Document()->SetNeedStyleFlush();
   }
--- a/layout/style/nsStyleSet.cpp
+++ b/layout/style/nsStyleSet.cpp
@@ -949,17 +949,18 @@ nsStyleSet::GetContext(nsStyleContext* a
       PresContext()->AnimationManager()->UpdateAnimations(result,
                                                           aElementForAnimation);
       PresContext()->EffectCompositor()->UpdateEffectProperties(
         result, aElementForAnimation, result->GetPseudoType());
 
       animRule = PresContext()->EffectCompositor()->
                    GetAnimationRule(aElementForAnimation,
                                     result->GetPseudoType(),
-                                    EffectCompositor::CascadeLevel::Animations);
+                                    EffectCompositor::CascadeLevel::Animations,
+                                    result);
     }
 
     MOZ_ASSERT(result->RuleNode() == aRuleNode,
                "unexpected rule node");
     MOZ_ASSERT(!result->GetStyleIfVisited() == !aVisitedRuleNode,
                "unexpected visited rule node");
     MOZ_ASSERT(!aVisitedRuleNode ||
                result->GetStyleIfVisited()->RuleNode() == aVisitedRuleNode,
@@ -1534,31 +1535,33 @@ nsStyleSet::RuleNodeWithReplacement(Elem
     if (doReplace) {
       switch (level->mLevel) {
         case SheetType::Animation: {
           if (aPseudoType == CSSPseudoElementType::NotPseudo ||
               aPseudoType == CSSPseudoElementType::before ||
               aPseudoType == CSSPseudoElementType::after) {
             nsIStyleRule* rule = PresContext()->EffectCompositor()->
               GetAnimationRule(aElement, aPseudoType,
-                               EffectCompositor::CascadeLevel::Animations);
+                               EffectCompositor::CascadeLevel::Animations,
+                               nullptr);
             if (rule) {
               ruleWalker.ForwardOnPossiblyCSSRule(rule);
               ruleWalker.CurrentNode()->SetIsAnimationRule();
             }
           }
           break;
         }
         case SheetType::Transition: {
           if (aPseudoType == CSSPseudoElementType::NotPseudo ||
               aPseudoType == CSSPseudoElementType::before ||
               aPseudoType == CSSPseudoElementType::after) {
             nsIStyleRule* rule = PresContext()->EffectCompositor()->
               GetAnimationRule(aElement, aPseudoType,
-                               EffectCompositor::CascadeLevel::Transitions);
+                               EffectCompositor::CascadeLevel::Transitions,
+                               nullptr);
             if (rule) {
               ruleWalker.ForwardOnPossiblyCSSRule(rule);
               ruleWalker.CurrentNode()->SetIsAnimationRule();
             }
           }
           break;
         }
         case SheetType::SVGAttrAnimation: {
--- a/layout/style/nsTransitionManager.cpp
+++ b/layout/style/nsTransitionManager.cpp
@@ -421,17 +421,18 @@ nsTransitionManager::StyleContextChanged
 
   if (collection) {
     EffectCompositor::UpdateCascadeResults(aElement, pseudoType,
                                            newStyleContext);
 
     collection->UpdateCheckGeneration(mPresContext);
     mPresContext->EffectCompositor()->MaybeUpdateAnimationRule(aElement,
                                                                pseudoType,
-                                                               cascadeLevel);
+                                                               cascadeLevel,
+                                                               newStyleContext);
   }
 
   // We want to replace the new style context with the after-change style.
   *aNewStyleContext = afterChangeStyle;
   if (collection) {
     // Since we have transition styles, we have to undo this replacement.
     // The check of collection->mCheckGeneration against the restyle
     // manager's GetAnimationGeneration() will ensure that we don't go