Bug 176261 - Async frame scrolling: handle fixed root backgrounds in frames
Summary: Async frame scrolling: handle fixed root backgrounds in frames
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on: 173644 176585 176591 179937
Blocks: 182925
  Show dependency treegraph
 
Reported: 2017-09-01 18:03 PDT by Simon Fraser (smfr)
Modified: 2018-02-19 06:35 PST (History)
10 users (show)

See Also:


Attachments
Patch (6.10 KB, patch)
2017-09-05 08:44 PDT, Simon Fraser (smfr)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Experimental test adjustments (26.91 KB, patch)
2017-09-05 09:11 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (324.48 KB, application/zip)
2017-09-05 09:32 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-elcapitan (458.17 KB, application/zip)
2017-09-05 09:42 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.49 MB, application/zip)
2017-09-05 09:58 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.30 MB, application/zip)
2017-09-05 10:19 PDT, Build Bot
no flags Details
Part 1: Extend tiled-drawing-async-frame-scrolling to check different kind of backgrounds. (29.40 KB, patch)
2017-09-06 02:34 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Part 2: Simon's patch with test expectation adjustments. (8.87 KB, patch)
2017-09-06 02:42 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Part 1 + Part 2 (33.39 KB, patch)
2017-09-06 02:46 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (422.92 KB, application/zip)
2017-09-06 03:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (1.20 MB, application/zip)
2017-09-06 03:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (1.27 MB, application/zip)
2017-09-06 04:21 PDT, Build Bot
no flags Details
Patch (82.47 KB, patch)
2017-09-08 03:44 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (362.51 KB, application/zip)
2017-09-08 04:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (286.69 KB, application/zip)
2017-09-08 04:35 PDT, Build Bot
no flags Details
Patch (31.34 KB, patch)
2017-09-08 07:46 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-elcapitan (2.12 MB, application/zip)
2017-09-08 08:57 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (1.39 MB, application/zip)
2017-09-08 09:01 PDT, Build Bot
no flags Details
Patch (35.17 KB, patch)
2017-09-08 09:37 PDT, Frédéric Wang (:fredw)
tonikitoo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2017-09-01 18:03:19 PDT
Need to correctly handle background-attachment: fixed; on the html/body in iframes, to avoid repaints etc.
Comment 1 Simon Fraser (smfr) 2017-09-01 18:06:21 PDT
We register a slow-repaint object because we hit RenderElement::styleWillChange() before we've made a backing for the iframe's RenderView so view().compositor().supportsFixedRootBackgroundCompositing() returns false.
Comment 2 Radar WebKit Bug Importer 2017-09-01 18:06:41 PDT
<rdar://problem/34220451>
Comment 3 Simon Fraser (smfr) 2017-09-01 18:07:15 PDT
Some WIP (needed to fix root layer opaqueness):

diff --git a/Source/WebCore/rendering/RenderLayerBacking.cpp b/Source/WebCore/rendering/RenderLayerBacking.cpp
index 32c24fa21a31aff1aa9b0345382c5c1a2d31de23..7776955752fcf3f0d3b4f46aa2384acb28a057f8 100644
--- a/Source/WebCore/rendering/RenderLayerBacking.cpp
+++ b/Source/WebCore/rendering/RenderLayerBacking.cpp
@@ -1023,7 +1023,7 @@ void RenderLayerBacking::updateGeometry()
         positionOverflowControlsLayers();
     }
 
