Layout Test animations/needs-layout.html is a flaky Image Failure.
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Apr 2018 07:33:57 +0000 (07:33 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Apr 2018 07:33:57 +0000 (07:33 +0000)
https://bugs.webkit.org/show_bug.cgi?id=172397

Reviewed by Dean Jackson.

Source/WebCore:

Animations that animate a transform and uses a relative value for either the x or y components
require a layout before starting, which CSSAnimationController would perform in the call to
CSSAnimationControllerPrivate::animationTimerFired() made immediately after a CSS animation was
created.

We now perform a similar task where upon setting new blending keyframes we compute a flag indicating
if the keyframe effect is animating a transform with relative x or y components. Then, when we perform
the first invalidation task, which runs in the next run loop after a change to the timing model has
been made, such as a call to play() on a CSSAnimation made in the TreeResolver::createAnimatedElementUpdate()
where the CSSAnimation was created, we call forceLayout() on this element's FrameView. We also ensure
we commit animations on the compositor immediately after that too, instead of waiting until the next
DisplayRefreshMonitor callback.

* animation/DocumentTimeline.cpp:
(WebCore::DocumentTimeline::performInvalidationTask):
(WebCore::DocumentTimeline::updateAnimations):
* animation/KeyframeEffectReadOnly.cpp:
(WebCore::KeyframeEffectReadOnly::forceLayoutIfNeeded):
(WebCore::KeyframeEffectReadOnly::setBlendingKeyframes):
(WebCore::KeyframeEffectReadOnly::computedNeedsForcedLayout):
(WebCore::KeyframeEffectReadOnly::applyPendingAcceleratedActions):
* animation/KeyframeEffectReadOnly.h:

LayoutTests:

No longer mark this test as flaky.

* platform/ios-wk2/TestExpectations:
* platform/mac-wk1/TestExpectations:
* platform/mac-wk2/TestExpectations:

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

LayoutTests/ChangeLog
LayoutTests/platform/ios-wk2/TestExpectations
LayoutTests/platform/mac-wk1/TestExpectations
LayoutTests/platform/mac-wk2/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/animation/DocumentTimeline.cpp
Source/WebCore/animation/KeyframeEffectReadOnly.cpp
Source/WebCore/animation/KeyframeEffectReadOnly.h

index 8a1937b..48bb0d1 100644 (file)
@@ -1,3 +1,16 @@
+2018-04-16  Antoine Quint  <graouts@apple.com>
+
+        Layout Test animations/needs-layout.html is a flaky Image Failure.
+        https://bugs.webkit.org/show_bug.cgi?id=172397
+
+        Reviewed by Dean Jackson.
+
+        No longer mark this test as flaky.
+
+        * platform/ios-wk2/TestExpectations:
+        * platform/mac-wk1/TestExpectations:
+        * platform/mac-wk2/TestExpectations:
+
 2018-04-16  Keith Rollin  <krollin@apple.com>
 
         REGRESSION: [mac-wk2 release] LayoutTest http/tests/security/contentSecurityPolicy/script-src-blocked-error-event.html is flaky
index deab5ea..aec5f6a 100644 (file)
@@ -1319,7 +1319,6 @@ webkit.org/b/173608 webrtc/video-replace-muted-track.html [ Skip ]
 
 webkit.org/b/177501 webrtc/video-mute.html [ Pass Timeout ]
 
-webkit.org/b/172397 [ Debug ] animations/needs-layout.html [ Pass ImageOnlyFailure ]
 webkit.org/b/172397 [ Debug ] legacy-animation-engine/animations/needs-layout.html [ Pass ImageOnlyFailure ]
 
 webkit.org/b/179176 [ Release ] svg/wicd/test-rightsizing-a.xhtml [ Pass Failure ]
index 6d2c92b..3faf69d 100644 (file)
@@ -492,7 +492,6 @@ webkit.org/b/175886 svg/animations/smil-leak-element-instances.svg [ Pass Failur
 [ HighSierra+ ] fast/forms/textarea-maxlength.html [ Crash ]
 [ HighSierra+ ] fast/text/system-font-fallback-emoji.html [ Crash ]
 
-webkit.org/b/172397 [ Debug ] animations/needs-layout.html [ Pass ImageOnlyFailure ]
 webkit.org/b/172397 [ Debug ] legacy-animation-engine/animations/needs-layout.html [ Pass ImageOnlyFailure ]
 
 webkit.org/b/179775 imported/w3c/web-platform-tests/XMLHttpRequest/firing-events-http-no-content-length.html [ Pass Failure ]
index b115038..067f73a 100644 (file)
@@ -730,7 +730,6 @@ webkit.org/b/172201 webaudio/silent-audio-interrupted-in-background.html [ Pass
 
 webkit.org/b/167757 workers/bomb.html [ Pass Timeout ]
 
-webkit.org/b/172397 [ Debug ] animations/needs-layout.html [ Pass ImageOnlyFailure ]
 webkit.org/b/172397 [ Debug ] legacy-animation-engine/animations/needs-layout.html [ Pass ImageOnlyFailure ]
 
 # Touch events are not available on open source bots, thus only tested on Mac.
index c32b0d4..920e9ec 100644 (file)
@@ -1,3 +1,33 @@
+2018-04-16  Antoine Quint  <graouts@apple.com>
+
+        Layout Test animations/needs-layout.html is a flaky Image Failure.
+        https://bugs.webkit.org/show_bug.cgi?id=172397
+
+        Reviewed by Dean Jackson.
+
+        Animations that animate a transform and uses a relative value for either the x or y components
+        require a layout before starting, which CSSAnimationController would perform in the call to
+        CSSAnimationControllerPrivate::animationTimerFired() made immediately after a CSS animation was
+        created.
+
+        We now perform a similar task where upon setting new blending keyframes we compute a flag indicating
+        if the keyframe effect is animating a transform with relative x or y components. Then, when we perform
+        the first invalidation task, which runs in the next run loop after a change to the timing model has
+        been made, such as a call to play() on a CSSAnimation made in the TreeResolver::createAnimatedElementUpdate()
+        where the CSSAnimation was created, we call forceLayout() on this element's FrameView. We also ensure
+        we commit animations on the compositor immediately after that too, instead of waiting until the next
+        DisplayRefreshMonitor callback.
+
+        * animation/DocumentTimeline.cpp:
+        (WebCore::DocumentTimeline::performInvalidationTask):
+        (WebCore::DocumentTimeline::updateAnimations):
+        * animation/KeyframeEffectReadOnly.cpp:
+        (WebCore::KeyframeEffectReadOnly::forceLayoutIfNeeded):
+        (WebCore::KeyframeEffectReadOnly::setBlendingKeyframes):
+        (WebCore::KeyframeEffectReadOnly::computedNeedsForcedLayout):
+        (WebCore::KeyframeEffectReadOnly::applyPendingAcceleratedActions):
+        * animation/KeyframeEffectReadOnly.h:
+
 2018-04-16  Pablo Saavedra  <psaavedra@igalia.com>
 
         Inconsistent EGL defines in ImageBufferCairo
index 9306f42..8e8cc23 100644 (file)
@@ -176,6 +176,8 @@ void DocumentTimeline::performInvalidationTask()
             downcast<DeclarativeAnimation>(*animation).invalidateDOMEvents();
     }
 
+    applyPendingAcceleratedAnimations();
+
     updateAnimationSchedule();
     m_cachedCurrentTime = std::nullopt;
 }
@@ -244,8 +246,6 @@ void DocumentTimeline::updateAnimations()
         m_document->updateStyleIfNeeded();
     }
 
-    applyPendingAcceleratedAnimations();
-
     // Time has advanced, the timing model requires invalidation now.
     timingModelDidChange();
 }
@@ -332,8 +332,16 @@ void DocumentTimeline::animationAcceleratedRunningStateDidChange(WebAnimation& a
 
 void DocumentTimeline::applyPendingAcceleratedAnimations()
 {
-    for (auto& animation : m_acceleratedAnimationsPendingRunningStateChange)
+    bool hasForcedLayout = false;
+    for (auto& animation : m_acceleratedAnimationsPendingRunningStateChange) {
+        if (!hasForcedLayout) {
+            auto* effect = animation->effect();
+            if (is<KeyframeEffectReadOnly>(effect))
+                hasForcedLayout |= downcast<KeyframeEffectReadOnly>(effect)->forceLayoutIfNeeded();
+        }
         animation->applyPendingAcceleratedActions();
+    }
+
     m_acceleratedAnimationsPendingRunningStateChange.clear();
 }
 
index f700df8..843bab6 100644 (file)
@@ -46,6 +46,7 @@
 #include "StylePendingResources.h"
 #include "StyleResolver.h"
 #include "TimingFunction.h"
+#include "TranslateTransformOperation.h"
 #include "WillChangeData.h"
 #include <wtf/UUID.h>
 
@@ -693,10 +694,28 @@ void KeyframeEffectReadOnly::updateBlendingKeyframes()
     setBlendingKeyframes(keyframeList);
 }
 
