REGRESSION (r246899): Subtitles show twice when controls show/hide on hulu.com
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 29 Jul 2019 18:37:50 +0000 (18:37 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 29 Jul 2019 18:37:50 +0000 (18:37 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200187
rdar://problem/53511121

Reviewed by Zalan Bujtas.

Source/WebCore:

When a layer that painted into shared backing moved, we'd fail to repaint its old position
because the RenderLayer's repaint rects are cleared via BackingSharingState::updateBeforeDescendantTraversal().

Recomputing repaint rects is expensive, so we only want to do it when necessary, which is for
layers that start and stop sharing (going into and out of compositing already recomputes them).
So add logic to RenderLayerBacking::setBackingSharingLayers() that recomputes repaint rects
on layers that will no longer use shared backing, and those that are newly using shared
backing.

Test: compositing/shared-backing/backing-sharing-repaint.html

* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::setBackingProviderLayer):
* rendering/RenderLayerBacking.cpp:
(WebCore::RenderLayerBacking::setBackingSharingLayers):
* rendering/RenderLayerCompositor.cpp:
(WebCore::RenderLayerCompositor::BackingSharingState::appendSharingLayer):
(WebCore::RenderLayerCompositor::updateBacking):

LayoutTests:

* compositing/shared-backing/backing-sharing-repaint-expected.html: Added.
* compositing/shared-backing/backing-sharing-repaint.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/compositing/shared-backing/backing-sharing-repaint-expected.html [new file with mode: 0644]
LayoutTests/compositing/shared-backing/backing-sharing-repaint.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/rendering/RenderLayerBacking.cpp
Source/WebCore/rendering/RenderLayerCompositor.cpp

index ccbf970..ce9a8ab 100644 (file)
@@ -1,5 +1,16 @@
 2019-07-29  Simon Fraser  <simon.fraser@apple.com>
 
+        REGRESSION (r246899): Subtitles show twice when controls show/hide on hulu.com
+        https://bugs.webkit.org/show_bug.cgi?id=200187
+        rdar://problem/53511121
+
+        Reviewed by Zalan Bujtas.
+
+        * compositing/shared-backing/backing-sharing-repaint-expected.html: Added.
+        * compositing/shared-backing/backing-sharing-repaint.html: Added.
+
+2019-07-29  Simon Fraser  <simon.fraser@apple.com>
+
         The touch-action property was ignored on replaced elements (canvas, img etc)
         https://bugs.webkit.org/show_bug.cgi?id=200205
         rdar://problem/53331224
diff --git a/LayoutTests/compositing/shared-backing/backing-sharing-repaint-expected.html b/LayoutTests/compositing/shared-backing/backing-sharing-repaint-expected.html
new file mode 100644 (file)
index 0000000..19296bb
--- /dev/null
@@ -0,0 +1,60 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style>
+        .container {
+            position: absolute;
+            margin: 40px;
+            padding: 50px;
+            width: 400px;
+            height: 300px;
+            border: 1px solid black;
+        }
+        
+        .caption-container {
+            position: absolute;
+            top: 0;
+            left: 0;
+            width: 100%;
+            height: 100%;
+            box-shadow: 0 0 10px black;
+        }
+        
+        .caption {
+            position: absolute;
+            width: 300px;
+            bottom: 20px;            
+            text-align: center;
+            height: 100px;
+            background-color: blue;
+        }
+        
+        .child {
+            position: absolute;
+            top: 10px;
+            left: 320px;
+            width: 100px;
+            height: 100px;
+            background-color: blue;
+        }
+        .composited {
+            transform: translateZ(0);
+            background-color: silver;
+        }
+    </style>
+</head>
+<body>
+    <div class="container">
+        <div class="composited">composited</div>
+
+        <div class="caption-container">
+            <div class="caption">
+                This should move, not duplicate
+                <div class="child">
+                    child
+                </div>
+            </div>
+        </div>
+    </div>
+</body>
+</html>
diff --git a/LayoutTests/compositing/shared-backing/backing-sharing-repaint.html b/LayoutTests/compositing/shared-backing/backing-sharing-repaint.html
new file mode 100644 (file)
index 0000000..22abb7a
--- /dev/null
@@ -0,0 +1,79 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style>
+        .container {
+            position: absolute;
+            margin: 40px;
+            padding: 50px;
+            width: 400px;
+            height: 300px;
+            border: 1px solid black;
+        }
+        
+        .caption-container {
+            position: absolute;
+            top: 0;
+            left: 0;
+            width: 100%;
+            height: 100%;
+            box-shadow: 0 0 10px black;
+        }
+        
+        .caption {
+            position: absolute;
+            width: 300px;
+            bottom: 200px;
+            text-align: center;
+            height: 100px;
+            background-color: blue;
+        }
+        
+        .child {
+            position: absolute;
+            top: 10px;
+            left: 320px;
+            width: 100px;
+            height: 100px;
+            background-color: blue;
+        }
+
+        body.changed .caption { 
+            bottom: 20px;            
+        }
+        
+        .composited {
+            transform: translateZ(0);
+            background-color: silver;
+        }
+    </style>
+    <script>
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        window.addEventListener('load', () => {
+            requestAnimationFrame(() => {
+                document.getElementById('trigger').classList.add('composited');
+                requestAnimationFrame(() => {
+                    document.body.classList.add('changed');
+                    if (window.testRunner)
+                        testRunner.notifyDone();
+                });
+            });
+        }, false);
+    </script>
+</head>
+<body>
+    <div class="container">
+        <div id="trigger">composited</div>
+        <div class="caption-container">
+            <div class="caption">
+                This should move, not duplicate
+                <div class="child">
+                    child
+                </div>
+            </div>
+        </div>
+    </div>
+</body>
+</html>
index db4ad8b..26471b1 100644 (file)
@@ -1,5 +1,32 @@
 2019-07-29  Simon Fraser  <simon.fraser@apple.com>
 
+        REGRESSION (r246899): Subtitles show twice when controls show/hide on hulu.com
+        https://bugs.webkit.org/show_bug.cgi?id=200187
+        rdar://problem/53511121
+
+        Reviewed by Zalan Bujtas.
+
+        When a layer that painted into shared backing moved, we'd fail to repaint its old position
+        because the RenderLayer's repaint rects are cleared via BackingSharingState::updateBeforeDescendantTraversal().
+
+        Recomputing repaint rects is expensive, so we only want to do it when necessary, which is for
+        layers that start and stop sharing (going into and out of compositing already recomputes them).
+        So add logic to RenderLayerBacking::setBackingSharingLayers() that recomputes repaint rects
+        on layers that will no longer use shared backing, and those that are newly using shared
+        backing.
+
+        Test: compositing/shared-backing/backing-sharing-repaint.html
+
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::setBackingProviderLayer):
+        * rendering/RenderLayerBacking.cpp:
+        (WebCore::RenderLayerBacking::setBackingSharingLayers):
+        * rendering/RenderLayerCompositor.cpp:
+        (WebCore::RenderLayerCompositor::BackingSharingState::appendSharingLayer):
+        (WebCore::RenderLayerCompositor::updateBacking):
+
+2019-07-29  Simon Fraser  <simon.fraser@apple.com>
+
         The touch-action property was ignored on replaced elements (canvas, img etc)
         https://bugs.webkit.org/show_bug.cgi?id=200205
         rdar://problem/53331224
