- Revision
- 217075
- Author
- [email protected]
- Date
- 2017-05-18 16:22:16 -0700 (Thu, 18 May 2017)
Log Message
Transform misplaces element 50% of the time
https://bugs.webkit.org/show_bug.cgi?id=172300
Source/WebCore:
Reviewed by Simon Fraser.
A hardware-accelerated animation of the transform property
requires layout to happen if it contains a translate operation
using percentages, otherwise it may create an incorrect
animation. The "50% of the time" comes in to play because
the layout timer may sometimes fire before the animation
timer. The test case contains a example that is much more
likely to fail without this fix.
Test: animations/needs-layout.html
* page/animation/CSSAnimationController.cpp:
(WebCore::CSSAnimationControllerPrivate::animationTimerFired): If
we've been told that we need a layout, and we have one pending, then
force it before doing the rest of the animation logic.
(WebCore::CSSAnimationController::updateAnimations): Check if the
CompositeAnimation depends on layout, and tell the private controller
that it should check for the necessity of a layout as the animation
timer fires.
* page/animation/CompositeAnimation.cpp:
(WebCore::CompositeAnimation::animate): Ask the keyframes if this
animation depends on layout.
* page/animation/CompositeAnimation.h:
(WebCore::CompositeAnimation::hasAnimationThatDependsOnLayout):
* page/animation/KeyframeAnimation.cpp:
(WebCore::KeyframeAnimation::KeyframeAnimation):
(WebCore::KeyframeAnimation::computeLayoutDependency): Look at all
the keyframe properties for something that is a translation using
percentages.
* page/animation/KeyframeAnimation.h:
LayoutTests:
<rdar://problem/29835668>
Reviewed by Simon Fraser.
A test case which has an animation that relies on
translation percentages. If all goes well, the
animating element will be completely obscured.
* animations/needs-layout-expected.html: Added.
* animations/needs-layout.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (217074 => 217075)
--- trunk/LayoutTests/ChangeLog 2017-05-18 23:20:34 UTC (rev 217074)
+++ trunk/LayoutTests/ChangeLog 2017-05-18 23:22:16 UTC (rev 217075)
@@ -1,3 +1,18 @@
+2017-05-18 Dean Jackson <[email protected]>
+
+ Transform misplaces element 50% of the time
+ https://bugs.webkit.org/show_bug.cgi?id=172300
+ <rdar://problem/29835668>
+
+ Reviewed by Simon Fraser.
+
+ A test case which has an animation that relies on
+ translation percentages. If all goes well, the
+ animating element will be completely obscured.
+
+ * animations/needs-layout-expected.html: Added.
+ * animations/needs-layout.html: Added.
+
2017-05-18 Daniel Bates <[email protected]>
Improve error message for Access-Control-Allow-Origin violation due to misconfigured server
Added: trunk/LayoutTests/animations/needs-layout-expected.html (0 => 217075)
--- trunk/LayoutTests/animations/needs-layout-expected.html (rev 0)
+++ trunk/LayoutTests/animations/needs-layout-expected.html 2017-05-18 23:22:16 UTC (rev 217075)
@@ -0,0 +1,15 @@
+<style>
+ #overlay {
+ position: absolute;
+ left: 100px;
+ top: 100px;
+ width: 200px;
+ height: 200px;
+ transform: translate(100px);
+ background-color: green;
+ }
+</style>
+<body>
+ <div id="overlay"></div>
+</body>
+</html>
\ No newline at end of file
Property changes on: trunk/LayoutTests/animations/needs-layout-expected.html
___________________________________________________________________
Added: svn:eol-style
+native
\ No newline at end of property
Added: svn:keywords
+Date Revision
\ No newline at end of property
Added: svn:mime-type
+text/html
\ No newline at end of property
Added: trunk/LayoutTests/animations/needs-layout.html (0 => 217075)
--- trunk/LayoutTests/animations/needs-layout.html (rev 0)
+++ trunk/LayoutTests/animations/needs-layout.html 2017-05-18 23:22:16 UTC (rev 217075)
@@ -0,0 +1,57 @@
+<style>
+ #overlay {
+ position: absolute;
+ left: 100px;
+ top: 100px;
+ width: 200px;
+ height: 200px;
+ transform: translate(100px);
+ background-color: green;
+ }
+
+ #container .child {
+ position: absolute;
+ left: 150px;
+ top: 150px;
+ width: 100px;
+ height: 100px;
+ background-color: red;
+ animation: 5s anim;
+ }
+
+ @keyframes anim {
+ 0% {
+ transform: translate(80%, 0);
+ }
+ 100% {
+ transform: translate(80%, 100px);
+ }
+ }
+
+</style>
+<script>
+if (window.testRunner)
+ testRunner.waitUntilDone();
+
+function test() {
+ var el = document.getElementById("container");
+ var child = document.createElement("div");
+
+ child.classList.add("child");
+ el.appendChild(child);
+ setTimeout(function () {
+ child.style.webkitAnimationPlayState = "paused";
+ if (window.testRunner)
+ testRunner.notifyDone();
+ }, 10);
+}
+
+window.addEventListener("load", function () {
+ setTimeout(test, 10);
+}, false);
+</script>
+<body>
+ <div id="container"></div>
+ <div id="overlay"></div>
+</body>
+</html>
\ No newline at end of file
Property changes on: trunk/LayoutTests/animations/needs-layout.html
___________________________________________________________________
Added: svn:eol-style
+native
\ No newline at end of property
Added: svn:keywords
+Date Revision
\ No newline at end of property
Added: svn:mime-type
+text/html
\ No newline at end of property
Modified: trunk/Source/WebCore/ChangeLog (217074 => 217075)
--- trunk/Source/WebCore/ChangeLog 2017-05-18 23:20:34 UTC (rev 217074)
+++ trunk/Source/WebCore/ChangeLog 2017-05-18 23:22:16 UTC (rev 217075)
@@ -1,3 +1,43 @@
+2017-05-18 Dean Jackson <[email protected]>
+
+ Transform misplaces element 50% of the time
+ https://bugs.webkit.org/show_bug.cgi?id=172300
+
+ Reviewed by Simon Fraser.
+
+ A hardware-accelerated animation of the transform property
+ requires layout to happen if it contains a translate operation
+ using percentages, otherwise it may create an incorrect
+ animation. The "50% of the time" comes in to play because
+ the layout timer may sometimes fire before the animation
+ timer. The test case contains a example that is much more
+ likely to fail without this fix.
+
+ Test: animations/needs-layout.html
+
+ * page/animation/CSSAnimationController.cpp:
+ (WebCore::CSSAnimationControllerPrivate::animationTimerFired): If
+ we've been told that we need a layout, and we have one pending, then
+ force it before doing the rest of the animation logic.
+ (WebCore::CSSAnimationController::updateAnimations): Check if the
+ CompositeAnimation depends on layout, and tell the private controller
+ that it should check for the necessity of a layout as the animation
+ timer fires.
+
+ * page/animation/CompositeAnimation.cpp:
+ (WebCore::CompositeAnimation::animate): Ask the keyframes if this
+ animation depends on layout.
+
+ * page/animation/CompositeAnimation.h:
+ (WebCore::CompositeAnimation::hasAnimationThatDependsOnLayout):
+ * page/animation/KeyframeAnimation.cpp:
+ (WebCore::KeyframeAnimation::KeyframeAnimation):
+ (WebCore::KeyframeAnimation::computeLayoutDependency): Look at all
+ the keyframe properties for something that is a translation using
+ percentages.
+
+ * page/animation/KeyframeAnimation.h:
+
2017-05-18 Wenson Hsieh <[email protected]>
Selection around attachment elements should not persist when beginning a drag
Modified: trunk/Source/WebCore/page/animation/CSSAnimationController.cpp (217074 => 217075)
--- trunk/Source/WebCore/page/animation/CSSAnimationController.cpp 2017-05-18 23:20:34 UTC (rev 217074)
+++ trunk/Source/WebCore/page/animation/CSSAnimationController.cpp 2017-05-18 23:22:16 UTC (rev 217075)
@@ -261,6 +261,17 @@
// We need to keep the frame alive, since it owns us.
Ref<Frame> protector(m_frame);
+ // The animation timer might fire before the layout timer, in
+ // which case we might create some animations with incorrect
+ // values if we don't layout first.
+ if (m_requiresLayout) {
+ if (auto* frameView = m_frame.document()->view()) {
+ if (frameView->needsLayout())
+ frameView->forceLayout();
+ }
+ m_requiresLayout = false;
+ }
+
// Make sure animationUpdateTime is updated, so that it is current even if no
// styleChange has happened (e.g. accelerated animations)
AnimationPrivateUpdateBlock updateBlock(*this);
@@ -657,8 +668,11 @@
bool animationStateChanged = rendererAnimations.animate(renderer, oldStyle, newStyle, animatedStyle);
if (renderer.parent() || newStyle.animations() || (oldStyle && oldStyle->animations())) {
+ auto& frameView = renderer.view().frameView();
+ if (rendererAnimations.hasAnimationThatDependsOnLayout())
+ m_data->setRequiresLayout();
m_data->updateAnimationTimerForRenderer(renderer);
- renderer.view().frameView().scheduleAnimation();
+ frameView.scheduleAnimation();
}
return animationStateChanged;
Modified: trunk/Source/WebCore/page/animation/CSSAnimationControllerPrivate.h (217074 => 217075)
--- trunk/Source/WebCore/page/animation/CSSAnimationControllerPrivate.h 2017-05-18 23:20:34 UTC (rev 217074)
+++ trunk/Source/WebCore/page/animation/CSSAnimationControllerPrivate.h 2017-05-18 23:22:16 UTC (rev 217075)
@@ -106,6 +106,8 @@
bool allowsNewAnimationsWhileSuspended() const { return m_allowsNewAnimationsWhileSuspended; }
void setAllowsNewAnimationsWhileSuspended(bool);
+ void setRequiresLayout() { m_requiresLayout = true; }
+
#if ENABLE(CSS_ANIMATIONS_LEVEL_2)
bool wantsScrollUpdates() const { return !m_animationsDependentOnScroll.isEmpty(); }
void addToAnimationsDependentOnScroll(AnimationBase*);
@@ -147,6 +149,7 @@
bool m_waitingForAsyncStartNotification;
bool m_isSuspended { false };
+ bool m_requiresLayout { false };
// Used to flag whether we should revert to previous buggy
// behavior of allowing new transitions and animations to
Modified: trunk/Source/WebCore/page/animation/CompositeAnimation.cpp (217074 => 217075)
--- trunk/Source/WebCore/page/animation/CompositeAnimation.cpp 2017-05-18 23:20:34 UTC (rev 217074)
+++ trunk/Source/WebCore/page/animation/CompositeAnimation.cpp 2017-05-18 23:22:16 UTC (rev 217075)
@@ -334,6 +334,7 @@
animationStateChanged = true;
forceStackingContext |= didBlendStyle && keyframeAnim->triggersStackingContext();
+ m_hasAnimationThatDependsOnLayout |= keyframeAnim->dependsOnLayout();
}
}
Modified: trunk/Source/WebCore/page/animation/CompositeAnimation.h (217074 => 217075)
--- trunk/Source/WebCore/page/animation/CompositeAnimation.h 2017-05-18 23:20:34 UTC (rev 217074)
+++ trunk/Source/WebCore/page/animation/CompositeAnimation.h 2017-05-18 23:22:16 UTC (rev 217075)
@@ -84,6 +84,8 @@
bool hasScrollTriggeredAnimation() const { return m_hasScrollTriggeredAnimation; }
#endif
+ bool hasAnimationThatDependsOnLayout() const { return m_hasAnimationThatDependsOnLayout; }
+
private:
CompositeAnimation(CSSAnimationControllerPrivate&);
@@ -98,6 +100,7 @@
AnimationNameMap m_keyframeAnimations;
Vector<AtomicStringImpl*> m_keyframeAnimationOrderMap;
bool m_suspended;
+ bool m_hasAnimationThatDependsOnLayout { false };
#if ENABLE(CSS_ANIMATIONS_LEVEL_2)
bool m_hasScrollTriggeredAnimation { false };
#endif
Modified: trunk/Source/WebCore/page/animation/KeyframeAnimation.cpp (217074 => 217075)
--- trunk/Source/WebCore/page/animation/KeyframeAnimation.cpp 2017-05-18 23:20:34 UTC (rev 217074)
+++ trunk/Source/WebCore/page/animation/KeyframeAnimation.cpp 2017-05-18 23:22:16 UTC (rev 217075)
@@ -40,6 +40,7 @@
#include "StylePendingResources.h"
#include "StyleResolver.h"
#include "StyleScope.h"
+#include "TranslateTransformOperation.h"
#include "WillChangeData.h"
namespace WebCore {
@@ -58,6 +59,7 @@
checkForMatchingBackdropFilterFunctionLists();
#endif
computeStackingContextImpact();
+ computeLayoutDependency();
}
KeyframeAnimation::~KeyframeAnimation()
@@ -77,6 +79,33 @@
}
}
+void KeyframeAnimation::computeLayoutDependency()
+{
+ if (!m_keyframes.containsProperty(CSSPropertyTransform))
+ return;
+
+ size_t numKeyframes = m_keyframes.size();
+ for (size_t i = 0; i < numKeyframes; i++) {
+ auto* keyframeStyle = m_keyframes[i].style();
+ if (!keyframeStyle) {
+ ASSERT_NOT_REACHED();
+ continue;
+ }
+ if (keyframeStyle->hasTransform()) {
+ auto& transformOperations = keyframeStyle->transform();
+ for (auto operation : transformOperations.operations()) {
+ if (operation->isTranslateTransformOperationType()) {
+ auto translation = downcast<TranslateTransformOperation>(operation.get());
+ if (translation->x().isPercent() || translation->y().isPercent()) {
+ m_dependsOnLayout = true;
+ return;
+ }
+ }
+ }
+ }
+ }
+}
+
void KeyframeAnimation::fetchIntervalEndpointsForProperty(CSSPropertyID property, const RenderStyle*& fromStyle, const RenderStyle*& toStyle, double& prog) const
{
size_t numKeyframes = m_keyframes.size();
Modified: trunk/Source/WebCore/page/animation/KeyframeAnimation.h (217074 => 217075)
--- trunk/Source/WebCore/page/animation/KeyframeAnimation.h 2017-05-18 23:20:34 UTC (rev 217074)
+++ trunk/Source/WebCore/page/animation/KeyframeAnimation.h 2017-05-18 23:22:16 UTC (rev 217075)
@@ -56,7 +56,8 @@
bool hasAnimationForProperty(CSSPropertyID) const;
bool triggersStackingContext() const { return m_triggersStackingContext; }
-
+ bool dependsOnLayout() const { return m_dependsOnLayout; }
+
void setUnanimatedStyle(std::unique_ptr<RenderStyle> style) { m_unanimatedStyle = WTFMove(style); }
RenderStyle* unanimatedStyle() const { return m_unanimatedStyle.get(); }
@@ -83,6 +84,7 @@
bool computeExtentOfAnimationForMatchingTransformLists(const FloatRect& rendererBox, LayoutRect&) const;
void computeStackingContextImpact();
+ void computeLayoutDependency();
void resolveKeyframeStyles();
void validateTransformFunctionList();
void checkForMatchingFilterFunctionLists();
@@ -102,6 +104,7 @@
bool m_startEventDispatched { false };
bool m_triggersStackingContext { false };
+ bool m_dependsOnLayout { false };
};
} // namespace WebCore
