Bug 1254419 - Fix zero-length segment handling; r=heycam draft
authorBrian Birtles <birtles@gmail.com>
Tue, 15 Mar 2016 21:13:46 +0800 (2016-03-15)
changeset 341453 90af4494b42fcbc8b9e2ed5aaef231bc4c0027bf
parent 341451 5cfc10c14aaea2a449f74dcbb366179a45442dd6
child 341454 91d5f177e1230f6ce6d59177bb9f32148d40503f
push id13214
push userbbirtles@mozilla.com
push dateThu, 17 Mar 2016 02:15:12 +0000 (2016-03-17)
reviewersheycam
bugs1254419
milestone48.0a1
Bug 1254419 - Fix zero-length segment handling; r=heycam Later in this patch series when we convert tests from web-platform tests to mochitest-chrome tests, some of the test cases that use zero-length segments (overlapping keyframes at certain offsets) would trigger failed assertions in KeyframeEffectReadOnly::ComposeStyle. This is because this method was originally written with CSS animations in mind where segments cannot be zero-length. Furthermore, when these same tests cases are run as web-platform-tests, the failed assertions are not visible. This patch adjusts the handling of segments to allow zero-length segments and adds a test to check that the handling matches that defined in Web Animations which is summarized in the following informative note, "this procedure permits overlapping keyframes. The behavior is that at the point of overlap the output value jumps to the value of the last defined keyframe at that offset. For overlapping frames at 0 or 1, the output value for iteration progress values less than 0 or greater than or equal to 1 is the value of the first keyframe or the last keyframe in keyframes respectively."[1] [1] https://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-animation-effect MozReview-Commit-ID: JdyYbGZtbot
dom/animation/KeyframeEffect.cpp
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html
--- a/dom/animation/KeyframeEffect.cpp
+++ b/dom/animation/KeyframeEffect.cpp
@@ -568,44 +568,54 @@ KeyframeEffectReadOnly::ComposeStyle(Ref
     aSetProperties.AddProperty(prop.mProperty);
 
     MOZ_ASSERT(prop.mSegments.Length() > 0,
                "property should not be in animations if it has no segments");
 
     // FIXME: Maybe cache the current segment?
     const AnimationPropertySegment *segment = prop.mSegments.Elements(),
                                 *segmentEnd = segment + prop.mSegments.Length();
-    while (segment->mToKey < computedTiming.mProgress.Value()) {
-      MOZ_ASSERT(segment->mFromKey < segment->mToKey, "incorrect keys");
+    while (segment->mToKey <= computedTiming.mProgress.Value()) {
+      MOZ_ASSERT(segment->mFromKey <= segment->mToKey, "incorrect keys");
       if ((segment+1) == segmentEnd) {
         break;
       }
       ++segment;
       MOZ_ASSERT(segment->mFromKey == (segment-1)->mToKey, "incorrect keys");
     }
-    MOZ_ASSERT(segment->mFromKey < segment->mToKey, "incorrect keys");
+    MOZ_ASSERT(segment->mFromKey <= segment->mToKey, "incorrect keys");
     MOZ_ASSERT(segment >= prop.mSegments.Elements() &&
                size_t(segment - prop.mSegments.Elements()) <
                  prop.mSegments.Length(),
                "out of array bounds");
 
     if (!aStyleRule) {
       // Allocate the style rule now that we know we have animation data.
       aStyleRule = new AnimValuesStyleRule();
     }
 
+    StyleAnimationValue* val = aStyleRule->AddEmptyValue(prop.mProperty);
+
+    // Special handling for zero-length segments
+    if (segment->mToKey == segment->mFromKey) {
+      if (computedTiming.mProgress.Value() < 0) {
+        *val = segment->mFromValue;
+      } else {
+        *val = segment->mToValue;
+      }
+      continue;
+    }
+
     double positionInSegment =
       (computedTiming.mProgress.Value() - segment->mFromKey) /
       (segment->mToKey - segment->mFromKey);
     double valuePosition =
       ComputedTimingFunction::GetPortion(segment->mTimingFunction,
                                          positionInSegment);
 
-    StyleAnimationValue *val = aStyleRule->AddEmptyValue(prop.mProperty);
-
 #ifdef DEBUG
     bool result =
 #endif
       StyleAnimationValue::Interpolate(prop.mProperty,
                                        segment->mFromValue,
                                        segment->mToValue,
                                        valuePosition, *val);
     MOZ_ASSERT(result, "interpolate must succeed now");
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -28413,16 +28413,20 @@
         "path": "web-animations/keyframe-effect/getComputedTiming-progress.html",
         "url": "/web-animations/keyframe-effect/getComputedTiming-progress.html"
       },
       {
         "path": "web-animations/keyframe-effect/getComputedTiming.html",
         "url": "/web-animations/keyframe-effect/getComputedTiming.html"
       },
       {
+        "path": "web-animations/keyframe-effect/keyframe-handling.html",
+        "url": "/web-animations/keyframe-effect/keyframe-handling.html"
+      },
+      {
         "path": "webaudio/the-audio-api/the-audiobuffer-interface/idl-test.html",
         "url": "/webaudio/the-audio-api/the-audiobuffer-interface/idl-test.html"
       },
       {
         "path": "webaudio/the-audio-api/the-audiodestinationnode-interface/idl-test.html",
         "url": "/webaudio/the-audio-api/the-audiodestinationnode-interface/idl-test.html"
       },
       {
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html
@@ -0,0 +1,76 @@
+<!DOCTYPE html>
+<meta charset=utf-8>
+<title>Keyframe handling tests</title>
+<link rel="help" href="https://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-animation-effect">
+<link rel="author" title="Brian Birtles" href="mailto:bbirtles@mozilla.com">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="../testcommon.js"></script>
+<link rel="stylesheet" href="/resources/testharness.css">
+<body>
+<div id="log"></div>
+<div id="target"></div>
+<script>
+'use strict';
+
+test(function(t) {
+  var div = createDiv(t);
+  var anim = div.animate([ { offset: 0, opacity: 0 },
+                           { offset: 0, opacity: 0.1 },
+                           { offset: 0, opacity: 0.2 },
+                           { offset: 1, opacity: 0.8 },
+                           { offset: 1, opacity: 0.9 },
+                           { offset: 1, opacity: 1 } ],
+                         { duration: 1000,
+                           easing: 'cubic-bezier(0.5, -0.5, 0.5, 1.5)' });
+  assert_equals(getComputedStyle(div).opacity, '0.2',
+                'When progress is zero the last keyframe with offset 0 should'
+                + ' be used');
+  // http://cubic-bezier.com/#.5,-0.5,.5,1.5
+  // At t=0.15, the progress should be negative
+  anim.currentTime = 150;
+  assert_equals(getComputedStyle(div).opacity, '0',
+                'When progress is negative, the first keyframe with a 0 offset'
+                + ' should be used');
+  // At t=0.71, the progress should be just less than 1
+  anim.currentTime = 710;
+  assert_approx_equals(parseFloat(getComputedStyle(div).opacity), 0.8, 0.01,
+                'When progress is just less than 1, the first keyframe with an'
+                + ' offset of 1 should be used as the interval endpoint');
+  // At t=0.85, the progress should be > 1
+  anim.currentTime = 850;
+  assert_equals(getComputedStyle(div).opacity, '1',
+                'When progress is greater than 1.0, the last keyframe with a 1'
+                + ' offset should be used');
+  anim.finish();
+  assert_equals(getComputedStyle(div).opacity, '1',
+                'When progress is equal to 1.0, the last keyframe with a 1'
+                + ' offset should be used');
+}, 'Overlapping keyframes at 0 and 1 use the appropriate value when the'
+   + ' progress is outside the range [0, 1]');
+
+test(function(t) {
+  var div = createDiv(t);
+  var anim = div.animate([ { offset: 0, opacity: 0 },
+                           { offset: 0.5, opacity: 0.3 },
+                           { offset: 0.5, opacity: 0.5 },
+                           { offset: 0.5, opacity: 0.7 },
+                           { offset: 1, opacity: 1 } ], 1000);
+  anim.currentTime = 250;
+  assert_equals(getComputedStyle(div).opacity, '0.15',
+                'Before the overlap point, the first keyframe from the'
+                + ' overlap point should be used as interval endpoint');
+  anim.currentTime = 500;
+  assert_equals(getComputedStyle(div).opacity, '0.7',
+                'At the overlap point, the last keyframe from the'
+                + ' overlap point should be used as interval startpoint');
+  anim.currentTime = 750;
+  assert_equals(getComputedStyle(div).opacity, '0.85',
+                'After the overlap point, the last keyframe from the'
+                + ' overlap point should be used as interval startpoint');
+}, 'Overlapping keyframes between 0 and 1 use the appropriate value on each'
+   + ' side of the overlap point');
+
+done();
+</script>
+</body>