Fix css3-animations handling of properties that are not present in all keyframes to match WebKit and generally be more sensible. (Bug 649400) r=bzbarsky

This inverts the relationship between segments and properties in the
animation data structures:  now each property has a set of segments,
since the segments differ between properties.

Furthermore, we now handle inability to interpolate between values by
dropping the entire property rather than dropping a single segment.

layout/style/Declaration.h

@@ -251,10 +251,12 @@ private:
                                       nsAutoString& aValue,
                                       nsAString& aResult) const;
 
+public:
   nsCSSProperty OrderValueAt(PRUint32 aValue) const {
     return nsCSSProperty(mOrder.ElementAt(aValue));
   }
 
+private:
   nsAutoTArray<PRUint8, 8> mOrder;
 
   // never null, except while expanded, or before the first call to

layout/style/nsAnimationManager.cpp

@@ -47,17 +47,17 @@
 
 using namespace mozilla;
 
-struct AnimationSegmentProperty
-{
-  nsCSSProperty mProperty;
-  nsStyleAnimation::Value mFromValue, mToValue;
-};
-
-struct AnimationSegment
+struct AnimationPropertySegment
 {
   float mFromKey, mToKey;
+  nsStyleAnimation::Value mFromValue, mToValue;
   css::ComputedTimingFunction mTimingFunction;
-  InfallibleTArray<AnimationSegmentProperty> mProperties;
+};
+
+struct AnimationProperty
+{
+  nsCSSProperty mProperty;
+  InfallibleTArray<AnimationPropertySegment> mSegments;
 };
 
 /**
@@ -102,7 +102,7 @@ struct ElementAnimation
   // whose start we last notified on.
   PRUint32 mLastNotification;
 
-  InfallibleTArray<AnimationSegment> mSegments;
+  InfallibleTArray<AnimationProperty> mProperties;
 };
 
 typedef nsAnimationManager::EventArray EventArray;
@@ -181,15 +181,14 @@ ElementAnimations::EnsureStyleRuleFor(TimeStamp aRefreshTime,
     mStyleRule = nsnull;
 
     // FIXME(spec): assume that properties in higher animations override
-    // those in lower ones (and that our |HasProperty| check in
-    // |BuildSegment| matches the definition of when they should do so.
+    // those in lower ones.
     // Therefore, we iterate from last animation to first.
     nsCSSPropertySet properties;
 
     for (PRUint32 animIdx = mAnimations.Length(); animIdx-- != 0; ) {
       ElementAnimation &anim = mAnimations[animIdx];
 
-      if (anim.mSegments.Length() == 0 ||
+      if (anim.mProperties.Length() == 0 ||
           anim.mIterationDuration.ToMilliseconds() <= 0.0) {
         // No animation data.
         continue;
@@ -278,51 +277,52 @@ ElementAnimations::EnsureStyleRuleFor(TimeStamp aRefreshTime,
                           positionInIteration <= 1.0,
                         "position should be in [0-1]");
 
-      NS_ABORT_IF_FALSE(anim.mSegments[0].mFromKey == 0.0,
-                        "incorrect first from key");
-      NS_ABORT_IF_FALSE(anim.mSegments[anim.mSegments.Length() - 1].mToKey
-                          == 1.0,
-                        "incorrect last to key");
+      for (PRUint32 propIdx = 0, propEnd = anim.mProperties.Length();
+           propIdx != propEnd; ++propIdx)
+      {
+        const AnimationProperty &prop = anim.mProperties[propIdx];
 
-      // FIXME: Maybe cache the current segment?
-      const AnimationSegment *segment = anim.mSegments.Elements();
-      while (segment->mToKey < positionInIteration) {
-        NS_ABORT_IF_FALSE(segment->mFromKey < segment->mToKey,
-                          "incorrect keys");
-        ++segment;
-        NS_ABORT_IF_FALSE(segment->mFromKey == (segment-1)->mToKey,
-                          "incorrect keys");
-      }
-      NS_ABORT_IF_FALSE(segment->mFromKey < segment->mToKey,
-                        "incorrect keys");
-      NS_ABORT_IF_FALSE(segment - anim.mSegments.Elements() <
-                          anim.mSegments.Length(),
-                        "ran off end");
+        NS_ABORT_IF_FALSE(prop.mSegments[0].mFromKey == 0.0,
+                          "incorrect first from key");
+        NS_ABORT_IF_FALSE(prop.mSegments[prop.mSegments.Length() - 1].mToKey
+                            == 1.0,
+                          "incorrect last to key");
 
-      if (segment->mProperties.IsEmpty()) {
-        // No animation data.
-        continue;
-      }
-
-      if (!mStyleRule) {
-        // Allocate the style rule now that we know we have animation data.
-        mStyleRule = new css::AnimValuesStyleRule();
-      }
-
-      double positionInSegment = (positionInIteration - segment->mFromKey) /
-                                 (segment->mToKey - segment->mFromKey);
-      double valuePosition =
-        segment->mTimingFunction.GetValue(positionInSegment);
-
-      for (PRUint32 propIdx = 0, propEnd = segment->mProperties.Length();
-           propIdx != propEnd; ++propIdx) {
-        const AnimationSegmentProperty &prop = segment->mProperties[propIdx];
         if (properties.HasProperty(prop.mProperty)) {
           // A later animation already set this property.
           continue;
         }
         properties.AddProperty(prop.mProperty);
 
+        NS_ABORT_IF_FALSE(prop.mSegments.Length() > 0,
+                          "property should not be in animations if it "
+                          "has no segments");
+
+        // FIXME: Maybe cache the current segment?
+        const AnimationPropertySegment *segment = prop.mSegments.Elements();
+        while (segment->mToKey < positionInIteration) {
+          NS_ABORT_IF_FALSE(segment->mFromKey < segment->mToKey,
+                            "incorrect keys");
+          ++segment;
+          NS_ABORT_IF_FALSE(segment->mFromKey == (segment-1)->mToKey,
+                            "incorrect keys");
+        }
+        NS_ABORT_IF_FALSE(segment->mFromKey < segment->mToKey,
+                          "incorrect keys");
+        NS_ABORT_IF_FALSE(segment - prop.mSegments.Elements() <
+                            prop.mSegments.Length(),
+                          "ran off end");
+
+        if (!mStyleRule) {
+          // Allocate the style rule now that we know we have animation data.
+          mStyleRule = new css::AnimValuesStyleRule();
+        }
+
+        double positionInSegment = (positionInIteration - segment->mFromKey) /
+                                   (segment->mToKey - segment->mFromKey);
+        double valuePosition =
+          segment->mTimingFunction.GetValue(positionInSegment);
+
         nsStyleAnimation::Value *val =
           mStyleRule->AddEmptyValue(prop.mProperty);
 
@@ -330,7 +330,7 @@ ElementAnimations::EnsureStyleRuleFor(TimeStamp aRefreshTime,
         PRBool result =
 #endif
           nsStyleAnimation::Interpolate(prop.mProperty,
-                                        prop.mFromValue, prop.mToValue,
+                                        segment->mFromValue, segment->mToValue,
                                         valuePosition, *val);
         NS_ABORT_IF_FALSE(result, "interpolate must succeed now");
       }
@@ -610,7 +610,7 @@ ResolvedStyleCache::Get(nsPresContext *aPresContext,
   // whether they are resolved relative to other animations:  I assume
   // that they're not, since that would prevent us from caching a lot of
   // data that we'd really like to cache (in particular, the
-  // nsStyleAnimation::Value values in AnimationSegmentProperty).
+  // nsStyleAnimation::Value values in AnimationPropertySegment).
   nsStyleContext *result = mCache.GetWeak(aKeyframe);
   if (!result) {
     nsCOMArray<nsIStyleRule> rules;
@@ -695,55 +695,111 @@ nsAnimationManager::BuildAnimations(nsStyleContext* aStyleContext,
       continue;
     }
 
-    KeyframeData fromKeyframe = sortedKeyframes[0];
-    nsRefPtr<nsStyleContext> fromContext =
-      resolvedStyles.Get(mPresContext, aStyleContext,
-                         fromKeyframe.mRule);
+    // Record the properties that are present in any keyframe rules we
+    // are using.
+    nsCSSPropertySet properties;
 
-    // If there's no rule for 0%, there's implicitly an empty rule.
-    if (fromKeyframe.mKey != 0.0f) {
-      BuildSegment(aDest.mSegments, aSrc,
-                   0.0f, aStyleContext, nsnull,
-                   fromKeyframe.mKey, fromContext,
-                     fromKeyframe.mRule->Declaration());
-    }
-
-    for (PRUint32 kfIdx = 1, kfEnd = sortedKeyframes.Length();
+    for (PRUint32 kfIdx = 0, kfEnd = sortedKeyframes.Length();
          kfIdx != kfEnd; ++kfIdx) {
-      KeyframeData toKeyframe = sortedKeyframes[kfIdx];
-      nsRefPtr<nsStyleContext> toContext =
-        resolvedStyles.Get(mPresContext, aStyleContext, toKeyframe.mRule);
-
-      BuildSegment(aDest.mSegments, aSrc,
-                   fromKeyframe.mKey, fromContext,
-                   fromKeyframe.mRule->Declaration(),
-                   toKeyframe.mKey, toContext,
-                   toKeyframe.mRule->Declaration());
-
-      fromContext = toContext;
-      fromKeyframe = toKeyframe;
+      css::Declaration *decl = sortedKeyframes[kfIdx].mRule->Declaration();
+      for (PRUint32 propIdx = 0, propEnd = decl->Count();
+           propIdx != propEnd; ++propIdx) {
+        properties.AddProperty(decl->OrderValueAt(propIdx));
+      }
     }
 
-    // If there's no rule for 100%, there's implicitly an empty rule.
-    if (fromKeyframe.mKey != 1.0f) {
-      BuildSegment(aDest.mSegments, aSrc,
-                   fromKeyframe.mKey, fromContext,
-                   fromKeyframe.mRule->Declaration(),
-                   1.0f, aStyleContext, nsnull);
+    for (nsCSSProperty prop = nsCSSProperty(0);
+         prop < eCSSProperty_COUNT_no_shorthands;
+         prop = nsCSSProperty(prop + 1)) {
+      if (!properties.HasProperty(prop) ||
+          nsCSSProps::kAnimTypeTable[prop] == eStyleAnimType_None) {
+        continue;
+      }
+
+      AnimationProperty &propData = *aDest.mProperties.AppendElement();
+      propData.mProperty = prop;
+
+      KeyframeData *fromKeyframe = nsnull;
+      nsRefPtr<nsStyleContext> fromContext;
+      bool interpolated = true;
+      for (PRUint32 kfIdx = 0, kfEnd = sortedKeyframes.Length();
+           kfIdx != kfEnd; ++kfIdx) {
+        KeyframeData &toKeyframe = sortedKeyframes[kfIdx];
+        if (!toKeyframe.mRule->Declaration()->HasProperty(prop)) {
+          continue;
+        }
+
+        nsRefPtr<nsStyleContext> toContext =
+          resolvedStyles.Get(mPresContext, aStyleContext, toKeyframe.mRule);
+
+        if (fromKeyframe) {
+          interpolated = interpolated &&
+            BuildSegment(propData.mSegments, prop, aSrc,
+                         fromKeyframe->mKey, fromContext,
+                         fromKeyframe->mRule->Declaration(),
+                         toKeyframe.mKey, toContext);
+        } else {
+          if (toKeyframe.mKey != 0.0f) {
+            // There's no data for this property at 0%, so use the
+            // cascaded value above us.
+            interpolated = interpolated &&
+              BuildSegment(propData.mSegments, prop, aSrc,
+                           0.0f, aStyleContext, nsnull,
+                           toKeyframe.mKey, toContext);
+          }
+        }
+
+        fromContext = toContext;
+        fromKeyframe = &toKeyframe;
+      }
+
+      if (fromKeyframe->mKey != 1.0f) {
+        // There's no data for this property at 100%, so use the
+        // cascaded value above us.
+        interpolated = interpolated &&
+          BuildSegment(propData.mSegments, prop, aSrc,
+                       fromKeyframe->mKey, fromContext,
+                       fromKeyframe->mRule->Declaration(),
+                       1.0f, aStyleContext);
+      }
+
+      // If we failed to build any segments due to inability to
+      // interpolate, remove the property from the animation.  (It's not
+      // clear if this is the right thing to do -- we could run some of
+      // the segments, but it's really not clear whether we should skip
+      // values (which?) or skip segments, so best to skip the whole
+      // thing for now.)
+      if (!interpolated) {
+        aDest.mProperties.RemoveElementAt(aDest.mProperties.Length() - 1);
+      }
     }
   }
 }
 
-void
-nsAnimationManager::BuildSegment(InfallibleTArray<AnimationSegment>& aSegments,
+bool
+nsAnimationManager::BuildSegment(InfallibleTArray<AnimationPropertySegment>&
+                                   aSegments,
+                                 nsCSSProperty aProperty,
                                  const nsAnimation& aAnimation,
                                  float aFromKey, nsStyleContext* aFromContext,
                                  mozilla::css::Declaration* aFromDeclaration,
-                                 float aToKey, nsStyleContext* aToContext,
-                                 mozilla::css::Declaration* aToDeclaration)
+                                 float aToKey, nsStyleContext* aToContext)
 {
-  AnimationSegment &segment = *aSegments.AppendElement();
+  nsStyleAnimation::Value fromValue, toValue, dummyValue;
+  if (!ExtractComputedValueForTransition(aProperty, aFromContext, fromValue) ||
+      !ExtractComputedValueForTransition(aProperty, aToContext, toValue) ||
+      // Check that we can interpolate between these values
+      // (If this is ever a performance problem, we could add a
+      // CanInterpolate method, but it seems fine for now.)
+      !nsStyleAnimation::Interpolate(aProperty, fromValue, toValue,
+                                    0.5, dummyValue)) {
+    return false;
+  }
 
+  AnimationPropertySegment &segment = *aSegments.AppendElement();
+
+  segment.mFromValue = fromValue;
+  segment.mToValue = toValue;
   segment.mFromKey = aFromKey;
   segment.mToKey = aToKey;
   const nsTimingFunction *tf;
@@ -755,33 +811,7 @@ nsAnimationManager::BuildSegment(InfallibleTArray<AnimationSegment>& aSegments,
   }
   segment.mTimingFunction.Init(*tf);
 
-  for (nsCSSProperty prop = nsCSSProperty(0);
-       prop < eCSSProperty_COUNT_no_shorthands;
-       prop = nsCSSProperty(prop + 1)) {
-    if (nsCSSProps::kAnimTypeTable[prop] == eStyleAnimType_None) {
-      continue;
-    }
-
-    if (!(aFromDeclaration && aFromDeclaration->HasProperty(prop)) &&
-        !(aToDeclaration && aToDeclaration->HasProperty(prop))) {
-      // Don't store an animation if neither declaration has the property.
-      continue;
-    }
-
-    nsStyleAnimation::Value fromValue, toValue, dummyValue;
-    if (ExtractComputedValueForTransition(prop, aFromContext, fromValue) &&
-        ExtractComputedValueForTransition(prop, aToContext, toValue) &&
-        // Check that we can interpolate between these values
-        // (If this is ever a performance problem, we could add a
-        // CanInterpolate method, but it seems fine for now.)
-        nsStyleAnimation::Interpolate(prop, fromValue, toValue,
-                                      0.5, dummyValue)) {
-      AnimationSegmentProperty &p = *segment.mProperties.AppendElement();
-      p.mProperty = prop;
-      p.mFromValue = fromValue;
-      p.mToValue = toValue;
-    }
-  }
+  return true;
 }
 
 nsIStyleRule*

layout/style/nsAnimationManager.h

@@ -47,7 +47,7 @@
 #include "nsThreadUtils.h"
 
 class nsCSSKeyframesRule;
-struct AnimationSegment;
+struct AnimationPropertySegment;
 struct ElementAnimation;
 struct ElementAnimations;
 
@@ -135,12 +135,11 @@ private:
                                           PRBool aCreateIfNeeded);
   void BuildAnimations(nsStyleContext* aStyleContext,
                        InfallibleTArray<ElementAnimation>& aAnimations);
-  void BuildSegment(InfallibleTArray<AnimationSegment>& aSegments,
-                    const nsAnimation& aAnimation,
+  bool BuildSegment(InfallibleTArray<AnimationPropertySegment>& aSegments,
+                    nsCSSProperty aProperty, const nsAnimation& aAnimation,
                     float aFromKey, nsStyleContext* aFromContext,
                     mozilla::css::Declaration* aFromDeclaration,
-                    float aToKey, nsStyleContext* aToContext,
-                    mozilla::css::Declaration* aToDeclaration);
+                    float aToKey, nsStyleContext* aToContext);
   nsIStyleRule* GetAnimationRule(mozilla::dom::Element* aElement,
                                  nsCSSPseudoElements::Type aPseudoType);
 

layout/style/test/test_animations.html

@@ -65,6 +65,24 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=435442
     content: "";
     -moz-animation: anim2 1s linear alternate infinite;
   }
+
+  @-moz-keyframes multiprop {
+    0% {
+      padding-top: 10px; padding-left: 30px;
+      -moz-animation-timing-function: ease;
+    }
+    25% {
+      padding-left: 50px;
+      -moz-animation-timing-function: ease-out;
+    }
+    50% {
+      padding-top: 40px;
+    }
+    75% {
+      padding-top: 80px; padding-left: 60px;
+      -moz-animation-timing-function: ease-in;
+    }
+  }
   </style>
 </head>
 <body>
@@ -1113,6 +1131,55 @@ is(cs_after.marginRight, "30px", ":after test at 2300ms");
 check_events([], "no events should be fired for animations on :after");
 done_div();
 
+/**
+ * Test handling of properties that are present in only some of the
+ * keyframes.
+ */
+new_div("-moz-animation: multiprop 1s ease-in-out alternate infinite");
+is(cs.paddingTop, "10px", "multiprop top at 0ms");
+is(cs.paddingLeft, "30px", "multiprop top at 0ms");
+advance_clock(100);
+is_approx(px_to_num(cs.paddingTop), 10 + 30 * gTF.ease(0.2), 0.01,
+          "multiprop top at 100ms");
+is_approx(px_to_num(cs.paddingLeft), 30 + 20 * gTF.ease(0.4), 0.01,
+          "multiprop left at 100ms");
+advance_clock(200);
+is_approx(px_to_num(cs.paddingTop), 10 + 30 * gTF.ease(0.6), 0.01,
+          "multiprop top at 300ms");
+is_approx(px_to_num(cs.paddingLeft), 50 + 10 * gTF.ease_out(0.1), 0.01,
+          "multiprop left at 300ms");
+advance_clock(300);
+is_approx(px_to_num(cs.paddingTop), 40 + 40 * gTF.ease_in_out(0.4), 0.01,
+          "multiprop top at 600ms");
+is_approx(px_to_num(cs.paddingLeft), 50 + 10 * gTF.ease_out(0.7), 0.01,
+          "multiprop left at 600ms");
+advance_clock(200);
+is_approx(px_to_num(cs.paddingTop), 80 - 80 * gTF.ease_in(0.2), 0.01,
+          "multiprop top at 800ms");
+is_approx(px_to_num(cs.paddingLeft), 60 - 60 * gTF.ease_in(0.2), 0.01,
+          "multiprop left at 800ms");
+advance_clock(400);
+is_approx(px_to_num(cs.paddingTop), 80 - 80 * gTF.ease_in(0.2), 0.01,
+          "multiprop top at 1200ms");
+is_approx(px_to_num(cs.paddingLeft), 60 - 60 * gTF.ease_in(0.2), 0.01,
+          "multiprop left at 1200ms");
+advance_clock(200);
+is_approx(px_to_num(cs.paddingTop), 40 + 40 * gTF.ease_in_out(0.4), 0.01,
+          "multiprop top at 1400ms");
+is_approx(px_to_num(cs.paddingLeft), 50 + 10 * gTF.ease_out(0.7), 0.01,
+          "multiprop left at 1400ms");
+advance_clock(300);
+is_approx(px_to_num(cs.paddingTop), 10 + 30 * gTF.ease(0.6), 0.01,
+          "multiprop top at 1700ms");
+is_approx(px_to_num(cs.paddingLeft), 50 + 10 * gTF.ease_out(0.1), 0.01,
+          "multiprop left at 1700ms");
+advance_clock(200);
+is_approx(px_to_num(cs.paddingTop), 10 + 30 * gTF.ease(0.2), 0.01,
+          "multiprop top at 1900ms");
+is_approx(px_to_num(cs.paddingLeft), 30 + 20 * gTF.ease(0.4), 0.01,
+          "multiprop left at 1900ms");
+done_div();
+
 SpecialPowers.DOMWindowUtils.restoreNormalRefresh();
 
 </script>