From 99a1a632e392955ece6e9629814ea7431aafd471 Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Wed, 30 Mar 2016 08:59:08 +0900 Subject: [PATCH] Bug 1260572 - Use 50% switch behavior if StyleAnimationValue::Interpolate fails; r=heycam In KeyframeEffectReadOnly::ComposeStyle we call StyleAnimationValue::Interpolate but assume that it always passes. That was true when that code was only used for CSS animations and CSS transitions since they check that their animation values can be interpolated before setting up segments. However, when we set up animations using the Web Animations API we don't perform that check so it is possible for this call to fail. In that case, we could just bail, but, according to CSS Transitions we should apply a 50% switch in this case: https://drafts.csswg.org/css-transitions/#step-types (In Web Animations, specifying this is an open issue. See: https://w3c.github.io/web-animations/#specific-animation-behaviors). Bug 1064937 tracks doing this in general (we'll likely need to mark various properties as being no longer unanimatable but instead as supporting discrete animation) but we can start to introduce it now. Later in bug 1245748, CSS animations and transitions will likely start using the same code path as the Web Animations API for setting up keyframes. As a result, unless we take care to add checks that the values we set are interpolable, the 50% switch behavior will begin to apply to CSS animations and transitions too at that point. Some concerns have been raised about possible web compatibility issues around the 50% switch behavior (see [1] and [2]). For CSS animations, Chrome already supports this behavior so it should be ok at least for CSS animations. When we switch CSS transitions over to the same code path, however, we will need to be careful to add checks that the transition endpoints are interpolable (we can investigate introducing this behavior to transitions as a separate bug that can be easily backed out / preffed off). Regarding the naming of the test added here, going forward we would like to restructure the tests under web-platform-tests to better match the structure of the Web Animations since that seems to be the convention there. However, this doesn't *quite* match the structure of the spec since there are upcoming changes to the spec in this area (e.g. renaming animation behaviors to animation types). However, it should be close enough that we don't have to move it around too much in future. [1] https://drafts.csswg.org/css-transitions/#step-types [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1064937#c0 MozReview-Commit-ID: KcxILrckJg9 --- dom/animation/KeyframeEffect.cpp | 20 +-- testing/web-platform/meta/MANIFEST.json | 8 +- .../animation-types/discrete-animation.html | 136 ++++++++++++++++++ 3 files changed, 150 insertions(+), 14 deletions(-) create mode 100644 testing/web-platform/tests/web-animations/animation-model/animation-types/discrete-animation.html diff --git a/dom/animation/KeyframeEffect.cpp b/dom/animation/KeyframeEffect.cpp index 9c1bee5d3594..63e7c9f68412 100644 --- a/dom/animation/KeyframeEffect.cpp +++ b/dom/animation/KeyframeEffect.cpp @@ -617,16 +617,16 @@ KeyframeEffectReadOnly::ComposeStyle(RefPtr& aStyleRule, computedTiming.mBeforeFlag); StyleAnimationValue val; -#ifdef DEBUG - bool result = -#endif - StyleAnimationValue::Interpolate(prop.mProperty, - segment->mFromValue, - segment->mToValue, - valuePosition, val); - MOZ_ASSERT(result, "interpolate must succeed now"); - - aStyleRule->AddValue(prop.mProperty, Move(val)); + if (StyleAnimationValue::Interpolate(prop.mProperty, + segment->mFromValue, + segment->mToValue, + valuePosition, val)) { + aStyleRule->AddValue(prop.mProperty, Move(val)); + } else if (valuePosition < 0.5) { + aStyleRule->AddValue(prop.mProperty, segment->mFromValue); + } else { + aStyleRule->AddValue(prop.mProperty, segment->mToValue); + } } } diff --git a/testing/web-platform/meta/MANIFEST.json b/testing/web-platform/meta/MANIFEST.json index b77881be3152..6a82b626b542 100644 --- a/testing/web-platform/meta/MANIFEST.json +++ b/testing/web-platform/meta/MANIFEST.json @@ -28389,10 +28389,6 @@ "path": "web-animations/animatable/animate.html", "url": "/web-animations/animatable/animate.html" }, - { - "path": "web-animations/animation-effect-timing/delay.html", - "url": "/web-animations/animation-effect-timing/delay.html" - }, { "path": "web-animations/animation-effect-timing/direction.html", "url": "/web-animations/animation-effect-timing/direction.html" @@ -28421,6 +28417,10 @@ "path": "web-animations/animation-effect-timing/iterations.html", "url": "/web-animations/animation-effect-timing/iterations.html" }, + { + "path": "web-animations/animation-model/animation-types/discrete-animation.html", + "url": "/web-animations/animation-model/animation-types/discrete-animation.html" + }, { "path": "web-animations/animation-timeline/document-timeline.html", "url": "/web-animations/animation-timeline/document-timeline.html" diff --git a/testing/web-platform/tests/web-animations/animation-model/animation-types/discrete-animation.html b/testing/web-platform/tests/web-animations/animation-model/animation-types/discrete-animation.html new file mode 100644 index 000000000000..864a9e2845b5 --- /dev/null +++ b/testing/web-platform/tests/web-animations/animation-model/animation-types/discrete-animation.html @@ -0,0 +1,136 @@ + + +Tests for discrete animation + + + + + + +
+