Transform misplaces element 50% of the time
authordino@apple.com <dino@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 18 May 2017 23:22:16 +0000 (23:22 +0000)
committerdino@apple.com <dino@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 18 May 2017 23:22:16 +0000 (23:22 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@217075 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/animations/needs-layout-expected.html [new file with mode: 0644]
LayoutTests/animations/needs-layout.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/animation/CSSAnimationController.cpp
Source/WebCore/page/animation/CSSAnimationControllerPrivate.h
Source/WebCore/page/animation/CompositeAnimation.cpp
Source/WebCore/page/animation/CompositeAnimation.h
Source/WebCore/page/animation/KeyframeAnimation.cpp
Source/WebCore/page/animation/KeyframeAnimation.h

index 236720a..c367ccb 100644 (file)
@@ -1,3 +1,18 @@
+2017-05-18  Dean Jackson  <dino@apple.com>
+
+        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  <dabates@apple.com>
 
         Improve error message for Access-Control-Allow-Origin violation due to misconfigured server
diff --git a/LayoutTests/animations/needs-layout-expected.html b/LayoutTests/animations/needs-layout-expected.html
new file mode 100644 (file)
index 0000000..24db768
--- /dev/null
@@ -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
diff --git a/LayoutTests/animations/needs-layout.html b/LayoutTests/animations/needs-layout.html
new file mode 100644 (file)
index 0000000..d7b930d
--- /dev/null
@@ -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
index d775bde..d8e2053 100644 (file)
@@ -1,3 +1,43 @@
+2017-05-18  Dean Jackson  <dino@apple.com>
+
+        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  <wenson_hsieh@apple.com>
 
         Selection around attachment elements should not persist when beginning a drag
index 00c73e1..b11da5c 100644 (file)
@@ -261,6 +261,17 @@ void CSSAnimationControllerPrivate::animationTimerFired()
     // 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 CSSAnimationController::updateAnimations(RenderElement& renderer, const Ren
     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;
index 955a887..15a4f72 100644 (file)
@@ -106,6 +106,8 @@ public:
     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 @@ private:
 
     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
index 77d1820..30b7b3b 100644 (file)
@@ -334,6 +334,7 @@ bool CompositeAnimation::animate(RenderElement& renderer, const RenderStyle* cur
                 animationStateChanged = true;
 
             forceStackingContext |= didBlendStyle && keyframeAnim->triggersStackingContext();
+            m_hasAnimationThatDependsOnLayout |= keyframeAnim->dependsOnLayout();
         }
     }
 
index 5342c7d..b2138cd 100644 (file)
@@ -84,6 +84,8 @@ public:
     bool hasScrollTriggeredAnimation() const { return m_hasScrollTriggeredAnimation; }
 #endif
 
+    bool hasAnimationThatDependsOnLayout() const { return m_hasAnimationThatDependsOnLayout; }
+
 private:
     CompositeAnimation(CSSAnimationControllerPrivate&);
 
@@ -98,6 +100,7 @@ private:
     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
index 4fff11c..00f72a9 100644 (file)
@@ -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 @@ KeyframeAnimation::KeyframeAnimation(const Animation& animation, RenderElement*
     checkForMatchingBackdropFilterFunctionLists();
 #endif
     computeStackingContextImpact();
+    computeLayoutDependency();
 }
 
 KeyframeAnimation::~KeyframeAnimation()
@@ -77,6 +79,33 @@ void KeyframeAnimation::computeStackingContextImpact()
     }
 }
 
+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();
index 4e000a1..d220edf 100644 (file)
@@ -56,7 +56,8 @@ public:
     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 @@ protected:
     bool computeExtentOfAnimationForMatchingTransformLists(const FloatRect& rendererBox, LayoutRect&) const;
 
     void computeStackingContextImpact();
+    void computeLayoutDependency();
     void resolveKeyframeStyles();
     void validateTransformFunctionList();
     void checkForMatchingFilterFunctionLists();
@@ -102,6 +104,7 @@ private:
 
     bool m_startEventDispatched { false };
     bool m_triggersStackingContext { false };
+    bool m_dependsOnLayout { false };
 };
 
 } // namespace WebCore