Hrm, I'm unsure what the correct 'web-compat' approach is here; is it better to clearly throw for as-yet unsupported content, or to (essentially) silently fail? flackr@?

Regarding the composite:'add' throwing; as far as I'm aware that should already be throwing for per-keyframe composite. We don't support additive compositing on a per-keyframe basis. My changes only landed in 65, but in 64 this test already fails: http://output.jsbin.com/wilowaj . I will look at the diff of my changes and try to see how this may have changed.

To be clear; we don't support additive animations unless CSSAdditiveAnimationsEnabled is set to true, and that is false by default I believe.

I realized that one change, http://crrev.com/41658d, did land in M64 which is now stable.

I have also found something strange. From my knowledge, http://output.jsbin.com/qekivej shouldn't work in either form (object or array), because there is an 'add' composite on the KeyframeEffect which isn't supported unless CSSAdditiveAnimations is enabled. However, in my stable browser today the object form works!

I'm going to do a bisect backwards before M64 to see if there is a change in behavior there (perhaps both object and list form used to work, both I assume incorrectly?).

Confirmed; before http://crrev.com/41658d you could specify composite:'add' as a KeyframeEffect options member without the code throwing for both object and array form. Afterwards that only works for object form (which, as noted above, is strange).

There are two next steps here:

i) Determine why the CL has differing behavior for object and array form. That's on me.

ii) Determine what CSSAdditiveAnimations was meant to cover as a flag. Particularly: was it meant to control composite: 'add' on KeyframeEffect options, or only on keyframe-specific composites.

cc-ing alancutter@ for (ii). Alan, can you give some history about CSSAdditiveAnimations as a flag, including why it still exists and defaults to false?

The crrev link in #4 is broken.

Whoops; 6 characters does not a unique SHA make. Let's be safe: http://crrev.com/41658d028f33d2ce5992165844e4dad9ff2015f5

