If we could land bug 1312301 before this, unlike bug 1320839 I will add test cases in this bug .

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48696c86969c31a4ec2ab59e4fd680199ba11f3d Though there are bunch of dependent patches, I think this is almost ready to be reviewed.

We might want to change 'transform: none' to an identity transform if the none value is the base style of additive animations. For example <style> div { transform: none; } </style> div.animate({ transform : [ 'rotate(0deg)', 'rotate(360deg)'] }, { duration: 1000, composite: 'add' }); In this case, the additive animation can be represented as an animation like this; div.animate({ transform : [ 'none rotate(0deg)', 'none rotate(360deg)'] }, { duration: 1000, composite: 'replace' }); I am still wondering why we don't rotate the latter animation at all, but as far as I can confirm, the latter case does not animate Firefox. Chrome does not either. So there must have been discussed about this (I can't find it thought). That's said, in case of additive animation, we definitely want to animate it, I think users want too, so I am now thinking we have to handle this special case.

The grammar for the 'transform' property is: none | <transform-list> So you can either have 'none' or a list of functions. 'none' itself is not a function so you can't include it in a list with transform functions. https://drafts.csswg.org/css-transforms/#transform-property

Wow! Thanks for the quick response! I did not notice it!

Comment on attachment 8820489 [details] Bug 1311620 - Part 7: addition result tests per properties. https://reviewboard.mozilla.org/r/99992/#review100446 ::: testing/web-platform/tests/web-animations/animation-model/animation-types/addition-per-property.html:49 (Diff revision 1) > + if (typeObject.testAddition && > + typeof typeObject.testAddition === 'function') { > + typeObject.testAddition(property, > + setupFunction, > + animationType.options); > + } > + }); Last time I and Brian discussed about addition-per-property.html, Brian suggested that we can write ready-made test cases for properties that define no additive operation, because additive for such properties behave as discrete type. But after some tries, I noticed that we need two style values for the ready-made test cases. I think it will be almost the same as 'discrete' type. So, I did not add such ready-made gimmick here.

Comment on attachment 8820483 [details] Bug 1311620 - Part 1: Test for effect/keyframe composite(add). https://reviewboard.mozilla.org/r/99980/#review100854 Commit message: s/keyfrrame/keyframe/ ::: testing/web-platform/tests/web-animations/animation-model/combining-effects/effect-composition.html:17 (Diff revision 1) > +[ 'accumulate', 'add' ].forEach(function(composite) { > -test(function(t) { > + test(function(t) { > - var div = createDiv(t); > + var div = createDiv(t); > - div.style.marginLeft = '10px'; > + div.style.marginLeft = '10px'; > - var anim = > + var anim = > - div.animate({ marginLeft: ['0px', '10px'], composite: 'accumulate' }, 100); > + div.animate({ marginLeft: ['0px', '10px'], composite: composite }, 100); It looks like support for object shorthand properties is good[1] so you could actually just do s/composite: composite/composite/ here and elsewhere in this file. [1] https://kangax.github.io/compat-table/es6/#test-object_literal_extensions_shorthand_properties

Comment on attachment 8820484 [details] Bug 1311620 - Part 2: Don't call StyleAnimationValue::GetUnit() against uninitialized values, use IsNull() instead. https://reviewboard.mozilla.org/r/99982/#review100858 (I think the previous check for 'none' was because at some point we used to store 'transform: none' using an actual none unit value as opposed to a none value inside the transform list--or maybe we just thought we did?)

Comment on attachment 8820485 [details] Bug 1311620 - Part 3: Incorporate null_t in Animatable. https://reviewboard.mozilla.org/r/99984/#review100864 Commit message: s/Incorpolate/Incorporate/ (or is it Interpolate?) Commit message: s/we has to/we have to/

