Implement frames() timing function

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM: Animation
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: birtles, Assigned: boris)

Tracking

({dev-doc-needed})

Trunk
mozilla55
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox55 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(11 attachments, 20 obsolete attachments)

43 bytes, text/plain
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
41 bytes, text/x-github-pull-request
Details | Review | Splinter Review
Comment hidden (obsolete)
(Reporter)

Updated

2 years ago
Blocks: 1245000
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Assignee: nobody → boris.chiou
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8725614 - Attachment is obsolete: true
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8725605 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725606 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725607 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725608 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725609 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725610 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725611 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725612 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725613 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8727312 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8727313 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Attachment #8725605 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725606 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725607 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725608 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725609 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725610 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725611 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725612 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8725613 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8727312 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8727313 - Flags: review?(bbirtles)
(Reporter)

Comment 27

2 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
(Reporter)

Comment 28

2 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.
(Reporter)

Updated

2 years ago
Summary: Implement step-middle → Implement frames() timing function
(Assignee)

Comment 29

2 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.
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1261275
(Reporter)

Comment 31

2 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.
No longer blocks: 1245000
(Assignee)

Updated

2 years ago
Status: ASSIGNED → NEW

Updated

2 years ago
Status: NEW → ASSIGNED
Whiteboard: tlt-wip
(Reporter)

Comment 32

a year 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).
(Assignee)

Comment 33

a year 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.
(Reporter)

Comment 34

a year 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.
Depends on: 1332206
(Reporter)

Comment 35

a year 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.
(Assignee)

Comment 36

a year 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.
(Reporter)

Comment 37

a year ago
FPWD has now been published: https://www.w3.org/TR/css-timing-1/
(Assignee)

Comment 38

a year 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.
Keywords: dev-doc-needed
(Assignee)

Updated

a year ago
Attachment #8725605 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8725606 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8725607 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8725608 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8725609 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8725610 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8725611 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8725612 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8725613 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8727312 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8727313 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8727314 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8727315 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8841301 - Attachment description: Servo issue. → Servo issue: https://github.com/servo/servo/issues/15740
(Assignee)

Comment 40

a year 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.
(Reporter)

Comment 41

a year 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 53

a year 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?
Attachment #8841574 - Flags: review?(bbirtles)
(Reporter)

Updated

a year ago
Attachment #8841574 - Flags: review?(dholbert)
(Reporter)

Comment 54

a year 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.
Attachment #8841575 - Flags: review?(bbirtles)
(Reporter)

Comment 55

a year 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
Attachment #8841576 - Flags: review?(bbirtles) → review+

Comment 56

a year 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.

Comment 57

a year 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".

Comment 58

a year 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.
Attachment #8841574 - Flags: review?(dholbert) → review-

Comment 59

a year 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?
(Reporter)

Comment 60

a year 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.
[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).
(Assignee)

Comment 62

a year 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).
(Assignee)

Comment 63

a year 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.
(Assignee)

Updated

a year ago
Attachment #8841578 - Flags: review?(bbirtles)
Attachment #8841579 - Flags: review?(bbirtles)
(Assignee)

Comment 64

a year 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.
(Assignee)

Updated

a year ago
Attachment #8841580 - Flags: review?(bbirtles)
Attachment #8841581 - Flags: review?(bbirtles)
Attachment #8841583 - Flags: review?(bbirtles)
Attachment #8841584 - Flags: review?(bbirtles)
(Assignee)

Updated

a year ago
Attachment #8841582 - Flags: review?(bbirtles)
(Assignee)

Comment 65

a year ago
clear the reviews because I should apply the changes mentioned in Part 3 into all other patches.
(Reporter)

Comment 66

a year 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.)
Attachment #8841577 - Flags: review?(bbirtles)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8841575 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8841577 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8841578 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8841579 - Attachment is obsolete: true
(Assignee)

Comment 74

a year 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.

Comment 75

a year 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)
Attachment #8841574 - Flags: review?(dholbert) → review+
(Reporter)

Comment 76

a year 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).
Attachment #8841580 - Flags: review?(bbirtles) → review+
(Reporter)

Comment 77

a year ago
mozreview-review
Comment on attachment 8841581 [details]
Bug 1248340 - Part 3: Implement Frames for OMTA.

https://reviewboard.mozilla.org/r/115562/#review118056
Attachment #8841581 - Flags: review?(bbirtles) → review+
(Reporter)

Comment 78