It turns out that my CL made it illegal for both forms, I just messed up the jsbin test case (new testcase is http://output.jsbin.com/vezehev). Remaining question is why it used to not be illegal in the code, investigating that now.

Ah, the simple answer is that before my CL, KeyframeEffectOptions did not have a composite member. One could specify:

element.animate(keyframes, { duration: 2000, composite: 'add' });

But the composite would just be silently dropped and we would do replace semantics instead.

You can see this by loading http://output.jsbin.com/jaqumof in Chrome 63; it won't throw but both animations will render exactly the same (using replacement composite). If you instead load it in Chrome 64 and pass --enable-experimental-web-platform-features, you will see that the animations render differently (as one is additive and one is replacement).


So turns out both issues are a question of - should we throw or should we console warn!

Re #4 (ii): The CSSAdditiveAnimations flag should cover composite: 'add' in animations in general. For KeyframeEffect options we simply haven't implemented it yet.
As for why it's not shipped we had decided not to ship until the polyfill was ready to support it, see https://bugs.chromium.org/p/chromium/issues/detail?id=416736#c8. Given that Firefox has shipped it and our recent discussions about what we want out of the polyfill this can be reconsidered.

Thanks Alan. To be clear, the current status for KeyframeEffectOptions::composite in various browsers is:

Firefox stable: Parses the enum (will throw for invalid values), but converts all valid values to 'replace'.

Firefox nightly: Parses the enum (will throw for invalid values), appears to support all valid values ('add', 'replace', 'accumulate').

Chrome M63 (no flags): Does not parse the enum. Anything specified will be ignored and silently converted to 'replace'.

Chrome M64+ (no flags): Parses the enum (will throw for invalid values), and also throws for valid values 'add' and 'accumulate'.

Chrome M64+ (CSSAdditiveAnimations flag enabled): Parses the enum (will throw for invalid values), supports 'replace' and 'add' values. Throws for valid value 'accumulate'.




So I think we should not ship 'add' flagless yet, since Firefox hasn't either, but the question is whether I should send out a CL that makes specifying 'add' for KeyframeEffectOptions::composite a silent failure again. I think I should given that this was backwards breaking between M63 and M64?

The list in comment #10 makes thing much clear to me. Thanks!

An alternative is to actually not parse the Composite if the flag is not enabled?
The composite value is only useful with the flag.

In any case, I agree that we should try to fix the breakage from M63 to M64 that is caused by throwing.

I don't like the idea of having unsupported KeyframeEffectOptions::composite modes handled differently than keyframe specific composite modes. I think we should also make the keyframe specific one silently fallback to replace when the requested mode isn't supported.

Ack. That would be a breaking change from our current API (going from throwing to not throwing); do we want/need to do anything special for that?

I will send out the two CLs separately, as I'm hoping to merge the backwards-behavior-fixing part back as it has broken developers.

Sounds good.

We should send out an intent thread, not only will this change make us more self consistent but it's also consistent with the behavior in Firefox so hopefully shouldn't be too breaking.

Intent thread posted: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/49iInq8luvo

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/823e14d71a44eeeda943e6a13d0b2a97739cbee2

commit 823e14d71a44eeeda943e6a13d0b2a97739cbee2
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Fri Feb 02 18:02:47 2018

Only throw for KeyframeEffectOptions::composite in the right cases

In http://crrev.com/41658d0 we added support for the composite member of
KeyframeEffectOptions, but the CL accidentally introduced a
backwards-compatibility bug. Prior to that CL we would not throw for
composite == 'add' or composite == 'accumulate', whereas after it we
would.

Bug:  806139 
Change-Id: I82271c4f5b7d7756cbaac101825a22eb12367f8d
Reviewed-on: https://chromium-review.googlesource.com/895049
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534091}
[add] https://crrev.com/823e14d71a44eeeda943e6a13d0b2a97739cbee2/third_party/WebKit/LayoutTests/animations/keyframe-effect-options-composite-parsing.html
[modify] https://crrev.com/823e14d71a44eeeda943e6a13d0b2a97739cbee2/third_party/WebKit/LayoutTests/external/wpt/web-animations/animation-model/combining-effects/effect-composition-expected.txt
[modify] https://crrev.com/823e14d71a44eeeda943e6a13d0b2a97739cbee2/third_party/WebKit/Source/core/animation/EffectModel.cpp
[modify] https://crrev.com/823e14d71a44eeeda943e6a13d0b2a97739cbee2/third_party/WebKit/Source/core/animation/EffectModel.h
[modify] https://crrev.com/823e14d71a44eeeda943e6a13d0b2a97739cbee2/third_party/WebKit/Source/core/animation/ElementAnimation.cpp
[modify] https://crrev.com/823e14d71a44eeeda943e6a13d0b2a97739cbee2/third_party/WebKit/Source/core/animation/KeyframeEffect.cpp
[modify] https://crrev.com/823e14d71a44eeeda943e6a13d0b2a97739cbee2/third_party/WebKit/Source/core/animation/KeyframeEffectReadOnly.cpp
[modify] https://crrev.com/823e14d71a44eeeda943e6a13d0b2a97739cbee2/third_party/WebKit/Source/platform/runtime_enabled_features.json5

Requesting permission to merge http://crrev.com/823e14d71a44eeeda943e6a13d0b2a97739cbee2 to M65. This CL fixes a regression that occurred from M63 to M64, but which was sadly not discovered until late January.

Specifically, Chrome began throwing Javascript exceptions for an Animations API where we did not previously, which breaks backwards compatibility. The original change was not easily revertable, so instead I landed this fix CL.

Is this CL safe enough to be merged into the branch?

Yes; the CL is minor and has a set of regression tests that are passing. It soaked successfully in canary over the weekend.

Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9c4a93cf6eedd067b8d79f691d7d572166ffd061

commit 9c4a93cf6eedd067b8d79f691d7d572166ffd061
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Tue Feb 06 19:22:26 2018

Only throw for KeyframeEffectOptions::composite in the right cases

In http://crrev.com/41658d0 we added support for the composite member of
KeyframeEffectOptions, but the CL accidentally introduced a
backwards-compatibility bug. Prior to that CL we would not throw for
composite == 'add' or composite == 'accumulate', whereas after it we
would.

