Attachment 345841 Details for Bug 186930 – Patch

[patch] Patch

bug-186930-20180726160849.patch (text/plain), 7.79 KB, created by Antoine Quint on 2018-07-26 07:08:51 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Antoine Quint

Created: 2018-07-26 07:08:51 PDT

Size: 7.79 KB

patch

obsolete

>Subversion Revision: 234250
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 03935da6a37b9dd240f596b47a0652f0375ca3a8..8a4767ba1a070722210e6dababc62bd179cce8a5 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,30 @@
>+2018-07-26  Antoine Quint  <graouts@apple.com>
>+
>+        [Web Animations] Accelerated animations don't respect a positive delay value
>+        https://bugs.webkit.org/show_bug.cgi?id=186930
>+        <rdar://problem/41393393>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Test: webanimations/accelerated-animation-with-delay.html
>+
>+        We would mistakenly clear the list of pending accelerated actions in KeyframeEffectReadOnly::applyPendingAcceleratedActions()
>+        even if we failed to proceed because of the target element's renderer not being composited yet. Now, we clear the list after
>+        we've established we can apply them, and if we can't we inform the animation with a call to acceleratedStateDidChange() which
>+        will in turn call into the document timeline so that we may consider these animations on the next tick.
>+
>+        For this to work correctly we must make a change to DocumentTimeline::applyPendingAcceleratedAnimations() which only cleared
>+        its list of pending accelerated animations _after_ iterating through them, which would be problematic since we would now add
>+        animations to this list. That list is now copied and cleared prior to iterating through its members.
>+
>+        We also fix the naming of the m_pendingAcceleratedActions copy in KeyframeEffectReadOnly::applyPendingAcceleratedActions()
>+        to have a more similar name.
>+
>+        * animation/DocumentTimeline.cpp:
>+        (WebCore::DocumentTimeline::applyPendingAcceleratedAnimations):
>+        * animation/KeyframeEffectReadOnly.cpp:
>+        (WebCore::KeyframeEffectReadOnly::applyPendingAcceleratedActions):
>+
> 2018-07-26  Antoine Quint  <graouts@apple.com>
> 
>         [Web Animations] REGRESSION: transition added immediately after element creation doesn't work
>diff --git a/Source/WebCore/animation/DocumentTimeline.cpp b/Source/WebCore/animation/DocumentTimeline.cpp
>index 9e8c30fcdb5c5d9fd01d70da4411d35af22545af..a386c18c177d5df98a856e3246524559791eedc4 100644
>--- a/Source/WebCore/animation/DocumentTimeline.cpp
>+++ b/Source/WebCore/animation/DocumentTimeline.cpp
>@@ -397,8 +397,11 @@ void DocumentTimeline::animationAcceleratedRunningStateDidChange(WebAnimation& a
> 
> void DocumentTimeline::applyPendingAcceleratedAnimations()
> {
>+    auto acceleratedAnimationsPendingRunningStateChange = m_acceleratedAnimationsPendingRunningStateChange;
>+    m_acceleratedAnimationsPendingRunningStateChange.clear();
>+
>     bool hasForcedLayout = false;
>-    for (auto& animation : m_acceleratedAnimationsPendingRunningStateChange) {
>+    for (auto& animation : acceleratedAnimationsPendingRunningStateChange) {
>         if (!hasForcedLayout) {
>             auto* effect = animation->effect();
>             if (is<KeyframeEffectReadOnly>(effect))
>@@ -406,8 +409,6 @@ void DocumentTimeline::applyPendingAcceleratedAnimations()
>         }
>         animation->applyPendingAcceleratedActions();
>     }
>-
>-    m_acceleratedAnimationsPendingRunningStateChange.clear();
> }
> 
> bool DocumentTimeline::resolveAnimationsForElement(Element& element, RenderStyle& targetStyle)
>diff --git a/Source/WebCore/animation/KeyframeEffectReadOnly.cpp b/Source/WebCore/animation/KeyframeEffectReadOnly.cpp
>index f54bb4c73506a71ddda23acc974d38d27ab98fb7..362dab0602b460be3f1ad2213bf89c17e1258a6e 100644
>--- a/Source/WebCore/animation/KeyframeEffectReadOnly.cpp
>+++ b/Source/WebCore/animation/KeyframeEffectReadOnly.cpp
>@@ -1264,15 +1264,17 @@ void KeyframeEffectReadOnly::applyPendingAcceleratedActions()
>     // pending accelerated actions.
>     m_needsForcedLayout = false;
> 
>-    auto pendingAccelerationActions = m_pendingAcceleratedActions;
>-    m_pendingAcceleratedActions.clear();
>-
>-    if (pendingAccelerationActions.isEmpty())
>+    if (m_pendingAcceleratedActions.isEmpty())
>         return;
> 
>     auto* renderer = this->renderer();
>-    if (!renderer || !renderer->isComposited())
>+    if (!renderer || !renderer->isComposited()) {
>+        animation()->acceleratedStateDidChange();
>         return;
>+    }
>+
>+    auto pendingAcceleratedActions = m_pendingAcceleratedActions;
>+    m_pendingAcceleratedActions.clear();
> 
>     auto* compositedRenderer = downcast<RenderBoxModelObject>(renderer);
> 
>@@ -1284,7 +1286,7 @@ void KeyframeEffectReadOnly::applyPendingAcceleratedActions()
>     if (timing()->delay() < 0_s)
>         timeOffset = -timing()->delay().seconds();
> 
>-    for (const auto& action : pendingAccelerationActions) {
>+    for (const auto& action : pendingAcceleratedActions) {
>         switch (action) {
>         case AcceleratedAction::Play:
>             if (!compositedRenderer->startAnimation(timeOffset, backingAnimationForCompositedRenderer().ptr(), m_blendingKeyframes)) {
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index e27824fbb00fe12e253c4ee4d1abba7e7e7386ae..b6e1350762398db9c9190f176220fb477717eb86 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,21 @@
>+2018-07-26  Antoine Quint  <graouts@apple.com>
>+
>+        [Web Animations] Accelerated animations don't respect a positive delay value
>+        https://bugs.webkit.org/show_bug.cgi?id=186930
>+        <rdar://problem/41393393>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Creating a new test that runs an accelerated animation on a non-positioned element with a small
>+        positive delay. Prior to this patch, the element would move slightly when the delay elapses but
>+        wouldn't animate towards the target value. With this patch, it animatea correctly. To check
>+        that it does animate, we add a cache over the element except for the first 25px which is the area
>+        within which it might have moved prior to this patch. This way the test only passes if the element
>+        is fully hidden by the cache.
>+
>+        * webanimations/accelerated-animation-with-delay-expected.html: Added.
>+        * webanimations/accelerated-animation-with-delay.html: Added.
>+
> 2018-07-26  Antoine Quint  <graouts@apple.com>
> 
>         [Web Animations] REGRESSION: transition added immediately after element creation doesn't work
>diff --git a/LayoutTests/webanimations/accelerated-animation-with-delay-expected.html b/LayoutTests/webanimations/accelerated-animation-with-delay-expected.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..89eaae6503641dca0f7a95bb54bf5e6ca006308c
>--- /dev/null
>+++ b/LayoutTests/webanimations/accelerated-animation-with-delay-expected.html
>@@ -0,0 +1 @@
>+<div style="position: absolute; left: 25px; top: 0; width: 775px; height: 100px; background-color: black;"></div>
>diff --git a/LayoutTests/webanimations/accelerated-animation-with-delay.html b/LayoutTests/webanimations/accelerated-animation-with-delay.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..a8eefef69aac86d49911d7a9e6c9bd53b01a6357
>--- /dev/null
>+++ b/LayoutTests/webanimations/accelerated-animation-with-delay.html
>@@ -0,0 +1,44 @@
>+<!DOCTYPE html>
>+<body>
>+<style>
>+    
>+    body {
>+        margin: 0;
>+    }
>+    
>+    div {
>+        height: 100px;
>+    }
>+
>+    #target {
>+        left: 0;
>+        width: 100px;
>+        background-color: red;
>+    }
>+
>+    #cache {
>+        top: 0;
>+        left: 25px;
>+        width: 775px;
>+        position: absolute;
>+        background-color: black;
>+    }
>+
>+</style>
>+<div id="target"></div>
>+<div id="cache"></div>
>+<script>
>+
>+if (window.testRunner)
>+    testRunner.waitUntilDone();
>+
>+document.getElementById("target").animate({ transform: "translateX(700px)" }, { delay: 10, duration: 1000 })
>+requestAnimationFrame(() => {
>+    setTimeout(() => {
>+        if (window.testRunner)
>+            testRunner.notifyDone();
>+    }, 100);
>+});
>+
>+</script>
>+</body>

Flags:

dino: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 186930: 345841