Closed Bug 1398038 Opened 7 years ago Closed 7 years ago

Implement extensions to property-indexed keyframes

Categories

(Core :: DOM: Animation, enhancement)

Product:
Core
Component:
DOM: Animation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

(
URL
)

Details

Attachments

(13 files, 1 obsolete file)

Bug 1398038 - Update web-platform-tests MANIFEST.json;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1398038 - Split keyframe-tests.js out of keyframe-utils.js;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1398038 - Use slightly more modern JS in keyframe-*.js;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1398038 - Use single quotes in keyframe-*.js;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1398038 - Merge property-indexed and sequence keyframe lists;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1398038 - Fix spacing of keyframes tests;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1398038 - Simplify keyframe test data;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1398038 - Drop redundant exception data from gInvalidKeyframesTests and gInvalidKeyframeEffectOptionTests;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1398038 - Organize keyframe tests into sections;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1398038 - Add tests for extended offset syntax;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1398038 - Add tests for extended easing syntax;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1398038 - Add tests for extended composite syntax;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1398038 - Implement extended property-indexed keyframe syntax;
59 bytes, text/x-review-board-request
bzbarsky
: review+
hiro
: review+
Details
Attached patch WIP patch (obsolete) — — Splinter Review 
These changes have yet to be merged into the spec. Spec issue:

  https://github.com/w3c/web-animations/issues/148

Proposed spec change:

  https://github.com/w3c/web-animations/commit/1f6e18a18605f37ea5cbe8bd64212dd90ec6d132

For now I'm just filing this as a place to park a WIP patch I wrote.

