1216842 - implement effect-level easing

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Cameron McCormack (:heycam)]

We don't yet support easing applied to an animation as a whole.  Once we accept and store easing from KeyframeEffectReadOnly's constructor's KeyframeEffectOptions argument in bug 1215406, we'll need to actually take it into account in the effect.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Pass easing in KeyframeEffectOptions to KeyframeEffect constructor.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8693400 [details] [diff] [review]
Pass easing in KeyframeEffectOptions to KeyframeEffect constructor.

I'm not sure this is quite right. The easing we need to introduce is an extra level of easing. Basically, we should add an mEasing (or mTimingFunction) member to *KeyframeEffectReadOnly* (or AnimationEffectReadOnly). Then we should use that when we calculate mProgress in GetComputedTimingAt.

It's this step from the spec:
http://w3c.github.io/web-animations/#calculating-the-transformed-time

Of course, we need to store the value passed-in as well but from a quick glance at this patch it seems like this was using the effect-level easing on a keyframe-level? We need to do both effect-level easing AND keyframe-level easing. We already do the keyframe-level easing but we don't do effect-level easing yet.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Thanks for pointing it out! I did totally misread the spec!

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Store easing function in KeyframeEffectOptions as a member of AnimationTiming

The reason why we use Maybe<> to store the function is that CSS
animations/transitions do not have the function property.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Change the first argument of ToTimingFunction a Maybe<ComputedTimingFunction>.

As a result of this change we can also use ToTimingFunction for animation
effect's timing function.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Add easing function to laryer::Animation.

null_t is also added because CSS animations/transitions do not have the
easing function property.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Add LayerAnimationUtils.

we need to a new class to share the function which convert TimingFunction
to ComputedTimingFunction for either keyframe's timing function or keyframe
effect's timing function.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 5: Set keyframe effect's timing function on compositor.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 8: Calculate transformed progress using animation effect's timing function

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 9: Tests that easing property in KeyframeEffectOptions is passed to KeyframeEffectReadOnly

This test relies on bug 1214536.
Other tests will be handled in bug 1228915.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Change the first argument of ToTimingFunction a Maybe<ComputedTimingFunction>.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8693830 [details] [diff] [review]
Part 9: Tests that easing property in KeyframeEffectOptions is passed to KeyframeEffectReadOnly

Normal tests will be coming in bug 1228915.

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I discussed this bug with hiro yesterday and I think we agreed that we should make the handling of linear timing functions consistent.

In particular, currently we represent linear timing functions as a type of cubic bezier:

  https://dxr.mozilla.org/mozilla-central/rev/a8965ae93c5d098a4f91ad9da72150bb43df07a7/layout/style/nsStyleStruct.cpp#2608

However, in this bug we're special-casing this and representing effect-level timing functions as a Maybe<> type which is Nothing() in the case where the animation is generated by CSS.

Firstly, representing linear timing functions as cubic beziers is a bit inefficient. ComputedTimingFunction objects contain an nsSMILKeySpline which contains a cache of sample values along the curve that we interpolate between. Firstly, it seems really inefficient to store the same cubic bezier points and the exact same cache of sample values for every single keyframe that using 'ease' for example. We really should have a common ComputedTimingFunction object for each of the cubic bezier keywords. That's a separate bug, however.

For this bug we could probably just say that for *both* keyframe-level timing functions and animation-level timing functions, linear timing functions are represented by an empty/null/Nothing() value. For example, we could rework ParseEasing in KeyframeEffect.cpp to return a Maybe<ComputedTimingFunction> value.

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #13)
> We really should have a common
> ComputedTimingFunction object for each of the cubic bezier keywords. That's
> a separate bug, however.

Filed bug 1231471 for that.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Thanks, Brian, for the suggestion.

I've almost finished to rewrite patches as your suggestion.
The patch set will be re-ordered and re-constructed except part 6 and 7.
So, Cameron, you can review the part 7 patch for now. Thanks for waiting for reviewing.

Rest of patches will come today if the result of a try which I just pushed now looks good.

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

One other thing I realised, if we have *both* effect-level easing and keyframe-level easing, *and* we have a cubic-bezier on the effect-level easing that produces *output* outside [0..1], then we might get an *input* to the keyframe-level easing function that is outside [0..1]. I guess assertions might fail if we have that.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #16)
> One other thing I realised, if we have *both* effect-level easing and
> keyframe-level easing, *and* we have a cubic-bezier on the effect-level
> easing that produces *output* outside [0..1], then we might get an *input*
> to the keyframe-level easing function that is outside [0..1]. I guess
> assertions might fail if we have that.

Thanks, Brian! I did not notice the fact at all.
According the spec [1], in case of the values is outside [0, 1] we need computed keyframe offset to obtain a proper AnimationPropertySegment. I filed bug 1232529 for it.

[1] https://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-animation-effect

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Add null_t into TimingFunction to skip calculation of linear timing function

This is a patch for compositor side to represent linear function as null_t/Nothing().

Also the first argument of ToTimingFunction changes to a Maybe<ComputedTimingFunction>.
As a result of this change we can also use ToTimingFunction for animation
effect's timing function.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Add LayerAnimationUtils.

This is also for compositor side.
we need a new class to share the function which converts TimingFunction
to ComputedTimingFunction for either keyframe's timing function or keyframe
effect's timing function.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Change ComputedTimingFunction* to Maybe<ComputedTimingFunction>

Nothing() represents linear function, i.e. skip calculation.
ParseEasing is changed to return a Maybe<ComputedTimingFunction>,
if timing function is linear function, ParseEasing returns Nothing().

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Move ParseEasing into AnimationUtils

ParseEasing will be also used in AnimationEffectTimingReadOnly class.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 5: Add easing property in TimingParams.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 6: Store ComputedTimingFunction in TimingParams

The reason why we use Maybe<> to store the function is that CSS
animations/transitions do not have the function property.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 7: Add easing function to laryer::Animation

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8693828 [details] [diff] [review]
Part 8: Calculate transformed progress using animation effect's timing function

This patch became part 8 now.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8693830 [details] [diff] [review]
Part 9: Tests that easing property in KeyframeEffectOptions is passed to KeyframeEffectReadOnly

This patch became part 9 now.

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 10: Remove the limit of the computed timing progress.

Now we produce cumputed timing progress outside [0,1] range.
We use the last segment to calculate animation values if the value is greater than 1.
We use the first segment to calculate animation values if the value is lesser than 0.

This patch includes 4 test cases:
1) progress value < 0 for compositor animation
2) progress value > 1 for compositor animation
3) progress value < 0 for main-thread animation
4) progress value > 1 for main-thread animation

I will ask Brian to review this once he returns to daily work.

Comment 28

[Cameron McCormack (:heycam)]

Comment on attachment 8712510 [details] [diff] [review]
Part 2: Add LayerAnimationUtils.