Bug:  806139 
Change-Id: I82271c4f5b7d7756cbaac101825a22eb12367f8d
Reviewed-on: https://chromium-review.googlesource.com/895049
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#534091}(cherry picked from commit 823e14d71a44eeeda943e6a13d0b2a97739cbee2)
Reviewed-on: https://chromium-review.googlesource.com/904562
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#348}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[add] https://crrev.com/9c4a93cf6eedd067b8d79f691d7d572166ffd061/third_party/WebKit/LayoutTests/animations/keyframe-effect-options-composite-parsing.html
[modify] https://crrev.com/9c4a93cf6eedd067b8d79f691d7d572166ffd061/third_party/WebKit/LayoutTests/external/wpt/web-animations/animation-model/combining-effects/effect-composition-expected.txt
[modify] https://crrev.com/9c4a93cf6eedd067b8d79f691d7d572166ffd061/third_party/WebKit/Source/core/animation/EffectModel.cpp
[modify] https://crrev.com/9c4a93cf6eedd067b8d79f691d7d572166ffd061/third_party/WebKit/Source/core/animation/EffectModel.h
[modify] https://crrev.com/9c4a93cf6eedd067b8d79f691d7d572166ffd061/third_party/WebKit/Source/core/animation/ElementAnimation.cpp
[modify] https://crrev.com/9c4a93cf6eedd067b8d79f691d7d572166ffd061/third_party/WebKit/Source/core/animation/KeyframeEffect.cpp
[modify] https://crrev.com/9c4a93cf6eedd067b8d79f691d7d572166ffd061/third_party/WebKit/Source/core/animation/KeyframeEffectReadOnly.cpp
[modify] https://crrev.com/9c4a93cf6eedd067b8d79f691d7d572166ffd061/third_party/WebKit/Source/platform/runtime_enabled_features.json5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0702168a17982081139b2ffbb7455f54d3b524b0

commit 0702168a17982081139b2ffbb7455f54d3b524b0
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Fri Feb 16 14:21:07 2018

Silently convert unsupported composite values to kCompositeReplace

This consolidates Chrome's behavior to be consistent everywhere
CompositeOperations are parsed, and to match Firefox. When a
CompositeOperation is found that we don't support, it is silently
replaced with kCompositeReplace.

When running without WebAnimationsAPI, developers should only expect to
have kCompositeReplace animations. When running with WebAnimationsAPI,
developers can detect which composite is in use by either querying:

  effect.composite;

  OR

  effect.getKeyframes(); // Check each keyframe for a keyframe-specific composite value.

Intent thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/wRsafuGKr5w

Bug:  806139 
Change-Id: I80fefff3fc833b62787c57d22cbec656365ec374
Reviewed-on: https://chromium-review.googlesource.com/896807
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537300}
[modify] https://crrev.com/0702168a17982081139b2ffbb7455f54d3b524b0/third_party/WebKit/LayoutTests/animations/svg/svg-composition-ignores-css-composition-flag.html
[modify] https://crrev.com/0702168a17982081139b2ffbb7455f54d3b524b0/third_party/WebKit/LayoutTests/external/wpt/web-animations/animation-model/combining-effects/effect-composition-expected.txt
[modify] https://crrev.com/0702168a17982081139b2ffbb7455f54d3b524b0/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/Animatable/animate-expected.txt
[modify] https://crrev.com/0702168a17982081139b2ffbb7455f54d3b524b0/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/constructor-expected.txt
[modify] https://crrev.com/0702168a17982081139b2ffbb7455f54d3b524b0/third_party/WebKit/LayoutTests/virtual/stable/web-animations-api/additive-animations-unsupported.html
[modify] https://crrev.com/0702168a17982081139b2ffbb7455f54d3b524b0/third_party/WebKit/Source/core/animation/EffectInput.cpp
[modify] https://crrev.com/0702168a17982081139b2ffbb7455f54d3b524b0/third_party/WebKit/Source/core/animation/EffectModel.cpp
[modify] https://crrev.com/0702168a17982081139b2ffbb7455f54d3b524b0/third_party/WebKit/Source/core/animation/EffectModel.h
[modify] https://crrev.com/0702168a17982081139b2ffbb7455f54d3b524b0/third_party/WebKit/Source/core/animation/ElementAnimation.cpp
[modify] https://crrev.com/0702168a17982081139b2ffbb7455f54d3b524b0/third_party/WebKit/Source/core/animation/KeyframeEffect.cpp
[modify] https://crrev.com/0702168a17982081139b2ffbb7455f54d3b524b0/third_party/WebKit/Source/core/animation/KeyframeEffectReadOnly.cpp
[modify] https://crrev.com/0702168a17982081139b2ffbb7455f54d3b524b0/third_party/WebKit/Source/core/animation/StringKeyframe.cpp
[modify] https://crrev.com/0702168a17982081139b2ffbb7455f54d3b524b0/third_party/WebKit/Source/core/animation/StringKeyframe.h