Commit 0702168a authored by Stephen McGruer's avatar Stephen McGruer Committed by Commit Bot

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: default avatarRobert Flack <flackr@chromium.org>
Reviewed-by: default avatarMajid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537300}
parent d935a150
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<div id="cssTarget"></div>
<div id="cssTarget" style="color: red;"></div>
<svg>
<rect id="svgTarget" color="red"></rect>
</svg>
......@@ -11,14 +11,13 @@ internals.disableCSSAdditiveAnimations();
test(() => {
assert_throws('NotSupportedError', () => {
cssTarget.animate({color: 'red'});
});
assert_throws('NotSupportedError', () => {
cssTarget.animate([
{color: 'red'},
{color: 'red', composite: 'add'},
]);
cssTarget.animate({color: 'green'});
});
var animation = cssTarget.animate([
{color: 'green', composite: 'add'},
{color: 'green', composite: 'add'},
], { fill: 'forwards' });
assert_equals(getComputedStyle(cssTarget).color, 'rgb(0, 128, 0)');
}, 'Precheck that disabling CSS additive animations works.');
test(() => {
......
This is a testharness.js-based test.
FAIL accumulate onto the base value Failed to execute 'animate' on 'Element': Invalid composite value: 'accumulate'
FAIL accumulate onto an underlying animation value Failed to execute 'animate' on 'Element': Invalid composite value: 'accumulate'
FAIL Composite when mixing accumulate and replace Failed to execute 'animate' on 'Element': Invalid composite value: 'accumulate'
FAIL accumulate onto the base value assert_equals: Animated margin-left style at 50% expected "15px" but got "5px"
FAIL accumulate onto an underlying animation value assert_equals: Animated style at 50% expected "20px" but got "5px"
FAIL Composite when mixing accumulate and replace assert_equals: Animated style at 50% expected "25px" but got "20px"
FAIL accumulate specified on a keyframe overrides the composite mode of the effect assert_equals: Animated style at 50% expected "20px" but got "15px"
FAIL unspecified composite mode on a keyframe is overriden by setting accumulate of the effect assert_equals: Animated style at 50% expected "20px" but got "15px"
PASS add onto the base value
......
This is a testharness.js-based test.
Found 121 tests; 110 PASS, 11 FAIL, 0 TIMEOUT, 0 NOTRUN.
Found 121 tests; 111 PASS, 10 FAIL, 0 TIMEOUT, 0 NOTRUN.
PASS Element.animate() creates an Animation object
PASS Element.animate() creates an Animation object in the relevant realm of the target element
PASS Element.animate() creates an Animation object with a KeyframeEffect
......@@ -41,9 +41,9 @@ PASS Element.animate() accepts a property-indexed keyframe with a single-element
PASS Element.animate() accepts a property-indexed keyframe with an empty array of easings
PASS Element.animate() accepts a property-indexed keyframe with an array of easings that is too long
PASS Element.animate() accepts a property-indexed keyframe with a single composite operation
FAIL Element.animate() accepts a property-indexed keyframe with a composite array Failed to execute 'animate' on 'Element': Invalid composite value: 'accumulate'
FAIL Element.animate() accepts a property-indexed keyframe with a composite array assert_equals: value for 'composite' on ComputedKeyframe #2 expected "accumulate" but got "replace"
PASS Element.animate() accepts a property-indexed keyframe with a composite array that is too short
FAIL Element.animate() accepts a property-indexed keyframe with a composite array that is too long Failed to execute 'animate' on 'Element': Invalid composite value: 'accumulate'
PASS Element.animate() accepts a property-indexed keyframe with a composite array that is too long
PASS Element.animate() accepts a property-indexed keyframe with a single-element composite array
PASS Element.animate() accepts a one property one keyframe sequence
PASS Element.animate() accepts a one property two keyframe sequence
......
This is a testharness.js-based test.
Found 167 tests; 150 PASS, 17 FAIL, 0 TIMEOUT, 0 NOTRUN.
Found 167 tests; 153 PASS, 14 FAIL, 0 TIMEOUT, 0 NOTRUN.
PASS A KeyframeEffectReadOnly can be constructed with no frames
PASS easing values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in KeyframeEffectOptions
PASS Invalid easing values are correctly rejected when passed to the KeyframeEffectReadOnly constructor in KeyframeEffectOptions
FAIL composite values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in property-indexed keyframes Failed to construct 'KeyframeEffectReadOnly': Invalid composite value: 'accumulate'
FAIL composite values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in regular keyframes Failed to construct 'KeyframeEffectReadOnly': Invalid composite value: 'accumulate'
FAIL composite values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in property-indexed keyframes assert_equals: resulting composite for 'accumulate' expected "accumulate" but got "replace"
FAIL composite values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in regular keyframes assert_equals: resulting composite for 'accumulate' expected "accumulate" but got "replace"
PASS composite value is null if the composite operation specified on the keyframe effect is being used
PASS A KeyframeEffectReadOnly can be constructed with a one property two value property-indexed keyframes specification
PASS A KeyframeEffectReadOnly constructed with a one property two value property-indexed keyframes specification roundtrips
......@@ -72,12 +72,12 @@ PASS A KeyframeEffectReadOnly can be constructed with a property-indexed keyfram
PASS A KeyframeEffectReadOnly constructed with a property-indexed keyframe with an array of easings that is too long roundtrips
PASS A KeyframeEffectReadOnly can be constructed with a property-indexed keyframe with a single composite operation
PASS A KeyframeEffectReadOnly constructed with a property-indexed keyframe with a single composite operation roundtrips
FAIL A KeyframeEffectReadOnly can be constructed with a property-indexed keyframe with a composite array Failed to construct 'KeyframeEffectReadOnly': Invalid composite value: 'accumulate'
FAIL A KeyframeEffectReadOnly constructed with a property-indexed keyframe with a composite array roundtrips Failed to construct 'KeyframeEffectReadOnly': Invalid composite value: 'accumulate'
FAIL A KeyframeEffectReadOnly can be constructed with a property-indexed keyframe with a composite array assert_equals: value for 'composite' on ComputedKeyframe #2 expected "accumulate" but got "replace"
PASS A KeyframeEffectReadOnly constructed with a property-indexed keyframe with a composite array roundtrips
PASS A KeyframeEffectReadOnly can be constructed with a property-indexed keyframe with a composite array that is too short
PASS A KeyframeEffectReadOnly constructed with a property-indexed keyframe with a composite array that is too short roundtrips
FAIL A KeyframeEffectReadOnly can be constructed with a property-indexed keyframe with a composite array that is too long Failed to construct 'KeyframeEffectReadOnly': Invalid composite value: 'accumulate'
FAIL A KeyframeEffectReadOnly constructed with a property-indexed keyframe with a composite array that is too long roundtrips Failed to construct 'KeyframeEffectReadOnly': Invalid composite value: 'accumulate'
PASS A KeyframeEffectReadOnly can be constructed with a property-indexed keyframe with a composite array that is too long
PASS A KeyframeEffectReadOnly constructed with a property-indexed keyframe with a composite array that is too long roundtrips
PASS A KeyframeEffectReadOnly can be constructed with a property-indexed keyframe with a single-element composite array
PASS A KeyframeEffectReadOnly constructed with a property-indexed keyframe with a single-element composite array roundtrips
PASS A KeyframeEffectReadOnly can be constructed with a one property one keyframe sequence
......
......@@ -25,8 +25,8 @@ test(function() {
test(function() {
var replaceReplaceKeyframes = [
{width: '0px', composite: 'replace'},
{width: '100px', composite: 'replace'}
{color: 'blue', composite: 'replace'},
{color: 'green', composite: 'replace'}
];
assert_not_equals(element.animate(replaceReplaceKeyframes), undefined);
......@@ -34,28 +34,32 @@ test(function() {
test(function() {
var addAddKeyframes = [
{width: '0px', composite: 'add'},
{width: '100px', composite: 'add'}
{color: 'blue', composite: 'add'},
{color: 'green', composite: 'add'}
];
assert_throws('NOT_SUPPORTED_ERR', function() { element.animate(addAddKeyframes); });
}, 'Calling element.animate() with an add -> add animation should throw a NotSupportedError.');
element.animate(addAddKeyframes, {fill: 'forwards'});
assert_equals(getComputedStyle(element).color, 'rgb(0, 128, 0)');
}, 'Calling element.animate() with an add -> add animation should result in a replace -> replace animation.');
test(function() {
var replaceAddKeyframes = [
{width: '0px', composite: 'replace'},
{width: '100px', composite: 'add'}
{color: 'blue', composite: 'replace'},
{color: 'green', composite: 'add'}
];
assert_throws('NOT_SUPPORTED_ERR', function() { element.animate(replaceAddKeyframes); });
}, 'Calling element.animate() with a replace -> add animation should throw a NotSupportedError.');
element.animate(replaceAddKeyframes, {fill: 'forwards'});
assert_equals(getComputedStyle(element).color, 'rgb(0, 128, 0)');
}, 'Calling element.animate() with a replace -> add animation should result in a replace -> replace animation.');
test(function() {
var addReplaceKeyframes = [
{width: '0px', composite: 'add'},
{width: '100px', composite: 'replace'}
{color: 'blue', composite: 'add'},
{color: 'green', composite: 'replace'}
];
assert_throws('NOT_SUPPORTED_ERR', function() { element.animate(addReplaceKeyframes); });
}, 'Calling element.animate() with an add -> replace animation should throw a NotSupportedError.');
const anim = element.animate(addReplaceKeyframes, {fill: 'backwards'});
anim.reverse();
assert_equals(getComputedStyle(element).color, 'rgb(0, 0, 255)');
}, 'Calling element.animate() with an add -> replace animation should result in a replace -> replace animation.');
</script>
......@@ -60,12 +60,8 @@ namespace {
// Converts the composite property of a BasePropertyIndexedKeyframe into a
// vector of EffectModel::CompositeOperation enums.
//
// If the composite property cannot be extracted or parsed for some reason, an
// exception will be thrown in |exception_state|.
Vector<WTF::Optional<EffectModel::CompositeOperation>> ParseCompositeProperty(
const BasePropertyIndexedKeyframe& keyframe,
ExceptionState& exception_state) {
const BasePropertyIndexedKeyframe& keyframe) {
const CompositeOperationOrCompositeOperationOrNullSequence& composite =
keyframe.composite();
......@@ -75,14 +71,8 @@ Vector<WTF::Optional<EffectModel::CompositeOperation>> ParseCompositeProperty(
return {WTF::nullopt};
if (composite.IsCompositeOperation()) {
EffectModel::CompositeOperation composite_operation;
if (!EffectModel::StringToCompositeOperation(
composite.GetAsCompositeOperation(), composite_operation,
&exception_state)) {
DCHECK(exception_state.HadException());
return {};
}
return {composite_operation};
return {EffectModel::StringToCompositeOperation(
composite.GetAsCompositeOperation())};
}
Vector<WTF::Optional<EffectModel::CompositeOperation>> result;
......@@ -91,14 +81,8 @@ Vector<WTF::Optional<EffectModel::CompositeOperation>> ParseCompositeProperty(
if (composite_operation_string.IsNull()) {
result.push_back(WTF::nullopt);
} else {
EffectModel::CompositeOperation composite_operation;
if (!EffectModel::StringToCompositeOperation(composite_operation_string,
composite_operation,
&exception_state)) {
DCHECK(exception_state.HadException());
return {};
}
result.push_back(composite_operation);
result.push_back(
EffectModel::StringToCompositeOperation(composite_operation_string));
}
}
return result;
......@@ -151,6 +135,34 @@ void SetKeyframeValue(Element& element,
keyframe.SetSVGAttributeValue(*svg_attribute, value);
}
// Ensures that a CompositeOperation is of an allowed value for a given
// StringKeyframe and the current runtime flags.
EffectModel::CompositeOperation ResolveCompositeOperation(
EffectModel::CompositeOperation composite,
const scoped_refptr<StringKeyframe>& keyframe) {
if (!RuntimeEnabledFeatures::CSSAdditiveAnimationsEnabled() &&
keyframe->HasCssProperty() && composite == EffectModel::kCompositeAdd) {
return EffectModel::kCompositeReplace;
}
return composite;
}
// Ensures that a CompositeOperation is of an allowed value for a set of
// StringKeyframes and the current runtime flags.
EffectModel::CompositeOperation ResolveCompositeOperation(
EffectModel::CompositeOperation composite,
const StringKeyframeVector& keyframes) {
EffectModel::CompositeOperation result = composite;
for (const scoped_refptr<StringKeyframe>& keyframe : keyframes) {
// Replace is always supported, so we can early-exit if and when we have
// that as our composite value.
if (result == EffectModel::kCompositeReplace)
break;
result = ResolveCompositeOperation(result, keyframe);
}
return result;
}
KeyframeEffectModelBase* CreateEmptyEffectModel(
EffectModel::CompositeOperation composite) {
return StringKeyframeEffectModel::Create(StringKeyframeVector(), composite);
......@@ -161,6 +173,8 @@ KeyframeEffectModelBase* CreateEffectModel(
const StringKeyframeVector& keyframes,
EffectModel::CompositeOperation composite,
ExceptionState& exception_state) {
composite = ResolveCompositeOperation(composite, keyframes);
StringKeyframeEffectModel* keyframe_effect_model =
StringKeyframeEffectModel::Create(keyframes, composite,
LinearTimingFunction::Shared());
......@@ -177,11 +191,8 @@ KeyframeEffectModelBase* CreateEffectModel(
kNotSupportedError, "Partial keyframes are not supported.");
return nullptr;
}
if (keyframe->Composite() != EffectModel::kCompositeReplace) {
exception_state.ThrowDOMException(
kNotSupportedError, "Additive animations are not supported.");
return nullptr;
}
// This should be enforced by the parsing code.
DCHECK(keyframe->Composite() != EffectModel::kCompositeAdd);
}
}
}
......@@ -189,7 +200,6 @@ KeyframeEffectModelBase* CreateEffectModel(
DCHECK(!exception_state.HadException());
return keyframe_effect_model;
}
} // namespace
// Implements "Processing a keyframes argument" from the web-animations spec.
......@@ -337,16 +347,6 @@ KeyframeEffectModelBase* EffectInput::ConvertArrayForm(
keyframe->SetOffset(processed_keyframe.base_keyframe.offset());
}
if (processed_keyframe.base_keyframe.hasComposite()) {
EffectModel::CompositeOperation composite_operation;
if (!EffectModel::StringToCompositeOperation(
processed_keyframe.base_keyframe.composite(), composite_operation,
&exception_state)) {
return nullptr;
}
keyframe->SetComposite(composite_operation);
}
// 8.1. For each property-value pair in frame, parse the property value
// using the syntax specified for that property.
for (const auto& pair : processed_keyframe.property_value_pairs) {
......@@ -355,6 +355,13 @@ KeyframeEffectModelBase* EffectInput::ConvertArrayForm(
execution_context);
}
if (processed_keyframe.base_keyframe.hasComposite()) {
keyframe->SetComposite(ResolveCompositeOperation(
EffectModel::StringToCompositeOperation(
processed_keyframe.base_keyframe.composite()),
keyframe));
}
// 8.2. Let the timing function of frame be the result of parsing the
// “easing” property on frame using the CSS syntax defined for the easing
// property of the AnimationEffectTimingReadOnly interface.
......@@ -450,9 +457,7 @@ KeyframeEffectModelBase* EffectInput::ConvertObjectForm(
easings = property_indexed_keyframe.easing().GetAsStringSequence();
Vector<WTF::Optional<EffectModel::CompositeOperation>> composite_operations =
ParseCompositeProperty(property_indexed_keyframe, exception_state);
if (exception_state.HadException())
return nullptr;
ParseCompositeProperty(property_indexed_keyframe);
// Next extract all animatable properties from the input argument and iterate
// through them, processing each as a list of values for that property. This
......@@ -591,8 +596,10 @@ KeyframeEffectModelBase* EffectInput::ConvertObjectForm(
// many items as property keyframes.
WTF::Optional<EffectModel::CompositeOperation> composite =
composite_operations[i % composite_operations.size()];
if (composite)
keyframe->SetComposite(composite.value());
if (composite) {
keyframe->SetComposite(
ResolveCompositeOperation(composite.value(), keyframe));
}
}
results.push_back(keyframe);
......
......@@ -9,41 +9,17 @@
#include "platform/runtime_enabled_features.h"
namespace blink {
EffectModel::CompositeOperation EffectModel::ExtractCompositeOperation(
const KeyframeEffectOptions& options) {
// We have to be careful to keep backwards compatible behavior here; no valid
// value of KeyframeEffectOptions should throw, even if it is unsupported in
// Chrome currently. Values we do not support should just be turned into
// kCompositeReplace.
//
// See http://crbug.com/806139.
if (RuntimeEnabledFeatures::CSSAdditiveAnimationsEnabled() &&
options.composite() == "add") {
EffectModel::CompositeOperation EffectModel::StringToCompositeOperation(
const String& composite_string) {
DCHECK(composite_string == "replace" || composite_string == "add" ||
composite_string == "accumulate");
if (composite_string == "add") {
return kCompositeAdd;
}
// TODO(crbug.com/788440): Support accumulate.
return kCompositeReplace;
}
bool EffectModel::StringToCompositeOperation(String composite_string,
CompositeOperation& result,
ExceptionState* exception_state) {
// TODO(crbug.com/788440): Once all CompositeOperations are supported we can
// just DCHECK the input and directly convert it instead of handling failure.
if (composite_string == "add") {
result = kCompositeAdd;
return true;
}
if (composite_string == "replace") {
result = kCompositeReplace;
return true;
}
if (exception_state) {
exception_state->ThrowTypeError("Invalid composite value: '" +
composite_string + "'");
}
return false;
}
String EffectModel::CompositeOperationToString(CompositeOperation composite) {
switch (composite) {
case EffectModel::kCompositeAdd:
......
......@@ -39,9 +39,7 @@
namespace blink {
class ExceptionState;
class Interpolation;
class KeyframeEffectOptions;
// Time independent representation of an Animation's content.
// Can be sampled for the active pairs of Keyframes (represented by
......@@ -52,13 +50,7 @@ class CORE_EXPORT EffectModel : public GarbageCollectedFinalized<EffectModel> {
kCompositeReplace,
kCompositeAdd,
};
// Returns the correct CompositeOperation for a KeyframeEffectOptions,
// respecting any relevant RuntimeFeatures that are enabled or disabled.
static CompositeOperation ExtractCompositeOperation(
const KeyframeEffectOptions&);
static bool StringToCompositeOperation(String,
CompositeOperation&,
ExceptionState* = nullptr);
static CompositeOperation StringToCompositeOperation(const String&);
static String CompositeOperationToString(CompositeOperation);
EffectModel() = default;
......
......@@ -28,8 +28,8 @@ Animation* ElementAnimation::animate(
ExceptionState& exception_state) {
EffectModel::CompositeOperation composite = EffectModel::kCompositeReplace;
if (options.IsKeyframeAnimationOptions()) {
composite = EffectModel::ExtractCompositeOperation(
options.GetAsKeyframeAnimationOptions());
composite = EffectModel::StringToCompositeOperation(
options.GetAsKeyframeAnimationOptions().composite());
}
KeyframeEffectModelBase* effect = EffectInput::Convert(
......
......@@ -71,8 +71,8 @@ KeyframeEffect* KeyframeEffect::Create(
EffectModel::CompositeOperation composite = EffectModel::kCompositeReplace;
if (options.IsKeyframeEffectOptions()) {
composite = EffectModel::ExtractCompositeOperation(
options.GetAsKeyframeEffectOptions());
composite = EffectModel::StringToCompositeOperation(
options.GetAsKeyframeEffectOptions().composite());
}
KeyframeEffectModelBase* model = EffectInput::Convert(
......@@ -120,9 +120,8 @@ KeyframeEffect::KeyframeEffect(Element* target,
KeyframeEffect::~KeyframeEffect() = default;
void KeyframeEffect::setComposite(String composite_string) {
EffectModel::CompositeOperation composite;
if (EffectModel::StringToCompositeOperation(composite_string, composite))
Model()->SetComposite(composite);
Model()->SetComposite(
EffectModel::StringToCompositeOperation(composite_string));
}
AnimationEffectTiming* KeyframeEffect::timing() {
......
......@@ -56,8 +56,8 @@ KeyframeEffectReadOnly* KeyframeEffectReadOnly::Create(
EffectModel::CompositeOperation composite = EffectModel::kCompositeReplace;
if (options.IsKeyframeEffectOptions()) {
composite = EffectModel::ExtractCompositeOperation(
options.GetAsKeyframeEffectOptions());
composite = EffectModel::StringToCompositeOperation(
options.GetAsKeyframeEffectOptions().composite());
}
KeyframeEffectModelBase* model = EffectInput::Convert(
......
......@@ -104,6 +104,15 @@ PropertyHandleSet StringKeyframe::Properties() const {
return properties;
}
bool StringKeyframe::HasCssProperty() const {
PropertyHandleSet properties = Properties();
for (const PropertyHandle& property : properties) {
if (property.IsCSSProperty())
return true;
}
return false;
}
void StringKeyframe::AddKeyframePropertiesToV8Object(
V8ObjectBuilder& object_builder) const {
Keyframe::AddKeyframePropertiesToV8Object(object_builder);
......
......@@ -76,6 +76,8 @@ class CORE_EXPORT StringKeyframe : public Keyframe {
PropertyHandleSet Properties() const override;
bool HasCssProperty() const;
void AddKeyframePropertiesToV8Object(V8ObjectBuilder&) const override;
class CSSPropertySpecificKeyframe
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment