Hiroyuki Ikezoe (:hiro) Assignee
	Description • 8 years ago

      No description provided.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 1 • 8 years ago

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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 2 • 8 years ago

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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 3 • 8 years ago

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.

	Brian Birtles (:birtles)
	Comment 4 • 8 years ago

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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 5 • 8 years ago

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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 17 • 8 years ago
mozreview-review

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.

	Brian Birtles (:birtles)
	Comment 18 • 8 years ago
mozreview-review

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

	Brian Birtles (:birtles)
	Comment 19 • 8 years ago
mozreview-review

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?)

	Brian Birtles (:birtles)
	Comment 20 • 8 years ago
mozreview-review

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/

	Brian Birtles (:birtles)
	Comment 21 • 8 years ago
mozreview-review

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.

	Brian Birtles (:birtles)
	Comment 22 • 8 years ago
mozreview-review

Comment on attachment 8820487 [details]
Bug 1311620 - Part 5: Implement effect composite(add).

https://reviewboard.mozilla.org/r/99988/#review100914

	Brian Birtles (:birtles)
	Comment 23 • 8 years ago
mozreview-review

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

	Boris Chiou [:boris]
	Comment 24 • 8 years ago
mozreview-review

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.
// [ ..... ]

	Boris Chiou [:boris]
	Comment 25 • 8 years ago
mozreview-review

Comment on attachment 8820490 [details]
Bug 1311620 - Part 8: Implement color addition.

https://reviewboard.mozilla.org/r/99994/#review101168

	Boris Chiou [:boris]
	Comment 26 • 8 years ago
mozreview-review

Comment on attachment 8820491 [details]
Bug 1311620 - Part 9: Implement transform list addtion. .

https://reviewboard.mozilla.org/r/99996/#review101170

	Boris Chiou [:boris]
	Comment 27 • 8 years ago
mozreview-review

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.

	Boris Chiou [:boris]
	Comment 28 • 8 years ago
mozreview-review

Comment on attachment 8820492 [details]
Bug 1311620 - Part 10: Implement filter list addition.

https://reviewboard.mozilla.org/r/99998/#review101174

	Boris Chiou [:boris]
	Comment 29 • 8 years ago
mozreview-review

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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 30 • 8 years ago
mozreview-review-reply

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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 31 • 8 years ago
mozreview-review-reply

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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 43 • 8 years ago

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

	Pulsebot
	Comment 44 • 8 years ago

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

	Phil Ringnalda (:philor)
	Comment 45 • 8 years ago
bugherder

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