Comment on attachment 8820486 [details] Bug 1311620 - Part 4: Implement keyframe composite(add). https://reviewboard.mozilla.org/r/99986/#review100912 ::: layout/style/StyleAnimationValue.h:161 (Diff revision 1) > Accumulate(nsCSSPropertyID aProperty, > const StyleAnimationValue& aA, > StyleAnimationValue&& aB, > uint64_t aCount = 1); > > + static StyleAnimationValue This should be added after the existing Add method. Also, it needs a comment. Perhaps something like: "Alternative version of Add that reflects the naming used in Web Animations and which returns the result using the return value." ::: layout/style/StyleAnimationValue.cpp:3282 (Diff revision 1) > +StyleAnimationValue > +StyleAnimationValue::Add(nsCSSPropertyID aProperty, > + const StyleAnimationValue& aA, > + StyleAnimationValue&& aB) > +{ > + StyleAnimationValue result(Move(aB)); > + > + Unused << AddWeighted(aProperty, > + 1.0, result, > + 1, aA, > + result); > + > + return result; > +} Likewise, this should be moved towards the top of the file to match the order in the header file.

Comment on attachment 8820487 [details] Bug 1311620 - Part 5: Implement effect composite(add). https://reviewboard.mozilla.org/r/99988/#review100914

Comment on attachment 8820488 [details] Bug 1311620 - Part 6: Fix test cases that checks keyframe composite is not specified but effect composite is specified. https://reviewboard.mozilla.org/r/99990/#review100916

Comment on attachment 8820489 [details] Bug 1311620 - Part 7: addition result tests per properties. https://reviewboard.mozilla.org/r/99992/#review101072 ::: testing/web-platform/tests/web-animations/animation-model/animation-types/addition-per-property.html:3 (Diff revision 1) > +<!doctype html> > +<meta charset=utf-8> > +<title>Tests for animation types</title> How about "Tests for animation types of addition"? ::: testing/web-platform/tests/web-animations/animation-model/animation-types/interpolation-per-property.html:3 (Diff revision 1) > <!DOCTYPE html> > <meta charset=utf-8> > -<title>Tests for animation types</title> > +<title>Tests for animation types of addition</title> This file is for interpolation, so I think we shouldn't add "addition" to the title. Or s/of addition/of interpolation/ ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:557 (Diff revision 1) > + // R: 255 * (0.4 * 0.5) / 0.6 = 85 > + // G: 255 * (0.8 * 0.5) / 0.6 = 170 This comment is for 50% case, i.e. time: 500, but we don't test it now, so I think we can remove it to avoid confusing others. ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:803 (Diff revision 1) > + // [ 1 + tan(10deg) * tan(-30deg) tan(20deg) + tan(10deg) ] > + // [ tan(10deg) + tan(-30deg) tan(10deg) * tan(20deg) + 1 ] nit: // [ ..... ] <--- these is one extra space. // [ ..... ]

Comment on attachment 8820490 [details] Bug 1311620 - Part 8: Implement color addition. https://reviewboard.mozilla.org/r/99994/#review101168

Comment on attachment 8820491 [details] Bug 1311620 - Part 9: Implement transform list addtion. . https://reviewboard.mozilla.org/r/99996/#review101170

Comment on attachment 8820491 [details] Bug 1311620 - Part 9: Implement transform list addtion. . https://reviewboard.mozilla.org/r/99996/#review101172 ::: layout/style/StyleAnimationValue.cpp:3317 (Diff revision 1) > + nsCSSValueList* listA = resultList.get(); > + while (listA->mNext) { > + listA = listA->mNext; > + } > + > + listA->mNext = result.GetCSSValueSharedListValue()->mHead->Clone(); Do we really need to clone this? Looks like no other ones use it, and we reassign |listA| to |result|, so I think could we just re-use the list, instead of cloning it? Just like what you do for filter/shadow function list.

Comment on attachment 8820492 [details] Bug 1311620 - Part 10: Implement filter list addition. https://reviewboard.mozilla.org/r/99998/#review101174

