Bug 179946 - ScrollingStateNode::reconcileLayerPositionForViewportRect is only called on direct children of the root
Summary: ScrollingStateNode::reconcileLayerPositionForViewportRect is only called on d...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on:
Blocks: 154399
  Show dependency treegraph
 
Reported: 2017-11-22 07:18 PST by Frédéric Wang (:fredw)
Modified: 2018-01-16 01:47 PST (History)
8 users (show)

See Also:


Attachments
Testcase (1.52 KB, text/html)
2017-11-22 07:18 PST, Frédéric Wang (:fredw)
no flags Details
WIP Patch (5.36 KB, patch)
2017-11-22 07:27 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (12.46 KB, patch)
2018-01-15 07:03 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (17.20 KB, patch)
2018-01-16 01:44 PST, Frédéric Wang (:fredw)
fred.wang: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2017-11-22 07:18:27 PST
Created attachment 327452 [details]
Testcase

AsyncScrollingCoordinator::reconcileViewportConstrainedLayerPositions has the following comment:

// FIXME: We'll have to traverse deeper into the tree at some point.
for (auto& child : *children)
     child->reconcileLayerPositionForViewportRect(viewportRect, action);

I'm not sure whether this is affecting bug 154399 but the attached testcase shows that reconcileLayerPositionForViewportRect is not called on the fixed node when one scrolls the main frame.
Comment 1 Frédéric Wang (:fredw) 2017-11-22 07:27:20 PST
Created attachment 327453 [details]
WIP Patch

This is patch to recursively calls reconcileLayerPositionForViewportRect but I'm not sure how to emulate non-programmatic scroll on iOS in order to write a test.
Comment 2 Frédéric Wang (:fredw) 2018-01-15 07:03:12 PST
Created attachment 331334 [details]
Patch
Comment 3 Darin Adler 2018-01-15 14:55:18 PST
Comment on attachment 331334 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:103
> +void ScrollingStateNode::reconcileLayerPositionForViewportRect(const LayoutRect& viewportRect, ScrollingLayerPositionAction action)

ScrollingStateStickyNode::reconcileLayerPositionForViewportRect and ScrollingStateFixedNode::reconcileLayerPositionForViewportRect don’t call through to ScrollingStateNode::reconcileLayerPositionForViewportRect.

Can those nodes have children? Is it OK to not call reconcileLayerPositionForViewportRect on those children?
Comment 4 Frédéric Wang (:fredw) 2018-01-16 01:44:11 PST
Created attachment 331376 [details]
Patch
Comment 5 Frédéric Wang (:fredw) 2018-01-16 01:46:13 PST
Comment on attachment 331334 [details]
Patch

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

>> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:103
>> +void ScrollingStateNode::reconcileLayerPositionForViewportRect(const LayoutRect& viewportRect, ScrollingLayerPositionAction action)
> 
> ScrollingStateStickyNode::reconcileLayerPositionForViewportRect and ScrollingStateFixedNode::reconcileLayerPositionForViewportRect don’t call through to ScrollingStateNode::reconcileLayerPositionForViewportRect.
> 
> Can those nodes have children? Is it OK to not call reconcileLayerPositionForViewportRect on those children?

Yes, I guess sticky/fixed nodes should also call reconcileLayerPositionForViewportRect on their descendants. I modified the code and extended the tests accordingly.
Comment 6 Frédéric Wang (:fredw) 2018-01-16 01:47:20 PST
For the record, here is the diff of the graphic layers of the test before and after the patch:

diff --git a/LayoutTests/fast/scrolling/ios/reconcile-layer-position-recursive-expected.txt b/LayoutTests/fast/scrolling/ios/reconcile-layer-position-recursive-expected.txt
index fc9b542a67d..1b6f6dbeaba 100644
--- a/LayoutTests/fast/scrolling/ios/reconcile-layer-position-recursive-expected.txt
+++ b/LayoutTests/fast/scrolling/ios/reconcile-layer-position-recursive-expected.txt
@@ -70,44 +70,48 @@
                               (contentsScale 2.00)
                               (children 2
                                 (GraphicsLayer
-                                  (position 7.00 -993.00)
+                                  (position 7.00 28.00)
+                                  (approximate position 7.00 28.00)
                                   (bounds 110.00 110.00)
                                   (contentsOpaque 1)
                                   (drawsContent 1)
-                                  (visible rect 0.00, 0.00 0.00 x 0.00)
-                                  (coverage rect -7.00, 993.00 200.00 x 245.00)
+                                  (visible rect 0.00, 0.00 110.00 x 110.00)
+                                  (coverage rect -7.00, -28.00 200.00 x 245.00)
                                   (intersects coverage rect 1)
                                   (contentsScale 2.00)
                                   (children 1
                                     (GraphicsLayer
                                       (position 55.00 55.00)
+                                      (approximate position 55.00 55.00)
                                       (bounds 110.00 110.00)
                                       (contentsOpaque 1)
                                       (drawsContent 1)
-                                      (visible rect 0.00, 0.00 0.00 x 0.00)
-                                      (coverage rect -62.00, 938.00 200.00 x 245.00)
+                                      (visible rect 0.00, 0.00 110.00 x 110.00)
+                                      (coverage rect -62.00, -83.00 200.00 x 245.00)
                                       (intersects coverage rect 1)
                                       (contentsScale 2.00)
                                     )
                                   )
                                 )
                                 (GraphicsLayer
-                                  (position 105.00 105.00)
+                                  (position 105.00 395.00)
+                                  (approximate position 105.00 395.00)
                                   (bounds 110.00 110.00)
                                   (contentsOpaque 1)
                                   (drawsContent 1)
-                                  (visible rect 0.00, 0.00 95.00 x 110.00)
-                                  (coverage rect -105.00, -105.00 200.00 x 245.00)
+                                  (visible rect 0.00, 0.00 0.00 x 0.00)
+                                  (coverage rect -105.00, -395.00 200.00 x 245.00)
                                   (intersects coverage rect 1)
                                   (contentsScale 2.00)
                                   (children 1
                                     (GraphicsLayer
                                       (position 55.00 55.00)
+                                      (approximate position 55.00 55.00)
                                       (bounds 110.00 110.00)
                                       (contentsOpaque 1)
                                       (drawsContent 1)
-                                      (visible rect 0.00, 0.00 40.00 x 85.00)
-                                      (coverage rect -160.00, -160.00 200.00 x 245.00)
+                                      (visible rect 0.00, 0.00 0.00 x 0.00)
+                                      (coverage rect -160.00, -450.00 200.00 x 245.00)
                                       (intersects coverage rect 1)
                                       (contentsScale 2.00)
                                     )