Closed Bug 1311620 Opened 3 years ago Closed 3 years ago

Implement keyframe/effect composite(add)

Categories

(Core :: DOM: Animation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(11 files)

58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
boris
: review+
Details
58 bytes, text/x-review-board-request
boris
: review+
Details
58 bytes, text/x-review-board-request
boris
: review+
Details
58 bytes, text/x-review-board-request
boris
: review+
Details
59 bytes, text/x-review-board-request
boris
: review+
Details
No description provided.
Depends on: 1305325
Priority: -- → P3
If we could land bug 1312301 before this, unlike bug 1320839 I will add test cases in this bug .
Depends on: 1312301
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!
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
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
Attachment #8820483 - Flags: review?(bbirtles) → 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?)
Attachment #8820484 - Flags: review?(bbirtles) → 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/
Attachment #8820485 - Flags: review?(bbirtles) → 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.
Attachment #8820486 - Flags: review?(bbirtles) → review+
Comment on attachment 8820487 [details]
Bug 1311620 - Part 5: Implement effect composite(add).

https://reviewboard.mozilla.org/r/99988/#review100914
Attachment #8820487 - Flags: review?(bbirtles) → 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
Attachment #8820488 - Flags: review?(bbirtles) → 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.
// [ ..... ]
Attachment #8820489 - Flags: review?(boris.chiou) → review+
Comment on attachment 8820490 [details]
Bug 1311620 - Part 8: Implement color addition.

https://reviewboard.mozilla.org/r/99994/#review101168
Attachment #8820490 - Flags: review?(boris.chiou) → review+
Comment on attachment 8820491 [details]
Bug 1311620 - Part 9: Implement transform list addtion. .

https://reviewboard.mozilla.org/r/99996/#review101170
Attachment #8820491 - Flags: review?(boris.chiou) → 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.
Comment on attachment 8820492 [details]
Bug 1311620 - Part 10: Implement filter list addition.

https://reviewboard.mozilla.org/r/99998/#review101174
Attachment #8820492 - Flags: review?(boris.chiou) → 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.
Attachment #8820493 - Flags: review?(boris.chiou) → review+
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
Depends on: 1330513
You need to log in before you can comment on or make changes to this bug.