1264822 - GetPropertyValuesPairs fails to skip shorthand properties where none of the sub-properties are animatable

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

The "process a keyframe-like object" procedure[1] contains the following step:

  "Let animatable properties be a list of property names (including shorthand properties that have longhand sub-properties that are animatable) and attribute names that can be animated by the implementation."

[1] http://w3c.github.io/web-animations/#process-a-keyframe-like-object

However, in GetPropertyValuesPairs[2] we are assuming that a no-longer-present call to StyleAnimationValue::ComputeValues which check shorthands for us. As a result, the following test will fail:

  assert_equals(elem.animate({ animation: [ 'abc 1s', 'abc 2s' ] }, 1000)
                .effect.getFrames().length, 0);

[2] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/dom/animation/KeyframeUtils.cpp#715

Bug 1264396 will add tests for this (but mark them failing based on this bug).

Comment 1

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1264822 part 1 - Move web-animations/animation-model/keyframes to keyframe-effects; r=hiro

To better match the outline of the spec.

Review commit: https://reviewboard.mozilla.org/r/53076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53076/

Comment 2

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1264822 part 2 - Move web-animations/keyframe-effect/keyframe-handling.html; r=hiro

To better match where the behavior under test is defined in the spec.

Review commit: https://reviewboard.mozilla.org/r/53078/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53078/

Comment 3

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1264822 part 3 - Add a test that non-animatable properties are not accessed; r=hiro

We already have a test under
web-animations/animation-model/animation-types/not-animatable.html that checks
that non-animatable properties don't show up in the result from getKeyframes().
However, the spec says we should not even *read* those properties off the input
object(s).

This patch adds a test to check that we don't read non-animatable properties.
In a later patch we will remove the existing test since it will be redundant
after this test is added.

Review commit: https://reviewboard.mozilla.org/r/53080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53080/

Comment 4

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1264822 part 4 - Ignore shorthand properties when all their subproperties are not-animatable; r=hiro

This patch also includes a tweak to not-animatable.html to match the order in
which properties are enumerated on the object. This test was always in error but
we never noticed since the test failed before reaching the test in question.

Making the test dependent on the order in which object properties is enumerated
is not good but we will remove this test in the next patch. In this patch we
just make sure it passes.

Review commit: https://reviewboard.mozilla.org/r/53082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53082/

Comment 5

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1264822 part 5 - Remove web-animations/animation-model/animation-types/not-animatable.html; r=hiro

Part 3 in this patch series introduces a test that checks this behavior more
strictly. If that test passes, this test is sure to pass as well.

Review commit: https://reviewboard.mozilla.org/r/53084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53084/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8753200 [details]
MozReview Request: Bug 1264822 part 1 - Move web-animations/animation-model/keyframes to keyframe-effects; r=hiro

https://reviewboard.mozilla.org/r/53076/#review49908

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8753200 [details]
MozReview Request: Bug 1264822 part 1 - Move web-animations/animation-model/keyframes to keyframe-effects; r=hiro

https://reviewboard.mozilla.org/r/53076/#review49910

Oh, I forgot to mention about the name.  I am just wondering, is there any strong reason using prulal form? I mean keyframe-effects is prefer to keyframe-effect?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8753200 [details]
MozReview Request: Bug 1264822 part 1 - Move web-animations/animation-model/keyframes to keyframe-effects; r=hiro

https://reviewboard.mozilla.org/r/53076/#review49912

Oops. "Ship it" check was accidentaly dropped.

Comment 9

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> Comment on attachment 8753200 [details]
> MozReview Request: Bug 1264822 part 1 - Move
> web-animations/animation-model/keyframes to keyframe-effects
> 
> https://reviewboard.mozilla.org/r/53076/#review49910
> 
> Oh, I forgot to mention about the name.  I am just wondering, is there any
> strong reason using prulal form? I mean keyframe-effects is prefer to
> keyframe-effect?

To match the corresponding heading in the spec: https://w3c.github.io/web-animations/#keyframe-effects

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8753201 [details]
MozReview Request: Bug 1264822 part 2 - Move web-animations/keyframe-effect/keyframe-handling.html; r=hiro

https://reviewboard.mozilla.org/r/53078/#review49914

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8753202 [details]
MozReview Request: Bug 1264822 part 3 - Add a test that non-animatable properties are not accessed; r=hiro

