Skip to content

Commit

Permalink
Fix bug where composite: replace did not replace previous effects
Browse files Browse the repository at this point in the history
Consider a set of effects:

e.animate([{ "width": "0" }, { "width": "5px" }], { duration: 100 });
e.animate([{ "width": "0" }, { "width": "5px" }], { duration: 100, composite: 'add' });
e.animate([{ "width": "0" }, { "width": "2px" }], { duration: 100 });
e.animate([{ "width": "0" }, { "width": "2px" }], { duration: 100, composite: 'add' });

Previously the code in CopyToActiveInterpolationsMap would incorrectly
move the third effect to the start of the list to replace the original
keyframe pair, resulting in an effect stack of:

{ "width": "0" }, { "width": "2px" }
{ "width": "0" }, { "width": "5px" }
{ "width": "0" }, { "width": "2px" }

This is wrong; not only has it retained an effect it shouldn't have, it
has also re-ordered keyframes which might break non-commutative additive
properties. This CL fixes the logic to properly clear out existing
effects when a composite: 'replace' effect is put onto the stack.

Bug: 992378
Change-Id: I94ae54429ac7d4d28a0702d397ab64c2e45dee65
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1746302
Commit-Queue: Stephen McGruer <[email protected]>
Reviewed-by: Yi Gu <[email protected]>
Cr-Commit-Position: refs/heads/master@{#686024}
  • Loading branch information
stephenmcgruer authored and natechapin committed Aug 23, 2019
1 parent 0f58408 commit 53673e5
Showing 1 changed file with 29 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,33 @@
+ ` ${composite} of the effect`);
}

test(t => {
const div = createDiv(t);
const anims = [];
anims.push(div.animate({ marginLeft: ['10px', '20px'],
composite: 'replace' },
100));
anims.push(div.animate({ marginLeft: ['0px', '10px'],
composite: 'add' },
100));
// This should fully replace the previous effects.
anims.push(div.animate({ marginLeft: ['20px', '30px'],
composite: 'replace' },
100));
anims.push(div.animate({ marginLeft: ['30px', '40px'],
composite: 'add' },
100));

for (const anim of anims) {
anim.currentTime = 50;
}

// The result of applying the above effect stack is:
// underlying = 0.5 * 20 + 0.5 * 30 = 25
// result = 0.5 * (underlying + 30px) + 0.5 * (underlying + 40px)
// = 60
assert_equals(getComputedStyle(div).marginLeft, '60px',
'Animated style at 50%');
}, 'Composite replace fully replaces the underlying animation value');

</script>

0 comments on commit 53673e5

Please sign in to comment.