Unreviewed, rolling out r222040.
authorryanhaddad@apple.com <ryanhaddad@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Sep 2017 19:54:01 +0000 (19:54 +0000)
committerryanhaddad@apple.com <ryanhaddad@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Sep 2017 19:54:01 +0000 (19:54 +0000)
The LayoutTest added with this change is a flaky image failure
on mac-wk1 debug bots.

Reverted changeset:

"Computing animated style should not require renderers"
https://bugs.webkit.org/show_bug.cgi?id=171926
http://trac.webkit.org/changeset/222040

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/transitions/transition-display-property-2-expected.html [deleted file]
LayoutTests/transitions/transition-display-property-2.html [deleted file]
LayoutTests/transitions/transition-display-property.html
Source/WebCore/ChangeLog
Source/WebCore/page/animation/CSSAnimationController.cpp
Source/WebCore/page/animation/CSSAnimationController.h
Source/WebCore/page/animation/CompositeAnimation.cpp
Source/WebCore/page/animation/CompositeAnimation.h
Source/WebCore/rendering/RenderElement.cpp
Source/WebCore/rendering/RenderElement.h
Source/WebCore/style/RenderTreeUpdater.cpp
Source/WebCore/style/RenderTreeUpdater.h
Source/WebCore/style/StyleTreeResolver.cpp

index f336037..dd5d349 100644 (file)
@@ -1,3 +1,16 @@
+2017-09-15  Ryan Haddad  <ryanhaddad@apple.com>
+
+        Unreviewed, rolling out r222040.
+
+        The LayoutTest added with this change is a flaky image failure
+        on mac-wk1 debug bots.
+
+        Reverted changeset:
+
+        "Computing animated style should not require renderers"
+        https://bugs.webkit.org/show_bug.cgi?id=171926
+        http://trac.webkit.org/changeset/222040
+
 2017-09-15  Matt Baker  <mattbaker@apple.com>
 
         Web Inspector: Canvas: recording parameters that include colors should show an InlineSwatch (2D canvas)
diff --git a/LayoutTests/transitions/transition-display-property-2-expected.html b/LayoutTests/transitions/transition-display-property-2-expected.html
deleted file mode 100644 (file)
index 55ef088..0000000
+++ /dev/null
@@ -1,4 +0,0 @@
-<style>
-test { display:inline-block; height:55px; width:10px; border: 2px solid green; }
-</style>
-<test></test>
diff --git a/LayoutTests/transitions/transition-display-property-2.html b/LayoutTests/transitions/transition-display-property-2.html
deleted file mode 100644 (file)
index 8bbf0d8..0000000
+++ /dev/null
@@ -1,22 +0,0 @@
-<style>
-test {
-    transition: 5s;
-    display: block;
-    height: 100px;
-    border: 2px solid green;
-    transition-timing-function: steps(2, start);
-}
-.animate { display:inline-block; height:10px; width:10px; }
-</style>
-<test></test>
-<script>
-if (window.testRunner) {
-    testRunner.waitUntilDone();
-    requestAnimationFrame(() => {
-        testRunner.notifyDone();
-    });
-}
-const test = document.querySelector("test");
-test.offsetWidth;
-test.className = "animate";
-</script>
index e94b17a..d89ff3b 100644 (file)
@@ -5,10 +5,18 @@ test { transition:0.1s; display:block; height:100px; border: 2px solid green; }
 <test></test>
 <script>
 var test = document.querySelector("test");
+var count = 0;
+function testUntilComplete()
+{
+    if (test.offsetWidth == 10 || ++count == 10)
+        testRunner.notifyDone();
+    else
+        setTimeout(testUntilComplete, 0.1);
+}
 
 if (window.testRunner) {
     testRunner.waitUntilDone();
-    test.ontransitionend = () => testRunner.notifyDone();
+    testUntilComplete();
 }
 test.offsetWidth;
 test.className = "animate";
index d70ca81..c733fc8 100644 (file)
@@ -1,3 +1,16 @@
+2017-09-15  Ryan Haddad  <ryanhaddad@apple.com>
+
+        Unreviewed, rolling out r222040.
+
+        The LayoutTest added with this change is a flaky image failure
+        on mac-wk1 debug bots.
+
+        Reverted changeset:
+
+        "Computing animated style should not require renderers"
+        https://bugs.webkit.org/show_bug.cgi?id=171926
+        http://trac.webkit.org/changeset/222040
+
 2017-09-15  Tim Horton  <timothy_horton@apple.com>
 
         Fix the macOS CMake build