+bool KeyframeEffectReadOnly::forceLayoutIfNeeded()
+{
+    if (!m_needsForcedLayout || !m_target)
+        return false;
+
+    auto* renderer = m_target->renderer();
+    if (!renderer || !renderer->parent())
+        return false;
+
+    auto* frameView = m_target->document().view();
+    if (!frameView)
+        return false;
+
+    frameView->forceLayout();
+    return true;
+}
+
 void KeyframeEffectReadOnly::setBlendingKeyframes(KeyframeList& blendingKeyframes)
 {
     m_blendingKeyframes = WTFMove(blendingKeyframes);
 
+    computedNeedsForcedLayout();
     computeStackingContextImpact();
     computeShouldRunAccelerated();
 
@@ -896,6 +915,34 @@ bool KeyframeEffectReadOnly::stylesWouldYieldNewCSSTransitionsBlendingKeyframes(
     return !CSSPropertyAnimation::propertiesEqual(property, m_blendingKeyframes[1].style(), &newStyle);
 }
 
+void KeyframeEffectReadOnly::computedNeedsForcedLayout()
+{
+    m_needsForcedLayout = false;
+    if (is<CSSTransition>(animation()) || !m_blendingKeyframes.containsProperty(CSSPropertyTransform))
+        return;
+
+    size_t numberOfKeyframes = m_blendingKeyframes.size();
+    for (size_t i = 0; i < numberOfKeyframes; i++) {
+        auto* keyframeStyle = m_blendingKeyframes[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_needsForcedLayout = true;
+                        return;
+                    }
+                }
+            }
+        }
+    }
+}
+
 void KeyframeEffectReadOnly::computeStackingContextImpact()
 {
     m_triggersStackingContext = false;
@@ -1180,6 +1227,11 @@ void KeyframeEffectReadOnly::addPendingAcceleratedAction(AcceleratedAction actio
 
 void KeyframeEffectReadOnly::applyPendingAcceleratedActions()
 {
+    // Once an accelerated animation has been committed, we no longer want to force a layout.
+    // This should have been performed by a call to forceLayoutIfNeeded() prior to applying
+    // pending accelerated actions.
+    m_needsForcedLayout = false;
+
     auto* renderer = this->renderer();
     if (!renderer || !renderer->isComposited())
         return;
index 5fc6f66..ce44040 100644 (file)
@@ -120,6 +120,7 @@ public:
     bool computeExtentOfTransformAnimation(LayoutRect&) const;
     bool computeTransformedExtentViaTransformList(const FloatRect&, const RenderStyle&, LayoutRect&) const;
     bool computeTransformedExtentViaMatrix(const FloatRect&, const RenderStyle&, LayoutRect&) const;
+    bool forceLayoutIfNeeded();
 
 protected:
     void copyPropertiesFromSource(Ref<KeyframeEffectReadOnly>&&);
@@ -136,6 +137,7 @@ private:
     void setAnimatedPropertiesInStyle(RenderStyle&, double);
     TimingFunction* timingFunctionForKeyframeAtIndex(size_t);
     Ref<const Animation> backingAnimationForCompositedRenderer() const;
+    void computedNeedsForcedLayout();
     void computeStackingContextImpact();
     void updateBlendingKeyframes();
     void computeCSSAnimationBlendingKeyframes();
@@ -149,6 +151,7 @@ private:
 #endif
 
     bool m_shouldRunAccelerated { false };
+    bool m_needsForcedLayout { false };
     bool m_triggersStackingContext { false };
     bool m_started { false };
     bool m_startedAccelerated { false };