Cancelled transitions on Google image search leave content with opacity 0 sometimes
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Sep 2019 01:39:59 +0000 (01:39 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Sep 2019 01:39:59 +0000 (01:39 +0000)
https://bugs.webkit.org/show_bug.cgi?id=201482
rdar://problem/54921036

Reviewed by Tim Horton.
Source/WebCore:

If, in a single rendering update, we started an accelerated opacity transition, and then removed
it, we'd still push the transition onto the CALayer with fillForwards and never remove it, so its
effects would last forever.

Fix by making GraphicsLayerCA::removeAnimation() remove animations from the uncomittedAnimations
list as well.

Also fix layer names in debug; if a layer's primaryLayerID changed, we'd fail to rename the
CALayer, causing confusion when logging at layer dumps. Fix by adding the layer ID just
before pushing the name to the platform layer.

Some drive-by logging cleanup.

Test: legacy-animation-engine/compositing/transitions/add-remove-transition.html

* platform/graphics/GraphicsLayer.cpp:
(WebCore::GraphicsLayer::debugName const):
* platform/graphics/GraphicsLayer.h:
* platform/graphics/ca/GraphicsLayerCA.cpp:
(WebCore::GraphicsLayerCA::setName):
(WebCore::GraphicsLayerCA::debugName const):
(WebCore::GraphicsLayerCA::addAnimation):
(WebCore::GraphicsLayerCA::pauseAnimation):
(WebCore::GraphicsLayerCA::seekAnimation):
(WebCore::GraphicsLayerCA::removeAnimation):
(WebCore::GraphicsLayerCA::platformCALayerAnimationStarted):
(WebCore::GraphicsLayerCA::platformCALayerAnimationEnded):
(WebCore::GraphicsLayerCA::updateNames):
(WebCore::GraphicsLayerCA::createTransformAnimationsFromKeyframes):
* platform/graphics/ca/GraphicsLayerCA.h:
* rendering/RenderLayerCompositor.cpp:
(WebCore::RenderLayerCompositor::logLayerInfo):

LayoutTests:

* legacy-animation-engine/compositing/transitions/add-remove-transition-expected.html: Added.
* legacy-animation-engine/compositing/transitions/add-remove-transition.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/legacy-animation-engine/compositing/transitions/add-remove-transition-expected.html [new file with mode: 0644]
LayoutTests/legacy-animation-engine/compositing/transitions/add-remove-transition.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/GraphicsLayer.cpp
Source/WebCore/platform/graphics/GraphicsLayer.h
Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
Source/WebCore/rendering/RenderLayerCompositor.cpp

index 5272aba..a7f299a 100644 (file)
         * inspector/debugger/resources/tail-deleted-frames-this-value.js:
         * inspector/timeline/line-column-expected.txt:
 
+2019-09-04  Simon Fraser  <simon.fraser@apple.com>
+
+        Cancelled transitions on Google image search leave content with opacity 0 sometimes
+        https://bugs.webkit.org/show_bug.cgi?id=201482
+        rdar://problem/54921036
+
+        Reviewed by Tim Horton.
+
+        * legacy-animation-engine/compositing/transitions/add-remove-transition-expected.html: Added.
+        * legacy-animation-engine/compositing/transitions/add-remove-transition.html: Added.
+
 2019-09-03  Jiewen Tan  <jiewen_tan@apple.com>
 
         [WebAuthn] Enable WebAuthn by default for MobileSafari and SafariViewService
diff --git a/LayoutTests/legacy-animation-engine/compositing/transitions/add-remove-transition-expected.html b/LayoutTests/legacy-animation-engine/compositing/transitions/add-remove-transition-expected.html
new file mode 100644 (file)
index 0000000..8a3458e
--- /dev/null
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style>
+        .box {
+            width: 100px;
+            height: 100px;
+            margin: 20px;
+            background-color: blue;
+        }
+        .composited {
+            transform: translateZ(0);
+        }
+    </style>
+</head>
+<body>
+    <p>This box should end up with opacity 1</p>
+    <div id="target" class="composited box">
+    </div>
+</body>
+</html>
diff --git a/LayoutTests/legacy-animation-engine/compositing/transitions/add-remove-transition.html b/LayoutTests/legacy-animation-engine/compositing/transitions/add-remove-transition.html
new file mode 100644 (file)
index 0000000..a50a842
--- /dev/null
@@ -0,0 +1,45 @@
+<!DOCTYPE html><!-- webkit-test-runner [ experimental:WebAnimationsCSSIntegrationEnabled=false ] -->
+<html>
+<head>
+    <style>
+        .box {
+            width: 100px;
+            height: 100px;
+            margin: 20px;
+            background-color: blue;
+            transition: opacity 10ms;
+        }
+        
+        .no-transition {
+            transition: none !important;
+            opacity: 1 !important;
+        }
+        
+        .transitioning {
+            opacity: 0.1;
+        }
+        
+        .composited {
+            transform: translateZ(0);
+        }
+    </style>
+    <script>
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        window.addEventListener('load', () => {
+            target.classList.add('transitioning');
+            document.body.offsetWidth;
+            target.classList.add('no-transition');
+            setTimeout(() => {
+                if (window.testRunner)
+                    testRunner.notifyDone();
+            }, 20);
+        }, false);
+    </script>
+</head>
+<body>
+    <p>This box should end up with opacity 1</p>
+    <div id="target" class="composited box"></div>
+</body>
+</html>
index cd3a665..72d38cc 100644 (file)
         (WebCore::Layout::InlineFormattingContext::InlineLayout::layout const):
         (WebCore::Layout::InlineFormattingContext::InlineLayout::createDisplayRuns const):
 
+2019-09-04  Simon Fraser  <simon.fraser@apple.com>
+
+        Cancelled transitions on Google image search leave content with opacity 0 sometimes
+        https://bugs.webkit.org/show_bug.cgi?id=201482
+        rdar://problem/54921036
+
+        Reviewed by Tim Horton.
+        
+        If, in a single rendering update, we started an accelerated opacity transition, and then removed
+        it, we'd still push the transition onto the CALayer with fillForwards and never remove it, so its
+        effects would last forever.
+
+        Fix by making GraphicsLayerCA::removeAnimation() remove animations from the uncomittedAnimations
+        list as well.
+        
+        Also fix layer names in debug; if a layer's primaryLayerID changed, we'd fail to rename the
+        CALayer, causing confusion when logging at layer dumps. Fix by adding the layer ID just
+        before pushing the name to the platform layer.
+
+        Some drive-by logging cleanup.
+
+        Test: legacy-animation-engine/compositing/transitions/add-remove-transition.html
+
+        * platform/graphics/GraphicsLayer.cpp:
+        (WebCore::GraphicsLayer::debugName const):
+        * platform/graphics/GraphicsLayer.h:
+        * platform/graphics/ca/GraphicsLayerCA.cpp:
+        (WebCore::GraphicsLayerCA::setName):
+        (WebCore::GraphicsLayerCA::debugName const):
+        (WebCore::GraphicsLayerCA::addAnimation):
+        (WebCore::GraphicsLayerCA::pauseAnimation):
+        (WebCore::GraphicsLayerCA::seekAnimation):
+        (WebCore::GraphicsLayerCA::removeAnimation):
+        (WebCore::GraphicsLayerCA::platformCALayerAnimationStarted):
+        (WebCore::GraphicsLayerCA::platformCALayerAnimationEnded):
+        (WebCore::GraphicsLayerCA::updateNames):
+        (WebCore::GraphicsLayerCA::createTransformAnimationsFromKeyframes):
+        * platform/graphics/ca/GraphicsLayerCA.h:
+        * rendering/RenderLayerCompositor.cpp:
+        (WebCore::RenderLayerCompositor::logLayerInfo):
+
 2019-09-03  Zalan Bujtas  <zalan@apple.com>
 
         [LFC] FloatingState should not need to query for display boxes.
index 24d2050..506b261 100644 (file)
@@ -199,6 +199,11 @@ void GraphicsLayer::clearClient()
     m_client = &EmptyGraphicsLayerClient::singleton();
 }
 