This patch includes changes to the type of Keyframe.offset which causes a *lot* of tests to fail (tests that are looking at the result of getKeyframes() which we haven't shipped yet).
One comment on the patch is that it currently suppresses errors from parsing easing. The spec is pretty unclear about this. It says you can't set an invalid easing via the setter for the `easing` member but then other algorithms seem to allow it. Probably the spec means to disallow it everywhere.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Attachment #8905800 - Attachment is obsolete: true
Comment on attachment 8919614 [details]
Bug 1398038 - Update web-platform-tests MANIFEST.json;

https://reviewboard.mozilla.org/r/190500/#review195742

::: commit-message-f78d5:5
(Diff revision 1)
> +Bug 1398038 - Update web-platform-tests MANIFEST.json; r?hiro
> +
> +This is just the output of running:
> +
> +  ./mach web-platform-tests --manifest-update yer

What does 'yer' mean?
Attachment #8919614 - Flags: review?(hikezoe) → review+
Comment on attachment 8919615 [details]
Bug 1398038 - Split keyframe-tests.js out of keyframe-utils.js;

https://reviewboard.mozilla.org/r/190502/#review195744
Attachment #8919615 - Flags: review?(hikezoe) → review+
Comment on attachment 8919616 [details]
Bug 1398038 - Use slightly more modern JS in keyframe-*.js;

https://reviewboard.mozilla.org/r/190504/#review195746
Attachment #8919616 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> Comment on attachment 8919614 [details]
> Bug 1398038 - Update web-platform-tests MANIFEST.json;
> 
> https://reviewboard.mozilla.org/r/190500/#review195742
> 
> ::: commit-message-f78d5:5
> (Diff revision 1)
> > +Bug 1398038 - Update web-platform-tests MANIFEST.json; r?hiro
> > +
> > +This is just the output of running:
> > +
> > +  ./mach web-platform-tests --manifest-update yer
> 
> What does 'yer' mean?

Nothing. It's just a deliberately invalid path. This might have changed, but at least in the past if you executed `./mach web-platform-tests --manifest-update` without a path it would run the tests.
Comment on attachment 8919617 [details]
Bug 1398038 - Use single quotes in keyframe-*.js;

https://reviewboard.mozilla.org/r/190506/#review195748
Attachment #8919617 - Flags: review?(hikezoe) → review+
Comment on attachment 8919618 [details]
Bug 1398038 - Merge property-indexed and sequence keyframe lists;

https://reviewboard.mozilla.org/r/190508/#review195750
Attachment #8919618 - Flags: review?(hikezoe) → review+
(In reply to Brian Birtles (:birtles) from comment #19)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> > Comment on attachment 8919614 [details]
> > Bug 1398038 - Update web-platform-tests MANIFEST.json;
> > 
> > https://reviewboard.mozilla.org/r/190500/#review195742
> > 
> > ::: commit-message-f78d5:5
> > (Diff revision 1)
> > > +Bug 1398038 - Update web-platform-tests MANIFEST.json; r?hiro
> > > +
> > > +This is just the output of running:
> > > +
> > > +  ./mach web-platform-tests --manifest-update yer
> > 
> > What does 'yer' mean?
> 
> Nothing. It's just a deliberately invalid path. This might have changed, but
> at least in the past if you executed `./mach web-platform-tests
> --manifest-update` without a path it would run the tests.

./mach wpt-manifest-update would work?
Comment on attachment 8919619 [details]
Bug 1398038 - Fix spacing of keyframes tests;

https://reviewboard.mozilla.org/r/190510/#review195766

::: testing/web-platform/tests/web-animations/resources/keyframe-tests.js:488
(Diff revision 1)
> +    desc:     'keyframes with an out-of-bounded positive offset',
>      input:    [ { opacity: 0 },

Now I realized we put a space between [ and {.  We should do the same manner in the previous entries, gKeyframesTests.
Attachment #8919619 - Flags: review?(hikezoe) → review+
Comment on attachment 8919620 [details]
Bug 1398038 - Simplify keyframe test data;

https://reviewboard.mozilla.org/r/190512/#review195768

::: testing/web-platform/tests/web-animations/resources/keyframe-tests.js:310
(Diff revision 1)
> -             { offset: 0.0, computedOffset: 0.0, easing: 'ease',
> -               top: '20px' },
> +             keyframe(offset(0.5), { left: '30px' }),
> +             keyframe(offset(0.5), { top:  '40px' }),

It would be nice to have 'linear' for this case to match test case?

::: testing/web-platform/tests/web-animations/resources/keyframe-tests.js:324
(Diff revision 1)
> -    output: [{ offset: 0.0, computedOffset: 0.0, easing: 'linear',
> -               composite: 'replace', left: '10px' },
> -             { offset: 0.0, computedOffset: 0.0, easing: 'linear',
> -               composite: 'replace', top: '20px' },
> -             { offset: 0.5, computedOffset: 0.5, easing: 'linear',
> -               composite: 'add', left: '30px' },
> +    output: [keyframe(offset(0),   { left: '10px' }, 'linear', 'replace'),
> +             keyframe(offset(0),   { top:  '20px' }, 'linear', 'replace'),
> +             keyframe(offset(0.5), { left: '30px' }, 'linear', 'add'),
> +             keyframe(offset(0.5), { top:  '40px' }, 'linear', 'add'),
> +             keyframe(offset(1),   { left: '50px' }, 'linear', 'replace'),
> +             keyframe(offset(1),   { top:  '60px' }, 'linear', 'replace')],

Why does this case have 'linear's?
Attachment #8919620 - Flags: review?(hikezoe) → review+
Comment on attachment 8919621 [details]
Bug 1398038 - Drop redundant exception data from gInvalidKeyframesTests and gInvalidKeyframeEffectOptionTests;

https://reviewboard.mozilla.org/r/190514/#review195770

::: testing/web-platform/tests/web-animations/resources/keyframe-tests.js:521
(Diff revision 1)
>  ];
>  
>  const gInvalidKeyframeEffectOptionTests = [
> -  { desc:     '-Infinity',
> -    input:    -Infinity,
> -    expected: { name: 'TypeError' } },
> +  { desc: '-Infinity', input: -Infinity },
> +  { desc: 'NaN', input: NaN },
> +  { desc: 'a negative value', input:    -1 },

Nit: redundant spaces between : and -1.
Attachment #8919621 - Flags: review?(hikezoe) → review+
Comment on attachment 8919622 [details]
Bug 1398038 - Organize keyframe tests into sections;

https://reviewboard.mozilla.org/r/190516/#review195772
Attachment #8919622 - Flags: review?(hikezoe) → review+
Comment on attachment 8919623 [details]
Bug 1398038 - Add tests for extended offset syntax;

https://reviewboard.mozilla.org/r/190518/#review195776

::: testing/web-platform/tests/web-animations/resources/keyframe-tests.js:196
(Diff revision 1)
> +             keyframe(offset(0.25), { left: '20px' }),
> +             keyframe(offset(0.5),  { left: '30px' })],
> +  },
> +  {
> +    desc:   'a property-indexed keyframe with an array of offsets with embedded'
> +            + ' null values',

Nit: a null value?
Attachment #8919623 - Flags: review?(hikezoe) → review+
Comment on attachment 8919624 [details]
Bug 1398038 - Add tests for extended easing syntax;

https://reviewboard.mozilla.org/r/190520/#review195778

::: commit-message-bd991:4
(Diff revision 1)
> +* * *
> +[mq]: moreEasingTests
> +
> +MozReview-Commit-ID: BUxaeE7bKh0

Nit: drop this.
Attachment #8919624 - Flags: review?(hikezoe) → review+
Comment on attachment 8919625 [details]
Bug 1398038 - Add tests for extended composite syntax;

https://reviewboard.mozilla.org/r/190522/#review195782

::: testing/web-platform/tests/web-animations/resources/keyframe-tests.js:306
(Diff revision 1)
> +    output: [keyframe(computedOffset(0),   { left: '10px' }, 'linear', 'add'),
> +             keyframe(computedOffset(0.5), { left: '20px' }, 'linear', 'add'),
> +             keyframe(computedOffset(1),   { left: '30px' }, 'linear', 'add')],

Can we omit 'linear's here and other test cases in this patch?
Attachment #8919625 - Flags: review?(hikezoe) → review+
Comment on attachment 8919626 [details]
Bug 1398038 - Implement extended property-indexed keyframe syntax;

https://reviewboard.mozilla.org/r/190524/#review195810
Attachment #8919626 - Flags: review?(hikezoe) → review+
>  ./mach web-platform-tests --manifest-update yer

You probably wanted "./mach wpt-manifest-update", which just updates the manifest without trying to run tests.
Comment on attachment 8919626 [details]
Bug 1398038 - Implement extended property-indexed keyframe syntax;

https://reviewboard.mozilla.org/r/190524/#review195850

::: dom/animation/KeyframeUtils.cpp:1454
(Diff revision 1)
>  
>    aResult.Sort(ComputedOffsetComparator());
> +
> +  // Fill in any specified offsets
> +  nsTArray<Nullable<double>> offsets;
> +  if (keyframeDict.mOffset.WasPassed()) {

OK.  So if the default behavior if not passed is the same as empty list, why don't we just set the default value to empty list in the IDL?

Why do we need to support nullable doubles here, exactly?  That's not clear to me from the processing model defined in the spec.

In fact, the processing model very much assumes the doubles are NOT nullable.  https://w3c.github.io/web-animations/#process-a-keyframes-argument step 5 "Otherwise" section, substep 5 talks about `sequence<double>`.

::: dom/animation/KeyframeUtils.cpp:1465
(Diff revision 1)
> +    }
> +  }
> +
> +  size_t offsetsToFill = std::min(offsets.Length(), aResult.Length());
> +  for (size_t i = 0; i < offsetsToFill; i++) {
> +    if (!offsets[i].IsNull()) {

I see no support for this check in the spec prose; in fact it seems to assume this can't happen.

::: dom/animation/KeyframeUtils.cpp:1470
(Diff revision 1)
> +    if (!offsets[i].IsNull()) {
> +      aResult[i].mOffset.emplace(offsets[i].Value());
> +    }
> +  }
> +
> +  // Check that the keyframes are loosely sorted and with values all

It would really be a good idea to have the steps here link to the relevant spec steps, since you're not doing them in the same order as the spec...

::: dom/animation/KeyframeUtils.cpp:1478
(Diff revision 1)
> +  // Fill in any easings, but only if something was specified. Otherwise the
> +  // default values are fine.

That's not what the spec prose says.  https://w3c.github.io/web-animations/#process-a-keyframes-argument step 5, "Otherwise" case, substep 7, says to use a "single-valued sequence" (whatever that is; it's not defined anywhere) with "linear" in it.

Of course then substep 8 contradicts substep 7?  I can't tell; it's talking about an "undefined value" but it's not clear to me whether that can even happen, because that's an ES concept and we're not talking about ES objects here.

Maybe substep 7 was supposed to go away?  If so, I suggest explicitly writing out what substep 8 should do, not kind of waving in the direction of substep 5. and saying "do something like that, but not quite", because the data types involved are different enough that people can mess this up easily.

::: dom/animation/KeyframeUtils.cpp:1502
(Diff revision 1)
> +        }
> +      }
> +    }
> +
> +    // Fill in and repeat easings as needed
> +    if (!easings.IsEmpty()) {

This IsEmpty() check has no equivalent in the spec.  This means that the spec breaks if an empty sequence is provided...  This needs to be fixed in the spec.

I assume there are tests for this case, and all other edge cases (not passing a value, etc, etc), right?

::: dom/animation/KeyframeUtils.cpp:1504
(Diff revision 1)
> +    }
> +
> +    // Fill in and repeat easings as needed
> +    if (!easings.IsEmpty()) {
> +      for (size_t i = 0; i < aResult.Length(); i++) {
> +        aResult[i].mTimingFunction = easings[i % easings.Length()];

OK, so here we override the input even if we had a None for the parsed easing?   It's not clear to me from the spec exactly what should happen here.  Might be worth a code comment explaining how this matches up to what the spec says.

::: dom/animation/KeyframeUtils.cpp:1510
(Diff revision 1)
> +      }
> +    }
> +  }
> +
> +  // And composite operations.
> +  if (keyframeDict.mComposite.WasPassed()) {

If "not passed" is meant to be equivalent to "empty sequence", as the spec suggests, why not make "empty sequence" the default value in the IDL?  Then the spec can not worry about "undefined" (which isn't a valid value at that point anyway) and this code can be simpler.
Attachment #8919626 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #32)
> Comment on attachment 8919626 [details]
> Bug 1398038 - Implement extended property-indexed keyframe syntax;
> 
> https://reviewboard.mozilla.org/r/190524/#review195850
> 
> ::: dom/animation/KeyframeUtils.cpp:1454
> (Diff revision 1)
> >  
> >    aResult.Sort(ComputedOffsetComparator());
> > +
> > +  // Fill in any specified offsets
> > +  nsTArray<Nullable<double>> offsets;
> > +  if (keyframeDict.mOffset.WasPassed()) {
> 
> OK.  So if the default behavior if not passed is the same as empty list, why
> don't we just set the default value to empty list in the IDL?
> 
> Why do we need to support nullable doubles here, exactly?  That's not clear
> to me from the processing model defined in the spec.

It allows you to leave some intermediate values unspecified and let the browser fill them in, e.g.

  elem.animate({ color: [ 'blue', 'green', 'red', 'yellow' ],
                 offset: [ null, null, 0.8, null ] }, 2000);

Also, we have a compatibility requirement to support:

  elem.animate([{ color: 'blue', offset: 0 },
                { color: 'green', offset: null }], 2000);

so it makes sense to support null values in both forms.

I tried dropping null values but then the former would break since null will be converted to zero at the IDL level.

Likewise, if we don't support nulls in the second form, then:

  elem.animate({ color: [ 'blue', 'green', 'red', 'yellow' ],
                 offset: [ null, null, 0.8, null ] }, 2000);

would give a very different result since the second null would become 0 instead of 0.4.

> In fact, the processing model very much assumes the doubles are NOT
> nullable. 
> https://w3c.github.io/web-animations/#process-a-keyframes-argument step 5
> "Otherwise" section, substep 5 talks about `sequence<double>`.

That's an oversight from when I tried to drop supporting doubles, I'll fix that shortly.

> ::: dom/animation/KeyframeUtils.cpp:1465
> (Diff revision 1)
> > +    }
> > +  }
> > +
> > +  size_t offsetsToFill = std::min(offsets.Length(), aResult.Length());
> > +  for (size_t i = 0; i < offsetsToFill; i++) {
> > +    if (!offsets[i].IsNull()) {
> 
> I see no support for this check in the spec prose; in fact it seems to
> assume this can't happen.

I believe that is the same oversight from the spec edit.

> ::: dom/animation/KeyframeUtils.cpp:1470
> (Diff revision 1)
> > +    if (!offsets[i].IsNull()) {
> > +      aResult[i].mOffset.emplace(offsets[i].Value());
> > +    }
> > +  }
> > +
> > +  // Check that the keyframes are loosely sorted and with values all
> 
> It would really be a good idea to have the steps here link to the relevant
> spec steps, since you're not doing them in the same order as the spec...

Good idea, will do.

> ::: dom/animation/KeyframeUtils.cpp:1478
> (Diff revision 1)
> > +  // Fill in any easings, but only if something was specified. Otherwise the
> > +  // default values are fine.
> 
> That's not what the spec prose says. 
> https://w3c.github.io/web-animations/#process-a-keyframes-argument step 5,
> "Otherwise" case, substep 7, says to use a "single-valued sequence"
> (whatever that is; it's not defined anywhere) with "linear" in it.

Due to the repeating behavior for easing, a sequence with a single value of "linear" will be repeated to the necessary length such that all keyframes are assigned an easing of "linear".

In our implementation, linear is represented by a None value for Maybe<ComputedTimingFunction>. As a result, an empty array will produce the same effect. That is we simply won't assign the mTimingFunctionMember so they will have a None value, i.e. "linear".

A "single-valued sequence" refers to a sequence with a single value although there's clearly a spec bug about what that value is which I'll fix shortly.

> Of course then substep 8 contradicts substep 7?  I can't tell; it's talking
> about an "undefined value" but it's not clear to me whether that can even
> happen, because that's an ES concept and we're not talking about ES objects
> here.

I guess the equivalent WebIDL term here is "not present". I'll update the spec to match.

> Maybe substep 7 was supposed to go away?  If so, I suggest explicitly
> writing out what substep 8 should do, not kind of waving in the direction of
> substep 5. and saying "do something like that, but not quite", because the
> data types involved are different enough that people can mess this up easily.

Yes, that's right. Sorry about that. That might be merge fall out or I might have just overlooked that when I reviewed it. I'll drop step 7.

> ::: dom/animation/KeyframeUtils.cpp:1502
> (Diff revision 1)
> > +        }
> > +      }
> > +    }
> > +
> > +    // Fill in and repeat easings as needed
> > +    if (!easings.IsEmpty()) {
> 
> This IsEmpty() check has no equivalent in the spec.  This means that the
> spec breaks if an empty sequence is provided...  This needs to be fixed in
> the spec.

Yes, will do.

> I assume there are tests for this case, and all other edge cases (not
> passing a value, etc, etc), right?

Not passing a value, single string, string element sequence, too many values, too few values etc. etc. are covered but empty sequences are not. I'll add that.

> ::: dom/animation/KeyframeUtils.cpp:1504
> (Diff revision 1)
> > +    }
> > +
> > +    // Fill in and repeat easings as needed
> > +    if (!easings.IsEmpty()) {
> > +      for (size_t i = 0; i < aResult.Length(); i++) {
> > +        aResult[i].mTimingFunction = easings[i % easings.Length()];
> 
> OK, so here we override the input even if we had a None for the parsed
> easing?   It's not clear to me from the spec exactly what should happen
> here.  Might be worth a code comment explaining how this matches up to what
> the spec says.

We parse "linear" as None so overriding is the correct thing to do here unless I misunderstand your concern.

> ::: dom/animation/KeyframeUtils.cpp:1510
> (Diff revision 1)
> > +      }
> > +    }
> > +  }
> > +
> > +  // And composite operations.
> > +  if (keyframeDict.mComposite.WasPassed()) {
> 
> If "not passed" is meant to be equivalent to "empty sequence", as the spec
> suggests, why not make "empty sequence" the default value in the IDL?  Then
> the spec can not worry about "undefined" (which isn't a valid value at that
> point anyway) and this code can be simpler.

That sounds like a good idea.
> It allows you to leave some intermediate values unspecified and let the browser fill them in

OK.  That wasn't obvious from the spec without reading the processing model carefully (and the processing model in the spec was confused).  Might be worth an informative something, though maybe that's already in the spec and I just missed it.

I'm still not entirely clear on what behavior null produces.  Does it do "use whatever value we already had from somewhere else", basically?

> That is we simply won't assign the mTimingFunctionMember so they will have a None value, i.e. "linear".

As in, the output array is known to only contain None values at this point?

> A "single-valued sequence" refers to a sequence with a single value

OK, but nothing defines that term anywhere.

I suggest either using "sequence with length 1" if you're talking about checking whether it's "single-valued" and using explicit list notation as defined by <https://infra.spec.whatwg.org/#list> (since a "sequence" in IDL becomes an infra list according to <https://heycam.github.io/webidl/#idl-sequence>) to construct such sequences.

> I guess the equivalent WebIDL term here is "not present".

OK.

> We parse "linear" as None so overriding is the correct thing to do here

Right.  I didn't realize that None was used to represent a valid value.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> Comment on attachment 8919619 [details]
> Bug 1398038 - Fix spacing of keyframes tests;
> 
> https://reviewboard.mozilla.org/r/190510/#review195766
> 
> ::: testing/web-platform/tests/web-animations/resources/keyframe-tests.js:488
> (Diff revision 1)
> > +    desc:     'keyframes with an out-of-bounded positive offset',
> >      input:    [ { opacity: 0 },
> 
> Now I realized we put a space between [ and {.  We should do the same manner
> in the previous entries, gKeyframesTests.

There shouldn't be a space there. Common JS style (i.e. prettier-style) is no spaces inside [] but spaces inside {}. I'll see if it makes sense to fix that in one of the later patches where I'm touching those lines.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> Comment on attachment 8919620 [details]
> Bug 1398038 - Simplify keyframe test data;
> 
> https://reviewboard.mozilla.org/r/190512/#review195768
> 
> ::: testing/web-platform/tests/web-animations/resources/keyframe-tests.js:310
> (Diff revision 1)
> > -             { offset: 0.0, computedOffset: 0.0, easing: 'ease',
> > -               top: '20px' },
> > +             keyframe(offset(0.5), { left: '30px' }),
> > +             keyframe(offset(0.5), { top:  '40px' }),
> 
> It would be nice to have 'linear' for this case to match test case?

Fixed.

> ::: testing/web-platform/tests/web-animations/resources/keyframe-tests.js:324
> (Diff revision 1)
> > -    output: [{ offset: 0.0, computedOffset: 0.0, easing: 'linear',
> > -               composite: 'replace', left: '10px' },
> > -             { offset: 0.0, computedOffset: 0.0, easing: 'linear',
> > -               composite: 'replace', top: '20px' },
> > -             { offset: 0.5, computedOffset: 0.5, easing: 'linear',
> > -               composite: 'add', left: '30px' },
> > +    output: [keyframe(offset(0),   { left: '10px' }, 'linear', 'replace'),
> > +             keyframe(offset(0),   { top:  '20px' }, 'linear', 'replace'),
> > +             keyframe(offset(0.5), { left: '30px' }, 'linear', 'add'),
> > +             keyframe(offset(0.5), { top:  '40px' }, 'linear', 'add'),
> > +             keyframe(offset(1),   { left: '50px' }, 'linear', 'replace'),
> > +             keyframe(offset(1),   { top:  '60px' }, 'linear', 'replace')],
> 
> Why does this case have 'linear's?

I looked into making keyframe() take either a timing function, or a composite mode, or both, but it would complicate the test code too much so for now if you specify a composite mode you need to specify the easing too.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #34)
> > It allows you to leave some intermediate values unspecified and let the browser fill them in
> 
> OK.  That wasn't obvious from the spec without reading the processing model
> carefully (and the processing model in the spec was confused).  Might be
> worth an informative something, though maybe that's already in the spec and
> I just missed it.

Yes, it's described in the long non-normative part at the start of that section. It's such a long section it's easy to miss.

> I'm still not entirely clear on what behavior null produces.  Does it do
> "use whatever value we already had from somewhere else", basically?

Example 19 from that section describes the behavior but roughly speaking the UA will automatically space out the keyframes without assigned offsets and store the result as the "computed offset".

> > That is we simply won't assign the mTimingFunctionMember so they will have a None value, i.e. "linear".
> 
> As in, the output array is known to only contain None values at this point?

Yes. The array contains default-constructed Keyframe objects whose mTimingFunctionMember is initialized to None.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=073639dd6f928fed1f00fe7bf705ecab9d55fadf
(Differs only from the forthcoming updated review version in code / changeset comments.)
> Spec changes based on review feedback:

Thanks, that's much better!
Comment on attachment 8919626 [details]
Bug 1398038 - Implement extended property-indexed keyframe syntax;

https://reviewboard.mozilla.org/r/190524/#review196408

r=me with the nits below.  Thank you for dealing with the spec end of things!

::: dom/animation/KeyframeUtils.cpp:1461
(Diff revision 2)
> +  nsTArray<Nullable<double>> offsets;
> +  auto& offset = keyframeDict.mOffset;
> +  if (offset.IsDouble()) {
> +    offsets.AppendElement(Nullable<double>(offset.GetAsDouble()));
> +  } else if (offset.IsDoubleOrNullSequence()) {
> +    offsets = offset.GetAsDoubleOrNullSequence();

We should strongly consider doing a fallible copy here and throwing OOM if it fails, instead of crashing.  The length of this array is entirely under script control.

Alternately, we could go for a no-copy solution, kinda like so:

    nsTArray<Nullable<double>>* offsets;
    nsAutoTArray<Nullable<double>>, 1> singleOffset;
    if (offset.IsDouble()) {
      // Append to singleOffset, set offsets to &singleOffset.
    } else {
      // Set offsets to &offset.GetAsDoubleOrNullSequence()
    }

::: dom/animation/KeyframeUtils.cpp:1510
(Diff revision 2)
> +      aResult.Clear();
> +      return;
> +    }
> +  } else {
> +    for (const nsString& easingString : easing.GetAsStringSequence()) {
> +      easings.AppendElement(

Again, these should probably be fallible appends, with OOM thrown on failure, instead of crashing.

::: dom/animation/KeyframeUtils.cpp:1542
(Diff revision 2)
> +  nsTArray<dom::CompositeOperation> compositeOps;
> +  auto& composite = keyframeDict.mComposite;
> +  if (composite.IsCompositeOperation()) {
> +    compositeOps.AppendElement(composite.GetAsCompositeOperation());
> +  } else {
> +    compositeOps = composite.GetAsCompositeOperationSequence();

Again, fallible copy or no-copy approach.
Attachment #8919626 - Flags: review?(bzbarsky) → review+
Thank you for the reviews!

I've taken a no-copy approach for offset and composite, and fallible appends for easing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=331f42fb364ddd6e6c9c328034576c7a583c706b
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c820067a02d3
Update web-platform-tests MANIFEST.json; r=hiro
https://hg.mozilla.org/integration/autoland/rev/5fe9bdcdb9fd
Split keyframe-tests.js out of keyframe-utils.js; r=hiro
https://hg.mozilla.org/integration/autoland/rev/05fa5b79e727
Use slightly more modern JS in keyframe-*.js; r=hiro
https://hg.mozilla.org/integration/autoland/rev/d699dda77ce4
Use single quotes in keyframe-*.js; r=hiro
https://hg.mozilla.org/integration/autoland/rev/e7824a6dfb94
Merge property-indexed and sequence keyframe lists; r=hiro
https://hg.mozilla.org/integration/autoland/rev/a420d1932576
Fix spacing of keyframes tests; r=hiro
https://hg.mozilla.org/integration/autoland/rev/e880f413c9fd
Simplify keyframe test data; r=hiro
https://hg.mozilla.org/integration/autoland/rev/4aaee38ec23b
Drop redundant exception data from gInvalidKeyframesTests and gInvalidKeyframeEffectOptionTests; r=hiro
https://hg.mozilla.org/integration/autoland/rev/b11e59e7b38e
Organize keyframe tests into sections; r=hiro
https://hg.mozilla.org/integration/autoland/rev/4cf4d05c1c1c
Add tests for extended offset syntax; r=hiro
https://hg.mozilla.org/integration/autoland/rev/a34dce42b2e7
Add tests for extended easing syntax; r=hiro
https://hg.mozilla.org/integration/autoland/rev/613864106ebc
Add tests for extended composite syntax; r=hiro
https://hg.mozilla.org/integration/autoland/rev/d4291c427bd7
Implement extended property-indexed keyframe syntax; r=bz,hiro
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: