[WIP] Web Animations API: Constructing an Animation from partial keyframes throws a JS exception

[WIP] Web Animations API: Constructing an Animation from partial keyframes throws a JavaScript exception

If a list of keyframes includes just one declaration for a particular property it is called a partial keyframe. Web Animations doesn't support Animations with partial keyframes yet.

Previously if a user tried to do this it would crash the renderer. There is an assert for it in StyleResolver::applyAnimatedProperties.

This change checks for mismatched property declarations in keyframe data after it has been parsed, and throws a JavaScript NotSupportedError exception if it finds any. It has to be done after parsing otherwise declarations with invalid values can interfere.

[rjwright, 2014-03-10 05:31:12]

ptal

[rjwright, 2014-03-10 05:56:35]

I just discovered that because of the version of
MutableStylePropertySet::setProperty() that we use, mixing shorthand and
longhand properties should work out-of-the-box (i.e. if you declare a shorthand
in a keyframe and then each of its subproperties in other keyframes in the set,
you shouldn't get an exception).

I should add a test case to ensure that behavior is maintained.

[rjwright, 2014-03-10 07:09:23]

On 2014/03/10 05:56:35, rjwright wrote:
> I just discovered that because of the version of
> MutableStylePropertySet::setProperty() that we use, mixing shorthand and
> longhand properties should work out-of-the-box (i.e. if you declare a
shorthand
> in a keyframe and then each of its subproperties in other keyframes in the
set,
> you shouldn't get an exception).
> 
> I should add a test case to ensure that behavior is maintained.

It turns out this isn't working. In fact, short hands don't seem to be working
at all.

[shans, 2014-03-10 23:28:40]

https://codereview.chromium.org/190763007/diff/1/Source/core/animation/Effect...
File Source/core/animation/EffectInput.cpp (right):

https://codereview.chromium.org/190763007/diff/1/Source/core/animation/Effect...
Source/core/animation/EffectInput.cpp:65: }
This won't pick up cases like:
element.animate(
  [{opacity: '0', offset: 0},
   {opacity: '1', offset: 0.5}], 1);

[rjwright, 2014-03-10 23:35:36]

https://codereview.chromium.org/190763007/diff/1/Source/core/animation/Effect...
File Source/core/animation/EffectInput.cpp (right):

https://codereview.chromium.org/190763007/diff/1/Source/core/animation/Effect...
Source/core/animation/EffectInput.cpp:65: }
On 2014/03/10 23:28:40, shans wrote:
> This won't pick up cases like:
> element.animate(
>   [{opacity: '0', offset: 0},
>    {opacity: '1', offset: 0.5}], 1);

It is meant to?

[rjwright, 2014-03-11 01:48:42]

https://codereview.chromium.org/190763007/diff/1/Source/core/animation/Effect...
File Source/core/animation/EffectInput.cpp (right):

https://codereview.chromium.org/190763007/diff/1/Source/core/animation/Effect...
Source/core/animation/EffectInput.cpp:65: }
On 2014/03/10 23:28:40, shans wrote:
> This won't pick up cases like:
> element.animate(
>   [{opacity: '0', offset: 0},
>    {opacity: '1', offset: 0.5}], 1);

Done.

[rjwright, 2014-03-11 02:22:24]

Note also that hasMismatchedKeyframeProperty will absorbed into the body of
EffectInput::convert in an immediate follow-up CL to fix shorthands.

[shans, 2014-03-11 03:00:28]

On 2014/03/11 02:22:24, rjwright wrote:
> Note also that hasMismatchedKeyframeProperty will absorbed into the body of
> EffectInput::convert in an immediate follow-up CL to fix shorthands.

