- Revision
- 263729
- Author
- grao...@webkit.org
- Date
- 2020-06-29 22:56:53 -0700 (Mon, 29 Jun 2020)
Log Message
[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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (263728 => 263729)
--- trunk/LayoutTests/ChangeLog 2020-06-30 05:55:14 UTC (rev 263728)
+++ trunk/LayoutTests/ChangeLog 2020-06-30 05:56:53 UTC (rev 263729)
@@ -1,3 +1,18 @@
+2020-06-29 Antoine Quint <grao...@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_hs...@apple.com>
REGRESSION (r263624): http/tests/quicklook/submit-form-blocked.html fails consistently
Added: trunk/LayoutTests/webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout-expected.txt (0 => 263729)
--- trunk/LayoutTests/webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout-expected.txt (rev 0)
+++ trunk/LayoutTests/webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout-expected.txt 2020-06-30 05:56:53 UTC (rev 263729)
@@ -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.
+
Added: trunk/LayoutTests/webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout.html (0 => 263729)
--- trunk/LayoutTests/webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout.html (rev 0)
+++ trunk/LayoutTests/webanimations/css-transition-retargeting-to-same-value-upon-completion-with-timeout.html 2020-06-30 05:56:53 UTC (rev 263729)
@@ -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=""
+<script src=""
+<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
Modified: trunk/Source/WebCore/ChangeLog (263728 => 263729)
--- trunk/Source/WebCore/ChangeLog 2020-06-30 05:55:14 UTC (rev 263728)
+++ trunk/Source/WebCore/ChangeLog 2020-06-30 05:56:53 UTC (rev 263729)
@@ -1,3 +1,49 @@
+2020-06-29 Antoine Quint <grao...@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 <beid...@apple.com>
_javascript_ cannot be injected into iframes
Modified: trunk/Source/WebCore/animation/AnimationTimeline.cpp (263728 => 263729)
--- trunk/Source/WebCore/animation/AnimationTimeline.cpp 2020-06-30 05:55:14 UTC (rev 263728)
+++ trunk/Source/WebCore/animation/AnimationTimeline.cpp 2020-06-30 05:56:53 UTC (rev 263729)
@@ -385,6 +385,13 @@
}
}
+ // 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.
@@ -391,7 +398,10 @@
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;
}
Modified: trunk/Source/WebCore/animation/DeclarativeAnimation.cpp (263728 => 263729)
--- trunk/Source/WebCore/animation/DeclarativeAnimation.cpp 2020-06-30 05:55:14 UTC (rev 263728)
+++ trunk/Source/WebCore/animation/DeclarativeAnimation.cpp 2020-06-30 05:56:53 UTC (rev 263729)
@@ -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
Modified: trunk/Source/WebCore/animation/WebAnimation.cpp (263728 => 263729)
--- trunk/Source/WebCore/animation/WebAnimation.cpp 2020-06-30 05:55:14 UTC (rev 263728)
+++ trunk/Source/WebCore/animation/WebAnimation.cpp 2020-06-30 05:56:53 UTC (rev 263729)
@@ -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.
Modified: trunk/Source/WebCore/animation/WebAnimation.h (263728 => 263729)
--- trunk/Source/WebCore/animation/WebAnimation.h 2020-06-30 05:55:14 UTC (rev 263728)
+++ trunk/Source/WebCore/animation/WebAnimation.h 2020-06-30 05:56:53 UTC (rev 263729)
@@ -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(); }
