Closed Bug 1291468 Opened 8 years ago Closed 8 years ago

Implement keyframe/effect composite(accumulate)

Categories

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

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(
URL
)

Details

Attachments

(3 files)

Bug 1291468 - Part 1: Tests for effect/keyframe composite(accumulate).
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1291468 - Part 2: Implement keyframe composite(accumulate).
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1291468 - Part 3: Implement effect composite(accumulate).
58 bytes, text/x-review-board-request
birtles
: review+
Details 
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.
Blocks: 1216844
Summary: Implement keyframe composition → Implement keyframe/effect composition(accumulate)
See Also: → 1311620
Depends on: 1305325
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Summary: Implement keyframe/effect composition(accumulate) → Implement keyframe/effect composite(accumulate)
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).
Attachment #8803159 - Flags: review?(bbirtles)
(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?
Attachment #8803160 - Flags: review?(bbirtles)
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.)
Attachment #8803161 - Flags: review?(bbirtles)
Depending on bug 1304886 to make Accumulate() easy.
Depends on: 1304886
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().
Blocks: 1320839
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
Attachment #8803159 - Flags: review?(bbirtles) → review+
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?
Attachment #8803160 - Flags: review?(bbirtles) → review+
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.)
Attachment #8803161 - Flags: review?(bbirtles)
Comment on attachment 8803161 [details]
Bug 1291468 - Part 3: Implement effect composite(accumulate).

https://reviewboard.mozilla.org/r/87408/#review97378
Attachment #8803161 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: