Bug 1248532 - Part 1: steps-start does not produce correct value at the beginning of the interval. r=birtles
authorDaisuke Akatsuka <daisuke@mozilla-japan.org>
Fri, 01 Apr 2016 14:00:57 +0900
changeset 291226 817264a13c52771c37fb3ab866ab47d680ec0c9d
parent 291225 170e0ac6b29219303c42209c9df5f4775fcbf951
child 291227 be1060e48a5a230334e3b0b1f757155fd64a17df
push id19656
push usergwagner@mozilla.com
push dateMon, 04 Apr 2016 13:43:23 +0000
treeherderb2g-inbound@e99061fde28a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbirtles
bugs1248532
milestone48.0a1
Bug 1248532 - Part 1: steps-start does not produce correct value at the beginning of the interval. r=birtles MozReview-Commit-ID: F9b1HCfEqU6
dom/animation/ComputedTiming.h
dom/animation/ComputedTimingFunction.cpp
dom/animation/ComputedTimingFunction.h
dom/animation/KeyframeEffect.cpp
gfx/layers/apz/src/AsyncPanZoomController.cpp
gfx/layers/apz/src/Axis.cpp
gfx/layers/composite/AsyncCompositionManager.cpp
layout/style/nsTransitionManager.cpp
testing/web-platform/meta/web-animations/keyframe-effect/effect-easing.html.ini
--- a/dom/animation/ComputedTiming.h
+++ b/dom/animation/ComputedTiming.h
@@ -4,16 +4,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef mozilla_ComputedTiming_h
 #define mozilla_ComputedTiming_h
 
 #include "mozilla/dom/Nullable.h"
 #include "mozilla/StickyTimeDuration.h"
+#include "mozilla/ComputedTimingFunction.h"
 
 // X11 has a #define for None
 #ifdef None
 #undef None
 #endif
 #include "mozilla/dom/AnimationEffectReadOnlyBinding.h" // FillMode
 
 namespace mozilla {
@@ -62,13 +63,16 @@ struct ComputedTiming
 
   enum class AnimationPhase {
     Null,   // Not sampled (null sample time)
     Before, // Sampled prior to the start of the active interval
     Active, // Sampled within the active interval
     After   // Sampled after (or at) the end of the active interval
   };
   AnimationPhase      mPhase = AnimationPhase::Null;
+
+  ComputedTimingFunction::BeforeFlag mBeforeFlag =
+    ComputedTimingFunction::BeforeFlag::Unset;
 };
 
 } // namespace mozilla
 
 #endif // mozilla_ComputedTiming_h
--- a/dom/animation/ComputedTimingFunction.cpp
+++ b/dom/animation/ComputedTimingFunction.cpp
@@ -19,25 +19,54 @@ ComputedTimingFunction::Init(const nsTim
                          aFunction.mFunc.mX2, aFunction.mFunc.mY2);
   } else {
     mSteps = aFunction.mSteps;
     mStepSyntax = aFunction.mStepSyntax;
   }
 }
 
 static inline double
-StepEnd(uint32_t aSteps, double aPortion)
+StepTiming(uint32_t aSteps,
+           double aPortion,
+           ComputedTimingFunction::BeforeFlag aBeforeFlag,
+           nsTimingFunction::Type aType)
 {
   MOZ_ASSERT(0.0 <= aPortion && aPortion <= 1.0, "out of range");
+  MOZ_ASSERT(aType != nsTimingFunction::Type::StepStart ||
+             aType != nsTimingFunction::Type::StepEnd, "invalid type");
+
+  if (aPortion == 1.0) {
+    return 1.0;
+  }
+
+  // Calculate current step using step-end behavior
   uint32_t step = uint32_t(aPortion * aSteps); // floor
+
+  // step-start is one step ahead
+  if (aType == nsTimingFunction::Type::StepStart) {
+    step++;
+  }
+
+  // If the "before flag" is set and we are at a transition point,
+  // drop back a step (but only if we are not already at the zero point--
+  // we do this clamping here since |step| is an unsigned integer)
+  if (step != 0 &&
+      aBeforeFlag == ComputedTimingFunction::BeforeFlag::Set &&
+      fmod(aPortion * aSteps, 1) == 0) {
+    step--;
+  }
+
+  // Convert to a progress value
   return double(step) / double(aSteps);
 }
 
 double
