From the spec; Each keyframe may also have a keyframe-specific composite operation that is applied to all values specified in that keyframe. The possible operations and their meanings are identical to those defined for the composite operation associated with the keyframe effect as a whole in §4.5.4 Effect composition. If no keyframe-specific composite operation is specified for a keyframe, the composite operation specified for the keyframe effect as a whole is used for values specified in that keyframe.

I'd like to re-use this bug for implementation composite(accumulate) for both of keyframe and effect.

Comment on attachment 8803159 [details] Bug 1291468 - Part 1: Tests for effect/keyframe composite(accumulate). https://reviewboard.mozilla.org/r/87404/#review91462 These tests are good but I think they need to be more focussed. Ultimately the things we need to test (including forthcoming bugs are): 1. Do we correctly expand the 'composite' member when specified on property-indexed keyframes? (this will likely become more complicated once [1] is resolved) 2. Do we correctly recognize the 'composite' member when specified on keyframe in a sequence? 3. Do we correctly recognize the 'composite' value passed to animate()? 4. Do we correctly recognize the 'composite' value passed to the KeyframeEffect constructor? 5. Do we correctly recognize the 'composite' value passed to the KeyframeEffectReadOnly constructor? 6. Do we correctly recognize the 'composite' value when set on the KeyframeEffect interface? (separate bug) 7. Do we correctly do accumulate composition when the underlying value comes from the base value? 8. Do we correctly do accumulate composition when the underlying value comes from another animation? 9. Do we correctly do composition when one keyframe use accumulate and one uses replace? 10. Do 'composite' values on keyframes override those on the effect (but fall back when they are not set)? We shouldn't try to test every possible combination of these things. Assuming 1-6, 7-8, 9-10 are mutually exclusive, there are still probably roughly 24 possible combinations we could test. When we come to implement 'add' composition, the set of things to test will increase even further to ~50 or so tests. Instead of testing each combination, we need a test strategy that tests the different pieces. I suggest something like this: API tests: * Test (1-2) by simply using getKeyframes() and checking the member * Test (3-5) in the tests for animate() / the KeyframeEffect(ReadOnly) constructors by checking the .composite member (And, in some cases, check the result of getKeyframes() to ensure that the effect composite mode is *not* set there) * (6) will be a separate bug Animation model tests: * Test (7) using a simplified version of the first test: test(function(t) { var div = createDiv(t); div.style.marginLeft = '10px'; div.animate({ marginLeft: ['10px', '20px'], composite: 'accumulate' }, 1); assert_equals(getComputedStyle(div).marginLeft, '20px'); }, 'Accumulate onto the base value'); * Test (8) using something similar test(function(t) { var div = createDiv(t); div.animate({ marginLeft: ['10px', '20px'] },1); div.animate({ marginLeft: ['10px', '20px'], composite: 'accumulate' }, 1); assert_equals(getComputedStyle(div).marginLeft, '20px'); }, 'Accumulate onto an underlying animation value'); * Test (9) with a slightly simplified version of your last test: test(function(t) { var div = createDiv(t); div.style.marginLeft = '10px'; var anim = div.animate([{ marginLeft: '10px', composite: 'accumulate' }, { marginLeft: '20px' }], 100); assert_equals(getComputedStyle(div).marginLeft, '20px', 'Animated style at 0%'); anim.finish(); assert_equals(getComputedStyle(div).marginLeft, '20px', 'Animated style at 100%'); }, 'Composition when one keyframe when mixing accumulate and replace'); (Personally, I might be inclined to make use different values so that the result is not always '20px' in failing to update the computed style masks a failure base.) * Test (10) by building on the above, as perhaps simply: test(function(t) { var div = createDiv(t); div.style.marginLeft = '10px'; var anim = div.animate([{ marginLeft: '10px' }, { marginLeft: '20px', composite: 'replace' }], { duration: 100, composite: 'accumulate' }); assert_equals(getComputedStyle(div).marginLeft, '20px', 'Animated style at 0%'); anim.finish(); assert_equals(getComputedStyle(div).marginLeft, '20px', 'Animated style at 100%'); }, 'Composite specified on a keyframe overrides the composite mode of the effect'); Can we try to simplify these tests and focus them a bit more? I've been trying to subtitle[2] a great video on this[3] so that I can get it translated and share it with everyone but I haven't gotten very far yet. [1] https://github.com/w3c/web-animations/issues/148 [2] https://amara.org/en/videos/sHcrjrn64Qvp/info/how-to-stop-hating-your-tests/ [3] http://blog.testdouble.com/posts/2015-11-16-how-to-stop-hating-your-tests.html Nit: The order of bug number and part number is reversed in the commit message. ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/composite.html:18 (Diff revision 1) > + var div = createDiv(t); > + div.style.marginLeft = '10px'; > + var anim = > + div.animate({ marginLeft: ['0px', '10px'], composite: 'accumulate' }, > + { duration: 100 * MS_PER_SEC, fill: 'forwards' }); > + anim.pause(); Here and elsewhere in this file pausing doesn't seem necessary? Nor does using a 100s duration? ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/composite.html:20 (Diff revision 1) > + assert_equals(getComputedStyle(div).marginLeft, '10px', > + 'Animated margin-left style at 0s'); > + anim.currentTime = anim.effect.timing.duration / 2; > + assert_equals(getComputedStyle(div).marginLeft, '15px', > + 'Animated margin-left style at 50s'); > + anim.currentTime = anim.effect.timing.duration; > + assert_equals(getComputedStyle(div).marginLeft, '20px', > + 'Animated margin-left style at 100s'); As mentioned above, if the point is to test accumulating with the base value then we only need to test one value. So this test would just become: test(function(t) { var div = createDiv(t); div.style.marginLeft = '10px'; div.animate({ marginLeft: ['10px', '20px'], composite: 'accumulate' }, 1); assert_equals(getComputedStyle(div).marginLeft, '20px'); }, 'keyframe composite(accumulate): onto static style'); It seems like that would test the same thing and be easier to read, maintain, and review? Or if you want to follow the arrange/act/assert pattern you could write it as: test(function(t) { var div = createDiv(t); div.style.marginLeft = '10px'; div.animate({ marginLeft: ['10px', '20px'], composite: 'accumulate' }, 1); assert_equals(getComputedStyle(div).marginLeft, '20px'); }, 'keyframe composite(accumulate): onto static style'); which might be easier to read (but is hard to apply to promise-based tests).