a year 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.
Attachment #8841582 - Flags: review?(bbirtles)
(Reporter)

Comment 79

a year 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.
Attachment #8841583 - Flags: review?(bbirtles) → review+
(Reporter)

Comment 80

a year 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?
Attachment #8841584 - Flags: review?(bbirtles)
(Assignee)

Comment 81

a year 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.
(Assignee)

Comment 82

a year 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.
(Reporter)

Comment 83

a year 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.
(Assignee)

Comment 84

a year 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.
(Assignee)

Comment 85

a year 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 94

a year 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.
(Reporter)

Comment 95

a year 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/
Attachment #8841582 - Flags: review?(bbirtles)
(Reporter)

Comment 96

a year 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
Attachment #8842809 - Flags: review?(bbirtles)
(Reporter)

Comment 97

a year 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" ?
Attachment #8841584 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 98

a year 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (obsolete)
(Reporter)

Comment 104

a year 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
Attachment #8841582 - Flags: review?(bbirtles) → review+
(Reporter)

Comment 105

a year 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.)
Attachment #8842809 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 106

a year 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 109

a year ago
Oops, I should update servo/components/style/gecko_bindings/sugar/ns_timing_function.rs to pass the compilation on stylo.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Whiteboard: tlt-wip
(Assignee)

Comment 128

a year 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.
(Reporter)

Comment 129

a year ago
Yes, that's right.
(Assignee)

Comment 130

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

Comment 131

a year 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
Attachment #8843237 - Flags: review?(cam) → review+
(Assignee)

Comment 132

a year 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?
(Reporter)

Comment 133

a year ago
Yes, we should.
(Assignee)

Comment 134

a year 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.
(Reporter)

Comment 135

a year 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.
(Assignee)

Comment 136

a year 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8843237 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8843248 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 146

a year ago
mozreview-review
Comment on attachment 8844758 [details]
Bug 1248340 - Part 9: Update stylo-failures.md.

https://reviewboard.mozilla.org/r/118064/#review119934
Attachment #8844758 - Flags: review?(xidorn+moz) → review+
(Assignee)

Comment 147

a year ago
Created attachment 8844769 [details] [review]
Servo PR, #15863

The binding part.

Comment 148

a year ago
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ceaf7e9060e
Part 1: Add the type of Frames in nsTimingFunction. r=birtles
https://hg.mozilla.org/integration/autoland/rev/74f2f1a62a56
Part 2: Implement Frames in ComputedTimingFunction. r=birtles
https://hg.mozilla.org/integration/autoland/rev/937a16a9fc42
Part 3: Implement Frames for OMTA. r=birtles
https://hg.mozilla.org/integration/autoland/rev/9f17f5e31f3d
Part 4: Implement Frames in the style system. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/e0865a760319
Part 5: Add tests to css-timing-1. r=birtles
https://hg.mozilla.org/integration/autoland/rev/b6685a2847b6
Part 6: Add tests in easing-test. r=birtles
https://hg.mozilla.org/integration/autoland/rev/028355800f0b
Part 7: Move the tests of input range outside [0, 1] into css-timing-1. r=birtles
https://hg.mozilla.org/integration/autoland/rev/a639a6afd1df
Part 8: Add tests for transformed-progress and input progress outside [0, 1]. r=birtles
https://hg.mozilla.org/integration/autoland/rev/31179f02c5c3
Part 9: Update stylo-failures.md. r=xidorn
(Assignee)

Updated

a year ago
Blocks: 1359719
(Assignee)

Updated

a year ago
Blocks: 1364004
(Reporter)

Comment 152

a year ago
You're not going to like this, but after we implemented this in Gecko, and Servo, and Google implemented it in Blink, the TAG decided to question the naming and it still hasn't been decided if we should rename or not.[1]

[1] https://github.com/w3c/csswg-drafts/issues/1301
(In reply to Brian Birtles (:birtles) from comment #152)
> You're not going to like this, but after we implemented this in Gecko, and
> Servo, and Google implemented it in Blink, the TAG decided to question the
> naming and it still hasn't been decided if we should rename or not.[1]
> 
> [1] https://github.com/w3c/csswg-drafts/issues/1301

Well, if it is just a name change, then it won't be that bad to update. I'll move this back to dev-doc-needed so I can monitor it for further changes.
Keywords: dev-doc-complete → dev-doc-needed
(Reporter)

Updated

11 months ago
Blocks: 1379582
You need to log in before you can comment on or make changes to this bug.