I think this is still wrong, sorry :(

I'm pretty sure all of these will work with our current implementation:
element.animate([{width: '100px'}, {width: '200px'}], 1);
element.animate([{width: '100px'}, {width: '200px', offset: 1}], 1);
element.animate([{width: '100px', offset: 0}, {width: '200px'}], 1);
element.animate([{width: '100px'}, {width: '500px', offset: 0.5}, {width:
'200px'}], 1);

It seems like these will be filtered out by your checks though? On the other
hand, this won't work but won't be caught:
element.animate([{width: '100px', height: '200px', offset: 0}, {height:
'100px'}, {width: '50px', offset: 1}]);

What I think we need to check is that:
* there are at least two keyframes for every property (this is the
hasPartialKeyframes function)
* the first keyframe for each property is either offset: 0 or has no offset
specified
* the last keyframe for each property is either offset: 1 or has no offset
specified

[rjwright1, 2014-03-11 03:12:22]

Offsets requirements are move complicated than I initially thought.


On Tue, Mar 11, 2014 at 2:00 PM, <shans@chromium.org> wrote:

> On 2014/03/11 02:22:24, rjwright wrote:
>
>> Note also that hasMismatchedKeyframeProperty will absorbed into the body
>> of
>> EffectInput::convert in an immediate follow-up CL to fix shorthands.
>>
>
> I think this is still wrong, sorry :(
>
> I'm pretty sure all of these will work with our current implementation:
> element.animate([{width: '100px'}, {width: '200px'}], 1);
> element.animate([{width: '100px'}, {width: '200px', offset: 1}], 1);
> element.animate([{width: '100px', offset: 0}, {width: '200px'}], 1);
> element.animate([{width: '100px'}, {width: '500px', offset: 0.5}, {width:
> '200px'}], 1);
>
> It seems like these will be filtered out by your checks though? On the
> other
> hand, this won't work but won't be caught:
> element.animate([{width: '100px', height: '200px', offset: 0}, {height:
> '100px'}, {width: '50px', offset: 1}]);
>
> What I think we need to check is that:
> * there are at least two keyframes for every property (this is the
> hasPartialKeyframes function)
> * the first keyframe for each property is either offset: 0 or has no offset
> specified
> * the last keyframe for each property is either offset: 1 or has no offset
> specified
>
>
>
> https://codereview.chromium.org/190763007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dstockwell, 2014-03-11 03:28:13]

https://codereview.chromium.org/190763007/diff/60001/Source/core/animation/Ef...
File Source/core/animation/EffectInput.cpp (right):

https://codereview.chromium.org/190763007/diff/60001/Source/core/animation/Ef...
Source/core/animation/EffectInput.cpp:51: static bool
hasMismatchedKeyframeProperty(Vector<RefPtr<MutableStylePropertySet> >
propertySetVector)
Instead of doing the checks here, we might be able to add a method to
KeyframeEffectModel that just calls ensureKeyframeGroups and then checks that
none require composite: add.

eg. bool KeyframeEffectModel::isReplaceOnly()

[rjwright, 2014-03-11 03:37:40]

On 2014/03/11 03:28:13, dstockwell wrote:
>
https://codereview.chromium.org/190763007/diff/60001/Source/core/animation/Ef...
> File Source/core/animation/EffectInput.cpp (right):
> 
>
https://codereview.chromium.org/190763007/diff/60001/Source/core/animation/Ef...
> Source/core/animation/EffectInput.cpp:51: static bool
> hasMismatchedKeyframeProperty(Vector<RefPtr<MutableStylePropertySet> >
> propertySetVector)
> Instead of doing the checks here, we might be able to add a method to
> KeyframeEffectModel that just calls ensureKeyframeGroups and then checks that
> none require composite: add.
> 
> eg. bool KeyframeEffectModel::isReplaceOnly()

Ok. I will try this instead.

[rjwright, 2014-03-12 06:25:26]

This is now a WIP.

I'm not 100% sure if this correct, as I don't know if there are cases when a
keyframe value dependsOnUnderlyingValue and is also supported in the API. But
this is the progress so far.

I need to test more thoroughly. A bunch of stuff that wasn't working yesterday
is working today, including first keyframe with offset != 0, last keyframe with
offset != 1, and single instances of properties (when the property value is
declared in the first keyframe).

[rjwright, 2014-03-19 07:40:30]

Moved to https://codereview.chromium.org/196053009 after totally changing the
approach & several rebases.