Comment on attachment 8820493 [details] Bug 1311620 - Part 11: Implement shadow list addition. https://reviewboard.mozilla.org/r/100000/#review101176 ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:930 (Diff revision 1) > + target.style[idlName] = 'rgb(0, 0, 0) 0px 0px 0px'; > + var animation = > + target.animate({ [idlName]: [ 'rgb(120, 120, 120) 10px 10px 10px', > + 'rgb(120, 120, 120) 10px 10px 10px'] }, > + { duration: 1000, composite: 'add' }); > + testAnimationSamples(animation, idlName, > + [ { time: 0, expected: 'rgb(0, 0, 0) 0px 0px 0px, ' + > + 'rgb(120, 120, 120) 10px 10px 10px' }]); > + }, property + ': shadow'); > + }, > +}; > + > + > +const boxShadowListType = { > + testAddition: function(property, setup) { > + test(function(t) { > + var idlName = propertyToIDL(property); > + var target = createTestElement(t, setup); > + target.style[idlName] = 'rgb(0, 0, 0) 0px 0px 0px 0px'; > + var animation = > + target.animate({ [idlName]: [ 'rgb(120, 120, 120) 10px 10px 10px 0px', > + 'rgb(120, 120, 120) 10px 10px 10px 0px'] }, > + { duration: 1000, composite: 'add' }); > + testAnimationSamples(animation, idlName, > + [ { time: 0, expected: 'rgb(0, 0, 0) 0px 0px 0px 0px, ' + > + 'rgb(120, 120, 120) 10px 10px 10px 0px' }]); nit: I prefer to write the css value following the order what the spec says: text-shadow: `[ <length>{2,3} && <color>? ]#` box-shadow: `<shadow> = inset? && <length>{2,4} && <color>?` put the color last.

Comment on attachment 8820491 [details] Bug 1311620 - Part 9: Implement transform list addtion. . https://reviewboard.mozilla.org/r/99996/#review101172 > Do we really need to clone this? Looks like no other ones use it, and we reassign |listA| to |result|, so I think could we just re-use the list, instead of cloning it? Just like what you do for filter/shadow function list. Yes, we will see crashes without Clone() because transform function is in *shared* list.

Comment on attachment 8820493 [details] Bug 1311620 - Part 11: Implement shadow list addition. https://reviewboard.mozilla.org/r/100000/#review101176 > nit: > I prefer to write the css value following the order what the spec says: > > text-shadow: > `[ <length>{2,3} && <color>? ]#` > > box-shadow: > `<shadow> = inset? && <length>{2,4} && <color>?` > > put the color last. I prefer it too but unfortunately getComputedStyle() returns the color value first, so I did use this order to match it.

Thank you guys for reviewing! I addressed all the other comments.

Pushed by hiikezoe@mozilla-japan.org: https://hg.mozilla.org/integration/autoland/rev/daca5b41a06b Part 1: Test for effect/keyframe composite(add). r=birtles https://hg.mozilla.org/integration/autoland/rev/106e18770eaa Part 2: Don't call StyleAnimationValue::GetUnit() against uninitialized values, use IsNull() instead. r=birtles https://hg.mozilla.org/integration/autoland/rev/d36e5d3ad6d2 Part 3: Incorporate null_t in Animatable. r=birtles https://hg.mozilla.org/integration/autoland/rev/fa262658ebf3 Part 4: Implement keyframe composite(add). r=birtles https://hg.mozilla.org/integration/autoland/rev/0ee7b94357c4 Part 5: Implement effect composite(add). r=birtles https://hg.mozilla.org/integration/autoland/rev/2d339fab2cd0 Part 6: Fix test cases that checks keyframe composite is not specified but effect composite is specified. r=birtles https://hg.mozilla.org/integration/autoland/rev/d1576df1e756 Part 7: addition result tests per properties. r=boris https://hg.mozilla.org/integration/autoland/rev/24374bb04102 Part 8: Implement color addition. r=boris https://hg.mozilla.org/integration/autoland/rev/b90533d6027b Part 9: Implement transform list addtion. r=boris. https://hg.mozilla.org/integration/autoland/rev/7aef39429e47 Part 10: Implement filter list addition. r=boris https://hg.mozilla.org/integration/autoland/rev/45f9d3962703 Part 11: Implement shadow list addition. r=boris

https://hg.mozilla.org/mozilla-central/rev/daca5b41a06b https://hg.mozilla.org/mozilla-central/rev/106e18770eaa https://hg.mozilla.org/mozilla-central/rev/d36e5d3ad6d2 https://hg.mozilla.org/mozilla-central/rev/fa262658ebf3 https://hg.mozilla.org/mozilla-central/rev/0ee7b94357c4 https://hg.mozilla.org/mozilla-central/rev/2d339fab2cd0 https://hg.mozilla.org/mozilla-central/rev/d1576df1e756 https://hg.mozilla.org/mozilla-central/rev/24374bb04102 https://hg.mozilla.org/mozilla-central/rev/b90533d6027b https://hg.mozilla.org/mozilla-central/rev/7aef39429e47 https://hg.mozilla.org/mozilla-central/rev/45f9d3962703