Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 27 • 8 years ago
important

Posted to www-style about whether or not we should do this, or something a little bit different: https://lists.w3.org/Archives/Public/www-style/2016Mar/0108.html

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 28 • 8 years ago

The outcome of that discussion, seems to be to replace steps-middle with a frames() function:

https://lists.w3.org/Archives/Public/www-style/2016Mar/0199.html

I'll update the spec in a few days if there are no objections.

	Boris Chiou [:boris] Assignee
	Comment 29 • 8 years ago

Thanks for your update, Brian. Looks like 'frames' is a good choice (much better then step-middle), and therefore, we will have the original step functions (i.e. step-start, step-end, and steps(n, start|end)), and frames() timing function for Web Animation API.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 31 • 8 years ago

Since the plan is now to drop step-middle and implement frames() (which Chrome doesn't yet implement--it's not even specced), this no longer needs to block shipping Element.animate.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 32 • 7 years ago

Hi Boris, we have a rough spec for this now (I still need to do the Web Animations' part, more accurately specifying how the after-flag works there). I was wondering if you still plan on doing this. If you're busy with Stylo work, I could blog about this feature and see if a new contributor wants to try it (it would be a fun bug to fix I think).

	Boris Chiou [:boris] Assignee
	Comment 33 • 7 years ago

(In reply to Brian Birtles (:birtles, away most of Jan 21 - Feb 1) from comment #32)
> Hi Boris, we have a rough spec for this now (I still need to do the Web
> Animations' part, more accurately specifying how the after-flag works
> there). I was wondering if you still plan on doing this. If you're busy with
> Stylo work, I could blog about this feature and see if a new contributor
> wants to try it (it would be a fun bug to fix I think).

Sure, I can work on this. (Need something other than stylo :)) Please let me know after you finish the Web animation spec part. Thanks.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 34 • 7 years ago

Making this depend on bug 1332206 since that bug will significantly rework our timing function tests and we should make the tests for this bug follow the arrangement there.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 35 • 7 years ago

Bug 1332206 has landed on m-c and the css-timing spec is up to date (it turns out I don't need to update Web Animations after all) so I think it would be ok to start implementing this. I sent a request to publish an First Public Working Draft (FPWD) to the CSSWG but they didn't get to it in their telcon last night. Hopefully that will happen next week.

We'll need to send an "Intent to implement" for this but that might be easier once we have a FPWD to point to.

	Boris Chiou [:boris] Assignee
	Comment 36 • 7 years ago

(In reply to Brian Birtles (:birtles) from comment #35)
> Bug 1332206 has landed on m-c and the css-timing spec is up to date (it
> turns out I don't need to update Web Animations after all) so I think it
> would be ok to start implementing this. I sent a request to publish an First
> Public Working Draft (FPWD) to the CSSWG but they didn't get to it in their
> telcon last night. Hopefully that will happen next week.
> 
> We'll need to send an "Intent to implement" for this but that might be
> easier once we have a FPWD to point to.

Thanks for update, Brian. After the FPWD is accepted, let's send an "Intent to implement" for this.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 37 • 7 years ago

FPWD has now been published: https://www.w3.org/TR/css-timing-1/

	Boris Chiou [:boris] Assignee
	Comment 38 • 7 years ago

(In reply to Brian Birtles (:birtles) from comment #37)
> FPWD has now been published: https://www.w3.org/TR/css-timing-1/

Thanks, I'm going to send a "intent to implement" mail for frames timing function.

	Boris Chiou [:boris] Assignee
	Comment 40 • 7 years ago

Hi Brian,

The spec says: "The output progress value is calculated from the input progress value and before flag as follows", but the following steps don't mention *before flag*. Is this a typo? or how do we use *before flag* for frames timing function?

BTW, the serialization doesn't mention frames, but I think the most reasonable way to serialize it is use something like "frames(n)", n is the frames number.

Thanks.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 41 • 7 years ago

Yes, that's a typo. I dropped the before flag after I realised we don't need it.

Serialization is not specified because it follows standard CSS serialization. We only need to specify it when there are special requirements.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 53 • 7 years ago
mozreview-review

Comment on attachment 8841574 [details]
Bug 1248340 - Part 4: Implement Frames in the style system.

https://reviewboard.mozilla.org/r/115548/#review117316

I *think* this makes sense (i.e. don't introduce a new unit-type for frames(), but just use "function" and store the function name as the first value in the array), but I'm going to defer to Daniel on this because he know this area better than I do.

::: layout/style/nsCSSParser.cpp:16823
(Diff revision 1)
> +  // The number of frames must be a positive integer greater than one.
> +  if (val->Item(1).GetIntValue() == 1) {
> +    return false;
> +  }

How does this work? Does VARIANT_INTEGER mean an integer >= 1?

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 54 • 7 years ago
mozreview-review

Comment on attachment 8841575 [details]
Bug 1248340 - Part 2: Add the preference.

https://reviewboard.mozilla.org/r/115550/#review117320

I'm going to clear the review on this because I'm not yet persuaded this needs a pref. I've been corresponding with Tom directly but I've yet to see a case for making this individually pref-able from a security point of view.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 55 • 7 years ago
mozreview-review

Comment on attachment 8841576 [details]
Bug 1248340 - Part 1: Add the type of Frames in nsTimingFunction.

https://reviewboard.mozilla.org/r/115552/#review117318

r=me with the following changes, assuming you agree (and I'm not missing an obvious reason to do it the way you have it based on something later in this patch series).

::: layout/style/nsStyleStruct.h:2356
(Diff revision 1)
> -    return aType != Type::StepStart && aType != Type::StepEnd;
> +    return aType != Type::StepStart && aType != Type::StepEnd &&
> +           aType != Type::Frames;

Now that this covers several lines, it would be easier to read if we put each condition on a new line.

    return aType != Type::StepStart &&
           aType != Type::StepEnd &&
           aType != Type::Frames;

::: layout/style/nsStyleStruct.h:2380
(Diff revision 1)
> +    if (mType == Type::Frames) {
> +      mFrames = aStepsOrFrames;
> +      return;
> +    }
> +
>      MOZ_ASSERT(mType == Type::StepStart || mType == Type::StepEnd,
>                 "wrong type");
> -    mSteps = aSteps;
> +    mSteps = aStepsOrFrames;

If you agree with my comments below, I think this would just become:

    MOZ_ASSERT(mType == Type::StepStart ||
               mType == Type::StepEnd ||
               mType == Type::Frames,
               "wrong type");
    mStepsOrFrames = aStepsOrFrames;

::: layout/style/nsStyleStruct.h:2403
(Diff revision 1)
>      struct {
>        uint32_t mSteps;
>      };
> +    struct {
> +      uint32_t mFrames;
> +    };

Why not just rename mSteps to mStepsOrFrames?

::: layout/style/nsStyleStruct.h:2425
(Diff revision 1)
> +    } else if (mType == Type::Frames) {
> +      mFrames = aOther.mFrames;
>      } else {

Then we won't need this

::: layout/style/nsStyleStruct.h:2443
(Diff revision 1)
> -    return mSteps == aOther.mSteps;
> +    return mType == Type::Frames
> +           ? mFrames == aOther.mFrames
> +           : mSteps == aOther.mSteps;

Or this

	Daniel Holbert [:dholbert]
	Comment 56 • 7 years ago
mozreview-review

Comment on attachment 8841575 [details]
Bug 1248340 - Part 2: Add the preference.

https://reviewboard.mozilla.org/r/115550/#review117324

::: layout/style/nsCSSParser.cpp:16814
(Diff revision 1)
> +  if(!Preferences::GetBool("layout.css.timing.frames.enabled")) {
> +    return false;
> +  }

Drive-by nit on this part:
Preferences::GetBool is somewhat expensive. Instead, please use AddBoolVarCache to set up a static variable as a read-only proxy for the preference.

(See other usages of that function in this file.)

Also, you should have a space between "if" and the open-paren here.

	Daniel Holbert [:dholbert]
	Comment 57 • 7 years ago
mozreview-review-reply

Comment on attachment 8841574 [details]
Bug 1248340 - Part 4: Implement Frames in the style system.

https://reviewboard.mozilla.org/r/115548/#review117316

> How does this work? Does VARIANT_INTEGER mean an integer >= 1?

VARIANT_INTEGER just means "parse an integer" -- but in the context of the function we're passing it to (which has "OneOrLarger" in the name), it does mean "an integer >= 1", yeah.

Boris: You should probably add an assertion to make this expectation clearer here (and also to ensure that e.g. integer overflow from huge input etc. doesn't make it to this code).

Maybe just before this "GetIntValue() == 1" return clause, add an assertion that GetIntValue() >= 1, with an assertion message like "Parsing function should've enforced OneOrLarger, per its name".

	Daniel Holbert [:dholbert]
	Comment 58 • 7 years ago
mozreview-review

Comment on attachment 8841574 [details]
Bug 1248340 - Part 4: Implement Frames in the style system.

https://reviewboard.mozilla.org/r/115548/#review117330

::: layout/style/nsCSSParser.cpp:7944
(Diff revision 1)
>          SkipUntil(')');
>          return CSSParseResult::Error;
>        }
>        return CSSParseResult::Ok;
>      }
> +    if (tk->mIdent.LowerCaseEqualsLiteral("frames")) {

A few missing pieces here (not tied directly to this line of code, just putting it here for lack of a better place): this new parser code should land with tests that exercise it in property_database.js [1] as part of the same changeset -- with valid syntax and invalid syntax, to exercise your new parsing code via the suite of tests that use property_database.js.

(1) Tests -- any patch that touches the CSS parser needs to update property_database.js to exercise the new values -- ideally as part of the same commit.  In this case, you'll want to add new entries to "other_values" and "invalid_values" for the "animation-timing-function" property in that file:

https://dxr.mozilla.org/mozilla-central/rev/106a96755d3bcebe64bbbc3b521d65d262ba9c02/layout/style/test/property_database.js#971

BUT, since your property is pref-controlled [as of the next patch], you don't really want to directly touch those arrays. Instead, you should append to them, contingent on your about:config preference.  So you'll really want to add an "if (IsCSSPropertyPrefEnabled([yourpref])" chunk to that file, and append to the array values, kind of like how we do for 'subgrid' here:
https://dxr.mozilla.org/mozilla-central/rev/106a96755d3bcebe64bbbc3b521d65d262ba9c02/layout/style/test/property_database.js#6247

(NOTE: For that ^^ property_database.js tweak to work correctly, I think you need to *already* have the pref in all.js. Otherwise IsCSSPropertyPrefEnabled will throw an exception (intentionally).  So you probably want to either merge part 2 into this patch, or just add the property_database.js tests after (or in) part 2.)

(2) I don't see any code here to handle this new value in nsRuleNode.cpp (where we go from specified-style to computed style) -- specifically nsRuleNode::ComputeTimingFunction, I think. Without some changes there, I suspect that this new parsing code will trigger the NS_NOTREACHED at the end of that function:
https://dxr.mozilla.org/mozilla-central/rev/106a96755d3bcebe64bbbc3b521d65d262ba9c02/layout/style/nsRuleNode.cpp#5513-5514

(3) You probably need to update our getComputedStyle implementation, for computed-style stringification.  (Does this report the correct stringified value back, via getComputedStyle, right now? I suspect it won't.)  I imagine you need to update AppendTimingFunction in nsComputedDOMStyle.cpp, or some related code.  

(4) You probably need to update nsCSSValue::AppendToString , for specified-style stringification. I see some stuff about "eCSSUnit_Steps" and some special cases for eCSSProperty_transition_timing_function at https://dxr.mozilla.org/mozilla-central/rev/106a96755d3bcebe64bbbc3b521d65d262ba9c02/layout/style/nsCSSValue.cpp#1323 -- it's not clear to me whether your new eCSSUnit_Function values will Just Work there or not.

::: layout/style/nsCSSParser.cpp:16817
(Diff revision 1)
> +  RefPtr<nsCSSValue::Array> val = aValue.InitFunction(functionName, 1);
> +  if (!ParseSingleTokenOneOrLargerVariant(val->Item(1), VARIANT_INTEGER,
> +                                          nullptr)) {

Nit: best practice is to avoid touching the outparam until you're sure we're going to return a success value. (As long as it's practical to do so.)

So: please just use a local nsCSSValue variable for the ParseSingleTokenOneOrLargerVariant call, and don't touch aValue until *after* we've gotten past all of the failure return statements.  And then (after the ExpectSymbol for the close-paren), you can initialize aValue and directly assign val->Item() = whateverYouCallYourLocalVariable, and unconditionally return true.

	Daniel Holbert [:dholbert]
	Comment 59 • 7 years ago
mozreview-review-reply

Comment on attachment 8841574 [details]
Bug 1248340 - Part 4: Implement Frames in the style system.

https://reviewboard.mozilla.org/r/115548/#review117330

> A few missing pieces here (not tied directly to this line of code, just putting it here for lack of a better place): this new parser code should land with tests that exercise it in property_database.js [1] as part of the same changeset -- with valid syntax and invalid syntax, to exercise your new parsing code via the suite of tests that use property_database.js.
> 
> (1) Tests -- any patch that touches the CSS parser needs to update property_database.js to exercise the new values -- ideally as part of the same commit.  In this case, you'll want to add new entries to "other_values" and "invalid_values" for the "animation-timing-function" property in that file:
> 
> https://dxr.mozilla.org/mozilla-central/rev/106a96755d3bcebe64bbbc3b521d65d262ba9c02/layout/style/test/property_database.js#971
> 
> BUT, since your property is pref-controlled [as of the next patch], you don't really want to directly touch those arrays. Instead, you should append to them, contingent on your about:config preference.  So you'll really want to add an "if (IsCSSPropertyPrefEnabled([yourpref])" chunk to that file, and append to the array values, kind of like how we do for 'subgrid' here:
> https://dxr.mozilla.org/mozilla-central/rev/106a96755d3bcebe64bbbc3b521d65d262ba9c02/layout/style/test/property_database.js#6247
> 
> (NOTE: For that ^^ property_database.js tweak to work correctly, I think you need to *already* have the pref in all.js. Otherwise IsCSSPropertyPrefEnabled will throw an exception (intentionally).  So you probably want to either merge part 2 into this patch, or just add the property_database.js tests after (or in) part 2.)
> 
> (2) I don't see any code here to handle this new value in nsRuleNode.cpp (where we go from specified-style to computed style) -- specifically nsRuleNode::ComputeTimingFunction, I think. Without some changes there, I suspect that this new parsing code will trigger the NS_NOTREACHED at the end of that function:
> https://dxr.mozilla.org/mozilla-central/rev/106a96755d3bcebe64bbbc3b521d65d262ba9c02/layout/style/nsRuleNode.cpp#5513-5514
> 
> (3) You probably need to update our getComputedStyle implementation, for computed-style stringification.  (Does this report the correct stringified value back, via getComputedStyle, right now? I suspect it won't.)  I imagine you need to update AppendTimingFunction in nsComputedDOMStyle.cpp, or some related code.  
> 
> (4) You probably need to update nsCSSValue::AppendToString , for specified-style stringification. I see some stuff about "eCSSUnit_Steps" and some special cases for eCSSProperty_transition_timing_function at https://dxr.mozilla.org/mozilla-central/rev/106a96755d3bcebe64bbbc3b521d65d262ba9c02/layout/style/nsCSSValue.cpp#1323 -- it's not clear to me whether your new eCSSUnit_Function values will Just Work there or not.

Argh, sorry, it's late. I'm now seeing that you've addressed my missing-thing #2 (nsRuleNode) in "Part 5", and #3 and #4 in "Part 6" (serialization).  That's good.

Ideally, those should all be in this same patch -- really, parts 2, 5, and 6 should all be folded in to part 1. Otherwise, this patch isn't an atomic commit -- it'll trip assertions (perhaps some fatal assertions even) if you try to run it with test CSS that exercises this new behavior.  Where possible, each intermediate state between commits should pass tests (and be usefully testable) on its own.

Would you mind folding those into this patch, so that it's clearer what the full lifecycle of this new CSS syntax is?

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 60 • 7 years ago

Also, I'm suggesting this doesn't need a pref. Boris added one because someone raised security concerns but I'm not persuaded yet that there is a legitimate concern here (and if there is, the pref should probably cover all timing functions, not just this one). If someone wants to argue for a pref based on compatibility, stability etc. that's a different case, however.

	Daniel Holbert [:dholbert]
	Comment 61 • 7 years ago

[I wrote this and midair'ed comment 60 -- if there ends up being no pref per comment 60, then clearly disregard this upcoming suggestion]:

ALTERNATELY to the folding request in comment 59: if you'd rather keep this split out into many microcommits, you could still achieve "each intermediate commit is atomic" status by:
 (a) Add the pref up-front (in part 1), *disabled by default*.
 (b) Add a one-off patch towards the end of your queue (*after* all the patches with the requisite functionality) which flips the pref to true on Nightly/Aurora.

Personally I'd still prefer for the full parsing/computing/serializing lifecycle to happen in the same patch, but I'd be OK with this alternate strategy as well if you'd like (or another similar strategy that similarly achieves the "each individual commit is a functional intermediate state" result).

	Boris Chiou [:boris] Assignee
	Comment 62 • 7 years ago

(In reply to Brian Birtles (:birtles) from comment #60)
> Also, I'm suggesting this doesn't need a pref. Boris added one because
> someone raised security concerns but I'm not persuaded yet that there is a
> legitimate concern here (and if there is, the pref should probably cover all
> timing functions, not just this one). If someone wants to argue for a pref
> based on compatibility, stability etc. that's a different case, however.

Yes, I agree. The pref here which covers only this Frame function is weird. I will drop part 2. If Tom or others have the more persuasive concern on compatibility, stability, or security, I will add it back (by AddBoolVarCache).

	Boris Chiou [:boris] Assignee
	Comment 63 • 7 years ago

(In reply to Daniel Holbert [:dholbert] from comment #61)
> [I wrote this and midair'ed comment 60 -- if there ends up being no pref per
> comment 60, then clearly disregard this upcoming suggestion]:
> 
> ALTERNATELY to the folding request in comment 59: if you'd rather keep this
> split out into many microcommits, you could still achieve "each intermediate
> commit is atomic" status by:
>  (a) Add the pref up-front (in part 1), *disabled by default*.
>  (b) Add a one-off patch towards the end of your queue (*after* all the
> patches with the requisite functionality) which flips the pref to true on
> Nightly/Aurora.
> 
> Personally I'd still prefer for the full parsing/computing/serializing
> lifecycle to happen in the same patch, but I'd be OK with this alternate
> strategy as well if you'd like (or another similar strategy that similarly
> achieves the "each individual commit is a functional intermediate state"
> result).

Thanks a lot, Daniel. I will fold part 1, part 5, part 6, and the change of updating property_database.js together into part 1, so the reviewer can easily check all the things you mentioned.

	Boris Chiou [:boris] Assignee
	Comment 64 • 7 years ago
mozreview-review-reply

Comment on attachment 8841576 [details]
Bug 1248340 - Part 1: Add the type of Frames in nsTimingFunction.

https://reviewboard.mozilla.org/r/115552/#review117318

> Why not just rename mSteps to mStepsOrFrames?

It's a good suggestion. At first, I rename it to mStepsOrFrames, but I'm not sure if it is a good idea to let people understand the code, so finally, I split it into different data members. I can rename it to mStepsOrFrames and do the same thing for the other patches. Thanks.

	Boris Chiou [:boris] Assignee
	Comment 65 • 7 years ago

clear the reviews because I should apply the changes mentioned in Part 3 into all other patches.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 66 • 7 years ago
mozreview-review

Comment on attachment 8841577 [details]
Bug 1248340 - Part 4: Parse Frames from Web Animations API.

https://reviewboard.mozilla.org/r/115554/#review117720

I'm guessing you meant to clear the review on this one too?

(It looks fine, I mean, it's only one line! But it's probably best I review it in the context of the other changes you're making. Also, I really appreciate you splitting up the patches into small pieces but, like Daniel said, we do need to make sure they're atomic and this patch series is currently a little hard to follow.)

	Boris Chiou [:boris] Assignee
	Comment 74 • 7 years ago

(In reply to Brian Birtles (:birtles) from comment #66)
> Comment on attachment 8841577 [details]
> Bug 1248340 - Part 4: Parse Frames from Web Animations API.
> 
> https://reviewboard.mozilla.org/r/115554/#review117720
> 
> I'm guessing you meant to clear the review on this one too?
> 
> (It looks fine, I mean, it's only one line! But it's probably best I review
> it in the context of the other changes you're making. Also, I really
> appreciate you splitting up the patches into small pieces but, like Daniel
> said, we do need to make sure they're atomic and this patch series is
> currently a little hard to follow.)

Thanks, Brian. I merged this into other patch. Besides, I moved the parser, serialization, and its related code to the final one in this series (excluding the test cases), so it shouldn't trigger other assertions. Sorry for making these patches so hard to follow.

	Daniel Holbert [:dholbert]
	Comment 75 • 7 years ago
mozreview-review

Comment on attachment 8841574 [details]
Bug 1248340 - Part 4: Implement Frames in the style system.

https://reviewboard.mozilla.org/r/115548/#review117934

r=me, with nits:

::: layout/style/nsCSSParser.cpp:16819
(Diff revision 2)
> +  nsCSSKeyword functionName = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> +  MOZ_ASSERT(functionName == eCSSKeyword_frames);
> +
> +  // ParseSingleTokenOneOrLargerVariant tries to parse the token as an integer
> +  // which is one or greater than one. If it is outside the range of a
> +  // 32-bit signed number, we clamped it to the maximum values.

"we clamped it" here is a little vague -- it's not clear whether "we" is code nearby this comment vs. code inside the helper-function.  I think you really are referring to the helper-function, so maybe:
  s/we clamped/this helper function clamps/

Alternately: you could just get rid of this whole comment, too; everything it says is pretty clear, given the function name and the assert that you've now added [thanks for that!]

::: layout/style/nsRuleNode.cpp:5514
(Diff revision 2)
> +    case eCSSUnit_Function:
> +      {
> +        nsCSSValue::Array* array = aValue.GetArrayValue();
> +        NS_ASSERTION(array && array->Count() == 2, "Need 2 items");
> +        NS_ASSERTION(array->Item(1).GetUnit() == eCSSUnit_Integer,
> +                     "unexpected frames function value");
> +        aResult = nsTimingFunction(nsTimingFunction::Type::Frames,

Please add an assertion here to verify that the function name is "frames", too:
NS_ASSERTION(array->Item(0).GetKeywordValue() == eCSSKeyword_frames, ...);

(It's possible that we'll add more possible function values here in the future, and/or maybe we'll convert e.g. "steps(...)" into a eCSSUnit_Function representation in the css parser.  And if anything like this happens, we need to be sure that we don't accidentally let these values fall into this code and produce nsTimingFunction::Type::Frames-flavored values from them. An assertion will help us detect that this code needs updates, in that circumstance.)

::: layout/style/test/property_database.js:976
(Diff revision 2)
>    "animation-timing-function": {
>      domProp: "animationTimingFunction",
>      inherited: false,
>      type: CSS_TYPE_LONGHAND,
>      initial_values: [ "ease" ],
> -    other_values: [ "cubic-bezier(0.25, 0.1, 0.25, 1.0)", "linear", "ease-in", "ease-out", "ease-in-out", "linear, ease-in, cubic-bezier(0.1, 0.2, 0.8, 0.9)", "cubic-bezier(0.5, 0.5, 0.5, 0.5)", "cubic-bezier(0.25, 1.5, 0.75, -0.5)", "step-start", "step-end", "steps(1)", "steps(2, start)", "steps(386)", "steps(3, end)" ],
> +    other_values: [ "cubic-bezier(0.25, 0.1, 0.25, 1.0)", "linear", "ease-in", "ease-out", "ease-in-out", "linear, ease-in, cubic-bezier(0.1, 0.2, 0.8, 0.9)", "cubic-bezier(0.5, 0.5, 0.5, 0.5)", "cubic-bezier(0.25, 1.5, 0.75, -0.5)", "step-start", "step-end", "steps(1)", "steps(2, start)", "steps(386)", "steps(3, end)", "frames(2)", "frames(1000)" ],

Please add (at least) one more "other_values" here -- frames with spaces inside the parens, like so:
  "frames( 2 )"

(If your ExpectSymbol call in nsCSSParser happened to use "false" instead of "true", then this value would be rejected (incorrectly).  We need a testcase like this, to be sure we've got that correct & that we don't break that without noticing in the future.)

::: layout/style/test/property_database.js:977
(Diff revision 2)
>      domProp: "animationTimingFunction",
>      inherited: false,
>      type: CSS_TYPE_LONGHAND,
>      initial_values: [ "ease" ],
> -    other_values: [ "cubic-bezier(0.25, 0.1, 0.25, 1.0)", "linear", "ease-in", "ease-out", "ease-in-out", "linear, ease-in, cubic-bezier(0.1, 0.2, 0.8, 0.9)", "cubic-bezier(0.5, 0.5, 0.5, 0.5)", "cubic-bezier(0.25, 1.5, 0.75, -0.5)", "step-start", "step-end", "steps(1)", "steps(2, start)", "steps(386)", "steps(3, end)" ],
> -    invalid_values: [ "none", "auto", "cubic-bezier(0.25, 0.1, 0.25)", "cubic-bezier(0.25, 0.1, 0.25, 0.25, 1.0)", "cubic-bezier(-0.5, 0.5, 0.5, 0.5)", "cubic-bezier(1.5, 0.5, 0.5, 0.5)", "cubic-bezier(0.5, 0.5, -0.5, 0.5)", "cubic-bezier(0.5, 0.5, 1.5, 0.5)", "steps(2, step-end)", "steps(0)", "steps(-2)", "steps(0, step-end, 1)" ]
> +    other_values: [ "cubic-bezier(0.25, 0.1, 0.25, 1.0)", "linear", "ease-in", "ease-out", "ease-in-out", "linear, ease-in, cubic-bezier(0.1, 0.2, 0.8, 0.9)", "cubic-bezier(0.5, 0.5, 0.5, 0.5)", "cubic-bezier(0.25, 1.5, 0.75, -0.5)", "step-start", "step-end", "steps(1)", "steps(2, start)", "steps(386)", "steps(3, end)", "frames(2)", "frames(1000)" ],
> +    invalid_values: [ "none", "auto", "cubic-bezier(0.25, 0.1, 0.25)", "cubic-bezier(0.25, 0.1, 0.25, 0.25, 1.0)", "cubic-bezier(-0.5, 0.5, 0.5, 0.5)", "cubic-bezier(1.5, 0.5, 0.5, 0.5)", "cubic-bezier(0.5, 0.5, -0.5, 0.5)", "cubic-bezier(0.5, 0.5, 1.5, 0.5)", "steps(2, step-end)", "steps(0)", "steps(-2)", "steps(0, step-end, 1)", "frames(1)", "frames(-2)" ]

Please also include a few more invalid_values, to test edge cases a bit better (to be sure we reject them & we don't crash or leave the parser in a broken state).  I think these all should be invalid & added to this list:
 frames
 frames()
 frames(,)
 frames(a)
 frames(2.0)
 frames(2.5)
 frames(2 3)

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 76 • 7 years ago
mozreview-review

Comment on attachment 8841580 [details]
Bug 1248340 - Part 2: Implement Frames in ComputedTimingFunction.

https://reviewboard.mozilla.org/r/115560/#review118040

::: dom/animation/ComputedTimingFunction.cpp:70
(Diff revision 2)
> +FramesTiming(uint32_t aFrames, double aPortion)
> +{
> +  MOZ_ASSERT(aFrames > 1, "the number of frames must be greater than 1");
> +  int32_t currentFrame = floor(aPortion * aFrames);
> +  double result = double(currentFrame) / double(aFrames - 1);
> +  if (result > 1.0 && aPortion <= 1.0) {

Even though this is just doing what the spec says, it would be good to add a comment explaining this:

  // Don't overshoot the natural range of the animation (by producing an output
  // progress greater than 1.0) when we are at the exact end of its interval
  // (i.e. the input progress is 1.0).

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 77 • 7 years ago
mozreview-review

Comment on attachment 8841581 [details]
Bug 1248340 - Part 3: Implement Frames for OMTA.

https://reviewboard.mozilla.org/r/115562/#review118056

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 78 • 7 years ago
mozreview-review

Comment on attachment 8841582 [details]
Bug 1248340 - Part 5: Add tests to css-timing-1.

https://reviewboard.mozilla.org/r/115710/#review118058

I'd really like to check with dbaron/gsnedders/jgraham if we're ok to go ahead and add this to wpt. My understanding is that csswg-test will merge there eventually but I don't know if it's ok to go ahead and contribute there yet. Unfortunately, in terms of contributing to csswg-test, my understanding is we only sync reftests from there with m-c(?) so I really hope we can add this to wpt.

Assuming this is ok, I think the folder should probably be named css-timing-1 to match csswg-test naming.

Also, can you check through http://web-platform-tests.org/writing-tests/general-guidelines.html and http://web-platform-tests.org/writing-tests/css-metadata.html and make sure we're following the guidelines for CSS tests there? (Those metadata requirements seem a bit of a pain. Perhaps we can skip the optional ones, and perhaps some are only intended for reftests?)

Finally, we should include a test here for input progress values < 0 and > 1. We'll need to use Web Animations to do that, of course, but provided it is a separate test, then any UAs that don't support Web Animations should be able mark that one test as an expected failure.

Clearing review for now because there is quite a bit of feedback below so I'd like to check this again after that.

(For my own reference, next time I review this I want to check if the order of tests makes sense. Currently it's not quite clear what the grouping is between tests.)

::: testing/web-platform/tests/css-timing/OWNERS:1
(Diff revision 2)
> +@birtles

We should probably add the other editors:
@ChumpChief
@grorg
@shans

::: testing/web-platform/tests/css-timing/frames-timing-functions.html:15
(Diff revision 2)
> +  createStyle(t, { '@keyframes anim': 'from { left: 0px; }' +
> +                                      '  to { left: 100px; }',
> +                   '.target': 'animation: anim 10s frames(1)' });
> +  var div = createDiv(t);
> +  div.className = 'target';
> +  assert_equals(getComputedStyle(div).animationTimingFunction, 'ease',
> +                'Default easing');

I think we could make this even simpler?

e.g.

  const div = createDiv(t);
  div.style.animation = 'abc 1s ease-in';
  div.style.animationTimingFunction = 'frames(1)';
  assert_equals(getComputedStyle(div).animationTimingFunction, 'ease-in');

?

(We'd need to update the description though to s/default easing/previously-set easing/)

::: testing/web-platform/tests/css-timing/frames-timing-functions.html:40
(Diff revision 2)
> +  createStyle(t, { '@keyframes anim': 'from { left: 0px; }' +
> +                                      '  to { left: 100px; }',
> +                   '.target': 'animation: anim 10s frames(2)' });

(Again, I'm not sure if we need to do this. It seems like we can just use div.style.animation = '...')

::: testing/web-platform/tests/css-timing/frames-timing-functions.html:42
(Diff revision 2)
> +                   '.target': 'animation: anim 10s frames(2)' });
> +  var div = createDiv(t);
> +  div.className = 'target';
> +  assert_equals(getComputedStyle(div).animationTimingFunction, 'frames(2)',
> +                'The serialization of frames');
> +}, 'The serialization of frames is \'frames(n)\', n is the number of frames');

Would it be more interesting to test:

 'frames(  2 )' => 'frames(2)'

?

::: testing/web-platform/tests/css-timing/frames-timing-functions.html:49
(Diff revision 2)
> +test(function(t) {
> +  createStyle(t, { '@keyframes anim': 'from { left: 0px; }' +
> +                                      '  to { left: 100px; }',
> +                   '.target': 'animation: anim 10s frames(9999999999)' });
> +  var div = createDiv(t);
> +  div.className = 'target';
> +  assert_equals(getComputedStyle(div).animationTimingFunction,
> +                'frames(2147483647)',
> +                'The clamped frames');
> +}, 'The frame number outside the range of a 32bit signed integer is clamped ' +
> +   'to the max value');

Where is this specified?

::: testing/web-platform/tests/css-timing/frames-timing-functions.html:61
(Diff revision 2)
> +test(function(t) {
> +  createStyle(t, { '@keyframes anim': 'from { left: 0px; }' +
> +                                      '  to { left: 100px; }',
> +                   '.target': 'animation: anim 11s frames(11) 2' });
> +  var div = createDiv(t);
> +  div.className = 'target';
> +
> +  // Animation should display the first and last frame of the animation for
> +  // an equal amount of time as each other frame during each loop.
> +  var anim = div.getAnimations()[0];
> +  anim.currentTime = 0;
> +  assert_equals(getComputedStyle(div).left, '0px');
> +  anim.currentTime = 999;
> +  assert_equals(getComputedStyle(div).left, '0px');
> +  anim.currentTime = 1000;
> +  assert_equals(getComputedStyle(div).left, '10px');
> +  anim.currentTime = 9999;
> +  assert_equals(getComputedStyle(div).left, '90px');
> +  anim.currentTime = 10000;
> +  assert_equals(getComputedStyle(div).left, '100px');
> +  anim.currentTime = 11000;
> +  assert_equals(getComputedStyle(div).left, '0px'); // The second loop.
> +}, 'The computed values are correct with Frames timing on CSS Animations');

This feels a bit hap-hazard to me.

I think we want to test:

'frames(2)'
=> Test zero time
=> Test transition-point 0.499 and 0.5
   (this will be easier if you set the duration to 10s)
=> Test 100% time
   (need to set fill mode to forwards)

That might be enough?

If you want to test that the number of steps is being correctly calculated, then add another test for 'frames(11)' and sample either side of the first step? i.e. just your first test from above?

I would make the above two or more separate tests

e.g.

* 'For an input progress of 0.0, the output of a frames timing function is 0.0'
* 'At a frame boundary, the output of a frames timing function is the next frame'
* 'For an input progress of 1.0, the output of a frames timing function is 1.0'
* 'The number of frames is correctly reflected in the frames timing function output'

::: testing/web-platform/tests/css-timing/frames-timing-functions.html:85
(Diff revision 2)
> +test(function(t) {
> +  var div = createDiv(t);
> +  div.style.left = '0px';
> +  getComputedStyle(div).transitionProperty;
> +
> +  div.style.transition = 'left 11s';
> +  div.style.transitionTimingFunction = 'frames(11)';
> +  div.style.left = '100px';
> +
> +  var anim = div.getAnimations()[0];
> +  anim.currentTime = 0;
> +  assert_equals(getComputedStyle(div).left, '0px');
> +  anim.currentTime = 999;
> +  assert_equals(getComputedStyle(div).left, '0px');
> +  anim.currentTime = 1000;
> +  assert_equals(getComputedStyle(div).left, '10px');
> +  anim.currentTime = 9999;
> +  assert_equals(getComputedStyle(div).left, '90px');
> +  anim.currentTime = 10000;
> +  assert_equals(getComputedStyle(div).left, '100px');
> +  anim.currentTime = 11000;
> +  assert_equals(getComputedStyle(div).left, '100px');
> +}, 'The computed values are correct with Frames timing on CSS Transitions');

I'm not sure how much we need to test with *both* animations and transitions but I guess it doesn't hurt. Perhaps we could just to the last of the above tests with transitions?

::: testing/web-platform/tests/css-timing/testcommon.js:1
(Diff revision 2)
> +/*
> +Distributed under both the W3C Test Suite License [1] and the W3C
> +3-clause BSD License [2]. To contribute to a W3C Test Suite, see the
> +policies and contribution forms [3].
> +
> +[1] http://www.w3.org/Consortium/Legal/2008/04-testsuite-license
> +[2] http://www.w3.org/Consortium/Legal/2008/03-bsd-license
> +[3] http://www.w3.org/2004/10/27-testcases
> + */

Just curious, why do we need this?

::: testing/web-platform/tests/css-timing/testcommon.js:13
(Diff revision 2)
> +[3] http://www.w3.org/2004/10/27-testcases
> + */
> +
> +'use strict';
> +
> +var MS_PER_SEC = 1000;

This doesn't seem to be used?

::: testing/web-platform/tests/css-timing/testcommon.js:15
(Diff revision 2)
> +// creates div element, appends it to the document body and
> +// removes the created element during test cleanup

s/creates div element/Creates a <div> element/

::: testing/web-platform/tests/css-timing/testcommon.js:21
(Diff revision 2)
> +// removes the created element during test cleanup
> +function createDiv(test, doc) {
> +  return createElement(test, 'div', doc);
> +}
> +
> +// creates element of given tagName, appends it to the document body and

s/creates element of given tagName/Creates an element with the given |tagName|/

::: testing/web-platform/tests/css-timing/testcommon.js:22
(Diff revision 2)
> +function createDiv(test, doc) {
> +  return createElement(test, 'div', doc);
> +}
> +
> +// creates element of given tagName, appends it to the document body and
> +// removes the created element during test cleanup

Period (.) needed at the end of this line.

::: testing/web-platform/tests/css-timing/testcommon.js:23
(Diff revision 2)
> +  return createElement(test, 'div', doc);
> +}
> +
> +// creates element of given tagName, appends it to the document body and
> +// removes the created element during test cleanup
> +// if tagName is null or undefined, returns div element

s/if tagName/If |tagName|/

s/returns div element/creates a <div> element/

::: testing/web-platform/tests/css-timing/testcommon.js:35
(Diff revision 2)
> +
> +// Creates a style element with the specified rules, appends it to the document
> +// head and removes the created element during test cleanup.
> +// |rules| is an object. For example:
> +// { '@keyframes anim': '' ,
> +//   '.className': 'animation: anim 100s;' };
> +// or
> +// { '.className1::before': 'content: ""; width: 0px; transition: all 10s;',
> +//   '.className2::before': 'width: 100px;' };
> +// The object property name could be a keyframes name, or a selector.
> +// The object property value is declarations which are property:value pairs
> +// split by a space.
> +function createStyle(test, rules, doc) {
> +  if (!doc) {
> +    doc = document;
> +  }
> +  var extraStyle = doc.createElement('style');
> +  doc.head.appendChild(extraStyle);
> +  if (rules) {
> +    var sheet = extraStyle.sheet;
> +    for (var selector in rules) {
> +      sheet.insertRule(selector + '{' + rules[selector] + '}',
> +                       sheet.cssRules.length);
> +    }
> +  }
> +  test.add_cleanup(function() {
> +    extraStyle.remove();
> +  });
> +}

I think based on my comments above we won't need this.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 79 • 7 years ago
mozreview-review

Comment on attachment 8841583 [details]
Bug 1248340 - Part 6: Add tests in easing-test.

https://reviewboard.mozilla.org/r/115712/#review118096

::: testing/web-platform/meta/web-animations/__dir__.ini:1
(Diff revision 2)
>  prefs: [dom.animations-api.core.enabled:true]

I'm just curious, MozReview says there are whitespace changes here but doesn't show what they are. Are they intentional?

::: testing/web-platform/tests/web-animations/resources/easing-tests.js:75
(Diff revision 2)
> +  },
> +  {
> +    desc: 'frames function',
> +    easing: 'frames(5)',
> +    easingFunction: framesTiming(5)

Let's move this up after the step timing tests.

::: testing/web-platform/tests/web-animations/resources/easing-tests.js:92
(Diff revision 2)
>    'cubic-bezier(0, 0, 1.1, 1)',
>    'cubic-bezier(-0.1, 0, 1, 1)',
>    'cubic-bezier(0, 0, -0.1, 1)',
>    'steps(-1, start)',
> -  'steps(0.1, start)'
> +  'steps(0.1, start)',
> +  'frames(1)'

We should add a few more of the invalid easing Daniel mentioned in comment 75.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 80 • 7 years ago
mozreview-review

Comment on attachment 8841584 [details]
Bug 1248340 - Part 8: Add tests for transformed-progress and input progress outside [0, 1].

https://reviewboard.mozilla.org/r/115714/#review118106

I'd like to check this again with the updated comment mentioned below.

Also, you might want to move the tests for values outside [0, 1] (i.e. the effect-value-transformed-distance ones) to the css-timing-1 tests in part 5 and leave a comment here that that's where they've moved to.

Long term I'd like to move the other similar tests in this file to css-timing-1.

::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/effect-value-transformed-distance.html:448
(Diff revision 2)
> +  // The bezier function produces values greater than 2 (but always less than 3)
> +  // in the range (~0.245, ~0.882), and the values greater than 1.5 from 0.114.
> +  anim.currentTime = 0;
> +  assert_equals(getComputedStyle(target).left, '0px');
> +  anim.currentTime = 114;
> +  assert_equals(getComputedStyle(target).left, '300px');
> +  anim.currentTime = 500;
> +  assert_equals(getComputedStyle(target).left, '400px');
> +  anim.currentTime = 900;
> +  assert_equals(getComputedStyle(target).left, '300px');

The comment is hard to follow and doesn't explain why currentTime of 900 gives 300px.

What is the range after ~0.882 where the bezier produces a value greater than 1.5 and less than 2?

::: testing/web-platform/tests/web-animations/timing-model/time-transformations/transformed-progress.html:256
(Diff revision 2)
> +  },
> +  {
> +    description: 'Test bounds point of frames easing',
> +    effect:     {
> +                  delay: 1000,
> +                  duration: 1000,
> +                  fill: 'both',
> +                  easing: 'frames(2)'
> +                },
> +    conditions: [
> +                  { currentTime: 0,    progress: 0 },
> +                  { currentTime: 1000, progress: 0 },
> +                  { currentTime: 1499, progress: 0 },
> +                  { currentTime: 1500, progress: 1 },
> +                  { currentTime: 2000, progress: 1 }
> +                ]
> +  },

I think this is the only test we need?

The iterationStart / direction tests we have for step timing functions are to test the "before flag", but since we don't have a before flag I don't think we need those tests here.

What might be worth adding is just one test that we *don't* do the same thing for frames(). So your fourth test, "Test bounds point of frames easing with iterationStart and delay" would probably be good.

So, just the 1st and 4th ones?

	Boris Chiou [:boris] Assignee
	Comment 81 • 7 years ago
mozreview-review-reply

Comment on attachment 8841582 [details]
Bug 1248340 - Part 5: Add tests to css-timing-1.

https://reviewboard.mozilla.org/r/115710/#review118058

> This feels a bit hap-hazard to me.
> 
> I think we want to test:
> 
> 'frames(2)'
> => Test zero time
> => Test transition-point 0.499 and 0.5
>    (this will be easier if you set the duration to 10s)
> => Test 100% time
>    (need to set fill mode to forwards)
> 
> That might be enough?
> 
> If you want to test that the number of steps is being correctly calculated, then add another test for 'frames(11)' and sample either side of the first step? i.e. just your first test from above?
> 
> I would make the above two or more separate tests
> 
> e.g.
> 
> * 'For an input progress of 0.0, the output of a frames timing function is 0.0'
> * 'At a frame boundary, the output of a frames timing function is the next frame'
> * 'For an input progress of 1.0, the output of a frames timing function is 1.0'
> * 'The number of frames is correctly reflected in the frames timing function output'

OK. Thanks. I will make this more separate tests.

> Just curious, why do we need this?

This file is copied from web-animations/testcommon.js, and I didn't remove this comment because I'm not sure if we need this. I will remove it in the updated patch, Thanks.

	Boris Chiou [:boris] Assignee
	Comment 82 • 7 years ago
mozreview-review-reply

Comment on attachment 8841582 [details]
Bug 1248340 - Part 5: Add tests to css-timing-1.

https://reviewboard.mozilla.org/r/115710/#review118058

> OK. Thanks. I will make this more separate tests.

Actually, I just notice one potential bug? We use getComputedTiming().progress for testing the transformed-progress, but it only works for script-generated animations, not for CSS Animations.

e.g.
1) CSS Animations

@keyframes anim {
  from { left: 0px; }
  to   { left: 100px; }
}
div.style.animation = 'anim 10s frames(2) forwards';
// I need to use a valid keyframes, so getAnimations() returns a valid animation.

var anim = div.getAnimations()[0];
anim.currentTime = 4999;
assert_equals(anim.effect.getComputedTiming().progress, 0.4999); // instead of 0.0
anim.currentTime = 5000;
assert_equals(anim.effect.getComputedTiming().progress, 0.5);    // instead of 1.0

2) Script-generated

var anim = div.animate([{ left:'0px' }, { left:'100px' }],
                        { duration: 10000, easing: 'frames(2)' } );
anim.currentTime = 4999;
assert_equals(anim.effect.getComputedTiming().progress, 0);
anim.currentTime = 5000;
assert_equals(anim.effect.getComputedTiming().progress, 1);

Therefore, I still need to use getComputedStyle(div).left to check, instead of getComputedTiming().progress, for CSS Animations/Transitions.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 83 • 7 years ago
mozreview-review-reply

Comment on attachment 8841582 [details]
Bug 1248340 - Part 5: Add tests to css-timing-1.

https://reviewboard.mozilla.org/r/115710/#review118058

> Actually, I just notice one potential bug? We use getComputedTiming().progress for testing the transformed-progress, but it only works for script-generated animations, not for CSS Animations.
> 
> e.g.
> 1) CSS Animations
> 
> @keyframes anim {
>   from { left: 0px; }
>   to   { left: 100px; }
> }
> div.style.animation = 'anim 10s frames(2) forwards';
> // I need to use a valid keyframes, so getAnimations() returns a valid animation.
> 
> var anim = div.getAnimations()[0];
> anim.currentTime = 4999;
> assert_equals(anim.effect.getComputedTiming().progress, 0.4999); // instead of 0.0
> anim.currentTime = 5000;
> assert_equals(anim.effect.getComputedTiming().progress, 0.5);    // instead of 1.0
> 
> 2) Script-generated
> 
> var anim = div.animate([{ left:'0px' }, { left:'100px' }],
>                         { duration: 10000, easing: 'frames(2)' } );
> anim.currentTime = 4999;
> assert_equals(anim.effect.getComputedTiming().progress, 0);
> anim.currentTime = 5000;
> assert_equals(anim.effect.getComputedTiming().progress, 1);
> 
> Therefore, I still need to use getComputedStyle(div).left to check, instead of getComputedTiming().progress, for CSS Animations/Transitions.

I'm not quite sure what you're asking, but for the transformed-progress tests, i.e. the ones checking inputs outside [0, 1], I would just leave them as script-generated animations. There's no need to use CSS animations/transitions for those.

	Boris Chiou [:boris] Assignee
	Comment 84 • 7 years ago
mozreview-review-reply

Comment on attachment 8841583 [details]
Bug 1248340 - Part 6: Add tests in easing-test.

https://reviewboard.mozilla.org/r/115712/#review118096

> I'm just curious, MozReview says there are whitespace changes here but doesn't show what they are. Are they intentional?

I touched this file by adding the frames preference, and then remove it. Maybe my editor did something for me. I also has no idea about why MozReview didn't shod anything. I will remove any change on this file.

	Boris Chiou [:boris] Assignee
	Comment 85 • 7 years ago
mozreview-review-reply

Comment on attachment 8841584 [details]
Bug 1248340 - Part 8: Add tests for transformed-progress and input progress outside [0, 1].

https://reviewboard.mozilla.org/r/115714/#review118106

> Also, you might want to move the tests for values outside [0, 1] (i.e. the effect-value-transformed-distance ones) to the css-timing-1 tests in part 5 and leave a comment here that that's where they've moved to.

OK, I will move them into css-timing-1. i.e. Move line 83-414 [1] into css-timing-1/input-progress-outside-range.html?

[1] http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/testing/web-platform/tests/web-animations/animation-model/keyframe-effects/effect-value-transformed-distance.html#83-414

> The comment is hard to follow and doesn't explain why currentTime of 900 gives 300px.
> 
> What is the range after ~0.882 where the bezier produces a value greater than 1.5 and less than 2?

I will add more comment for this.

> I think this is the only test we need?
> 
> The iterationStart / direction tests we have for step timing functions are to test the "before flag", but since we don't have a before flag I don't think we need those tests here.
> 
> What might be worth adding is just one test that we *don't* do the same thing for frames(). So your fourth test, "Test bounds point of frames easing with iterationStart and delay" would probably be good.
> 
> So, just the 1st and 4th ones?

Sure. It makes sense.

	Boris Chiou [:boris] Assignee
	Comment 94 • 7 years ago
mozreview-review

Comment on attachment 8842809 [details]
Bug 1248340 - Part 7: Move the tests of input range outside [0, 1] into css-timing-1.

https://reviewboard.mozilla.org/r/116580/#review118192

Hi Brian. I only moved tests, so there is no other changes. But I think the file name may not be good enough, so please give me some advices for it. Thanks. Besides, the CSS Metadata suggest us add a "assert" meta, but its description shouldn't be copyied from the title, so I'm not sure whether this one is suitable or not.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 95 • 7 years ago
mozreview-review

Comment on attachment 8841582 [details]
Bug 1248340 - Part 5: Add tests to css-timing-1.

https://reviewboard.mozilla.org/r/115710/#review118586

Sorry, I need to take another look at this one with the following tweaks. Please r? or needingo? gsnedders when you've made the following changes to see if he agrees with adding this to web-platform-tests.

::: testing/web-platform/tests/css-timing-1/OWNERS:1
(Diff revision 3)
> +@birtles
> +@ChumpChief
> +@grorg
> +@shans

I've just discovered this now:

  "OWNERS files are used only to indicate who should be notified of pull requests. If you are interested in receiving notifications of proposed changes to tests in a given directory, feel free to add yourself to the OWNERS file."[1]

So it seems like we probably shouldn't add other people without asking them. Just add @birtles for now (but feel free to add yourself if you like too!).

[1] https://github.com/w3c/web-platform-tests/#test-review

::: testing/web-platform/tests/css-timing-1/frames-timing-functions.html:5
(Diff revision 3)
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<meta name="assert"
> +content="This test checks the output of frame timing functions with different frame numbers" />
> +<title>Frames timing function tests</title>

Can we split this into two files:

* frames-timing-functions-syntax.html (first 3/4 tests)
* frames-timing-functions-output.html (the rest)

::: testing/web-platform/tests/css-timing-1/frames-timing-functions.html:7
(Diff revision 3)
> +<meta charset=utf-8>
> +<meta name="assert"
> +content="This test checks the output of frame timing functions with different frame numbers" />
> +<title>Frames timing function tests</title>
> +<link rel="help"
> +href="https://www.w3.org/TR/css-timing-1/#frames-timing-functions">

Please point to the ED: https://drafts.csswg.org/css-timing/#frames-timing-functions

(In general we tend to ignore TR specs as they're often outdated before they're even published)

::: testing/web-platform/tests/css-timing-1/frames-timing-functions.html:28
(Diff revision 3)
> +  const div = createDiv(t);
> +  div.style.animation = 'abc 1s ease-in';
> +  div.style.animationTimingFunction = 'frames(1)';
> +  assert_equals(getComputedStyle(div).animationTimingFunction, 'ease-in');
> +}, 'The number of frames must be a positive integer greater than 1, or we ' +
> +   'fallback to the previously-set easing for CSS Animation Timing Function');

s/CSS Animation Timing Function/CSS Animations/

::: testing/web-platform/tests/css-timing-1/frames-timing-functions.html:30
(Diff revision 3)
> +test(function(t) {
> +  var div = createDiv(t);
> +  div.style.left = '0px';
> +  getComputedStyle(div).transitionProperty;
> +
> +  div.style.transition = 'left 1s ease-in';
> +  div.style.left = '100px';
> +  div.style.transitionTimingFunction = 'frames(1)';
> +
> +  assert_equals(getComputedStyle(div).transitionTimingFunction, 'ease-in');
> +}, 'The number of frames must be a positive integer greater than 1, or we ' +
> +   'fallback to the previously-set easing for CSS Transition Timing Function');

(Personally, I don't think we need to test that both animations and transitions reject this in the same way. And if we do, it begs the question why we don't also test Web Animations. I'd just drop this test but it's up to you.)

::: testing/web-platform/tests/css-timing-1/frames-timing-functions.html:31
(Diff revision 3)
> +  assert_equals(getComputedStyle(div).animationTimingFunction, 'ease-in');
> +}, 'The number of frames must be a positive integer greater than 1, or we ' +
> +   'fallback to the previously-set easing for CSS Animation Timing Function');
> +
> +test(function(t) {
> +  var div = createDiv(t);

We use 'var' in some places and 'const' in others. We should either use ES6 style (const/let) or not. My feeling is that this part of ES6 is sufficiently widely supported now that we should just write new tests using const/let.

::: testing/web-platform/tests/css-timing-1/frames-timing-functions.html:65
(Diff revision 3)
> +
> +  var anim = div.getAnimations()[0];
> +  anim.currentTime = 0;
> +  assert_equals(getComputedStyle(div).left, '0px');

I'd avoid using the Web Animations API for CSS Animations tests for now. Just use a negative animationDelay to seek the animation.

::: testing/web-platform/tests/css-timing-1/frames-timing-functions.html:115
(Diff revision 3)
> +  getComputedStyle(div).transitionProperty;
> +
> +  div.style.transition = 'left 11s frames(11)';
> +  div.style.left = '100px';
> +
> +  // We have 11 frames in 11s, so the first step happens between 0.999 and 1.0.

s/step happens between 0.999 and 1.0/step happens at 1.0/

::: testing/web-platform/tests/css-timing-1/frames-timing-functions.html:116
(Diff revision 3)
> +
> +  div.style.transition = 'left 11s frames(11)';
> +  div.style.left = '100px';
> +
> +  // We have 11 frames in 11s, so the first step happens between 0.999 and 1.0.
> +  var anim = div.getAnimations()[0];

Likewise here, can we seek using transitionDelay?

::: testing/web-platform/tests/css-timing-1/testcommon.js:9
(Diff revision 3)
> +// removes the created element during test cleanup.
> +function createDiv(test, doc) {
> +  return createElement(test, 'div', doc);
> +}
> +
> +// etes an element with the given |tagName|, appends it to the document body

s/etes/Creates/ ?

::: testing/web-platform/tests/css-timing-1/testcommon.js:10
(Diff revision 3)
> +// and removes the created element during test cleanup if |tagName| is null or
> +// undefined, creates a <div> element.

s/cleanup if/cleanup.\nIf/

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 96 • 7 years ago
mozreview-review

Comment on attachment 8842809 [details]
Bug 1248340 - Part 7: Move the tests of input range outside [0, 1] into css-timing-1.

https://reviewboard.mozilla.org/r/116580/#review118588

I didn't actually expect you to do this as part of this bug but thanks for doing it anyway!

Regarding the file naming, however, as with Web Animations, let's try to match the headings in the spec. That would suggest we want to call this

* cubic-bezier-timing-functions-output.html
* step-timing-functions-output.html

I'd like to check this once more after splitting it into those files just to make sure the comments make sense.

::: testing/web-platform/tests/css-timing-1/input-progress-outside-range.html:6
(Diff revision 1)
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<meta name="assert"
> +content="This test checks the output of different timing functions if inputs are outside [0, 1]" />
> +<title>Tests for handling inputs outside the range [0, 1]</title>
> +<link rel="help" href="https://www.w3.org/TR/css-timing-1/#timing-functions">

Again, ED link please

::: testing/web-platform/tests/css-timing-1/input-progress-outside-range.html:7
(Diff revision 1)
> +<link rel="help"
> +href="https://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-animation-effect">

(Not sure if we need this -- we're testing the timing spec, not the Web Animations spec. It's just the means we use to test the timing spec.)

::: testing/web-platform/tests/css-timing-1/input-progress-outside-range.html:25
(Diff revision 1)
> +// on a keyframe.
> +
> +function assert_style_left_at(animation, time, easingFunction) {
> +  animation.currentTime = time;
> +  var portion = time / animation.effect.timing.duration;
> +  assert_approx_equals(pxToNum(getComputedStyle(animation.effect.target).left),

(Nit: I don't know why we don't just use parseFloat here, but I guess we should make that change as a separate patch.)

::: testing/web-platform/tests/css-timing-1/input-progress-outside-range.html:31
(Diff revision 1)
> +
> +test(function(t) {
> +  var target = createDiv(t);
> +  target.style.position = 'absolute';
> +  var anim = target.animate([ { left: '0px', easing: 'step-start' },
> +                              { left: '100px' } ],
> +                            { duration: 1000,
> +                              fill: 'forwards',
> +                              easing: 'cubic-bezier(0, 1.5, 1, 1.5)' });
> +
> +  // The bezier function produces values greater than 1 (but always less than 2)
> +  // in (0.23368794, 1)
> +  anim.currentTime = 0;
> +  assert_equals(getComputedStyle(target).left, '100px');
> +  anim.currentTime = 230;
> +  assert_equals(getComputedStyle(target).left, '100px');
> +  anim.currentTime = 250;
> +  assert_equals(getComputedStyle(target).left, '200px');
> +  anim.currentTime = 1000;
> +  assert_equals(getComputedStyle(target).left, '100px');
> +}, 'step-start easing with input progress greater than 1');

This is testing the out-of-range behavior of step-start so we should put it in

* step-timing-functions-output.html

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 97 • 7 years ago
mozreview-review

Comment on attachment 8841584 [details]
Bug 1248340 - Part 8: Add tests for transformed-progress and input progress outside [0, 1].

https://reviewboard.mozilla.org/r/115714/#review118596

Likewise here, these tests for the output of the frames timing function should go in frames-timing-functions-output.html.

r=me with that and the following comments addressed.

::: testing/web-platform/tests/css-timing-1/input-progress-outside-range.html:165
(Diff revision 3)
> +  // The bezier function produces values greater than 1 (but always less than 2)
> +  // in (0.23368794, 1), and the values become [0.5, 1] in (~0.0442, 0.23368).

"The bezier function produces values between 0.5 and 1 in (~0.0442, 0.23368), and values between 1 and 2 in (0.23368794, 1)."

::: testing/web-platform/tests/css-timing-1/input-progress-outside-range.html:215
(Diff revision 3)
> +                              { left: '100px' } ],
> +                            { duration: 1000,
> +                              fill: 'forwards',
> +                              easing: 'cubic-bezier(0, -0.5, 1, -0.5)' });
> +
> +  // The bezier function produces negative values (but always greater than -0.4)

Was this supposed to be "greater than -0.5" ?

	Boris Chiou [:boris] Assignee
	Comment 98 • 7 years ago
mozreview-review-reply

Comment on attachment 8841582 [details]
Bug 1248340 - Part 5: Add tests to css-timing-1.

https://reviewboard.mozilla.org/r/115710/#review118586

> (Personally, I don't think we need to test that both animations and transitions reject this in the same way. And if we do, it begs the question why we don't also test Web Animations. I'd just drop this test but it's up to you.)

OK. I will drop the tests for transitions, except for the output one.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 104 • 7 years ago
mozreview-review

Comment on attachment 8841582 [details]
Bug 1248340 - Part 5: Add tests to css-timing-1.

https://reviewboard.mozilla.org/r/115710/#review118606

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 105 • 7 years ago
mozreview-review

Comment on attachment 8842809 [details]
Bug 1248340 - Part 7: Move the tests of input range outside [0, 1] into css-timing-1.

https://reviewboard.mozilla.org/r/116580/#review118608

::: testing/web-platform/meta/MANIFEST.json:83317
(Diff revision 2)
>      [
>       "/css-timing-1/frames-timing-functions-syntax.html",
>       {}
>      ]
>     ],
> +   "css-timing-1/steps-timing-functions-output.html": [

This should be 'step-timing-functions-output.html' to match the spec heading.

::: testing/web-platform/tests/css-timing-1/cubic-bezier-timing-functions-output.html:4
(Diff revision 2)
> +content="This test checks the output of Cubic-Bezier functions if inputs are outside [0, 1]" />
> +<title>Tests for handling inputs outside the range [0, 1] for Cubic Bezier</title>

I would just make this test file cover all output from the function (not just the range outside [0, 1]). In otherwords, I would just make the summary:

"Tests for the output of Cubic Bézier timing functions"

::: testing/web-platform/tests/css-timing-1/steps-timing-functions-output.html:4
(Diff revision 2)
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<meta name="assert"
> +content="This test checks the output of Steps functions if inputs are outside [0, 1]" />

s/Step functions/step timing functions/

(And once again, I would drop the mention of being outside the range [0, 1]. In the future we should add tests for the regular range to this file.)

	Boris Chiou [:boris] Assignee
	Comment 106 • 7 years ago
mozreview-review-reply

Comment on attachment 8842809 [details]
Bug 1248340 - Part 7: Move the tests of input range outside [0, 1] into css-timing-1.

https://reviewboard.mozilla.org/r/116580/#review118608

> s/Step functions/step timing functions/
> 
> (And once again, I would drop the mention of being outside the range [0, 1]. In the future we should add tests for the regular range to this file.)

Thanks for review, Brian. I will drop them.

	Boris Chiou [:boris] Assignee
	Comment 109 • 7 years ago

Oops, I should update servo/components/style/gecko_bindings/sugar/ns_timing_function.rs to pass the compilation on stylo.

	Boris Chiou [:boris] Assignee
	Comment 128 • 7 years ago

(In reply to Brian Birtles (:birtles) from comment #95)
> Comment on attachment 8841582 [details]
> Bug 1248340 - Part 5: Add tests to css-timing-1.
> 
> https://reviewboard.mozilla.org/r/115710/#review118586
> 
> Sorry, I need to take another look at this one with the following tweaks.
> Please r? or needingo? gsnedders when you've made the following changes to
> see if he agrees with adding this to web-platform-tests.

Hi Brian, Looks like gsnedders agrees with this to web-platform-tests. I want to make sure where should I put css-timing-1/. I just merge these test files in css-timing-1 into m-c, and someone will sync them with web-platform-tests?

Thank you.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 129 • 7 years ago

Yes, that's right.

	Boris Chiou [:boris] Assignee
	Comment 130 • 7 years ago

Hi heycam, the servo changes is related to the Part 1, which replaces mSteps with mStepsOrFrames in nsTimingFucntion. Thanks.

	Cameron McCormack (:heycam)
	Comment 131 • 7 years ago
mozreview-review

Comment on attachment 8843237 [details]
Bug 1248340 - [Servo] Update the step data member in ns_timing_function.rs.

https://reviewboard.mozilla.org/r/117022/#review119128

	Boris Chiou [:boris] Assignee
	Comment 132 • 7 years ago

(In reply to Brian Birtles (:birtles) from comment #129)
> Yes, that's right.

Although we don't have any pref for this. should we need to send an "Intent to Ship" for this bug?

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 133 • 7 years ago

Yes, we should.

	Boris Chiou [:boris] Assignee
	Comment 134 • 7 years ago

(In reply to Brian Birtles (:birtles) from comment #133)
> Yes, we should.

Thanks. I sent a mail for this and plan to land these code tomorrow.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 135 • 7 years ago

(In reply to Boris Chiou [:boris] from comment #40)
> The spec says: "The output progress value is calculated from the input
> progress value and before flag as follows", but the following steps don't
> mention *before flag*. Is this a typo? or how do we use *before flag* for
> frames timing function?

I've fixed this in the spec now.

	Boris Chiou [:boris] Assignee
	Comment 136 • 7 years ago

(In reply to Brian Birtles (:birtles) from comment #135)
> (In reply to Boris Chiou [:boris] from comment #40)
> > The spec says: "The output progress value is calculated from the input
> > progress value and before flag as follows", but the following steps don't
> > mention *before flag*. Is this a typo? or how do we use *before flag* for
> > frames timing function?
> 
> I've fixed this in the spec now.

Thanks, Brian. Looks like it's time to land these code.