[Web Animations] REGRESSION: Bootstrap Carousel component v4.1 regressed with Web Animations
https://bugs.webkit.org/show_bug.cgi?id=213376
<rdar://problem/64531242>
Reviewed by Dean Jackson.
Source/WebCore:
An older version of the Bootstrap CSS and JS library had a rather odd way to implement a completion callback
for a transition: it would register a "transitionend" event but also set a timeout of the transition's duration
and use whichever came first as a callback to run completion tasks for the transition.
Additionally, in that callback, it would set the transitioned value to the same computed value but using a different
specified value, for instance setting the "transform" CSS property to "translateX(0)" instead of "translateY(0)".
In our implementation this would make the completed transition repeat. Indeed, we would first incorrectly assume that
the transition was still "running" and not "finished", per the CSS Transitions spec terminology as we only update
that status when we update animations under Page::updateRendering(). We now update an existing transition's
status first in AnimationTimeline::updateCSSTransitionsForElementAndProperty().
Another issue is that when we considered the existing transition to be running, even though it was finished, we would
use the "timeline time at creation" to compute its current progress, which would yield situations where we computed
the before-change style to be the existing transition's current computed value, except that transition's progress was
0 since the "timeline time at creation" happens before the transition's resolved start time. We now only use the
"timeline time at creation" in the situations it was designed to be used: either when the transition has not yet had
a resolved start time, or its resolved start time is the current timeline time (ie. it was just set).
To be able to compare the transition's resolved start time and the current timeline time, we also updated the internal
start time getter and setter methods to use Seconds instead of double which is only needed for the JS bindings.
Test: webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout.html
* animation/AnimationTimeline.cpp:
(WebCore::AnimationTimeline::updateCSSTransitionsForElementAndProperty):
* animation/DeclarativeAnimation.cpp:
(WebCore::DeclarativeAnimation::bindingsStartTime const):
(WebCore::DeclarativeAnimation::setBindingsStartTime):
* animation/WebAnimation.cpp:
(WebCore::WebAnimation::bindingsStartTime const):
(WebCore::WebAnimation::setBindingsStartTime):
(WebCore::WebAnimation::setStartTime):
(WebCore::WebAnimation::startTime const): Deleted.
* animation/WebAnimation.h:
(WebCore::WebAnimation::startTime const):
(WebCore::WebAnimation::bindingsStartTime const): Deleted.
LayoutTests:
Add a test that uses a timeout instead of a "transitionend" event to retarget a transition upon completion
to the same computed value but not the same specified value to check that we generate a transition that has
no visual effect.
* webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout-expected.txt: Added.
* webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout.html: Added.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@263729 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index e3b5c02..bb77e21 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,18 @@
+2020-06-29 Antoine Quint <graouts@webkit.org>
+
+ [Web Animations] REGRESSION: Bootstrap Carousel component v4.1 regressed with Web Animations
+ https://bugs.webkit.org/show_bug.cgi?id=213376
+ <rdar://problem/64531242>
+
+ Reviewed by Dean Jackson.
+
+ Add a test that uses a timeout instead of a "transitionend" event to retarget a transition upon completion
+ to the same computed value but not the same specified value to check that we generate a transition that has
+ no visual effect.
+
+ * webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout-expected.txt: Added.
+ * webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout.html: Added.
+
2020-06-29 Wenson Hsieh <wenson_hsieh@apple.com>
REGRESSION (r263624): http/tests/quicklook/submit-form-blocked.html fails consistently
diff --git a/LayoutTests/webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout-expected.txt b/LayoutTests/webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout-expected.txt
new file mode 100644
index 0000000..455ef07
--- /dev/null
+++ b/LayoutTests/webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout-expected.txt
@@ -0,0 +1,3 @@
+
+PASS A CSS transition that is retargeted upon completion based on a timer should not use its to style as its before-change style.
+
diff --git a/LayoutTests/webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout.html b/LayoutTests/webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout.html
new file mode 100644
index 0000000..bc550b3
--- /dev/null
+++ b/LayoutTests/webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout.html
@@ -0,0 +1,63 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<style>
+
+.target {
+ transform: translateX(100px);
+ transition: transform 100ms linear;
+}
+
+.target.in-flight {
+ transform: translateX(0);
+}
+
+.target.in-flight.retargeted {
+ transform: translateY(0);
+}
+
+</style>
+<body>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+<script>
+
+'use strict';
+
+promise_test(async test => {
+ const target = document.body.appendChild(document.createElement("div"));
+ target.classList.add("target");
+
+ const getAnimation = () => {
+ const animations = target.getAnimations();
+ assert_equals(animations.length, 1, "There is one animation applied to the target.");
+
+ const transition = animations[0];
+ assert_true(transition instanceof CSSTransition, "There is one transition applied to the target.");
+
+ return transition;
+ }
+
+ let initialTransition;
+ let retargetedTransition;
+
+ // Start the initial transition.
+ await new Promise(requestAnimationFrame);
+ target.classList.add("in-flight");
+ initialTransition = getAnimation();
+
+ // Wait until the animation is complete and retarget the transition to the same value.
+ await(initialTransition.ready);
+ await(new Promise(resolve => setTimeout(resolve, 100)));
+ target.classList.add("retargeted");
+ retargetedTransition = getAnimation();
+
+ assert_not_equals(initialTransition, retargetedTransition, "Retargeting yielded a new transition.");
+
+ const transformAtKeyframeIndex = (animation, index) => new DOMMatrixReadOnly(animation.effect.getKeyframes()[index].transform).toString();
+ assert_not_equals(transformAtKeyframeIndex(initialTransition, 0), transformAtKeyframeIndex(retargetedTransition, 0), "The initial and retargeted transitions have different from values.");
+ assert_equals(transformAtKeyframeIndex(initialTransition, 1), transformAtKeyframeIndex(retargetedTransition, 1), "The initial and retargeted transitions have the same to values.");
+ assert_equals(transformAtKeyframeIndex(retargetedTransition, 0), transformAtKeyframeIndex(retargetedTransition, 1), "The retargeted transition from and to values are the same.");
+}, `A CSS transition that is retargeted upon completion based on a timer should not use its to style as its before-change style.`);
+
+</script>
+</body>
\ No newline at end of file
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 8d6ab64..66ad559 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,49 @@
+2020-06-29 Antoine Quint <graouts@webkit.org>
+
+ [Web Animations] REGRESSION: Bootstrap Carousel component v4.1 regressed with Web Animations
+ https://bugs.webkit.org/show_bug.cgi?id=213376
+ <rdar://problem/64531242>
+
+ Reviewed by Dean Jackson.
+
+ An older version of the Bootstrap CSS and JS library had a rather odd way to implement a completion callback
+ for a transition: it would register a "transitionend" event but also set a timeout of the transition's duration
+ and use whichever came first as a callback to run completion tasks for the transition.
+
+ Additionally, in that callback, it would set the transitioned value to the same computed value but using a different
+ specified value, for instance setting the "transform" CSS property to "translateX(0)" instead of "translateY(0)".
+
+ In our implementation this would make the completed transition repeat. Indeed, we would first incorrectly assume that
+ the transition was still "running" and not "finished", per the CSS Transitions spec terminology as we only update
+ that status when we update animations under Page::updateRendering(). We now update an existing transition's
+ status first in AnimationTimeline::updateCSSTransitionsForElementAndProperty().
+
+ Another issue is that when we considered the existing transition to be running, even though it was finished, we would
+ use the "timeline time at creation" to compute its current progress, which would yield situations where we computed
+ the before-change style to be the existing transition's current computed value, except that transition's progress was
+ 0 since the "timeline time at creation" happens before the transition's resolved start time. We now only use the
+ "timeline time at creation" in the situations it was designed to be used: either when the transition has not yet had
+ a resolved start time, or its resolved start time is the current timeline time (ie. it was just set).
+
+ To be able to compare the transition's resolved start time and the current timeline time, we also updated the internal
+ start time getter and setter methods to use Seconds instead of double which is only needed for the JS bindings.
+
+ Test: webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout.html
+
+ * animation/AnimationTimeline.cpp:
+ (WebCore::AnimationTimeline::updateCSSTransitionsForElementAndProperty):
+ * animation/DeclarativeAnimation.cpp:
+ (WebCore::DeclarativeAnimation::bindingsStartTime const):
+ (WebCore::DeclarativeAnimation::setBindingsStartTime):
+ * animation/WebAnimation.cpp:
+ (WebCore::WebAnimation::bindingsStartTime const):
+ (WebCore::WebAnimation::setBindingsStartTime):
+ (WebCore::WebAnimation::setStartTime):
+ (WebCore::WebAnimation::startTime const): Deleted.
+ * animation/WebAnimation.h:
+ (WebCore::WebAnimation::startTime const):
+ (WebCore::WebAnimation::bindingsStartTime const): Deleted.
+
2020-06-29 Brady Eidson <beidson@apple.com>
JavaScript cannot be injected into iframes
diff --git a/Source/WebCore/animation/AnimationTimeline.cpp b/Source/WebCore/animation/AnimationTimeline.cpp
index c955dc9..a2a0a57 100644
--- a/Source/WebCore/animation/AnimationTimeline.cpp
+++ b/Source/WebCore/animation/AnimationTimeline.cpp
@@ -385,13 +385,23 @@
}
}
+ // A CSS Transition might have completed since the last time animations were updated so we must
+ // update the running and completed transitions membership in that case.
+ if (is<CSSTransition>(animation) && element.hasRunningTransitionsForProperty(property) && animation->playState() == WebAnimation::PlayState::Finished) {
+ element.ensureCompletedTransitionsByProperty().set(property, element.ensureRunningTransitionsByProperty().take(property));
+ animation = nullptr;
+ }
+
// https://drafts.csswg.org/css-transitions-1/#before-change-style
// Define the before-change style as the computed values of all properties on the element as of the previous style change event, except with
// any styles derived from declarative animations such as CSS Transitions, CSS Animations, and SMIL Animations updated to the current time.
auto beforeChangeStyle = [&]() -> const RenderStyle {
if (animation && animation->isRelevant()) {
auto animatedStyle = RenderStyle::clone(currentStyle);
- animation->resolve(animatedStyle, is<CSSTransition>(animation) ? downcast<CSSTransition>(*animation).timelineTimeAtCreation() : WTF::nullopt);
+ // If a transition has not yet started or started when animations were last updated, use the timeline time at its creation
+ // as its start time to ensure that it will produce a style with progress > 0.
+ bool shouldUseTimelineTimeAtCreation = is<CSSTransition>(animation) && (!animation->startTime() || *animation->startTime() == currentTime());
+ animation->resolve(animatedStyle, shouldUseTimelineTimeAtCreation ? downcast<CSSTransition>(*animation).timelineTimeAtCreation() : WTF::nullopt);
return animatedStyle;
}
diff --git a/Source/WebCore/animation/DeclarativeAnimation.cpp b/Source/WebCore/animation/DeclarativeAnimation.cpp
index 8324e84..ee21fa3 100644
--- a/Source/WebCore/animation/DeclarativeAnimation.cpp
+++ b/Source/WebCore/animation/DeclarativeAnimation.cpp
@@ -132,13 +132,13 @@
Optional<double> DeclarativeAnimation::bindingsStartTime() const
{
flushPendingStyleChanges();
- return WebAnimation::startTime();
+ return WebAnimation::bindingsStartTime();
}
void DeclarativeAnimation::setBindingsStartTime(Optional<double> startTime)
{
flushPendingStyleChanges();
- return WebAnimation::setStartTime(startTime);
+ return WebAnimation::setBindingsStartTime(startTime);
}
Optional<double> DeclarativeAnimation::bindingsCurrentTime() const
diff --git a/Source/WebCore/animation/WebAnimation.cpp b/Source/WebCore/animation/WebAnimation.cpp
index f37ea12..aad4f1c 100644
--- a/Source/WebCore/animation/WebAnimation.cpp
+++ b/Source/WebCore/animation/WebAnimation.cpp
@@ -301,29 +301,26 @@
InspectorInstrumentation::didChangeWebAnimationEffectTarget(*this);
}
-Optional<double> WebAnimation::startTime() const
+Optional<double> WebAnimation::bindingsStartTime() const
{
if (!m_startTime)
return WTF::nullopt;
- return secondsToWebAnimationsAPITime(m_startTime.value());
+ return secondsToWebAnimationsAPITime(*m_startTime);
}
-void WebAnimation::setBindingsStartTime(Optional<double> startTime)
+void WebAnimation::setBindingsStartTime(Optional<double> newStartTime)
{
- setStartTime(startTime);
+ if (newStartTime)
+ setStartTime(Seconds::fromMilliseconds(*newStartTime));
+ else
+ setStartTime(WTF::nullopt);
}
-void WebAnimation::setStartTime(Optional<double> startTime)
+void WebAnimation::setStartTime(Optional<Seconds> newStartTime)
{
// 3.4.6 The procedure to set the start time of animation, animation, to new start time, is as follows:
// https://drafts.csswg.org/web-animations/#setting-the-start-time-of-an-animation
- Optional<Seconds> newStartTime;
- if (!startTime)
- newStartTime = WTF::nullopt;
- else
- newStartTime = Seconds::fromMilliseconds(startTime.value());
-
// 1. Let timeline time be the current time value of the timeline that animation is associated with. If
// there is no timeline associated with animation or the associated timeline is inactive, let the timeline
// time be unresolved.
diff --git a/Source/WebCore/animation/WebAnimation.h b/Source/WebCore/animation/WebAnimation.h
index 6ad7ee9..bb39a0a 100644
--- a/Source/WebCore/animation/WebAnimation.h
+++ b/Source/WebCore/animation/WebAnimation.h
@@ -104,10 +104,10 @@
void persist();
ExceptionOr<void> commitStyles();
- virtual Optional<double> bindingsStartTime() const { return startTime(); }
+ virtual Optional<double> bindingsStartTime() const;
virtual void setBindingsStartTime(Optional<double>);
- Optional<double> startTime() const;
- void setStartTime(Optional<double>);
+ Optional<Seconds> startTime() const { return m_startTime; }
+ void setStartTime(Optional<Seconds>);
virtual Optional<double> bindingsCurrentTime() const;
virtual ExceptionOr<void> setBindingsCurrentTime(Optional<double>);
virtual PlayState bindingsPlayState() const { return playState(); }