index ad1a73b..276ce82 100644 (file)
@@ -1796,10 +1796,8 @@ void RenderLayer::setBackingProviderLayer(RenderLayer* backingProvider)
     if (backingProvider == m_backingProviderLayer)
         return;
 
-    if (!renderer().renderTreeBeingDestroyed()) {
-        clearRepaintRects();
+    if (!renderer().renderTreeBeingDestroyed())
         clearClipRectsIncludingDescendants();
-    }
 
     m_backingProviderLayer = makeWeakPtr(backingProvider);
 }
index b0f85f5..7f14366 100644 (file)
@@ -281,15 +281,35 @@ static void clearBackingSharingLayerProviders(Vector<WeakPtr<RenderLayer>>& shar
 
 void RenderLayerBacking::setBackingSharingLayers(Vector<WeakPtr<RenderLayer>>&& sharingLayers)
 {
+    bool sharingLayersChanged = m_backingSharingLayers != sharingLayers;
+    if (sharingLayersChanged) {
+        // For layers that used to share and no longer do, and are not composited, recompute repaint rects.
+        for (auto& oldSharingLayer : m_backingSharingLayers) {
+            // Layers that go from shared to composited have their repaint rects recomputed in RenderLayerCompositor::updateBacking().
+            // FIXME: Two O(n^2) traversals in this funtion. Probably OK because sharing lists are usually small, but still.
+            if (!sharingLayers.contains(oldSharingLayer) && !oldSharingLayer->isComposited())
+                oldSharingLayer->computeRepaintRectsIncludingDescendants();
+        }
+    }
+
     clearBackingSharingLayerProviders(m_backingSharingLayers);
 
     if (sharingLayers != m_backingSharingLayers)
-        setContentsNeedDisplay(); // This could be optimize to only repaint rects for changed layers.
+        setContentsNeedDisplay(); // This could be optimized to only repaint rects for changed layers.
 
+    auto oldSharingLayers = WTFMove(m_backingSharingLayers);
     m_backingSharingLayers = WTFMove(sharingLayers);
 
     for (auto& layerWeakPtr : m_backingSharingLayers)
         layerWeakPtr->setBackingProviderLayer(&m_owningLayer);
+
+    if (sharingLayersChanged) {
+        // For layers that are newly sharing, recompute repaint rects.
+        for (auto& currentSharingLayer : m_backingSharingLayers) {
+            if (!oldSharingLayers.contains(currentSharingLayer))
+                currentSharingLayer->computeRepaintRectsIncludingDescendants();
+        }
+    }
 }
 
 void RenderLayerBacking::removeBackingSharingLayer(RenderLayer& layer)
index 82f1fec..ea4e37c 100644 (file)
@@ -207,6 +207,7 @@ public:
     
     void appendSharingLayer(RenderLayer& layer)
     {
+        ASSERT(m_backingProviderCandidate);
         m_backingSharingLayers.append(makeWeakPtr(layer));
     }
 
@@ -1621,7 +1622,7 @@ bool RenderLayerCompositor::updateBacking(RenderLayer& layer, RequiresCompositin
             // the repaint container, so change when compositing changes; we need to update them here.
             if (layer.parent())
                 layer.computeRepaintRectsIncludingDescendants();
-            
+
             layer.setNeedsCompositingGeometryUpdate();
             layer.setNeedsCompositingConfigurationUpdate();
             layer.setNeedsCompositingPaintOrderChildrenUpdate();