+String GraphicsLayer::debugName() const
+{
+    return name();
+}
+
 void GraphicsLayer::setClient(GraphicsLayerClient& client)
 {
     m_client = &client;
index 7a1feaa..f08eeb3 100644 (file)
@@ -268,6 +268,7 @@ public:
     // Layer name. Only used to identify layers in debug output
     const String& name() const { return m_name; }
     virtual void setName(const String& name) { m_name = name; }
+    virtual String debugName() const;
 
     GraphicsLayer* parent() const { return m_parent; };
     void setParent(GraphicsLayer*); // Internal use only.
index 8765344..9b9f59c 100644 (file)
@@ -480,16 +480,23 @@ void GraphicsLayerCA::willBeDestroyed()
 
 void GraphicsLayerCA::setName(const String& name)
 {
+    if (name == this->name())
+        return;
+
+    GraphicsLayer::setName(name);
+    noteLayerPropertyChanged(NameChanged);
+}
+
+String GraphicsLayerCA::debugName() const
+{
 #if ENABLE(TREE_DEBUGGING)
     String caLayerDescription;
     if (!m_layer->isPlatformCALayerRemote())
         caLayerDescription = makeString("CALayer(0x", hex(reinterpret_cast<uintptr_t>(m_layer->platformLayer()), Lowercase), ") ");
-    GraphicsLayer::setName(makeString(caLayerDescription, "GraphicsLayer(0x", hex(reinterpret_cast<uintptr_t>(this), Lowercase), ", ", primaryLayerID(), ") ", name));
+    return makeString(caLayerDescription, "GraphicsLayer(0x", hex(reinterpret_cast<uintptr_t>(this), Lowercase), ", ", primaryLayerID(), ") ", name());
 #else
-    GraphicsLayer::setName(name);
+    return name();
 #endif
-
-    noteLayerPropertyChanged(NameChanged);
 }
 
 GraphicsLayer::PlatformLayerID GraphicsLayerCA::primaryLayerID() const
@@ -1027,7 +1034,7 @@ void GraphicsLayerCA::addProcessingActionForAnimation(const String& animationNam
 
 bool GraphicsLayerCA::addAnimation(const KeyframeValueList& valueList, const FloatSize& boxSize, const Animation* anim, const String& animationName, double timeOffset)
 {
-    LOG(Animations, "GraphicsLayerCA %p %llu addAnimation %s (can be accelerated %d)", this, primaryLayerID(), animationName.utf8().data(), animationCanBeAccelerated(valueList, anim));
+    LOG_WITH_STREAM(Animations, stream << "GraphicsLayerCA " << this << " id " << primaryLayerID() << " addAnimation " << anim << " " << animationName << " duration " << anim->duration() << " (can be accelerated " << animationCanBeAccelerated(valueList, anim) << ")");
 
     ASSERT(!animationName.isEmpty());
 
@@ -1058,7 +1065,7 @@ bool GraphicsLayerCA::addAnimation(const KeyframeValueList& valueList, const Flo
 
 void GraphicsLayerCA::pauseAnimation(const String& animationName, double timeOffset)
 {
-    LOG(Animations, "GraphicsLayerCA %p pauseAnimation %s (running %d)", this, animationName.utf8().data(), animationIsRunning(animationName));
+    LOG_WITH_STREAM(Animations, stream << "GraphicsLayerCA " << this << " id " << primaryLayerID() << " pauseAnimation " << animationName << " (is running " << animationIsRunning(animationName) << ")");
 
     // Call add since if there is already a Remove in there, we don't want to overwrite it with a Pause.
     addProcessingActionForAnimation(animationName, AnimationProcessingAction { Pause, Seconds { timeOffset } });
@@ -1068,7 +1075,7 @@ void GraphicsLayerCA::pauseAnimation(const String& animationName, double timeOff
 
 void GraphicsLayerCA::seekAnimation(const String& animationName, double timeOffset)
 {
-    LOG(Animations, "GraphicsLayerCA %p seekAnimation %s (running %d)", this, animationName.utf8().data(), animationIsRunning(animationName));
+    LOG_WITH_STREAM(Animations, stream << "GraphicsLayerCA " << this << " id " << primaryLayerID() << " seekAnimation " << animationName << " (is running " << animationIsRunning(animationName) << ")");
 
     // Call add since if there is already a Remove in there, we don't want to overwrite it with a Pause.
     addProcessingActionForAnimation(animationName, AnimationProcessingAction { Seek, Seconds { timeOffset } });
@@ -1078,10 +1085,12 @@ void GraphicsLayerCA::seekAnimation(const String& animationName, double timeOffs
 
 void GraphicsLayerCA::removeAnimation(const String& animationName)
 {
-    LOG(Animations, "GraphicsLayerCA %p removeAnimation %s (running %d)", this, animationName.utf8().data(), animationIsRunning(animationName));
+    LOG_WITH_STREAM(Animations, stream << "GraphicsLayerCA " << this << " id " << primaryLayerID() << " removeAnimation " << animationName << " (is running " << animationIsRunning(animationName) << ")");
 
-    if (!animationIsRunning(animationName))
+    if (!animationIsRunning(animationName)) {
+        removeFromUncommittedAnimations(animationName);
         return;
+    }
 
     addProcessingActionForAnimation(animationName, AnimationProcessingAction(Remove));
     noteLayerPropertyChanged(AnimationChanged | CoverageRectChanged);
@@ -1089,13 +1098,13 @@ void GraphicsLayerCA::removeAnimation(const String& animationName)
 
 void GraphicsLayerCA::platformCALayerAnimationStarted(const String& animationKey, MonotonicTime startTime)
 {
-    LOG(Animations, "GraphicsLayerCA %p platformCALayerAnimationStarted %s at %f", this, animationKey.utf8().data(), startTime.secondsSinceEpoch().seconds());
+    LOG_WITH_STREAM(Animations, stream << "GraphicsLayerCA " << this << " id " << primaryLayerID() << " platformCALayerAnimationStarted " << animationKey);
     client().notifyAnimationStarted(this, animationKey, startTime);
 }
 
 void GraphicsLayerCA::platformCALayerAnimationEnded(const String& animationKey)
 {
-    LOG(Animations, "GraphicsLayerCA %p platformCALayerAnimationEnded %s", this, animationKey.utf8().data());
+    LOG_WITH_STREAM(Animations, stream << "GraphicsLayerCA " << this << " id " << primaryLayerID() << " platformCALayerAnimationEnded " << animationKey);
     client().notifyAnimationEnded(this, animationKey);
 }
 
@@ -1912,20 +1921,21 @@ void GraphicsLayerCA::commitLayerChangesAfterSublayers(CommitState& commitState)
 
 void GraphicsLayerCA::updateNames()
 {
+    auto name = debugName();
     switch (structuralLayerPurpose()) {
     case StructuralLayerForPreserves3D:
-        m_structuralLayer->setName("preserve-3d: " + name());
+        m_structuralLayer->setName("preserve-3d: " + name);
         break;
     case StructuralLayerForReplicaFlattening:
-        m_structuralLayer->setName("replica flattening: " + name());
+        m_structuralLayer->setName("replica flattening: " + name);
         break;
     case StructuralLayerForBackdrop:
-        m_structuralLayer->setName("backdrop hosting: " + name());
+        m_structuralLayer->setName("backdrop hosting: " + name);
         break;
     case NoStructuralLayer:
         break;
     }
-    m_layer->setName(name());
+    m_layer->setName(name);
 }
 
 void GraphicsLayerCA::updateSublayerList(bool maxLayerDepthReached)
@@ -3149,6 +3159,16 @@ void GraphicsLayerCA::appendToUncommittedAnimations(LayerPropertyAnimation&& ani
     m_animations->uncomittedAnimations.append(WTFMove(animation));
 }
 
+void GraphicsLayerCA::removeFromUncommittedAnimations(const String& animationName)
+{
+    if (!m_animations)
+        return;
+
+    m_animations->uncomittedAnimations.removeFirstMatching([&animationName] (auto& current) {
+        return current.m_name == animationName;
+    });
+}
+
 bool GraphicsLayerCA::createFilterAnimationsFromKeyframes(const KeyframeValueList& valueList, const Animation* animation, const String& animationName, Seconds timeOffset)
 {
 #if ENABLE(FILTERS_LEVEL_2)
index c4b2c9c..b62bc93 100644 (file)
@@ -55,6 +55,7 @@ public:
     WEBCORE_EXPORT void initialize(Type) override;
 
     WEBCORE_EXPORT void setName(const String&) override;
+    WEBCORE_EXPORT String debugName() const override;
 
     WEBCORE_EXPORT PlatformLayerID primaryLayerID() const override;
 
@@ -492,6 +493,7 @@ private:
     bool appendToUncommittedAnimations(const KeyframeValueList&, const TransformOperations*, const Animation*, const String& animationName, const FloatSize& boxSize, int animationIndex, Seconds timeOffset, bool isMatrixAnimation);
     bool appendToUncommittedAnimations(const KeyframeValueList&, const FilterOperation*, const Animation*, const String& animationName, int animationIndex, Seconds timeOffset);
     void appendToUncommittedAnimations(LayerPropertyAnimation&&);
+    void removeFromUncommittedAnimations(const String&);
 
     enum LayerChange : uint64_t {
         NoChange                                = 0,
index b59ebec..be135d8 100644 (file)
@@ -1391,7 +1391,7 @@ void RenderLayerCompositor::logLayerInfo(const RenderLayer& layer, const char* p
     absoluteBounds.move(layer.offsetFromAncestor(m_renderView.layer()));
     
     StringBuilder logString;
-    logString.append(pad(' ', 12 + depth * 2, hex(reinterpret_cast<uintptr_t>(&layer))), " id ", backing->graphicsLayer()->primaryLayerID(), " (", FormattedNumber::fixedWidth(absoluteBounds.x().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.y().toFloat(), 3), '-', FormattedNumber::fixedWidth(absoluteBounds.maxX().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.maxY().toFloat(), 3), ") ", FormattedNumber::fixedWidth(backing->backingStoreMemoryEstimate() / 1024, 2), "KB");
+    logString.append(pad(' ', 12 + depth * 2, hex(reinterpret_cast<uintptr_t>(&layer), Lowercase)), " id ", backing->graphicsLayer()->primaryLayerID(), " (", FormattedNumber::fixedWidth(absoluteBounds.x().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.y().toFloat(), 3), '-', FormattedNumber::fixedWidth(absoluteBounds.maxX().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.maxY().toFloat(), 3), ") ", FormattedNumber::fixedWidth(backing->backingStoreMemoryEstimate() / 1024, 2), "KB");
 
     if (!layer.renderer().style().hasAutoZIndex())
         logString.append(" z-index: ", layer.renderer().style().zIndex());