Review of attachment 8712510 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.cpp
@@ +478,5 @@
>      for (uint32_t j = 0; j < segments.Length(); j++) {
>        TimingFunction tf = segments.ElementAt(j).sampleFn();
> +
> +      Maybe<ComputedTimingFunction> ctf;
> +      ctf = AnimationUtils::TimingFunctionToComputedTimingFunction(tf);

Nit: just join this into the one variable declaration/initialization statement.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +617,5 @@
>        (segment->endPortion() - segment->startPortion());
>  
> +    double portion = animData.mFunctions[segmentIndex].isSome() ?
> +      animData.mFunctions[segmentIndex]->GetValue(positionInSegment) :
> +      positionInSegment;

I wonder if it would make sense to have a static helper method on ComputedTimingFunction that takes a |const Maybe<ComputedTimingFunction>&| so that it can do this "isSome() ? ... : ..." work, since Part 3 also needs to do it?

::: gfx/layers/ipc/LayerAnimationUtils.cpp
@@ +15,5 @@
> +/* static */ Maybe<ComputedTimingFunction>
> +AnimationUtils::TimingFunctionToComputedTimingFunction(
> +  const TimingFunction& aTimingFunction)
> +{
> +  ComputedTimingFunction result;

Move this variable into the two switch cases that need it.

Comment 29

[Cameron McCormack (:heycam)]

Comment on attachment 8712511 [details] [diff] [review]
Part 3: Change ComputedTimingFunction* to Maybe<ComputedTimingFunction>

Review of attachment 8712511 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the OrderedKeyframeValueEntry::mTiming change.

::: dom/animation/KeyframeEffect.cpp
@@ +539,5 @@
>        (computedTiming.mProgress.Value() - segment->mFromKey) /
>        (segment->mToKey - segment->mFromKey);
> +    double valuePosition = segment->mTimingFunction.isSome() ?
> +      segment->mTimingFunction->GetValue(positionInSegment) :
> +      positionInSegment;

Use the helper static method I mentioned in the previous part's review, if you decide to add it.

@@ +1706,5 @@
>    nsTArray<OrderedKeyframeValueEntry> entries;
>  
> +  // We need a linear function here to sort key frames correctly.
> +  // mTimingFunction in AnimationPropertySegment is Nothing() in case of
> +  // the timining function is 'linear'. So if the mTimingFunction is

s/timining/timing/

@@ +1710,5 @@
> +  // the timining function is 'linear'. So if the mTimingFunction is
> +  // Nothing(), we need a dummy ComputedTimingFunction to be passed to
> +  // ComputedTimingFunction::Compare.
> +  ComputedTimingFunction linear;
> +  linear.Init(nsTimingFunction(NS_STYLE_TRANSITION_TIMING_FUNCTION_LINEAR));

Since AnimationPropertySegment::mTimingFunction is now a Maybe<>, we could make OrderedKeyframeValueEntry::mTiming function be a Maybe<>*.  Then we wouldn't need to create this dummy linear ComputedTimingFunction -- pointing to the existing Maybe<>(Nothing()) value would be fine.

Comment 30

[Cameron McCormack (:heycam)]

Comment on attachment 8712512 [details] [diff] [review]
Part 4: Move ParseEasing into AnimationUtils

Review of attachment 8712512 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/AnimationUtils.cpp
@@ +62,5 @@
> +        // don't support a list of timing functions
> +        break;
> +      }
> +      switch (list->mValue.GetUnit()) {
> +        case eCSSUnit_Enumerated:

Why do we not return Nothing() when "linear" was passed in?

::: dom/animation/AnimationUtils.h
@@ +52,5 @@
>                                         const nsIContent* aContent = nullptr);
> +
> +  /**
> +   * Parses a CSS <single-transition-timing-function> value from
> +   * aEasing into a ComputedTimingFunction.  If parsing fails, 'Nohting' will

s/Nohthing/Nothing/

Comment 31

[Cameron McCormack (:heycam)]

Comment on attachment 8712513 [details] [diff] [review]
Part 5: Add easing property in TimingParams.

Review of attachment 8712513 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/AnimationEffectTimingReadOnly.h
@@ +48,5 @@
>    TimeDuration mDelay;      // Initializes to zero
>    double mIterations = 1.0; // Can be NaN, negative, +/-Infinity
>    dom::PlaybackDirection mDirection = dom::PlaybackDirection::Normal;
>    dom::FillMode mFill = dom::FillMode::Auto;
> +  nsString mEasing;

Is there a reason we store a string rather than a timing function that we serialize when GetEasing is called?

Comment 32

[Cameron McCormack (:heycam)]

Comment on attachment 8712515 [details] [diff] [review]
Part 7: Add easing function to laryer::Animation

Review of attachment 8712515 [details] [diff] [review]:
-----------------------------------------------------------------

s/laryer/layers/ in the commit message.

Comment 33

[Cameron McCormack (:heycam)]

ni? for comment 30 and comment 31 questions.

Comment 34

[Hiroyuki Ikezoe (:hiro)]

Thank you, Cameron.

(In reply to Cameron McCormack (:heycam) (busy January 30 – February 6) from comment #31)
> Comment on attachment 8712513 [details] [diff] [review]
> Part 5: Add easing property in TimingParams.
> 
> Review of attachment 8712513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/AnimationEffectTimingReadOnly.h
> @@ +48,5 @@
> >    TimeDuration mDelay;      // Initializes to zero
> >    double mIterations = 1.0; // Can be NaN, negative, +/-Infinity
> >    dom::PlaybackDirection mDirection = dom::PlaybackDirection::Normal;
> >    dom::FillMode mFill = dom::FillMode::Auto;
> > +  nsString mEasing;
> 
> Is there a reason we store a string rather than a timing function that we
> serialize when GetEasing is called?

My intention here was to return the easing string as it is.  I did not realized that it does not match to KeyframeEffectReadOnly.getFrames()[].easing.  I will fix it.
 
(In reply to Cameron McCormack (:heycam) (busy January 30 – February 6) from comment #30)
> Comment on attachment 8712512 [details] [diff] [review]
> Part 4: Move ParseEasing into AnimationUtils
> 
> Review of attachment 8712512 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/AnimationUtils.cpp
> @@ +62,5 @@
> > +        // don't support a list of timing functions
> > +        break;
> > +      }
> > +      switch (list->mValue.GetUnit()) {
> > +        case eCSSUnit_Enumerated:
> 
> Why do we not return Nothing() when "linear" was passed in?

I did not realize the fact at all!  Thank you, Cameron!  I will change it to return Nothing() if "linear" is specified there.

Comment 35

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Move ParseEasing into AnimationUtils v2

Changes from v1:

* Returns Nothing if "linear" is specified
* Adds "const" to the dom::Element argument

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 5: Store ComputedTimingFunction in TimingParams

Now GetEasing returns a serialized string from ComputedTimingFunction.

I did forget to mention these patch series applied upon patches for bug 1096773.  A patch (attachment 8712403 [details] [diff] [review]) in that bug introduces utility methods to create TimingParams, this patch modifies the methods.

'Part 6' is a missing number for now.

Comment 37

[Cameron McCormack (:heycam)]

Comment on attachment 8712596 [details] [diff] [review]
Part 5: Store ComputedTimingFunction in TimingParams

Review of attachment 8712596 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Add LayerAnimationUtils v2.

Added a helper method to get value from Maybe<ComputedTimingFunction>.

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Change ComputedTimingFunction* to Maybe<ComputedTimingFunction>

Cameron, could you please take a look this patch again?
I did forget to fix timingFunction.GetValue() in nsTransitionManager.cpp.

I've decided to left the part of making OrderedKeyframeValueEntry::mTiming as Maybe<>* in a subsequent patch because it needs a bit of tweaking since 'ease' is prior to 'linear' in nsTimingFunction::Type.

Comment 40

[Cameron McCormack (:heycam)]

Comment on attachment 8713034 [details] [diff] [review]
Part 3: Change ComputedTimingFunction* to Maybe<ComputedTimingFunction>

Review of attachment 8713034 [details] [diff] [review]:
-----------------------------------------------------------------

I think a better name for the helper function would be GetPortion.  GetValueOr is similar to the Maybe methods, so to me it sounds like it would return the value inside the Maybe or the second argument, but it actually is passing in that second value to the Maybe's timing function.

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 6: Make mTimingFunction in OrderedKeyframeValueEntry cosnt Maybe<>*.

This is the part of the making OrderedKeyframeValueEntry::mTiming Maybe<>*. Yay, now this patch fits in part 6.

Actually I am not a big fan of this patch.  If we could change the nsTimingFunction::Type, things gets easier.  But the order of nsTimingFunction::Type matches to the spec[1].  I don't think we can change the order.  I don't know why 'ease' is the top of them though...

[1] https://drafts.csswg.org/css-transitions-1/#single-transition-timing-function

Comment 42

[Cameron McCormack (:heycam)]

Comment on attachment 8713041 [details] [diff] [review]
Part 6: Make mTimingFunction in OrderedKeyframeValueEntry cosnt Maybe<>*.

Review of attachment 8713041 [details] [diff] [review]:
-----------------------------------------------------------------

s/cosnt/const/ in the commit message.

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Add LayerAnimationUtils v3

GetValueOr -> GetPortion.

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Change ComputedTimingFunction* to Maybe<ComputedTimingFunction>

GetValueOr -> GetPortion

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 5: Store ComputedTimingFunction in TimingParams

Rebased.

Comment 46

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 6: Make mTimingFunction in OrderedKeyframeValueEntry cosnt Maybe<>*.

Rebased.

Comment 47

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 8: Calculate transformed progress using animation effect's timing function

Rebased.

Comment 48

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 9: Tests that easing property in KeyframeEffectOptions is passed to KeyframeEffectReadOnly

Change the part number in the commit message.

Comment 49

[Hiroyuki Ikezoe (:hiro)]

Sheriff, could you please check part 1 to 9 patches in?  It's harmless without part 10.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c3915509961

Comment 50

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/987f0cff9467
https://hg.mozilla.org/integration/mozilla-inbound/rev/346880930a78
https://hg.mozilla.org/integration/mozilla-inbound/rev/eabcf625bf8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/28618bbe4f24
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa3d8793e82d
https://hg.mozilla.org/integration/mozilla-inbound/rev/66c96ae07ed5
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffd0302537fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/c57d21d620ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/978349072992

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/987f0cff9467
https://hg.mozilla.org/mozilla-central/rev/346880930a78
https://hg.mozilla.org/mozilla-central/rev/eabcf625bf8e
https://hg.mozilla.org/mozilla-central/rev/28618bbe4f24
https://hg.mozilla.org/mozilla-central/rev/aa3d8793e82d
https://hg.mozilla.org/mozilla-central/rev/66c96ae07ed5
https://hg.mozilla.org/mozilla-central/rev/ffd0302537fe
https://hg.mozilla.org/mozilla-central/rev/c57d21d620ce
https://hg.mozilla.org/mozilla-central/rev/978349072992

Comment 52

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8712517 [details] [diff] [review]
Part 10: Remove the limit of the computed timing progress.

I am going to fix a typo 'cumputed' in commit message once this patch gets reviewed.

Comment 53

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8712517 [details] [diff] [review]
Part 10: Remove the limit of the computed timing progress.

Review of attachment 8712517 [details] [diff] [review]:
-----------------------------------------------------------------

Two issues:

1. I don't think this works when we have a step timing function. I think when we get a progress < 0 or > 1 we'll end up with positionInSegment also < 0 or > 1 and pass that to ComputedTimingFunction::GetValue. Then, if we have a step timing function we'll pass that on to StepEnd which has an assertion for aPortion being between 0 and 1. So that assertion will fail.

2. When we have a cubic bezier curve as a timing function, we'll pass the progress onto nsSMILKeySpline::GetSplineValue. It's not clear to me that that will return a sensible value when the progress is < 0 or > 1. In fact, I think the spec was supposed to say that in that case we should treat the timing function as 'linear' outside the [0, 1] range but I can't see where that's defined. I'll talk to the Google guys and see what they did here.

We also need to update the documentation for StyleAnimatinValue::Interpolate to say that aPortion can be outside the range [0.0, 1.0].

I'm reluctant to review this without tests that we actually do something sensible when the progress value is outside the [0.0, 1.0] range. As discussed, I'd like to add mochitests to this bug (i.e. fold in bug 1228915). Do you mind re-requesting review after we have those tests?

The rest looks good.

Comment 54

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #53)
> 2. When we have a cubic bezier curve as a timing function, we'll pass the
> progress onto nsSMILKeySpline::GetSplineValue. It's not clear to me that
> that will return a sensible value when the progress is < 0 or > 1. In fact,
> I think the spec was supposed to say that in that case we should treat the
> timing function as 'linear' outside the [0, 1] range but I can't see where
> that's defined. I'll talk to the Google guys and see what they did here.

I think what the spec says is that when the interval progress is < 0 or > 1 you basically end up just using the first or last keyframe's value.

 http://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-animation-effect

The spec doesn't seem to actually apply the keyframe-level timing function however.

Comment 55

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #54)
> The spec doesn't seem to actually apply the keyframe-level timing function
> however.

Filed https://github.com/w3c/web-animations/issues/139 for that.

Comment 56

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #54)
> (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #53)
> > 2. When we have a cubic bezier curve as a timing function, we'll pass the
> > progress onto nsSMILKeySpline::GetSplineValue. It's not clear to me that
> > that will return a sensible value when the progress is < 0 or > 1. In fact,
> > I think the spec was supposed to say that in that case we should treat the
> > timing function as 'linear' outside the [0, 1] range but I can't see where
> > that's defined. I'll talk to the Google guys and see what they did here.
> 
> I think what the spec says is that when the interval progress is < 0 or > 1
> you basically end up just using the first or last keyframe's value.

Thank you, Brian. Let me clarify it.
We do not need any extrapolation whatever easing function is used if positionInSegment is outside [0, 1] range?

Comment 57

[[:fabrice] Fabrice Desré]

That broke b2g builds:
In file included from Unified_cpp_gfx_layers6.cpp:2:0:
/home/fabrice/dev/mozilla-inbound/gfx/layers/ipc/LayerAnimationUtils.cpp:17:40: error: no 'mozilla::Maybe<mozilla::ComputedTimingFunction> mozilla::layers::AnimationUtils::TimingFunctionToComputedTimingFunction(const mozilla::layers::TimingFunction&)' member function declared in class 'mozilla::layers::AnimationUtils'

Comment 58

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #56)
> (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #54)
> > (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #53)
> > > 2. When we have a cubic bezier curve as a timing function, we'll pass the
> > > progress onto nsSMILKeySpline::GetSplineValue. It's not clear to me that
> > > that will return a sensible value when the progress is < 0 or > 1. In fact,
> > > I think the spec was supposed to say that in that case we should treat the
> > > timing function as 'linear' outside the [0, 1] range but I can't see where
> > > that's defined. I'll talk to the Google guys and see what they did here.
> > 
> > I think what the spec says is that when the interval progress is < 0 or > 1
> > you basically end up just using the first or last keyframe's value.
> 
> Thank you, Brian. Let me clarify it.
> We do not need any extrapolation whatever easing function is used if
> positionInSegment is outside [0, 1] range?

Right, I believe that's what it says. Are you able to test what Chrome does? (Or check the chromium source?)

Comment 59

[Hiroyuki Ikezoe (:hiro)]

Attachment: An example for easing which generates outside [0,1] values

(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #58)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #56)
> > (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #54)
> > > (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #53)
> > > > 2. When we have a cubic bezier curve as a timing function, we'll pass the
> > > > progress onto nsSMILKeySpline::GetSplineValue. It's not clear to me that
> > > > that will return a sensible value when the progress is < 0 or > 1. In fact,
> > > > I think the spec was supposed to say that in that case we should treat the
> > > > timing function as 'linear' outside the [0, 1] range but I can't see where
> > > > that's defined. I'll talk to the Google guys and see what they did here.
> > > 
> > > I think what the spec says is that when the interval progress is < 0 or > 1
> > > you basically end up just using the first or last keyframe's value.
> > 
> > Thank you, Brian. Let me clarify it.
> > We do not need any extrapolation whatever easing function is used if
> > positionInSegment is outside [0, 1] range?
> 
> Right, I believe that's what it says. Are you able to test what Chrome does?
> (Or check the chromium source?)

Chromium 50.0.2639.0 does calculate extrapolation.  I did use attaching HTML file.

Comment 60

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Additional overflow test

I expanded attachment 8715148 [details] to further debug what Chrome does.

Comment 61

[Brian Birtles (:birtles, Feb 26~ parental leave)]

For cubic beziers, Chromium uses the tangent at the extremity:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/animation/TimingFunction.cpp&l=101

For step timing functions, Chromium clamps to [0, 1] so that the steps basically extend out:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/animation/TimingFunction.cpp&l=303

That's not what the spec says so I will take it up with Google on Monday next week.

Comment 62

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I spoke to Google about this and it seems like we do want to extrapolate behavior. Otherwise, even if you only have "linear" as the easing on your keyframe segments, if the effect-level easing goes outside the range [0, 1] then those segments won't animate beyond their bounds as expected.

I'll update the spec sometime next week I hope, but basically we want to do what Chrome does here:

* Extrapolate linear interpolation in a linear fashion,
* Extrapolate bezier functions using the tangent at the endpoint
* Extrapolate step timing functions by just projecting the 0,1 value infinitely.

Comment 63

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #62)
> I spoke to Google about this and it seems like we do want to extrapolate
> behavior. Otherwise, even if you only have "linear" as the easing on your
> keyframe segments, if the effect-level easing goes outside the range [0, 1]
> then those segments won't animate beyond their bounds as expected.
> 
> I'll update the spec sometime next week I hope, but basically we want to do
> what Chrome does here:
> 
> * Extrapolate linear interpolation in a linear fashion,
> * Extrapolate bezier functions using the tangent at the endpoint
> * Extrapolate step timing functions by just projecting the 0,1 value
> infinitely.

Thank you, Brian.
A test case I am concerned about Chrome implementation is the edge value for bezier functions.
It's (3) case in chromium code:
https://src.chromium.org/viewvc/blink/trunk/Source/platform/animation/UnitBezier.h?r1=172821&r2=172820&pathrev=172821

Here are two typical animations:
  var anim = target.animate(
    [ { width: "0px", easing: "cubic-bezier(1,1.5,1,1.5)" },
      { width: "100px" } ],
    { duration: 1000, easing: "cubic-bezier(0,1.5,1,1.5)" });

  var anim = target.animate(
    [ { width: "0px", easing: "cubic-bezier(1,1.5,0.99,1.5)" },
      { width: "100px" } ],
    { duration: 1000, easing: "cubic-bezier(0,1.5,1,1.5)" });

These animations have very similar bezier functions but its behaviors on chrome are totally different because the tangent of the first animation is minus infinity, so they use 0 as the tangent in this case.  On the other hand, the tangent of the second animation is a big minus value....
I have no good idea to fill this gap between these two similar animations though...

Comment 64

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 10: Remove the limit of the computed timing progress.

Extrapolations are handled in subsequent patches.

Comment 65

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 11: Part 10: Do not calculate any value if the function can be considered as linear functions.

If both of control points of cubic bezier functions are on y = x line,
we can consider the function as a linear function, then skip calculations.

With this patch it will be easier to handle extrapolation of cubic-bezier(1,1,1,1) or cubic-bezier(0,0,0,0).

Comment 66

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 11: Clamp values of step functions outside [0, 1].

Comment 67

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 13: Extrapolate bezier function outside [0,1]

We use the tangent at the each boundary points as same as Chrome does.

Comment 68

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 14: Tests for effect-level easing.

Comment 69

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8717866 [details] [diff] [review]
Part 11: Part 10: Do not calculate any value if the function can be considered as linear functions.

Review of attachment 8717866 [details] [diff] [review]:
-----------------------------------------------------------------

This should be "Part 11" actually.

Comment 70

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8717860 [details] [diff] [review]
Part 10: Remove the limit of the computed timing progress.

Review of attachment 8717860 [details] [diff] [review]:
-----------------------------------------------------------------

This is brilliant work.

r=birtles. I think we should probably change the main thread tests to seek to the extrapolated range. The compositor tests are probably fine as-is unless you think it makes sense to use a non-integral iteration count to make sure we fill in the extrapolated range (that would also let us make the test duration shorter).

::: dom/animation/test/crashtests/1216842-1.html
@@ +20,5 @@
> +        { opacity: [0, 1] },
> +        {
> +          fill: "forwards",
> +          easing: "cubic-bezier(0,-0.5,1,-0.5)",
> +          duration: 100

I guess we're just hoping that we'll get a tick that lies outside the [0,1] range. I was thinking that if we use 'iterations: 0.75' or something like that so that the animation finishes in the extrapolated range we might have a higher chance of getting a tick there (since we normally get one tick where the compositor animation fills forwards while waiting for the main thread to remove it). What do you think?

Otherwise I think this is fine.

::: dom/animation/test/crashtests/1216842-3.html
@@ +14,5 @@
> +        { color: ["red", "blue"] },
> +        {
> +          fill: "forwards",
> +          easing: "cubic-bezier(0,1.5,1,1.5)",
> +          duration: 100

For the main thread tests, would it be better to pause the animation and seek it to the extrapolated range?

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +609,3 @@
>  
> +    uint32_t segmentIndex = 0;
> +    size_t segmentSize = animation.segments().Length();

s/segmentSize/segmentLength/ ?

Comment 71

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8717866 [details] [diff] [review]
Part 11: Part 10: Do not calculate any value if the function can be considered as linear functions.

Review of attachment 8717866 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/ComputedTimingFunction.cpp
@@ +16,5 @@
>    mType = aFunction.mType;
>    if (nsTimingFunction::IsSplineType(mType)) {
> +    mIsPseudoLinear =
> +      aFunction.mFunc.mX1 == aFunction.mFunc.mY1 &&
> +      aFunction.mFunc.mX2 == aFunction.mFunc.mY2;

I thought nsSMILKeySpline already did this?

https://dxr.mozilla.org/mozilla-central/rev/ac39fba33c6daf95b2cda71e588ca18e2eb752ab/dom/smil/nsSMILKeySpline.cpp#37

Why do we need this?

Comment 72

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8717866 [details] [diff] [review]
Part 11: Part 10: Do not calculate any value if the function can be considered as linear functions.

Review of attachment 8717866 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #71)
> Comment on attachment 8717866 [details] [diff] [review]
> Part 11: Part 10: Do not calculate any value if the function can be
> considered as linear functions.
> 
> Review of attachment 8717866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/ComputedTimingFunction.cpp
> @@ +16,5 @@
> >    mType = aFunction.mType;
> >    if (nsTimingFunction::IsSplineType(mType)) {
> > +    mIsPseudoLinear =
> > +      aFunction.mFunc.mX1 == aFunction.mFunc.mY1 &&
> > +      aFunction.mFunc.mX2 == aFunction.mFunc.mY2;
> 
> I thought nsSMILKeySpline already did this?
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> ac39fba33c6daf95b2cda71e588ca18e2eb752ab/dom/smil/nsSMILKeySpline.cpp#37
> 
> Why do we need this?

Oh, I just read the comment now. This is for the extrapolation case. We should at least add a comment saying that.

Alternatively, I wonder if we need to store mIsPseudoLinear as a member variable. Can we just check it every time in GetValue?

e.g.

if (<portion it out of range>) {
  if (HasSpline() && mTimingFunction.X1() == mTimingFunction.Y1() &&
                     mTimingFunction.X2() == mTimingFunction.Y2()) {
     return portion;
  }
  // Other extrapolation behavior from part 13 etc.
}

I think this calculation is cheap enough and rare enough that we could do it each time?

What do you think?

Comment 73

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8717870 [details] [diff] [review]
Part 13: Extrapolate bezier function outside [0,1]

Review of attachment 8717870 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/ComputedTimingFunction.h
@@ +60,5 @@
>    uint32_t mSteps;
>    nsTimingFunction::StepSyntax mStepSyntax;
>    bool mIsPseudoLinear = false;
> +  double mTangentForLessThanZero;
> +  double mTangentForGreaterThanOne;

As with part 11, I wonder if we need to store this as member variables or whether we could just perform the calculation as-necessary in GetValue. What do you think?

Comment 74

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8717871 [details] [diff] [review]
Part 14: Tests for effect-level easing.

This testing is brilliant but I'm going to come back to it later so I can check it properly.

Comment 75

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8717871 [details] [diff] [review]
Part 14: Tests for effect-level easing.

Review of attachment 8717871 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/web-animations/keyframe-effect/easing.html
@@ +52,5 @@
> +      // return the tangent at 0.
> +    }
> +    if (x > 1) {
> +      // return the tangent at 1.
> +    }

I just noticed now these lines should be removed.

Comment 76

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #72)
> Comment on attachment 8717866 [details] [diff] [review]
> Part 11: Part 10: Do not calculate any value if the function can be
> considered as linear functions.
> 
> Review of attachment 8717866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #71)
> > Comment on attachment 8717866 [details] [diff] [review]
> > Part 11: Part 10: Do not calculate any value if the function can be
> > considered as linear functions.
> > 
> > Review of attachment 8717866 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/animation/ComputedTimingFunction.cpp
> > @@ +16,5 @@
> > >    mType = aFunction.mType;
> > >    if (nsTimingFunction::IsSplineType(mType)) {
> > > +    mIsPseudoLinear =
> > > +      aFunction.mFunc.mX1 == aFunction.mFunc.mY1 &&
> > > +      aFunction.mFunc.mX2 == aFunction.mFunc.mY2;
> > 
> > I thought nsSMILKeySpline already did this?
> > 
> > https://dxr.mozilla.org/mozilla-central/rev/
> > ac39fba33c6daf95b2cda71e588ca18e2eb752ab/dom/smil/nsSMILKeySpline.cpp#37
> > 
> > Why do we need this?
> 
> Oh, I just read the comment now. This is for the extrapolation case. We
> should at least add a comment saying that.
> 
> Alternatively, I wonder if we need to store mIsPseudoLinear as a member
> variable. Can we just check it every time in GetValue?
> 
> e.g.
> 
> if (<portion it out of range>) {
>   if (HasSpline() && mTimingFunction.X1() == mTimingFunction.Y1() &&
>                      mTimingFunction.X2() == mTimingFunction.Y2()) {
>      return portion;
>   }
>   // Other extrapolation behavior from part 13 etc.
> }
> 
> I think this calculation is cheap enough and rare enough that we could do it
> each time?
> 
> What do you think?

In the pseudo linear case I wanted to avoid calling GetValue at all, but your suggestion looks much better! Thanks! I will do it.

Comment 77

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #73)
> Comment on attachment 8717870 [details] [diff] [review]
> Part 13: Extrapolate bezier function outside [0,1]
> 
> Review of attachment 8717870 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/ComputedTimingFunction.h
> @@ +60,5 @@
> >    uint32_t mSteps;
> >    nsTimingFunction::StepSyntax mStepSyntax;
> >    bool mIsPseudoLinear = false;
> > +  double mTangentForLessThanZero;
> > +  double mTangentForGreaterThanOne;
> 
> As with part 11, I wonder if we need to store this as member variables or
> whether we could just perform the calculation as-necessary in GetValue. What
> do you think?

Actually I did it first, then adjusted to Chrome style. :-)  I will go back the first style.

Comment 78

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 12: Extrapolate bezier function outside [0,1]

Thank you, Brian.  Now this patch gets easier to understand I think.

The part of part 11 is absorbed into this patch.  Previous part 12 becomes part 11, and this patch becomes part 12 now.

Comment 80

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8718134 [details] [diff] [review]
Part 12: Extrapolate bezier function outside [0,1]

Review of attachment 8718134 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/ComputedTimingFunction.cpp
@@ +33,5 @@
>  
>  double
>  ComputedTimingFunction::GetValue(double aPortion) const
>  {
> +  if (aPortion < 0.0 || aPortion > 1.0) {

// Handle the case where we need to extrapolate the function

@@ +36,5 @@
>  {
> +  if (aPortion < 0.0 || aPortion > 1.0) {
> +    if (HasSpline()) {
> +      // If all the control points are on y = x line,
> +      // it's a linear.

// Check for a linear curve.

@@ +47,5 @@
> +      // (p2 - p0).
> +      if (aPortion < 0.0) {
> +        if (mTimingFunction.X1() > 0.0) {
> +          return aPortion * mTimingFunction.Y1() / mTimingFunction.X1();
> +        } else if (mTimingFunction.X2() > 0.0) {

What is the logic here? This doesn't seem to match what Chromium does. This seems like it should correspond to case-2 in Chromium's code[1] which is about when the first control point is coincident (not just horizontally coincident) with the end point. i.e. it seems like we need a check that mTimingFunction.Y1() == 0 here?

[1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/animation/UnitBezier.h&l=50&rcl=1455510463

@@ +61,5 @@
> +      if (aPortion > 1.0) {
> +        if (mTimingFunction.X2() < 1.0) {
> +          return 1.0 + (aPortion - 1.0) *
> +            (mTimingFunction.Y2() - 1) / (mTimingFunction.X2() - 1);
> +        } else if (mTimingFunction.X1() < 1.0) {

Similarly here.

@@ +78,1 @@
>    if (HasSpline()) {

It seems a little bit unusual to me to have a big block at the start for the (unusual) case of extrapolation. What do you think about something like the following:

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;
  }

  // For negative values, try to extrapolate with tangent (p1 - p0) or,
  // if p1 is coincident with p0, with (p2 - p0).
  if (aPortion < 0.0) {
    ...
  }

  // For values greater than 1, try to extrapolate with tangent
  // (p2 - p3) or, if p2 is coincident with p3, with (p1 - p3).
  if (aPortion > 1.0) {
    ...
  }

  return mTimingFunction.GetSplineValue(aPortion);
}

aPortion = clamped(aPortion, 0.0, 1.0);
...

Comment 81

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #80)
> @@ +47,5 @@
> > +      // (p2 - p0).
> > +      if (aPortion < 0.0) {
> > +        if (mTimingFunction.X1() > 0.0) {
> > +          return aPortion * mTimingFunction.Y1() / mTimingFunction.X1();
> > +        } else if (mTimingFunction.X2() > 0.0) {
> 
> What is the logic here? This doesn't seem to match what Chromium does. This
> seems like it should correspond to case-2 in Chromium's code[1] which is
> about when the first control point is coincident (not just horizontally
> coincident) with the end point. i.e. it seems like we need a check that
> mTimingFunction.Y1() == 0 here?
> 
> [1]
> https://code.google.com/p/chromium/codesearch#chromium/src/third_party/
> WebKit/Source/platform/animation/UnitBezier.h&l=50&rcl=1455510463

As I sent a e-mail privately to google guys and Brian, I think the check is a bug,
it should actually X1 == 0 there.  Otherwise, we return 0 in case of Y1 == 0, e.g. cubic-bezier(0,0,0.4,0.7) <http://cubic-bezier.com/#0,0,.4,.7>, And (X1==0) is not useful there.


> @@ +78,1 @@
> >    if (HasSpline()) {
> 
> It seems a little bit unusual to me to have a big block at the start for the
> (unusual) case of extrapolation. What do you think about something like the
> following:
> 
> 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;
>   }
> 
>   // For negative values, try to extrapolate with tangent (p1 - p0) or,
>   // if p1 is coincident with p0, with (p2 - p0).
>   if (aPortion < 0.0) {
>     ...
>   }
> 
>   // For values greater than 1, try to extrapolate with tangent
>   // (p2 - p3) or, if p2 is coincident with p3, with (p1 - p3).
>   if (aPortion > 1.0) {
>     ...
>   }
> 
>   return mTimingFunction.GetSplineValue(aPortion);
> }
> 
> aPortion = clamped(aPortion, 0.0, 1.0);
> ...

Thanks. I will do it.

Comment 82

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #81)
> (In reply to Brian Birtles (:birtles) from comment #80)
> > @@ +47,5 @@
> > > +      // (p2 - p0).
> > > +      if (aPortion < 0.0) {
> > > +        if (mTimingFunction.X1() > 0.0) {
> > > +          return aPortion * mTimingFunction.Y1() / mTimingFunction.X1();
> > > +        } else if (mTimingFunction.X2() > 0.0) {
> > 
> > What is the logic here? This doesn't seem to match what Chromium does. This
> > seems like it should correspond to case-2 in Chromium's code[1] which is
> > about when the first control point is coincident (not just horizontally
> > coincident) with the end point. i.e. it seems like we need a check that
> > mTimingFunction.Y1() == 0 here?
> > 
> > [1]
> > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/
> > WebKit/Source/platform/animation/UnitBezier.h&l=50&rcl=1455510463
> 
> As I sent a e-mail privately to google guys and Brian, I think the check is
> a bug,
> it should actually X1 == 0 there.  Otherwise, we return 0 in case of Y1 ==
> 0, e.g. cubic-bezier(0,0,0.4,0.7) <http://cubic-bezier.com/#0,0,.4,.7>, And
> (X1==0) is not useful there.

Oh my. This comment is totally wrong.  I will post an example of Y1 != 0 later.

Comment 83

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #82)
> Oh my. This comment is totally wrong.  I will post an example of Y1 != 0
> later.

Ok, thanks. The Chromium code seems to match the comment next to the code at least. If p1x is always >= 0, then the first branch is for when p1x > 0 and the second branch is for when p1x == 0 && p1y == 0 && p2x > 0.

Comment 84

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #82)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #81)
> > (In reply to Brian Birtles (:birtles) from comment #80)
> > > @@ +47,5 @@
> > > > +      // (p2 - p0).
> > > > +      if (aPortion < 0.0) {
> > > > +        if (mTimingFunction.X1() > 0.0) {
> > > > +          return aPortion * mTimingFunction.Y1() / mTimingFunction.X1();
> > > > +        } else if (mTimingFunction.X2() > 0.0) {
> > > 
> > > What is the logic here? This doesn't seem to match what Chromium does. This
> > > seems like it should correspond to case-2 in Chromium's code[1] which is
> > > about when the first control point is coincident (not just horizontally
> > > coincident) with the end point. i.e. it seems like we need a check that
> > > mTimingFunction.Y1() == 0 here?
> > > 
> > > [1]
> > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/
> > > WebKit/Source/platform/animation/UnitBezier.h&l=50&rcl=1455510463
> > 
> > As I sent a e-mail privately to google guys and Brian, I think the check is
> > a bug,
> > it should actually X1 == 0 there.  Otherwise, we return 0 in case of Y1 ==
> > 0, e.g. cubic-bezier(0,0,0.4,0.7) <http://cubic-bezier.com/#0,0,.4,.7>, And
> > (X1==0) is not useful there.
> 
> Oh my. This comment is totally wrong.  I will post an example of Y1 != 0
> later.

Here is an example: X1 == 0 and Y1 != 0, but we can calculate the tangent.
cubic-bezier(0,.64,.39,.77) <http://cubic-bezier.com/#0,.64,.39,.77>

(In reply to Brian Birtles (:birtles) from comment #83)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #82)
> > Oh my. This comment is totally wrong.  I will post an example of Y1 != 0
> > later.
> 
> Ok, thanks. The Chromium code seems to match the comment next to the code at
> least. If p1x is always >= 0, then the first branch is for when p1x > 0 and
> the second branch is for when p1x == 0 && p1y == 0 && p2x > 0.

Yes, the 'p1y == 0'  is a problematic code there.

Comment 85

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #84)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #82)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #81)
> > > (In reply to Brian Birtles (:birtles) from comment #80)
> > > > @@ +47,5 @@
> > > > > +      // (p2 - p0).
> > > > > +      if (aPortion < 0.0) {
> > > > > +        if (mTimingFunction.X1() > 0.0) {
> > > > > +          return aPortion * mTimingFunction.Y1() / mTimingFunction.X1();
> > > > > +        } else if (mTimingFunction.X2() > 0.0) {
> > > > 
> > > > What is the logic here? This doesn't seem to match what Chromium does. This
> > > > seems like it should correspond to case-2 in Chromium's code[1] which is
> > > > about when the first control point is coincident (not just horizontally
> > > > coincident) with the end point. i.e. it seems like we need a check that
> > > > mTimingFunction.Y1() == 0 here?
> > > > 
> > > > [1]
> > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/
> > > > WebKit/Source/platform/animation/UnitBezier.h&l=50&rcl=1455510463
> > > 
> > > As I sent a e-mail privately to google guys and Brian, I think the check is
> > > a bug,
> > > it should actually X1 == 0 there.  Otherwise, we return 0 in case of Y1 ==
> > > 0, e.g. cubic-bezier(0,0,0.4,0.7) <http://cubic-bezier.com/#0,0,.4,.7>, And
> > > (X1==0) is not useful there.
> > 
> > Oh my. This comment is totally wrong.  I will post an example of Y1 != 0
> > later.
> 
> Here is an example: X1 == 0 and Y1 != 0, but we can calculate the tangent.
> cubic-bezier(0,.64,.39,.77) <http://cubic-bezier.com/#0,.64,.39,.77>

What is the tangent in this case?

Comment 86

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #85)

> > Here is an example: X1 == 0 and Y1 != 0, but we can calculate the tangent.
> > cubic-bezier(0,.64,.39,.77) <http://cubic-bezier.com/#0,.64,.39,.77>
> 
> What is the tangent in this case?

0.39 / 0.77 for negative portion.

Comment 87

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #86)
> (In reply to Brian Birtles (:birtles) from comment #85)
> 
> > > Here is an example: X1 == 0 and Y1 != 0, but we can calculate the tangent.
> > > cubic-bezier(0,.64,.39,.77) <http://cubic-bezier.com/#0,.64,.39,.77>
> > 
> > What is the tangent in this case?
> 
> 0.39 / 0.77 for negative portion.

Why is that the tangent? If you change the value of Y1, the shape of the curve changes so I would expect the tangent to change.

Comment 88

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #87)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #86)
> > (In reply to Brian Birtles (:birtles) from comment #85)
> > 
> > > > Here is an example: X1 == 0 and Y1 != 0, but we can calculate the tangent.
> > > > cubic-bezier(0,.64,.39,.77) <http://cubic-bezier.com/#0,.64,.39,.77>
> > > 
> > > What is the tangent in this case?
> > 
> > 0.39 / 0.77 for negative portion.
> 
> Why is that the tangent? If you change the value of Y1, the shape of the
> curve changes so I would expect the tangent to change.

I meant the tangent is just an angle from P2 to P0 here.  I am not sure |tangent| is a good word.

Comment 89

[Hiroyuki Ikezoe (:hiro)]

This is a figure explaining the tangents (inclinations) in most cases.
http://hiikezoe.github.io/Extrapolation_normal.png

Blue arrow is inclination to be used for extrapolation for negative portions.
Red arrow is inclination to be used for values greater than 1.

This is an edge case we don't calculate the inclinations on both boundaries because it's infinity (or minus infinity).  Inclination against another control points is used.  Should we also use 0 in this case instead?
http://hiikezoe.github.io/Extrapolation_substitute.png

Comment 90

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #89)
> This is a figure explaining the tangents (inclinations) in most cases.
> http://hiikezoe.github.io/Extrapolation_normal.png
> 
> Blue arrow is inclination to be used for extrapolation for negative portions.
> Red arrow is inclination to be used for values greater than 1.
> 
> This is an edge case we don't calculate the inclinations on both boundaries
> because it's infinity (or minus infinity).  Inclination against another
> control points is used.  Should we also use 0 in this case instead?
> http://hiikezoe.github.io/Extrapolation_substitute.png

I don't really know the mathematically correct thing to do here so I'm inclined to just match Chrome. i.e. I think it makes sense to use 0 and 1 for those cases where we can't use the inclination to the nearest control point (except for the special case where the near control point is coincident with the endpoint since then the near control point has no effect).

What do you think?

Comment 91

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #90)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #89)
> > This is a figure explaining the tangents (inclinations) in most cases.
> > http://hiikezoe.github.io/Extrapolation_normal.png
> > 
> > Blue arrow is inclination to be used for extrapolation for negative portions.
> > Red arrow is inclination to be used for values greater than 1.
> > 
> > This is an edge case we don't calculate the inclinations on both boundaries
> > because it's infinity (or minus infinity).  Inclination against another
> > control points is used.  Should we also use 0 in this case instead?
> > http://hiikezoe.github.io/Extrapolation_substitute.png
> 
> I don't really know the mathematically correct thing to do here so I'm
> inclined to just match Chrome. i.e. I think it makes sense to use 0 and 1
> for those cases where we can't use the inclination to the nearest control
> point (except for the special case where the near control point is
> coincident with the endpoint since then the near control point has no
> effect).
> 
> What do you think?

OK, it makes sense.  I will post a revised patch and test.

Comment 92

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8717871 [details] [diff] [review]
Part 14: Tests for effect-level easing.

Review of attachment 8717871 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but I'm not sure about the behavior of the step-start timing functions when the time is zero.

::: testing/web-platform/tests/web-animations/keyframe-effect/easing.html
@@ +1,3 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>Effect level easing tests</title>

Nit: Effect-level

@@ +12,5 @@
> +<div id="target"></div>
> +<script>
> +"use strict";
> +
> +function px_to_num(str) {

Can we move px_to_num, cubicBezier, step_end etc. to ../testcommon.js?

Also, do we normally use _ to separate words in Javascript?

(assert_style_left_at, at least, is fine since it matches testharness.js's asssertions)

@@ +77,5 @@
> +  var portion = time / animation.effect.timing.duration;
> +  assert_approx_equals(px_to_num(getComputedStyle(animation.effect.target).left),
> +                       easingFunction(portion) * 100,
> +                       0.01,
> +                       'The width of the animation should be approximately ' +

width of the animation? I thought we were testing the left offset of the animation target?

@@ +83,5 @@
> +}
> +
> +var gEffectEasingTests = [
> +  {
> +    desc: 'steps-start function',

Call this 'steps(start)'? 'steps-start' looks like a misspelling of 'step-start'? (Likewise elsewhere)

@@ +129,5 @@
> +    easingFunction: cubicBezier(0, -0.5, 1, -0.5)
> +  },
> +];
> +
> +gEffectEasingTests.forEach(function(option) {

s/option/options/

@@ +136,5 @@
> +    target.style.position = 'absolute';
> +    var anim = target.animate([ { left: '0px' }, { left: '100px' } ],
> +                              { duration: 1000,
> +                                fill: 'forwards',
> +                                easing: option.easingOption });

'easingOption' is a bit strange. 'easingString', 'easingSpec', or just 'easing' would be better I think.

@@ +154,5 @@
> +  {
> +    desc: 'effect easing produces values greater than 1 with keyframe ' +
> +          'easing cubic-bezier(0, 0, 0, 0)',
> +    easingOption: 'cubic-bezier(0, 1.5, 1, 1.5)',
> +    keyframeEasingOption: 'cubic-bezier(0, 0, 0, 0)', // linear

Likewise, just 'keyframeEasing' would be fine I think.

@@ +215,5 @@
> +  anim.pause();
> +
> +  /* The bezier function produces values greater than 1 in (0.23368794, 1) */
> +  anim.currentTime = 0;
> +  assert_equals(getComputedStyle(target).left, '0px');

This should be 100px, right? Maybe our implementation of step timing functions is wrong?

@@ +259,5 @@
> +  /* The bezier function produces negative values in (0, 0.766312060) */
> +  anim.currentTime = 0;
> +  assert_equals(getComputedStyle(target).left, '0px');
> +  anim.currentTime = 750;
> +  assert_equals(getComputedStyle(target).left, '0px');

For this I would expect:

currentTime = 0; left => 100px
currentTime = 250; left => 0px
currentTime = 750; left => 0px
currentTime = 800; left => 100px

?

@@ +299,5 @@
> +        easing: 'cubic-bezier(0, 1.5, 1, 1.5)' });
> +  var easing = function(x) {
> +    if (x > 1.0) {
> +      /* p3x + (p1y - p3y) / (p1x - p3x) * (x - p3x) */
> +      return 1.0 + (1.5 - 1) / (0 - 1) * (x - 1.0);

I guess this depends on what we agree upon for the extrapolation case. I was expecting the result would just be 1.0 in this case.

@@ +364,5 @@
> +  var target = createDiv(t);
> +  target.style.position = 'absolute';
> +  var anim = target.animate(
> +    // http://cubic-bezier.com/#1,1.5,1,1.5
> +    [ { left: '0px', easing: 'cubic-bezier(1, 1.5, 1, 1.5)' },

Shouldn't we test some cases where we have both keyframe-level and effect-level easing and where the control points are *not* horizontally coincident with the end points?

Comment 93

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 12: Extrapolate bezier function outside [0,1] v2

Comment 94

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #92)

> @@ +215,5 @@
> > +  anim.pause();
> > +
> > +  /* The bezier function produces values greater than 1 in (0.23368794, 1) */
> > +  anim.currentTime = 0;
> > +  assert_equals(getComputedStyle(target).left, '0px');
> 
> This should be 100px, right? Maybe our implementation of step timing
> functions is wrong?

Unfortunately we seem to encounter a similar pitfall as bug 1246893.

Comment 95

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8719675 [details] [diff] [review]
Part 12: Extrapolate bezier function outside [0,1] v2

Review of attachment 8719675 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/ComputedTimingFunction.cpp
@@ +51,5 @@
> +      } else if (mTimingFunction.Y1() == 0 && mTimingFunction.X2() > 0.0) {
> +        return aPortion * mTimingFunction.Y2() / mTimingFunction.X2();
> +      }
> +      // We don't extrapolate at all in case of we can not calculate
> +      // the tangent.

// If we can't calculate a sensible tangent, don't extrapolate at all.

@@ +66,5 @@
> +        return 1.0 + (aPortion - 1.0) *
> +          (mTimingFunction.Y1() - 1) / (mTimingFunction.X1() - 1);
> +      }
> +      // We don't extrapolate at all in case of we can not calculate
> +      // the tangent.

// If we can't calculate a sensible tangent, don't extrapolate at all.

Comment 96

[Hiroyuki Ikezoe (:hiro)]

Brian, I am going to put the tests related to steps(start) into dom/animation/mozilla until bug 1248532 is fixed.  Is it OK with you?

Comment 97

[Hiroyuki Ikezoe (:hiro)]

Removing bug 1248532 dependency.  I have not read web animations spec carefully.  steps(start) is exactly well-defined in the spec.

Comment 98

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 13: Tests for effect-level easing

The tests for steps(start) in this patch rely on the patch I attached in bug 1248532.
I am holding a review request until that bug is closed.

(In reply to Brian Birtles (:birtles) from comment #92)

> @@ +259,5 @@
> > +  /* The bezier function produces negative values in (0, 0.766312060) */
> > +  anim.currentTime = 0;
> > +  assert_equals(getComputedStyle(target).left, '0px');
> > +  anim.currentTime = 750;
> > +  assert_equals(getComputedStyle(target).left, '0px');
> 
> For this I would expect:
> 
> currentTime = 0; left => 100px
> currentTime = 250; left => 0px
> currentTime = 750; left => 0px
> currentTime = 800; left => 100px
> 
> ?

In my understandings, step-start with negative values still always produces 100px here because the value of step-start at 0.0 is 1.0. Right?

> @@ +364,5 @@
> > +  var target = createDiv(t);
> > +  target.style.position = 'absolute';
> > +  var anim = target.animate(
> > +    // http://cubic-bezier.com/#1,1.5,1,1.5
> > +    [ { left: '0px', easing: 'cubic-bezier(1, 1.5, 1, 1.5)' },
> 
> Shouldn't we test some cases where we have both keyframe-level and
> effect-level easing and where the control points are *not* horizontally
> coincident with the end points?

The cases we have both of easings was split into two cases, one is for the case you mentioned, the other is for coincident case.

Comment 99

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #98)
> Created attachment 8720044 [details] [diff] [review]
> Part 13: Tests for effect-level easing
> 
> The tests for steps(start) in this patch rely on the patch I attached in bug
> 1248532.
> I am holding a review request until that bug is closed.
> 
> (In reply to Brian Birtles (:birtles) from comment #92)
> 
> > @@ +259,5 @@
> > > +  /* The bezier function produces negative values in (0, 0.766312060) */
> > > +  anim.currentTime = 0;
> > > +  assert_equals(getComputedStyle(target).left, '0px');
> > > +  anim.currentTime = 750;
> > > +  assert_equals(getComputedStyle(target).left, '0px');
> > 
> > For this I would expect:
> > 
> > currentTime = 0; left => 100px
> > currentTime = 250; left => 0px
> > currentTime = 750; left => 0px
> > currentTime = 800; left => 100px
> > 
> > ?
> 
> In my understandings, step-start with negative values still always produces
> 100px here because the value of step-start at 0.0 is 1.0. Right?

Brian told me I was wrong on IRC.  We need to fix part 11 patch (attachment 8717868 [details] [diff] [review]) too.

Comment 100

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #99)
> Brian told me I was wrong on IRC.  We need to fix part 11 patch (attachment
> 8717868 [details] [diff] [review]) too.

I think my original comment is possibly wrong but I agree that we probably need to change the patch to match what the "before flag" handling in the spec.[1]

[1] http://w3c.github.io/web-animations/#before-flag

Comment 101

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 11: Clamp values of step functions outside [0, 1]

Based on comment #100, this patch needs to be reviewed again.

Comment 102

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 13: Tests for effect-level easing v2

Adapted to the steps(start) function behavior to the spec.
Three tests in this patch still fail due to bug 1248532.

Comment 103

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8720525 [details] [diff] [review]
Part 11: Clamp values of step functions outside [0, 1]

Review of attachment 8720525 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/ComputedTimingFunction.cpp
@@ +40,5 @@
> +
> +  // For steps(start) function, we force to return 0.0 in nagative range
> +  // to achieve before-flag situations because clamping below makes
> +  // steps(start) function's value range (0,1].
> +  // See http://w3c.github.io/web-animations/#before-flag

Although this seems correct to me, I don't think this actually relates to the before flag, does it? This is just how step functions should work?

So perhaps the comment should just be:
// Since we use endpoint-exclusive timing, the output of a steps(start) timing
// function when aPortion = 0.0 is the top of the first step. When aPortion is
// negative, however, we should use the bottom of the first step. We handle negative
// values of aPortion specially here since once we clamp aPortion to [0,1] below we
// will no longer be able to distinguish to the two cases.

Comment 104

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8720535 [details] [diff] [review]
Part 13: Tests for effect-level easing v2

Review of attachment 8720535 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles with those two confusing tests made easier to follow

::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html
@@ +241,5 @@
> +        easing: 'cubic-bezier(0, 1.5, 1, 1.5)' });
> +  var easing = function(x) {
> +    if (x > 1.0) {
> +      /* p3x + (p2y - p3y) / (p2x - p3x) * (x - p3x) */
> +      return 1.0 + (0 - 1) / (0.5 - 1) * (x - 1.0);

I don't understand how this test works.

We have a function called 'easing' that we call like so:

  easing(easing(x))

However, in the case where x <= 1.0, easing is simply:

  cubicBezier(0, 1.5, 1, 1.5)(x);

i.e. we're evaluating

  cubicBezier(0, 1.5, 1, 1.5)(cubicBezier(0, 1.5, 1, 1.5)(x));

But I thought that this test was supposed to be testing:

  cubicBezier(0.5, 1, 0.5, 0)(cubicBezier(0, 1.5, 1, 1.5)(x));

I can see that for x > 1 we do the extrapolation behavior of cubic-bezier(0.5, 1, 0.5, 0), but for any case where x < 1 we won't.

I think we're getting lucky because of the sample points we chose where 0 is 0 no matter where what cubic bezier we use, and 250,500,750 all happen to produce values > 1 for cubicBezier(0, 1.5, 1, 1.5). Can we make this test actually use the two functions correctly so that if we choose other sample points it still works?

@@ +278,5 @@
> +        easing: 'cubic-bezier(0, 1.5, 1, 1.5)' });
> +  var easing = function(x) {
> +    if (x > 1.0) {
> +      // In case of the tangent on the upper boundary is infinity,
> +      // don't extrapolate outer the upper boundary

// For cubic-bezier(0, 1.5, 1, 1.5), the tangent at the endpoint (x = 1.0) is
// infinity so we should just return 1.0.

@@ +319,5 @@
> +    if (x < 0.0) {
> +      /* p0x + (p1y - p0y) / (p1x - p0x) * (x - p0x) */
> +      return (1 / 0.5) * x;
> +    }
> +    return cubicBezier(0, -0.5, 1, -0.5)(x);

As before, I'd prefer if we use the actual functions such that we get the right results even when x > 0.0. But perhaps I'm misunderstanding something?

@@ +354,5 @@
> +        easing: 'cubic-bezier(0, -0.5, 1, -0.5)' });
> +  var easing = function(x) {
> +    if (x < 0.0) {
> +      // In case of the tangent on the lower boundary is infinity,
> +      // don't extrapolate outer the lower boundary

// For cubic-bezier(0, -0.5, 1, -0.5), the tangent at the endpoint (x = 0.0) is
// infinity so we should just return 0.0.

::: testing/web-platform/tests/web-animations/testcommon.js
@@ +66,5 @@
> +function cubicBezier(x1, y1, x2, y2) {
> +    function xForT(t) {
> +      var omt = 1-t;
> +      return 3 * omt * omt * t * x1 + 3 * omt * t * t * x2 + t * t * t;
> +    }

This is a bit weird: We mix 4-space and 2-space indents here.

I think the 4-space indents just came about because this file was originally written by some contractors for Google. We should just go ahead and make it all use 2-spaces (and I'm pretty sure we don't require reviews for whitespace-only changes).

Comment 105

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #104)
> Comment on attachment 8720535 [details] [diff] [review]
> Part 13: Tests for effect-level easing v2
> 
> Review of attachment 8720535 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=birtles with those two confusing tests made easier to follow
> 
> :::
> testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html
> @@ +241,5 @@
> > +        easing: 'cubic-bezier(0, 1.5, 1, 1.5)' });
> > +  var easing = function(x) {
> > +    if (x > 1.0) {
> > +      /* p3x + (p2y - p3y) / (p2x - p3x) * (x - p3x) */
> > +      return 1.0 + (0 - 1) / (0.5 - 1) * (x - 1.0);
> 
> I don't understand how this test works.
> 
> We have a function called 'easing' that we call like so:
> 
>   easing(easing(x))
> 
> However, in the case where x <= 1.0, easing is simply:
> 
>   cubicBezier(0, 1.5, 1, 1.5)(x);
> 
> i.e. we're evaluating
> 
>   cubicBezier(0, 1.5, 1, 1.5)(cubicBezier(0, 1.5, 1, 1.5)(x));
> 
> But I thought that this test was supposed to be testing:
> 
>   cubicBezier(0.5, 1, 0.5, 0)(cubicBezier(0, 1.5, 1, 1.5)(x));
> 
> I can see that for x > 1 we do the extrapolation behavior of
> cubic-bezier(0.5, 1, 0.5, 0), but for any case where x < 1 we won't.
> 
> I think we're getting lucky because of the sample points we chose where 0 is
> 0 no matter where what cubic bezier we use, and 250,500,750 all happen to
> produce values > 1 for cubicBezier(0, 1.5, 1, 1.5). Can we make this test
> actually use the two functions correctly so that if we choose other sample
> points it still works?

Thanks!  I did forget to change the inside of easing function.  Your observations are quite right.  The test accidentally passes.  I will revise it and request a review once more.  Sorry for taking time.

Comment 106

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 13: Tests for effect-level easing v3

* Extrapolation function are separated as each other function variable and have some assertions in itself.
* Indentation changes in testcommon.js
* Making comment styles consistent (there were both '//' and '/**/'.)

Thank you for careful reviewing, and thank you always for corrections in English sentence!

Comment 107

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8720595 [details] [diff] [review]
Part 13: Tests for effect-level easing v3

Review of attachment 8720595 [details] [diff] [review]:
-----------------------------------------------------------------

You need to fix the commit message: it looks like you used hg qfold.

::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html
@@ +247,5 @@
> +    return cubicBezier(0.5, 1, 0.5, 0)(x);
> +  }
> +  var keyframeEasingExtrapolated = function(x) {
> +    assert_greater_than(x, 1.0,
> +      'This function should be called in (1.0, inifinity) range');

s/inifinity/infinity/

@@ +299,5 @@
> +    return cubicBezier(0, 1.5, 1, 1.5)(x);
> +  }
> +  var easingExtrapolated = function(x) {
> +    assert_greater_than(x, 1.0,
> +      'This function should be called in nagative range');

s/nagative/negative/

@@ +350,5 @@
> +    return cubicBezier(0.5, 1, 0.5, 0)(x);
> +  }
> +  var keyframeEasingExtrapolated = function(x) {
> +    assert_less_than(x, 0.0,
> +      'This function should be called in nagative range');

negative

@@ +402,5 @@
> +    return cubicBezier(0, -0.5, 1, -0.5)(x);
> +  }
> +  var easingExtrapolated = function(x) {
> +    assert_less_than(x, 0.0,
> +      'This function should be called in nagative range');

negative

Comment 108

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 10: Remove the limit of the computed timing progress.

Tests have been changed what Brian suggested in comment #70.  I've never thought of using iterations for reftest. Amazing!

Comment 109

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 11: Clamp values of step functions outside [0, 1] v2

Fix comments.

Comment 110

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 12: Extrapolate bezier function outside [0,1] v3

Fix comments.

Comment 111

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 13: Tests for effect-level easing v4

Fix commit message and descriptions for assertions.

Comment 112

[Hiroyuki Ikezoe (:hiro)]

Sorry for lots of spamy messages.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ced244b5b7b

Hello sheriffs, please check-in part 10 to part 13 patches. Thank you!

Comment 113

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99a2bb2048e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ae4553b5b21
https://hg.mozilla.org/integration/mozilla-inbound/rev/94f20e10fc28
https://hg.mozilla.org/integration/mozilla-inbound/rev/98d27c4336aa

Comment 114

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/99a2bb2048e1
https://hg.mozilla.org/mozilla-central/rev/2ae4553b5b21
https://hg.mozilla.org/mozilla-central/rev/94f20e10fc28
https://hg.mozilla.org/mozilla-central/rev/98d27c4336aa