-    if (!m_isMainFrameRenderViewLayer) {
+    if (!m_isFrameLayerWithTiledBacking) {
         // For non-root layers, background is always painted by the primary graphics layer.
         ASSERT(!m_backgroundLayer);
         // Subpixel offset from graphics layer or size changed.
@@ -1966,12 +1966,14 @@ void RenderLayerBacking::updateRootLayerConfiguration()
     bool viewIsTransparent = compositor().viewHasTransparentBackground(&backgroundColor);
 
     if (m_backgroundLayerPaintsFixedRootBackground && m_backgroundLayer) {
-        m_backgroundLayer->setBackgroundColor(backgroundColor);
-        m_backgroundLayer->setContentsOpaque(!viewIsTransparent);
+        if (m_isMainFrameRenderViewLayer) {
+            m_backgroundLayer->setBackgroundColor(backgroundColor);
+            m_backgroundLayer->setContentsOpaque(!viewIsTransparent);
+        }
 
         m_graphicsLayer->setBackgroundColor(Color());
         m_graphicsLayer->setContentsOpaque(false);
-    } else {
+    } else if (m_isMainFrameRenderViewLayer) {
         m_graphicsLayer->setBackgroundColor(backgroundColor);
         m_graphicsLayer->setContentsOpaque(!viewIsTransparent);
     }
Comment 4 Simon Fraser (smfr) 2017-09-05 08:44:15 PDT
Created attachment 319899 [details]
Patch
Comment 5 Build Bot 2017-09-05 08:47:13 PDT
Attachment 319899 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Frédéric Wang (:fredw) 2017-09-05 09:03:42 PDT
Hi Simon. I was testing your WIP and was improving the test from bug 173644 to consider transparent, opaque and fixed backgrounds. I can provide it if you want. In any case, the expectation will have to be updated now that the opaque white background is removed.
Comment 7 Frédéric Wang (:fredw) 2017-09-05 09:11:04 PDT
Created attachment 319902 [details]
Experimental test adjustments

Feel free to use it as you want.
Comment 8 Build Bot 2017-09-05 09:32:16 PDT
Comment on attachment 319899 [details]
Patch

Attachment 319899 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4454784

Number of test failures exceeded the failure limit.
Comment 9 Build Bot 2017-09-05 09:32:18 PDT
Created attachment 319904 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 10 Build Bot 2017-09-05 09:42:54 PDT
Comment on attachment 319899 [details]
Patch

Attachment 319899 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4454841

Number of test failures exceeded the failure limit.
Comment 11 Build Bot 2017-09-05 09:42:56 PDT
Created attachment 319908 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 Build Bot 2017-09-05 09:58:02 PDT
Comment on attachment 319899 [details]
Patch

Attachment 319899 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4454848

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html
Comment 13 Build Bot 2017-09-05 09:58:04 PDT
Created attachment 319911 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 14 Build Bot 2017-09-05 10:19:38 PDT
Comment on attachment 319899 [details]
Patch

Attachment 319899 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4454858

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html
Comment 15 Build Bot 2017-09-05 10:19:40 PDT
Created attachment 319913 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 16 Frédéric Wang (:fredw) 2017-09-06 02:34:18 PDT
Created attachment 319992 [details]
Part 1: Extend tiled-drawing-async-frame-scrolling to check different kind of backgrounds.

Sorry, my previous patch was not quite correct, here is a better one. Note that the gradient background test exhibits a crash.
Comment 17 Frédéric Wang (:fredw) 2017-09-06 02:42:00 PDT
Created attachment 319993 [details]
Part 2: Simon's patch with test expectation adjustments.

This is attachment 319899 [details], with some adjustments to test expectations. It applies on top of attachment 319992 [details]. Note how the backgroundColor and contentsOpaque are removed. The crash due to ASSERT(!m_backgroundLayer) is also fixed.
Comment 18 Frédéric Wang (:fredw) 2017-09-06 02:46:03 PDT
Created attachment 319995 [details]
Part 1 + Part 2
Comment 19 Build Bot 2017-09-06 03:31:58 PDT
Comment on attachment 319995 [details]
Part 1 + Part 2

Attachment 319995 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4461719

Number of test failures exceeded the failure limit.
Comment 20 Build Bot 2017-09-06 03:31:59 PDT
Created attachment 319999 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 21 Build Bot 2017-09-06 03:41:38 PDT
Comment on attachment 319995 [details]
Part 1 + Part 2

Attachment 319995 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4461659

Number of test failures exceeded the failure limit.
Comment 22 Build Bot 2017-09-06 03:41:40 PDT
Created attachment 320001 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 23 Build Bot 2017-09-06 04:21:32 PDT
Comment on attachment 319995 [details]
Part 1 + Part 2

Attachment 319995 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4461880

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html
Comment 24 Build Bot 2017-09-06 04:21:34 PDT
Created attachment 320006 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 25 Frédéric Wang (:fredw) 2017-09-06 04:40:36 PDT
Comment on attachment 319992 [details]
Part 1: Extend tiled-drawing-async-frame-scrolling to check different kind of backgrounds.

View in context: https://bugs.webkit.org/attachment.cgi?id=319992&action=review

> LayoutTests/compositing/tiling/tiled-drawing-async-frame-scrolling.html:29
> +  <body onload="setTimeout(doTest, 0)">

OK the ios bot is failing so probably we still need to check that all iframes are loaded before calling doTest.
Comment 26 Frédéric Wang (:fredw) 2017-09-08 03:44:21 PDT
Created attachment 320254 [details]
Patch
Comment 27 Build Bot 2017-09-08 04:31:39 PDT
Comment on attachment 320254 [details]
Patch

Attachment 320254 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4486103

Number of test failures exceeded the failure limit.
Comment 28 Build Bot 2017-09-08 04:31:40 PDT
Created attachment 320255 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 29 Build Bot 2017-09-08 04:35:15 PDT
Comment on attachment 320254 [details]
Patch

Attachment 320254 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4486093

Number of test failures exceeded the failure limit.
Comment 30 Build Bot 2017-09-08 04:35:16 PDT
Created attachment 320256 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 31 Frédéric Wang (:fredw) 2017-09-08 07:46:02 PDT
Created attachment 320264 [details]
Patch
Comment 32 Build Bot 2017-09-08 08:57:54 PDT
Comment on attachment 320264 [details]
Patch

Attachment 320264 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4487612

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html
Comment 33 Build Bot 2017-09-08 08:57:56 PDT
Created attachment 320267 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 34 Build Bot 2017-09-08 09:01:14 PDT
Comment on attachment 320264 [details]
Patch

Attachment 320264 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4487762

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html
Comment 35 Build Bot 2017-09-08 09:01:16 PDT
Created attachment 320268 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 36 Frédéric Wang (:fredw) 2017-09-08 09:37:53 PDT
Created attachment 320269 [details]
Patch

Taking bug, per discusson with Simon yesterday.
Comment 37 Ali Juma 2017-10-17 12:55:19 PDT
Comment on attachment 320269 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320269&action=review

> Source/WebCore/rendering/RenderLayerBacking.cpp:1477
> +        renderer().view().frameView().removeSlowRepaintObject(renderer().view().rendererForRootBackground());

This logic assumes that the backing won't ever be destroyed while the frame is still scrollable and has a fixed root background (since otherwise something needs to add back the slow repaint object). It looks like this assumption is currently true, but I wonder if it would be possible to add an assertion somewhere to verify this?

Also, if we're making this kind of assumption about backing lifetime, we could alternatively change RenderLayerCompositor::supportsFixedRootBackgroundCompositing to depend on the same settings we use for computing the value of m_isFrameLayerWithTiledBacking, and then we'd avoid adding the slow repaint object in the first place.
Comment 38 Frédéric Wang (:fredw) 2017-10-23 06:26:30 PDT
Comment on attachment 320269 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320269&action=review

>> Source/WebCore/rendering/RenderLayerBacking.cpp:1477
>> +        renderer().view().frameView().removeSlowRepaintObject(renderer().view().rendererForRootBackground());
> 
> This logic assumes that the backing won't ever be destroyed while the frame is still scrollable and has a fixed root background (since otherwise something needs to add back the slow repaint object). It looks like this assumption is currently true, but I wonder if it would be possible to add an assertion somewhere to verify this?
> 
> Also, if we're making this kind of assumption about backing lifetime, we could alternatively change RenderLayerCompositor::supportsFixedRootBackgroundCompositing to depend on the same settings we use for computing the value of m_isFrameLayerWithTiledBacking, and then we'd avoid adding the slow repaint object in the first place.

Thanks for the review. Sorry, I checked that last week but forgot to reply.

As I see, the RenderLayerBacking is only released in the destructor of RenderLayer, so that should not be a problem. Normally, the slowRepainObject is added/removed in RenderElement::styleWillChange, maybe we should have an "addSlowRepaint" call in RenderLayerBacking::setBackgroundLayerPaintsFixedRootBackground too, although I don't know if that will be needed.

I believe it would indeed be possible to have RenderLayerCompositor::supportsFixedRootBackgroundCompositing use the same computation as for m_isFrameLayerWithTiledBacking, however we also need to know whether a RenderLayerBacking will be created and I'm not sure it is straightforward as RenderLayerCompositor::updateBacking does quite a fair amount of work.
Comment 39 Antonio Gomes 2017-11-21 13:09:40 PST
Comment on attachment 320269 [details]
Patch

r=me.

This is based on Simon's original patch, plus a nice set of tests added by Fred. I also test on a spare Macbook Pro I have, and it is really a progression on the opaque background case.
Comment 40 Frédéric Wang (:fredw) 2017-11-21 22:30:29 PST
Committed r225089: <https://trac.webkit.org/changeset/225089>
Comment 41 WebKit Commit Bot 2017-11-21 22:35:58 PST
Re-opened since this is blocked by bug 179937
Comment 42 Frédéric Wang (:fredw) 2017-11-21 22:47:53 PST
removeSlowRepaintObject was changed two days ago to take a reference https://trac.webkit.org/changeset/225052 :-( ; will reland soon.
Comment 43 Frédéric Wang (:fredw) 2017-11-21 23:17:18 PST
Committed r225092: <https://trac.webkit.org/changeset/225092>