index d0cd635..eb67fd6 100644 (file)
@@ -637,18 +637,19 @@ void CSSAnimationController::cancelAnimations(Element& element)
     element.invalidateStyleAndLayerComposition();
 }
 
-AnimationUpdate CSSAnimationController::updateAnimations(Element& element, const RenderStyle& newStyle, const RenderStyle* oldStyle)
+bool CSSAnimationController::updateAnimations(Element& element, const RenderStyle& newStyle, std::unique_ptr<RenderStyle>& animatedStyle)
 {
-    bool hasOrHadAnimations = (oldStyle && oldStyle->hasAnimationsOrTransitions()) || newStyle.hasAnimationsOrTransitions();
-    if (!hasOrHadAnimations)
-        return { };
+    auto* renderer = element.renderer();
+    auto* oldStyle = (renderer && renderer->hasInitializedStyle()) ? &renderer->style() : nullptr;
+    if ((!oldStyle || (!oldStyle->animations() && !oldStyle->transitions())) && (!newStyle.animations() && !newStyle.transitions()))
+        return false;
 
     if (element.document().pageCacheState() != Document::NotInPageCache)
-        return { };
+        return false;
 
     // Don't run transitions when printing.
     if (element.document().renderView()->printing())
-        return { };
+        return false;
 
     // Fetch our current set of implicit animations from a hashtable. We then compare them
     // against the animations in the style and make sure we're in sync. If destination values
@@ -656,9 +657,8 @@ AnimationUpdate CSSAnimationController::updateAnimations(Element& element, const
     // a new style.
 
     CompositeAnimation& compositeAnimation = m_data->ensureCompositeAnimation(element);
-    auto update = compositeAnimation.animate(element, oldStyle, newStyle);
+    bool animationStateChanged = compositeAnimation.animate(element, oldStyle, newStyle, animatedStyle);
 
-    auto* renderer = element.renderer();
     if ((renderer && renderer->parent()) || newStyle.animations() || (oldStyle && oldStyle->animations())) {
         auto& frameView = *element.document().view();
         if (compositeAnimation.hasAnimationThatDependsOnLayout())
@@ -667,7 +667,7 @@ AnimationUpdate CSSAnimationController::updateAnimations(Element& element, const
         frameView.scheduleAnimation();
     }
 
-    return update;
+    return animationStateChanged;
 }
 
 std::unique_ptr<RenderStyle> CSSAnimationController::animatedStyleForRenderer(RenderElement& renderer)
index 25c86d3..8647a23 100644 (file)
@@ -30,7 +30,6 @@
 
 #include "AnimationBase.h"
 #include "CSSPropertyNames.h"
-#include "RenderStyle.h"
 #include <wtf/Forward.h>
 
 namespace WebCore {
@@ -41,18 +40,7 @@ class Element;
 class Frame;
 class LayoutRect;
 class RenderElement;
-
-struct AnimationUpdate {
-#if !COMPILER_SUPPORTS(NSDMI_FOR_AGGREGATES)
-    AnimationUpdate() = default;
-    AnimationUpdate(std::unique_ptr<RenderStyle> style, bool stateChanged)
-        : style(WTFMove(style))
-        , stateChanged(stateChanged)
-    { }
-#endif
-    std::unique_ptr<RenderStyle> style;
-    bool stateChanged { false };
-};
+class RenderStyle;
 
 class CSSAnimationController {
     WTF_MAKE_FAST_ALLOCATED;
@@ -61,7 +49,7 @@ public:
     ~CSSAnimationController();
 
     void cancelAnimations(Element&);
-    AnimationUpdate updateAnimations(Element&, const RenderStyle& newStyle, const RenderStyle* oldStyle);
+    bool updateAnimations(Element&, const RenderStyle& newStyle, std::unique_ptr<RenderStyle>& animatedStyle);
     std::unique_ptr<RenderStyle> animatedStyleForRenderer(RenderElement&);
 
     // If possible, compute the visual extent of any transform animation on the given renderer
index e5e2c11..c195133 100644 (file)
@@ -287,7 +287,7 @@ void CompositeAnimation::updateKeyframeAnimations(Element& element, const Render
     std::swap(newAnimations, m_keyframeAnimations);
 }
 
-AnimationUpdate CompositeAnimation::animate(Element& element, const RenderStyle* currentStyle, const RenderStyle& targetStyle)
+bool CompositeAnimation::animate(Element& element, const RenderStyle* currentStyle, const RenderStyle& targetStyle, std::unique_ptr<RenderStyle>& blendedStyle)
 {
     // We don't do any transitions if we don't have a currentStyle (on startup).
     updateTransitions(element, currentStyle, targetStyle);
@@ -297,32 +297,30 @@ AnimationUpdate CompositeAnimation::animate(Element& element, const RenderStyle*
     bool animationStateChanged = false;
     bool forceStackingContext = false;
 
-    std::unique_ptr<RenderStyle> animatedStyle;
-
     if (currentStyle) {
         // Now that we have transition objects ready, let them know about the new goal state.  We want them
         // to fill in a RenderStyle*& only if needed.
         bool checkForStackingContext = false;
         for (auto& transition : m_transitions.values()) {
             bool didBlendStyle = false;
-            if (transition->animate(*this, targetStyle, animatedStyle, didBlendStyle))
+            if (transition->animate(*this, targetStyle, blendedStyle, didBlendStyle))
                 animationStateChanged = true;
 
             if (didBlendStyle)
                 checkForStackingContext |= WillChangeData::propertyCreatesStackingContext(transition->animatingProperty());
         }
 
-        if (animatedStyle && checkForStackingContext) {
+        if (blendedStyle && checkForStackingContext) {
             // Note that this is similar to code in StyleResolver::adjustRenderStyle() but only needs to consult
             // animatable properties that can trigger stacking context.
-            if (animatedStyle->opacity() < 1.0f
-                || animatedStyle->hasTransformRelatedProperty()
-                || animatedStyle->hasMask()
-                || animatedStyle->clipPath()
-                || animatedStyle->boxReflect()
-                || animatedStyle->hasFilter()
+            if (blendedStyle->opacity() < 1.0f
+                || blendedStyle->hasTransformRelatedProperty()
+                || blendedStyle->hasMask()
+                || blendedStyle->clipPath()
+                || blendedStyle->boxReflect()
+                || blendedStyle->hasFilter()
 #if ENABLE(FILTERS_LEVEL_2)
-                || animatedStyle->hasBackdropFilter()
+                || blendedStyle->hasBackdropFilter()
 #endif
                 )
             forceStackingContext = true;
@@ -335,7 +333,7 @@ AnimationUpdate CompositeAnimation::animate(Element& element, const RenderStyle*
         RefPtr<KeyframeAnimation> keyframeAnim = m_keyframeAnimations.get(name);
         if (keyframeAnim) {
             bool didBlendStyle = false;
-            if (keyframeAnim->animate(*this, targetStyle, animatedStyle, didBlendStyle))
+            if (keyframeAnim->animate(*this, targetStyle, blendedStyle, didBlendStyle))
                 animationStateChanged = true;
 
             forceStackingContext |= didBlendStyle && keyframeAnim->triggersStackingContext();
@@ -347,12 +345,12 @@ AnimationUpdate CompositeAnimation::animate(Element& element, const RenderStyle*
     // While an animation is applied but has not finished, or has finished but has an animation-fill-mode of forwards or both,
     // the user agent must act as if the will-change property ([css-will-change-1]) on the element additionally
     // includes all the properties animated by the animation.
-    if (forceStackingContext && animatedStyle) {
-        if (animatedStyle->hasAutoZIndex())
-            animatedStyle->setZIndex(0);
+    if (forceStackingContext && blendedStyle) {
+        if (blendedStyle->hasAutoZIndex())
+            blendedStyle->setZIndex(0);
     }
 
-    return { WTFMove(animatedStyle), animationStateChanged };
+    return animationStateChanged;
 }
 
 std::unique_ptr<RenderStyle> CompositeAnimation::getAnimatedStyle() const
index fb8aa1e..b20f00e 100644 (file)
@@ -28,7 +28,6 @@
 
 #pragma once
 
-#include "CSSAnimationController.h"
 #include "ImplicitAnimation.h"
 #include "KeyframeAnimation.h"
 #include <wtf/HashMap.h>
@@ -55,7 +54,7 @@ public:
     
     void clearElement();
 
-    AnimationUpdate animate(Element&, const RenderStyle* currentStyle, const RenderStyle& targetStyle);
+    bool animate(Element&, const RenderStyle* currentStyle, const RenderStyle& targetStyle, std::unique_ptr<RenderStyle>& blendedStyle);
     std::unique_ptr<RenderStyle> getAnimatedStyle() const;
     bool computeExtentOfTransformAnimation(LayoutRect&) const;
 
index 7279362..b2a6b07 100644 (file)
@@ -26,6 +26,7 @@
 #include "RenderElement.h"
 
 #include "AXObjectCache.h"
+#include "CSSAnimationController.h"
 #include "ContentData.h"
 #include "CursorList.h"
 #include "ElementChildIterator.h"
@@ -101,6 +102,7 @@ inline RenderElement::RenderElement(ContainerNode& elementOrDocument, RenderStyl
     , m_baseTypeFlags(baseTypeFlags)
     , m_ancestorLineBoxDirty(false)
     , m_hasInitializedStyle(false)
+    , m_hasInitialAnimatedStyle(false)
     , m_renderInlineAlwaysCreatesLineBoxes(false)
     , m_renderBoxNeedsLazyRepaint(false)
     , m_hasPausedImageAnimations(false)
@@ -1102,6 +1104,9 @@ void RenderElement::willBeDestroyed()
     if (m_style.hasFixedBackgroundImage() && !settings().fixedBackgroundsPaintRelativeToDocument())
         view().frameView().removeSlowRepaintObject(this);
 
+    if (element())
+        animation().cancelAnimations(*element());
+
     destroyLeftoverChildren();
 
     unregisterForVisibleInViewportCallback();
index 51827ce..8a7bf49 100644 (file)
@@ -131,6 +131,9 @@ public:
     // and so only should be called when the style is known not to have changed (or from setStyle).
     void setStyleInternal(RenderStyle&& style) { m_style = WTFMove(style); }
 
+    bool hasInitialAnimatedStyle() const { return m_hasInitialAnimatedStyle; }
+    void setHasInitialAnimatedStyle(bool b) { m_hasInitialAnimatedStyle = b; }
+
     // Repaint only if our old bounds and new bounds are different. The caller may pass in newBounds and newOutlineBox if they are known.
     bool repaintAfterLayoutIfNeeded(const RenderLayerModelObject* repaintContainer, const LayoutRect& oldBounds, const LayoutRect& oldOutlineBox, const LayoutRect* newBoundsPtr = nullptr, const LayoutRect* newOutlineBoxPtr = nullptr);
 
@@ -326,6 +329,7 @@ private:
     unsigned m_baseTypeFlags : 6;
     unsigned m_ancestorLineBoxDirty : 1;
     unsigned m_hasInitializedStyle : 1;
+    unsigned m_hasInitialAnimatedStyle : 1;
 
     unsigned m_renderInlineAlwaysCreatesLineBoxes : 1;
     unsigned m_renderBoxNeedsLazyRepaint : 1;
index eca8818..f662a00 100644 (file)
@@ -307,10 +307,7 @@ void RenderTreeUpdater::updateElementRenderer(Element& element, const Style::Ele
             // We may be tearing down a descendant renderer cached in renderTreePosition.
             renderTreePosition().invalidateNextSibling();
         }
-
-        // display:none cancels animations.
-        auto teardownType = update.style->display() == NONE ? TeardownType::RendererUpdateCancelingAnimations : TeardownType::RendererUpdate;
-        tearDownRenderers(element, teardownType);
+        tearDownRenderers(element, TeardownType::KeepHoverAndActive);
     }
 
     bool hasDisplayContents = update.style->display() == CONTENTS;
@@ -399,6 +396,14 @@ void RenderTreeUpdater::createRenderer(Element& element, RenderStyle&& style)
 
     element.setRenderer(newRenderer);
 
+    auto& initialStyle = newRenderer->style();
+    std::unique_ptr<RenderStyle> animatedStyle;
+    newRenderer->animation().updateAnimations(element, initialStyle, animatedStyle);
+    if (animatedStyle) {
+        newRenderer->setStyleInternal(WTFMove(*animatedStyle));
+        newRenderer->setHasInitialAnimatedStyle(true);
+    }
+
     newRenderer->initializeStyle();
 
 #if ENABLE(FULLSCREEN_API)
@@ -520,11 +525,6 @@ void RenderTreeUpdater::invalidateWhitespaceOnlyTextSiblingsAfterAttachIfNeeded(
     }
 }
 
-void RenderTreeUpdater::tearDownRenderers(Element& root)
-{
-    tearDownRenderers(root, TeardownType::Full);
-}
-
 void RenderTreeUpdater::tearDownRenderers(Element& root, TeardownType teardownType)
 {
     WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
@@ -537,18 +537,12 @@ void RenderTreeUpdater::tearDownRenderers(Element& root, TeardownType teardownTy
         teardownStack.append(&element);
     };
 
-    auto& animationController = root.document().frame()->animation();
-
     auto pop = [&] (unsigned depth) {
         while (teardownStack.size() > depth) {
             auto& element = *teardownStack.takeLast();
 
-            if (teardownType == TeardownType::Full || teardownType == TeardownType::RendererUpdateCancelingAnimations)
-                animationController.cancelAnimations(element);
-
-            if (teardownType == TeardownType::Full)
+            if (teardownType != TeardownType::KeepHoverAndActive)
                 element.clearHoverAndActiveStatusBeforeDetachingRenderer();
-
             element.clearStyleDerivedDataBeforeDetachingRenderer();
 
             if (auto* renderer = element.renderer()) {
index b3fb7b8..03e67bc 100644 (file)
@@ -47,7 +47,8 @@ public:
 
     void commit(std::unique_ptr<const Style::Update>);
 
-    static void tearDownRenderers(Element&);
+    enum class TeardownType { Normal, KeepHoverAndActive };
+    static void tearDownRenderers(Element&, TeardownType = TeardownType::Normal);
     static void tearDownRenderer(Text&);
 
     class FirstLetter;
@@ -82,9 +83,6 @@ private:
     void popParent();
     void popParentsToDepth(unsigned depth);
 
-    enum class TeardownType { Full, RendererUpdate, RendererUpdateCancelingAnimations };
-    static void tearDownRenderers(Element&, TeardownType);
-
     RenderView& renderView();
 
     Document& m_document;
index b615fc3..8c0c577 100644 (file)
@@ -241,25 +241,52 @@ const RenderStyle* TreeResolver::parentBoxStyle() const
 
 ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderStyle> newStyle, Element& element, Change parentChange)
 {
-    auto& animationController = element.document().frame()->animation();
-
-    auto* oldStyle = renderOrDisplayContentsStyle(element);
-    auto animationUpdate = animationController.updateAnimations(element, *newStyle, oldStyle);
+    auto validity = element.styleValidity();
+    bool recompositeLayer = element.styleResolutionShouldRecompositeLayer();
+
+    auto makeUpdate = [&] (std::unique_ptr<RenderStyle> style, Change change) {
+        if (validity >= Validity::SubtreeInvalid)
+            change = std::max(change, validity == Validity::SubtreeAndRenderersInvalid ? Detach : Force);
+        if (parentChange >= Force)
+            change = std::max(change, parentChange);
+        return ElementUpdate { WTFMove(style), change, recompositeLayer };
+    };
 
-    if (animationUpdate.style)
-        newStyle = WTFMove(animationUpdate.style);
+    auto* renderer = element.renderer();
 
-    auto change = oldStyle ? determineChange(*oldStyle, *newStyle) : Detach;
+    bool shouldReconstruct = validity >= Validity::SubtreeAndRenderersInvalid || parentChange == Detach;
+    if (shouldReconstruct)
+        return makeUpdate(WTFMove(newStyle), Detach);
 
-    auto validity = element.styleValidity();
-    if (validity >= Validity::SubtreeInvalid)
-        change = std::max(change, validity == Validity::SubtreeAndRenderersInvalid ? Detach : Force);
-    if (parentChange >= Force)
-        change = std::max(change, parentChange);
+    if (!renderer) {
+        auto change = Detach;
+        if (auto* oldStyle = renderOrDisplayContentsStyle(element))
+            change = determineChange(*oldStyle, *newStyle);
+        return makeUpdate(WTFMove(newStyle), change);
+    }
 
-    bool shouldRecompositeLayer = element.styleResolutionShouldRecompositeLayer() || animationUpdate.stateChanged;
+    std::unique_ptr<RenderStyle> animatedStyle;
+    if (element.document().frame()->animation().updateAnimations(element, *newStyle, animatedStyle))
+        recompositeLayer = true;
+
+    if (animatedStyle) {
+        auto change = determineChange(renderer->style(), *animatedStyle);
+        if (renderer->hasInitialAnimatedStyle()) {
+            renderer->setHasInitialAnimatedStyle(false);
+            // When we initialize a newly created renderer with initial animated style we don't inherit it to descendants.
+            // The first animation frame needs to correct this.
+            // FIXME: We should compute animated style correctly during initial style resolution when we don't have renderers yet.
+            //        https://bugs.webkit.org/show_bug.cgi?id=171926
+            change = std::max(change, Inherit);
+        }
+        // If animation forces render tree reconstruction pass the original style. The animation will be applied on renderer construction.
+        // FIXME: We should always use the animated style here.
+        auto style = change == Detach ? WTFMove(newStyle) : WTFMove(animatedStyle);
+        return makeUpdate(WTFMove(style), change);
+    }
 
-    return { WTFMove(newStyle), change, shouldRecompositeLayer };
+    auto change = determineChange(renderer->style(), *newStyle);
+    return makeUpdate(WTFMove(newStyle), change);
 }
 
 void TreeResolver::pushParent(Element& element, const RenderStyle& style, Change change)