Attachment 345786 Details for Bug 187942 – Patch

[patch] Patch

bug-187942-20180725234400.patch (text/plain), 10.55 KB, created by Antoine Quint on 2018-07-25 14:44:02 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Antoine Quint

Created: 2018-07-25 14:44:02 PDT

Size: 10.55 KB

patch

obsolete

>Subversion Revision: 234170
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index c9e08a60eaeb350a48c50e5327d73a74a3f5d230..5d943c37d5e39826aa564f60952d1ff6fb767cb7 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,30 @@
>+2018-07-25  Antoine Quint  <graouts@apple.com>
>+
>+        [Web Animations] REGRESSION: transition added immediately after element creation doesn't work
>+        https://bugs.webkit.org/show_bug.cgi?id=187942
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Test: webanimations/accelerated-transition-by-removing-property.html
>+
>+        The Style::TreeResolver::createAnimatedElementUpdate() function expected a flag to be set that indicates that
>+        running animations on an element should yield a change in composited state for that element's layer. We did not
>+        have code accounting for this flag in the Web Animations engine. We now have a resolveAnimationsForElement()
>+        method on DocumentTimeline which looks at all animations resolved on the element and see if all of them are
>+        running accelerated and whether at least one of them is pending. In that case, we set the shouldRecompositeLayer
>+        flag in createAnimatedElementUpdate() to true which guarantees the element's layer will have a backing when
>+        we attempt to start the animation in KeyframeEffectReadOnly::applyPendingAcceleratedActions() where we would
>+        have previously failed to have layer-backed renderer to perform an accelerated animation on (under certain
>+        circumstances, see test).
>+
>+        * animation/DocumentTimeline.cpp:
>+        (WebCore::DocumentTimeline::resolveAnimationsForElement):
>+        * animation/DocumentTimeline.h:
>+        * animation/KeyframeEffectReadOnly.h:
>+        (WebCore::KeyframeEffectReadOnly::hasPendingAcceleratedAction const):
>+        * style/StyleTreeResolver.cpp:
>+        (WebCore::Style::TreeResolver::createAnimatedElementUpdate):
>+
> 2018-07-24  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r234121.
>diff --git a/Source/WebCore/animation/DocumentTimeline.cpp b/Source/WebCore/animation/DocumentTimeline.cpp
>index d6308ee463ded95fbd2d0c8615b82d3c92924eae..9e8c30fcdb5c5d9fd01d70da4411d35af22545af 100644
>--- a/Source/WebCore/animation/DocumentTimeline.cpp
>+++ b/Source/WebCore/animation/DocumentTimeline.cpp
>@@ -27,6 +27,7 @@
> #include "DocumentTimeline.h"
> 
> #include "AnimationPlaybackEvent.h"
>+#include "CSSPropertyAnimation.h"
> #include "DOMWindow.h"
> #include "DeclarativeAnimation.h"
> #include "Document.h"
>@@ -409,6 +410,34 @@ void DocumentTimeline::applyPendingAcceleratedAnimations()
>     m_acceleratedAnimationsPendingRunningStateChange.clear();
> }
> 
>+bool DocumentTimeline::resolveAnimationsForElement(Element& element, RenderStyle& targetStyle)
>+{
>+    bool hasNonAcceleratedAnimations = false;
>+    bool hasPendingAcceleratedAnimations = true;
>+    for (const auto& animation : animationsForElement(element)) {
>+        animation->resolve(targetStyle);
>+        if (!hasNonAcceleratedAnimations) {
>+            if (auto* effect = animation->effect()) {
>+                if (is<KeyframeEffectReadOnly>(effect)) {
>+                    auto* keyframeEffect = downcast<KeyframeEffectReadOnly>(effect);
>+                    for (auto cssPropertyId : keyframeEffect->animatedProperties()) {
>+                        if (!CSSPropertyAnimation::animationOfPropertyIsAccelerated(cssPropertyId)) {
>+                            hasNonAcceleratedAnimations = true;
>+                            continue;
>+                        }
>+                        if (!hasPendingAcceleratedAnimations)
>+                            hasPendingAcceleratedAnimations = keyframeEffect->hasPendingAcceleratedAction();
>+                    }
>+                }
>+            }
>+        }
>+    }
>+
>+    // If there are no non-accelerated animations and we've encountered at least one pending
>+    // accelerated animation, we should recomposite this element's layer for animation purposes.
>+    return !hasNonAcceleratedAnimations && hasPendingAcceleratedAnimations;
>+}
>+
> bool DocumentTimeline::runningAnimationsForElementAreAllAccelerated(Element& element)
> {
>     // FIXME: This will let animations run using hardware compositing even if later in the active
>diff --git a/Source/WebCore/animation/DocumentTimeline.h b/Source/WebCore/animation/DocumentTimeline.h
>index 58c9ae9976f306da958722adceb5f2c72f87c26d..d267d4f6cca954213f67e064969688d0297a5817 100644
>--- a/Source/WebCore/animation/DocumentTimeline.h
>+++ b/Source/WebCore/animation/DocumentTimeline.h
>@@ -60,6 +60,7 @@ public:
>     void animationAcceleratedRunningStateDidChange(WebAnimation&);
>     void applyPendingAcceleratedAnimations();
>     bool runningAnimationsForElementAreAllAccelerated(Element&);
>+    bool resolveAnimationsForElement(Element&, RenderStyle&);
>     void detachFromDocument();
> 
>     void enqueueAnimationPlaybackEvent(AnimationPlaybackEvent&);
>diff --git a/Source/WebCore/animation/KeyframeEffectReadOnly.h b/Source/WebCore/animation/KeyframeEffectReadOnly.h
>index 321a7bb4231df3f7a4ff27320195b0be51386131..eeab01470c721cb079e8ab5e7204489b4a3b34c7 100644
>--- a/Source/WebCore/animation/KeyframeEffectReadOnly.h
>+++ b/Source/WebCore/animation/KeyframeEffectReadOnly.h
>@@ -108,6 +108,7 @@ public:
>     void animationSuspensionStateDidChange(bool) final;
>     void applyPendingAcceleratedActions();
>     bool isRunningAccelerated() const { return m_lastRecordedAcceleratedAction != AcceleratedAction::Stop; }
>+    bool hasPendingAcceleratedAction() const { return !m_pendingAcceleratedActions.isEmpty() && isRunningAccelerated(); }
> 
>     RenderElement* renderer() const override;
>     const RenderStyle& currentStyle() const override;
>diff --git a/Source/WebCore/style/StyleTreeResolver.cpp b/Source/WebCore/style/StyleTreeResolver.cpp
>index f4c5c7c1dce6e6cd910afd81126b5673daa943f9..29c2be02df749e567de82084890ab972cf4da326 100644
>--- a/Source/WebCore/style/StyleTreeResolver.cpp
>+++ b/Source/WebCore/style/StyleTreeResolver.cpp
>@@ -286,6 +286,8 @@ ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderSt
> {
>     auto* oldStyle = renderOrDisplayContentsStyle(element);
> 
>+    bool shouldRecompositeLayer = false;
>+
>     // New code path for CSS Animations and CSS Transitions.
>     if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
>         // First, we need to make sure that any new CSS animation occuring on this element has a matching WebAnimation
>@@ -303,17 +305,11 @@ ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderSt
>     if (auto timeline = m_document.existingTimeline()) {
>         // Now we can update all Web animations, which will include CSS Animations as well
>         // as animations created via the JS API.
>-        auto webAnimations = timeline->animationsForElement(element);
>-        if (!webAnimations.isEmpty()) {
>-            auto animatedStyle = RenderStyle::clonePtr(*newStyle);
>-            for (const auto& animation : webAnimations)
>-                animation->resolve(*animatedStyle);
>-            newStyle = WTFMove(animatedStyle);
>-        }
>+        auto animatedStyle = RenderStyle::clonePtr(*newStyle);
>+        shouldRecompositeLayer = timeline->resolveAnimationsForElement(element, *animatedStyle);
>+        newStyle = WTFMove(animatedStyle);
>     }
> 
>-    bool shouldRecompositeLayer = false;
>-
>     // Old code path for CSS Animations and CSS Transitions.
>     if (!RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
>         auto& animationController = m_document.frame()->animation();
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 5703bead75c9f688a96c640ebc944f440ec03000..67107ff2be87e5324b8359f7d6ee5136ed22a7f3 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,20 @@
>+2018-07-25  Antoine Quint  <graouts@apple.com>
>+
>+        [Web Animations] REGRESSION: transition added immediately after element creation doesn't work
>+        https://bugs.webkit.org/show_bug.cgi?id=187942
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Creating a new test that runs a transition based on an explicit value being removed in favor
>+        of the implicit value of a property that can be accelerated. To check that we indeed run the
>+        animation, we have a cache that covers the entire range of interpolated values except for the
>+        start and end values and wait 100ms after creating the transition to end the test. Prior to this
>+        patch, the element would be at its start value and a 1px red line would show to the right of the
>+        cache. With this patch, the red line is hidden by the cache as it's animated.
>+
>+        * webanimations/accelerated-transition-by-removing-property-expected.html: Added.
>+        * webanimations/accelerated-transition-by-removing-property.html: Added.
>+
> 2018-07-24  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Mark http/tests/workers/service/serviceworkerclients-matchAll.https.html as flaky.
>diff --git a/LayoutTests/webanimations/accelerated-transition-by-removing-property-expected.html b/LayoutTests/webanimations/accelerated-transition-by-removing-property-expected.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..3747a24e77942d921812121b0227d57e147308f5
>--- /dev/null
>+++ b/LayoutTests/webanimations/accelerated-transition-by-removing-property-expected.html
>@@ -0,0 +1 @@
>+<div style="position: absolute; top: 0; left: 1px; width: 798px; height: 100px; background-color: black;"></div>
>\ No newline at end of file
>diff --git a/LayoutTests/webanimations/accelerated-transition-by-removing-property.html b/LayoutTests/webanimations/accelerated-transition-by-removing-property.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..e5997e20b22ab75f7388d0a95348926593511a0e
>--- /dev/null
>+++ b/LayoutTests/webanimations/accelerated-transition-by-removing-property.html
>@@ -0,0 +1,29 @@
>+<!DOCTYPE html>
>+<body>
>+<style>
>+    div {
>+        position: absolute;
>+        height: 100px;
>+        top: 0;
>+    }
>+</style>
>+<div id="target" style="left: 0; width: 1px; background-color: red;"></div>
>+<div style="left: 1px; width: 798px; background-color: black;"></div>
>+<script>
>+
>+if (window.testRunner)
>+    testRunner.waitUntilDone();
>+
>+const target = document.getElementById("target");
>+target.style.transform = "translateX(799px)";
>+setTimeout(() => {
>+    target.style.removeProperty("transform");
>+    target.style.transition = "transform 10s linear";
>+    setTimeout(() => {
>+        if (window.testRunner)
>+            testRunner.notifyDone();
>+    }, 100);
>+});
>+
>+</script>
>+</body>

Flags:

dino: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 187942: 345640 | 345670 | 345674 | 345786