(In reply to Brian Birtles (:birtles, high review load) from comment #6) > * Test (9) with a slightly simplified version of your last test: > > test(function(t) { > var div = createDiv(t); > div.style.marginLeft = '10px'; > > var anim = div.animate([{ marginLeft: '10px', composite: 'accumulate' }, > { marginLeft: '20px' }], > 100); > assert_equals(getComputedStyle(div).marginLeft, '20px', > 'Animated style at 0%'); > > anim.finish(); > assert_equals(getComputedStyle(div).marginLeft, '20px', > 'Animated style at 100%'); > }, 'Composition when one keyframe when mixing accumulate and replace'); > > (Personally, I might be inclined to make use different values so that the > result is not always '20px' in failing to update the computed style masks > a failure base.) Better still, we could just test a single time representing the mid-point of the animation: e.g. test(function(t) { var div = createDiv(t); div.style.marginLeft = '10px'; var anim = div.animate([{ marginLeft: '10px', composite: 'accumulate' }, { marginLeft: '30px' }], 100); anim.currentTime = 50; assert_equals(getComputedStyle(div).marginLeft, '25px', 'Animated style at 50%'); }, 'Composition when one keyframe when mixing accumulate and replace'); That way: * If we fail to apply accumulate at all we'll get 10px->30px and the result will be 15px != 25px * If we apply accumulate to *both* endpoints we'll get 20px->40px and the result will be 30px != 25px

Comment on attachment 8803160 [details] Bug 1291468 - Part 2: Implement keyframe composite(accumulate). https://reviewboard.mozilla.org/r/87406/#review91480 This looks good but I'd like to see the simplifications to the tests ::: dom/animation/KeyframeEffectReadOnly.cpp:375 (Diff revision 1) > - // FIXME: Bug 1291468: Implement accumulate operation. > - MOZ_ASSERT_UNREACHABLE("Not implemented yet"); > + DebugOnly<bool> result = > + StyleAnimationValue::Accumulate(aProperty, aResult, aInputValue, 1); > + MOZ_ASSERT(result, "Could not accumulate"); Why might this fail? ::: dom/animation/KeyframeUtils.cpp:274 (Diff revision 1) > { > nsCSSPropertyID mProperty; > StyleAnimationValue mValue; > float mOffset; > Maybe<ComputedTimingFunction> mTimingFunction; > + Maybe<dom::CompositeOperation> mComposite; I think in bug 1305325 we discussed resolving this to its concrete value in UpdateProperties (which calls GetAnimationPropertiesFromKeyframes which uses this) so we shouldn't need Maybe<> here. Unless we decide to do that in BuildSegmentsFromValueEntries? ::: dom/animation/KeyframeUtils.cpp:804 (Diff revision 1) > + // FIXME: Bug 1311620: We don't support additive animation yet. > + if (keyframeDict.mComposite.Value() != dom::CompositeOperation::Add) { > + keyframe->mComposite.emplace(keyframeDict.mComposite.Value()); > + } (What happens if we just set it here? Does anything break?) ::: dom/animation/KeyframeUtils.cpp:1306 (Diff revision 1) > + // FIXME: Bug 1311620: We don't support additive animation yet. > + if (keyframeDict.mComposite.Value() != dom::CompositeOperation::Add) { > + composite.emplace(keyframeDict.mComposite.Value()); > + } (Likewise here) ::: dom/animation/test/style/file_composite.html:43 (Diff revision 1) > + return waitForPaintsFlushed().then(() => { > + var transform = > + SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'transform'); > + assert_matrix_equals(transform, 'matrix(1, 0, 0, 1, 100, 0)', > + 'Transform value at 0s'); > + > + SpecialPowers.DOMWindowUtils.advanceTimeAndRefresh(50 * MS_PER_SEC); > + transform = > + SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'transform'); > + assert_matrix_equals(transform, 'matrix(1, 0, 0, 1, 200, 0)', > + 'Transform value at 50s'); > + > + SpecialPowers.DOMWindowUtils.advanceTimeAndRefresh(100 * MS_PER_SEC); > + transform = > + SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'transform'); > + assert_matrix_equals(transform, 'matrix(1, 0, 0, 1, 300, 0)', > + 'Transform value at 100s'); > + }); As with the previous patch, do we need to test three different times? If we are testing that the compositor can do accumulation, we only need one time. If we are testing that we are passing the correct values to the compositor then we still probably only need one time (e.g. see comment 7). Can we simplify these tests similar to the suggestions for part 1?

Comment on attachment 8803161 [details] Bug 1291468 - Part 3: Implement effect composite(accumulate). https://reviewboard.mozilla.org/r/87408/#review91482 I'd like to see the test simplifications for this part too. ::: dom/animation/KeyframeEffectReadOnly.cpp:598 (Diff revision 1) > + // FIXME: Bug 1311620: We don't support additive animation yet. > + if (options.mComposite != dom::CompositeOperation::Add) { > + result.mComposite = options.mComposite; > + } (Again, I'm curious to now what breaks if we just set this regardless.)

Depending on bug 1304886 to make Accumulate() easy.

Comment on attachment 8803159 [details] Bug 1291468 - Part 1: Tests for effect/keyframe composite(accumulate). https://reviewboard.mozilla.org/r/87404/#review91462 For 1, 2 and 5 we have already the cases. https://hg.mozilla.org/mozilla-central/file/8e476f8bd52d/testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/constructor.html#l85 https://hg.mozilla.org/mozilla-central/file/8e476f8bd52d/testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/constructor.html#l105 https://hg.mozilla.org/mozilla-central/file/8e476f8bd52d/testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/constructor.html#l123 For 3 and 4, there is a test case that composite is pased to animate() and KeyframeEffect constructor. If it's not sufficient, I will add test cases as part 4 patch. https://hg.mozilla.org/mozilla-central/file/8e476f8bd52d/testing/web-platform/tests/web-animations/resources/keyframe-utils.js#l332

Comment on attachment 8803160 [details] Bug 1291468 - Part 2: Implement keyframe composite(accumulate). https://reviewboard.mozilla.org/r/87406/#review91480 > (What happens if we just set it here? Does anything break?) Though I did forget to add a change to return the specified composite value in keyframes obtained by getKeyframes(), if we pass 'add' here, users will see the 'add' in getKeyframes().

Comment on attachment 8803159 [details] Bug 1291468 - Part 1: Tests for effect/keyframe composite(accumulate). https://reviewboard.mozilla.org/r/87404/#review97368 ::: testing/web-platform/meta/MANIFEST.json:38748 (Diff revision 2) > { > "path": "web-animations/animation-model/animation-types/spacing-keyframes-shapes.html", > "url": "/web-animations/animation-model/animation-types/spacing-keyframes-shapes.html" > } > ], > + "web-animations/animation-model/keyframe-effects/composite.html": [ In terms of matching the headings of the spec, should this be: web-animations/animation-model/combining-effects/effect-composition.html ? ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/composite.html:3 (Diff revision 2) > +<!doctype html> > +<meta charset=utf-8> > +<title>KeyframeEffect.composite tests</title> Tests for effect composition

Comment on attachment 8803160 [details] Bug 1291468 - Part 2: Implement keyframe composite(accumulate). https://reviewboard.mozilla.org/r/87406/#review97372 ::: dom/animation/KeyframeUtils.cpp:817 (Diff revision 2) > + // FIXME: Bug 1311620: We don't support additive animation yet. > + if (keyframeDict.mComposite.Value() != dom::CompositeOperation::Add) { > + keyframe->mComposite.emplace(keyframeDict.mComposite.Value()); > + } (It's up to you how you want to approach this, I'm just curious why we don't just unconditionally set mComposite here but ignore it when it is add?) ::: dom/animation/test/style/file_composite.html:72 (Diff revision 2) > +promise_test(t => { > + useTestRefreshMode(t); > + > + var div = addDiv(t, { style: 'transform: translateX(100px)' }); > + div.animate([{ transform: 'translateX(100px)', composite: 'accumulate' }, > + { transform: 'translateX(200px)', composite: 'replace' }], > + 100 * MS_PER_SEC); > + > + return waitForPaintsFlushed().then(() => { > + SpecialPowers.DOMWindowUtils.advanceTimeAndRefresh(50 * MS_PER_SEC); > + var transform = > + SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'transform'); > + assert_matrix_equals(transform, 'matrix(1, 0, 0, 1, 200, 0)', > + 'Transform value at 50s'); > + }); > +}, 'Composite when mixing accumulate and replace'); If I understand this correctly, our resulting animation will go from translate(200px) -> translate(200px). Could we use different values so that we fail if, for example, we used the wrong time? e.g. make the second (replace) value 300px so that we interpolate from 200px to 300px and get a result at 50% of 250px?

Comment on attachment 8803161 [details] Bug 1291468 - Part 3: Implement effect composite(accumulate). https://reviewboard.mozilla.org/r/87408/#review97374 ::: dom/animation/test/style/file_composite.html:89 (Diff revision 2) > +promise_test(t => { > + useTestRefreshMode(t); > + > + var div = addDiv(t, { style: 'transform: translateX(100px)' }); > + div.animate([{ transform: 'translateX(100px)', composite: 'replace' }, > + { transform: 'translateX(200px)' }], > + { duration: 100 * MS_PER_SEC, composite: 'accumulate' }); > + > + return waitForPaintsFlushed().then(() => { > + SpecialPowers.DOMWindowUtils.advanceTimeAndRefresh(50 * MS_PER_SEC); > + var transform = > + SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'transform'); > + assert_matrix_equals(transform, 'matrix(1, 0, 0, 1, 200, 0)', > + 'Transform value at 50%'); > + }); > +}, 'Composite specified on a keyframe overrides the composite mode of the ' + > + 'effect'); (Once again I would slightly prefer if we produced different values for the cases of (a) interpolating to 50% and compositing correctly and (b) failing to composite and sampling the last value.)

Comment on attachment 8803161 [details] Bug 1291468 - Part 3: Implement effect composite(accumulate). https://reviewboard.mozilla.org/r/87408/#review97378

Pushed by hiikezoe@mozilla-japan.org: https://hg.mozilla.org/integration/autoland/rev/be8333efe362 Part 1: Tests for effect/keyframe composite(accumulate). r=birtles https://hg.mozilla.org/integration/autoland/rev/d6c7a5bc5452 Part 2: Implement keyframe composite(accumulate). r=birtles https://hg.mozilla.org/integration/autoland/rev/60f747f5ce44 Part 3: Implement effect composite(accumulate). r=birtles

https://hg.mozilla.org/mozilla-central/rev/be8333efe362 https://hg.mozilla.org/mozilla-central/rev/d6c7a5bc5452 https://hg.mozilla.org/mozilla-central/rev/60f747f5ce44