[Web Animations] REGRESSION: transition added immediately after element creation...
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Jul 2018 08:33:31 +0000 (08:33 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Jul 2018 08:33:31 +0000 (08:33 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187942

Reviewed by Dean Jackson.

Source/WebCore:

Tests: webanimations/accelerated-transition-by-removing-property.html
       webanimations/partly-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):

LayoutTests:

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.

We also add a test that checks that we do not create a composited layer when several transitions,
with only one being potentially accelerated, target the same element.

* webanimations/accelerated-transition-by-removing-property-expected.html: Added.
* webanimations/accelerated-transition-by-removing-property.html: Added.
* webanimations/partly-accelerated-transition-by-removing-property-expected.txt: Added.
* webanimations/partly-accelerated-transition-by-removing-property.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/webanimations/accelerated-transition-by-removing-property-expected.html [new file with mode: 0644]
LayoutTests/webanimations/accelerated-transition-by-removing-property.html [new file with mode: 0644]
LayoutTests/webanimations/partly-accelerated-transition-by-removing-property-expected.txt [new file with mode: 0644]
LayoutTests/webanimations/partly-accelerated-transition-by-removing-property.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/animation/DocumentTimeline.cpp
Source/WebCore/animation/DocumentTimeline.h
Source/WebCore/animation/KeyframeEffectReadOnly.h
Source/WebCore/style/StyleTreeResolver.cpp

index 7245b76..e27824f 100644 (file)
@@ -1,3 +1,25 @@
+2018-07-26  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 Dean Jackson.
+
+        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.
+
+        We also add a test that checks that we do not create a composited layer when several transitions,
+        with only one being potentially accelerated, target the same element.
+
+        * webanimations/accelerated-transition-by-removing-property-expected.html: Added.
+        * webanimations/accelerated-transition-by-removing-property.html: Added.
+        * webanimations/partly-accelerated-transition-by-removing-property-expected.txt: Added.
+        * webanimations/partly-accelerated-transition-by-removing-property.html: Added.
+
 2018-07-26  Basuke Suzuki  <Basuke.Suzuki@sony.com>
 
         [Curl] Test gardening
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 (file)
index 0000000..3747a24
--- /dev/null
@@ -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 (file)
index 0000000..e5997e2
--- /dev/null
@@ -0,0 +1,29 @@
+<!DOCTYPE html><!-- webkit-test-runner [ enableWebAnimationsCSSIntegration=true ] -->
+<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>
diff --git a/LayoutTests/webanimations/partly-accelerated-transition-by-removing-property-expected.txt b/LayoutTests/webanimations/partly-accelerated-transition-by-removing-property-expected.txt
new file mode 100644 (file)
index 0000000..8b13789
--- /dev/null
@@ -0,0 +1 @@
+
diff --git a/LayoutTests/webanimations/partly-accelerated-transition-by-removing-property.html b/LayoutTests/webanimations/partly-accelerated-transition-by-removing-property.html
new file mode 100644 (file)
index 0000000..503df5f
--- /dev/null
@@ -0,0 +1,29 @@
+<!DOCTYPE html><!-- webkit-test-runner [ enableWebAnimationsCSSIntegration=true ] -->
+<body>
+<pre id="results"></pre>
+<div id="target" style="width: 100px; height: 100px; background-color: black;"></div>
+<script>
+
+if (window.testRunner) {
+    testRunner.waitUntilDone();
+    testRunner.dumpAsText();
+}
+
+const target = document.getElementById("target");
+target.style.opacity = "0.5";
+target.style.marginLeft = "100px";
+setTimeout(() => {
+    target.style.removeProperty("opacity");
+    target.style.removeProperty("margin-left");
+    target.style.transitionProperty = "margin-left opacity";
+    target.style.transitionDuration = "1s";
+    requestAnimationFrame(() => {
+        if (window.internals)
+            document.getElementById("results").innerText = internals.layerTreeAsText(document);
+        if (window.testRunner)
+            testRunner.notifyDone();
+    });
+});
+
+</script>
+</body>
index e16c2a6..03935da 100644 (file)
@@ -1,3 +1,31 @@
+2018-07-26  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 Dean Jackson.
+
+        Tests: webanimations/accelerated-transition-by-removing-property.html
+               webanimations/partly-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-25  Chris Dumez  <cdumez@apple.com>
 
         Allow ActiveDOMObject's canSuspend / suspend / resume overrides to destroy ActiveDOMObjects
index d6308ee..9e8c30f 100644 (file)
@@ -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
index 58c9ae9..d267d4f 100644 (file)
@@ -60,6 +60,7 @@ public:
     void animationAcceleratedRunningStateDidChange(WebAnimation&);
     void applyPendingAcceleratedAnimations();
     bool runningAnimationsForElementAreAllAccelerated(Element&);
+    bool resolveAnimationsForElement(Element&, RenderStyle&);
     void detachFromDocument();
 
     void enqueueAnimationPlaybackEvent(AnimationPlaybackEvent&);
index 321a7bb..eeab014 100644 (file)
@@ -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;
index f4c5c7c..29c2be0 100644 (file)
@@ -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();