https://reviewboard.mozilla.org/r/53080/#review49906

TestKeyframe object looks like a Mock object.  I love it!

::: testing/web-platform/tests/web-animations/keyframe-effect/processing-a-keyframes-argument.html:26
(Diff revision 1)
> +  'animationName',
> +  'animationTimingFunction',
> +  'animationDelay',
> +  'animationIterationCount',
> +  'animationDirection',
> +  'animationFillMode',
> +  'animationPlayState',

The list does not include animationDuration.  I wonder it is animatable?.

And this array should be alphaberical ordered?  It was actually little bit hard to check that it has all animation properties.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8753203 [details]
MozReview Request: Bug 1264822 part 4 - Ignore shorthand properties when all their subproperties are not-animatable; r=hiro

https://reviewboard.mozilla.org/r/53082/#review49930

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8753204 [details]
MozReview Request: Bug 1264822 part 5 - Remove web-animations/animation-model/animation-types/not-animatable.html; r=hiro

https://reviewboard.mozilla.org/r/53084/#review49932

Review board does not track files which are added and then removed in the middle of patch set.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Comment on attachment 8753204 [details]
> MozReview Request: Bug 1264822 part 5 - Remove
> web-animations/animation-model/animation-types/not-animatable.html
> 
> https://reviewboard.mozilla.org/r/53084/#review49932
> 
> Review board does not track files which are added and then removed in the
> middle of patch set.

Filed bug 1273385.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #9)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> > Comment on attachment 8753200 [details]
> > MozReview Request: Bug 1264822 part 1 - Move
> > web-animations/animation-model/keyframes to keyframe-effects
> > 
> > https://reviewboard.mozilla.org/r/53076/#review49910
> > 
> > Oh, I forgot to mention about the name.  I am just wondering, is there any
> > strong reason using prulal form? I mean keyframe-effects is prefer to
> > keyframe-effect?
> 
> To match the corresponding heading in the spec:
> https://w3c.github.io/web-animations/#keyframe-effects

Ah, thanks. Got it.

Comment 16

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> :::
> testing/web-platform/tests/web-animations/keyframe-effect/processing-a-
> keyframes-argument.html:26
> (Diff revision 1)
> > +  'animationName',
> > +  'animationTimingFunction',
> > +  'animationDelay',
> > +  'animationIterationCount',
> > +  'animationDirection',
> > +  'animationFillMode',
> > +  'animationPlayState',
> 
> The list does not include animationDuration.  I wonder it is animatable?.
> 
> And this array should be alphaberical ordered?  It was actually little bit
> hard to check that it has all animation properties.

Thanks! Good idea!

Comment 17

[Brian Birtles (:birtles)]

Comment on attachment 8753200 [details]
MozReview Request: Bug 1264822 part 1 - Move web-animations/animation-model/keyframes to keyframe-effects; r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53076/diff/1-2/

Comment 18

[Brian Birtles (:birtles)]

Comment on attachment 8753201 [details]
MozReview Request: Bug 1264822 part 2 - Move web-animations/keyframe-effect/keyframe-handling.html; r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53078/diff/1-2/

Comment 19

[Brian Birtles (:birtles)]

Comment on attachment 8753202 [details]
MozReview Request: Bug 1264822 part 3 - Add a test that non-animatable properties are not accessed; r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53080/diff/1-2/

Comment 20

[Brian Birtles (:birtles)]

Comment on attachment 8753203 [details]
MozReview Request: Bug 1264822 part 4 - Ignore shorthand properties when all their subproperties are not-animatable; r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53082/diff/1-2/

Comment 21

[Brian Birtles (:birtles)]

Comment on attachment 8753204 [details]
MozReview Request: Bug 1264822 part 5 - Remove web-animations/animation-model/animation-types/not-animatable.html; r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53084/diff/1-2/

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/511a30e1cd52
https://hg.mozilla.org/integration/mozilla-inbound/rev/895f77c858f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5dd925fd231
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d85de1a5c24
https://hg.mozilla.org/integration/mozilla-inbound/rev/f25b5705f25d

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/511a30e1cd52
https://hg.mozilla.org/mozilla-central/rev/895f77c858f8
https://hg.mozilla.org/mozilla-central/rev/c5dd925fd231
https://hg.mozilla.org/mozilla-central/rev/8d85de1a5c24
https://hg.mozilla.org/mozilla-central/rev/f25b5705f25d