-ComputedTimingFunction::GetValue(double aPortion) const
+ComputedTimingFunction::GetValue(
+    double aPortion,
+    ComputedTimingFunction::BeforeFlag aBeforeFlag) const
 {
   if (HasSpline()) {
     // Check for a linear curve.
     // (GetSplineValue(), below, also checks this but doesn't work when
     // aPortion is outside the range [0.0, 1.0]).
     if (mTimingFunction.X1() == mTimingFunction.Y1() &&
         mTimingFunction.X2() == mTimingFunction.Y2()) {
       return aPortion;
@@ -79,27 +108,17 @@ ComputedTimingFunction::GetValue(double 
   // to [0,1] below we will no longer be able to distinguish to the two cases.
   if (aPortion < 0.0) {
     return 0.0;
   }
 
   // Clamp in case of steps(end) and steps(start) for values greater than 1.
   aPortion = clamped(aPortion, 0.0, 1.0);
 
-  if (mType == nsTimingFunction::Type::StepStart) {
-    // There are diagrams in the spec that seem to suggest this check
-    // and the bounds point should not be symmetric with StepEnd, but
-    // should actually step up at rather than immediately after the
-    // fraction points.  However, we rely on rounding negative values
-    // up to zero, so we can't do that.  And it's not clear the spec
-    // really meant it.
-    return 1.0 - StepEnd(mSteps, 1.0 - aPortion);
-  }
-  MOZ_ASSERT(mType == nsTimingFunction::Type::StepEnd, "bad type");
-  return StepEnd(mSteps, aPortion);
+  return StepTiming(mSteps, aPortion, aBeforeFlag, mType);
 }
 
 int32_t
 ComputedTimingFunction::Compare(const ComputedTimingFunction& aRhs) const
 {
   if (mType != aRhs.mType) {
     return int32_t(mType) - int32_t(aRhs.mType);
   }
--- a/dom/animation/ComputedTimingFunction.h
+++ b/dom/animation/ComputedTimingFunction.h
@@ -10,18 +10,24 @@
 #include "nsSMILKeySpline.h"  // nsSMILKeySpline
 #include "nsStyleStruct.h"    // nsTimingFunction
 
 namespace mozilla {
 
 class ComputedTimingFunction
 {
 public:
+  // BeforeFlag is used in step timing function.
+  // https://w3c.github.io/web-animations/#before-flag
+  enum class BeforeFlag {
+    Unset,
+    Set
+  };
   void Init(const nsTimingFunction &aFunction);
-  double GetValue(double aPortion) const;
+  double GetValue(double aPortion, BeforeFlag aBeforeFlag) const;
   const nsSMILKeySpline* GetFunction() const
   {
     NS_ASSERTION(HasSpline(), "Type mismatch");
     return &mTimingFunction;
   }
   nsTimingFunction::Type GetType() const { return mType; }
   bool HasSpline() const { return nsTimingFunction::IsSplineType(mType); }
   uint32_t GetSteps() const { return mSteps; }
@@ -37,19 +43,20 @@ public:
   bool operator!=(const ComputedTimingFunction& aOther) const
   {
     return !(*this == aOther);
   }
   int32_t Compare(const ComputedTimingFunction& aRhs) const;
   void AppendToString(nsAString& aResult) const;
 
   static double GetPortion(const Maybe<ComputedTimingFunction>& aFunction,
-                           double aPortion)
+                           double aPortion,
+                           BeforeFlag aBeforeFlag)
   {
-    return aFunction ? aFunction->GetValue(aPortion) : aPortion;
+    return aFunction ? aFunction->GetValue(aPortion, aBeforeFlag) : aPortion;
   }
   static int32_t Compare(const Maybe<ComputedTimingFunction>& aLhs,
                          const Maybe<ComputedTimingFunction>& aRhs);
 
 private:
   nsTimingFunction::Type mType;
   nsSMILKeySpline mTimingFunction;
   uint32_t mSteps;
--- a/dom/animation/KeyframeEffect.cpp
+++ b/dom/animation/KeyframeEffect.cpp
@@ -380,19 +380,27 @@ KeyframeEffectReadOnly::GetComputedTimin
       break;
     default:
       MOZ_ASSERT(true, "Unknown PlaybackDirection type");
   }
   if (thisIterationReverse) {
     result.mProgress.SetValue(1.0 - result.mProgress.Value());
   }
 
+  if ((result.mPhase == ComputedTiming::AnimationPhase::After &&
+       thisIterationReverse) ||
+      (result.mPhase == ComputedTiming::AnimationPhase::Before &&
+       !thisIterationReverse)) {
+    result.mBeforeFlag = ComputedTimingFunction::BeforeFlag::Set;
+  }
+
   if (aTiming.mFunction) {
     result.mProgress.SetValue(
-      aTiming.mFunction->GetValue(result.mProgress.Value()));
+      aTiming.mFunction->GetValue(result.mProgress.Value(),
+                                  result.mBeforeFlag));
   }
 
   return result;
 }
 
 StickyTimeDuration
 KeyframeEffectReadOnly::ActiveDuration(
   const StickyTimeDuration& aIterationDuration,
@@ -602,17 +610,18 @@ KeyframeEffectReadOnly::ComposeStyle(Ref
       continue;
     }
 
     double positionInSegment =
       (computedTiming.mProgress.Value() - segment->mFromKey) /
       (segment->mToKey - segment->mFromKey);
     double valuePosition =
       ComputedTimingFunction::GetPortion(segment->mTimingFunction,
-                                         positionInSegment);
+                                         positionInSegment,
+                                         computedTiming.mBeforeFlag);
 
 #ifdef DEBUG
     bool result =
 #endif
       StyleAnimationValue::Interpolate(prop.mProperty,
                                        segment->mFromValue,
                                        segment->mToValue,
                                        valuePosition, *val);
--- a/gfx/layers/apz/src/AsyncPanZoomController.cpp
+++ b/gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -25,26 +25,26 @@
 #include "base/task.h"                  // for NewRunnableMethod, etc
 #include "base/tracked.h"               // for FROM_HERE
 #include "gfxPrefs.h"                   // for gfxPrefs
 #include "gfxTypes.h"                   // for gfxFloat
 #include "LayersLogging.h"              // for print_stderr
 #include "mozilla/Assertions.h"         // for MOZ_ASSERT, etc
 #include "mozilla/BasicEvents.h"        // for Modifiers, MODIFIER_*
 #include "mozilla/ClearOnShutdown.h"    // for ClearOnShutdown
+#include "mozilla/ComputedTimingFunction.h" // for ComputedTimingFunction
 #include "mozilla/EventForwards.h"      // for nsEventStatus_*
 #include "mozilla/MouseEvents.h"        // for WidgetWheelEvent
 #include "mozilla/Preferences.h"        // for Preferences
 #include "mozilla/ReentrantMonitor.h"   // for ReentrantMonitorAutoEnter, etc
 #include "mozilla/StaticPtr.h"          // for StaticAutoPtr
 #include "mozilla/Telemetry.h"          // for Telemetry
 #include "mozilla/TimeStamp.h"          // for TimeDuration, TimeStamp
 #include "mozilla/dom/CheckerboardReportService.h" // for CheckerboardEventStorage
              // note: CheckerboardReportService.h actually lives in gfx/layers/apz/util/
-#include "mozilla/dom/KeyframeEffect.h" // for ComputedTimingFunction
 #include "mozilla/dom/Touch.h"          // for Touch
 #include "mozilla/gfx/BasePoint.h"      // for BasePoint
 #include "mozilla/gfx/BaseRect.h"       // for BaseRect
 #include "mozilla/gfx/Point.h"          // for Point, RoundedToInt, etc
 #include "mozilla/gfx/Rect.h"           // for RoundedIn
 #include "mozilla/gfx/ScaleFactor.h"    // for ScaleFactor
 #include "mozilla/layers/APZCTreeManager.h"  // for ScrollableLayerGuid
 #include "mozilla/layers/APZThreadUtils.h"  // for AssertOnControllerThread, etc
@@ -617,17 +617,19 @@ public:
     if (animPosition >= 1.0) {
       aFrameMetrics.SetZoom(mEndZoom);
       aFrameMetrics.SetScrollOffset(mEndOffset);
       return false;
     }
 
     // Sample the zoom at the current time point.  The sampled zoom
     // will affect the final computed resolution.
-    float sampledPosition = gZoomAnimationFunction->GetValue(animPosition);
+    float sampledPosition =
+      gZoomAnimationFunction->GetValue(animPosition,
+        ComputedTimingFunction::BeforeFlag::Unset);
 
     // We scale the scrollOffset linearly with sampledPosition, so the zoom
     // needs to scale inversely to match.
     aFrameMetrics.SetZoom(CSSToParentLayerScale2D(
       1 / (sampledPosition / mEndZoom.xScale + (1 - sampledPosition) / mStartZoom.xScale),
       1 / (sampledPosition / mEndZoom.yScale + (1 - sampledPosition) / mStartZoom.yScale)));
 
     aFrameMetrics.SetScrollOffset(CSSPoint::FromUnknownPoint(gfx::Point(
--- a/gfx/layers/apz/src/Axis.cpp
+++ b/gfx/layers/apz/src/Axis.cpp
@@ -3,21 +3,21 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "Axis.h"
 #include <math.h>                       // for fabsf, pow, powf
 #include <algorithm>                    // for max
 #include "AsyncPanZoomController.h"     // for AsyncPanZoomController
-#include "mozilla/dom/KeyframeEffect.h" // for ComputedTimingFunction
 #include "mozilla/layers/APZCTreeManager.h" // for APZCTreeManager
 #include "mozilla/layers/APZThreadUtils.h" // for AssertOnControllerThread
 #include "FrameMetrics.h"               // for FrameMetrics
 #include "mozilla/Attributes.h"         // for final
+#include "mozilla/ComputedTimingFunction.h" // for ComputedTimingFunction
 #include "mozilla/Preferences.h"        // for Preferences
 #include "mozilla/gfx/Rect.h"           // for RoundedIn
 #include "mozilla/mozalloc.h"           // for operator new
 #include "mozilla/FloatingPoint.h"      // for FuzzyEqualsAdditive
 #include "mozilla/StaticPtr.h"          // for StaticAutoPtr
 #include "nsMathUtils.h"                // for NS_lround
 #include "nsPrintfCString.h"            // for nsPrintfCString
 #include "nsThreadUtils.h"              // for NS_DispatchToMainThread, etc
@@ -97,17 +97,19 @@ void Axis::UpdateWithTouchAtDevicePoint(
     newVelocity = std::min(newVelocity, maxVelocity);
 
     if (gfxPrefs::APZCurveThreshold() > 0.0f && gfxPrefs::APZCurveThreshold() < gfxPrefs::APZMaxVelocity()) {
       float curveThreshold = ToLocalVelocity(gfxPrefs::APZCurveThreshold());
       if (newVelocity > curveThreshold) {
         // here, 0 < curveThreshold < newVelocity <= maxVelocity, so we apply the curve
         float scale = maxVelocity - curveThreshold;
         float funcInput = (newVelocity - curveThreshold) / scale;
-        float funcOutput = gVelocityCurveFunction->GetValue(funcInput);
+        float funcOutput =
+          gVelocityCurveFunction->GetValue(funcInput,
+            ComputedTimingFunction::BeforeFlag::Unset);
         float curvedVelocity = (funcOutput * scale) + curveThreshold;
         AXIS_LOG("%p|%s curving up velocity from %f to %f\n",
           mAsyncPanZoomController, Name(), newVelocity, curvedVelocity);
         newVelocity = curvedVelocity;
       }
     }
 
     if (velocityIsNegative) {
--- a/gfx/layers/composite/AsyncCompositionManager.cpp
+++ b/gfx/layers/composite/AsyncCompositionManager.cpp
@@ -617,17 +617,18 @@ SampleAnimations(Layer* aLayer, TimeStam
     }
 
     double positionInSegment =
       (computedTiming.mProgress.Value() - segment->startPortion()) /
       (segment->endPortion() - segment->startPortion());
 
     double portion =
       ComputedTimingFunction::GetPortion(animData.mFunctions[segmentIndex],
-                                         positionInSegment);
+                                         positionInSegment,
+                                         computedTiming.mBeforeFlag);
 
     // interpolate the property
     Animatable interpolatedValue;
     SampleValue(portion, animation, animData.mStartValues[segmentIndex],
                 animData.mEndValues[segmentIndex], &interpolatedValue, aLayer);
     LayerComposite* layerComposite = aLayer->AsLayerComposite();
     switch (animation.property()) {
     case eCSSProperty_opacity:
--- a/layout/style/nsTransitionManager.cpp
+++ b/layout/style/nsTransitionManager.cpp
@@ -64,17 +64,17 @@ ElementPropertyTransition::CurrentValueP
   MOZ_ASSERT(!computedTiming.mProgress.IsNull(),
              "Got a null progress for a fill mode of 'both'");
   MOZ_ASSERT(mProperties.Length() == 1,
              "Should have one animation property for a transition");
   MOZ_ASSERT(mProperties[0].mSegments.Length() == 1,
              "Animation property should have one segment for a transition");
   return ComputedTimingFunction::GetPortion(
            mProperties[0].mSegments[0].mTimingFunction,
-           computedTiming.mProgress.Value());
+           computedTiming.mProgress.Value(), computedTiming.mBeforeFlag);
 }
 
 ////////////////////////// CSSTransition ////////////////////////////
 
 JSObject*
 CSSTransition::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
 {
   return dom::CSSTransitionBinding::Wrap(aCx, this, aGivenProto);
deleted file mode 100644
--- a/testing/web-platform/meta/web-animations/keyframe-effect/effect-easing.html.ini
+++ /dev/null
@@ -1,14 +0,0 @@
-[effect-easing.html]
-  type: testharness
-  [steps(start) function]
-    expected: FAIL
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1248532
-
-  [effect easing produces values greater than 1 with step-start keyframe]
-    expected: FAIL
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1248532
-
-  [effect easing produces negative values with step-start keyframe]
-    expected: FAIL
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1248532
-