1338404 - Keyframes with single values sometimes not combined correctly

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Try this test:

  JSON.stringify((new KeyframeEffect(null, { left: '100px', opacity: ['1'] })).getKeyframes())

The result is currently:

  [{"easing":"linear","offset":null,"computedOffset":0,"opacity":"1"},{"easing":"linear","offset":null,"computedOffset":1,"left":"100px"}]

That is, the opacity ends up in the keyframe with offset 0 but the left ends up in a keyframe with offset 1.

Stepping through this in a debugger the result of GetPropertyValuesPairs seems correct. I think we go astray later in GetKeyframeListFromPropertyIndexedKeyframe when we attempt to merge the keyframes.

In particular, this line is suspicious:

  double offset = i++ / double(n);

(http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/dom/animation/KeyframeUtils.cpp#1490)

In this test case the result ends up being "-nan(ind)" in my debugger since |n| here is zero. I suspect we failed to update this code to handle single-value property lists when we implemented that.

Incidentally this is also likely the reason we fail one of the tests in process-a-keyframes-argument.html.ini[1]

(And on that note, I really wish we would get bug reports when web-platform-tests repo is synced and there are new failures. We would have found about this a lot sooner if that happened!)

[1] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/testing/web-platform/meta/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument.html.ini#3

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1338404 - Fix keyframe handling for single-valued lists

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

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Unfortunately, I can't run web-platform-tests locally due to bug 1338397 so I'm going to have to verify this on try.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8835874 [details]
Bug 1338404 - Fix keyframe handling for single-valued lists

https://reviewboard.mozilla.org/r/111446/#review112990

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d596478bc931
Fix keyframe handling for single-valued lists r=hiro

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d596478bc931