Bug 176914

Summary: Allow control over child order when adding nodes to the scrolling tree
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: FramesAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, buildbot, cmarcelo, commit-queue, dvoytenko, ews, fred.wang, jamesr, luiz, rbuis, rniwa, simon.fraser, tonikitoo, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=192395
Bug Depends on: 178508, 192398, 192537    
Bug Blocks: 172917    
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews113 for mac-elcapitan
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
Patch 327061, rebased on top of bug 192398 (untested)
simon.fraser: review-
Simon's patch (bug 192395)
fred.wang: review-
Patch none

Description Frédéric Wang (:fredw) 2017-09-14 09:16:28 PDT
See bug 172917 comment 15.
Comment 1 Frédéric Wang (:fredw) 2017-09-14 09:20:21 PDT
Created attachment 320777 [details]
Patch

This patch adds a test for that issue. The layer tree should order iframes in z-order (i.e. increasing sizes) but the output is currently random.
Comment 2 Build Bot 2017-09-14 10:18:03 PDT
Comment on attachment 320777 [details]
Patch

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

New failing tests:
fast/scrolling/scrolling-tree-order-iframe.html
Comment 3 Build Bot 2017-09-14 10:18:04 PDT
Created attachment 320782 [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 4 Build Bot 2017-09-14 10:24:56 PDT
Comment on attachment 320777 [details]
Patch

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

New failing tests:
fast/scrolling/scrolling-tree-order-iframe.html
Comment 5 Build Bot 2017-09-14 10:24:57 PDT
Created attachment 320785 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 6 Build Bot 2017-09-14 10:41:58 PDT
Comment on attachment 320777 [details]
Patch

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

New failing tests:
fast/scrolling/scrolling-tree-order-iframe.html
Comment 7 Build Bot 2017-09-14 10:41:59 PDT
Created attachment 320787 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 8 Build Bot 2017-09-14 11:31:19 PDT
Comment on attachment 320777 [details]
Patch

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

New failing tests:
fast/scrolling/scrolling-tree-order-iframe.html
Comment 9 Build Bot 2017-09-14 11:31:20 PDT
Created attachment 320795 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 10 Frédéric Wang (:fredw) 2017-09-15 03:19:04 PDT
I meant the scrolling tree, sorry.
Comment 11 Frédéric Wang (:fredw) 2017-09-28 08:29:02 PDT
So I checked this a bit. The scrolling nodes happen to be attached at several places, for example some of them are: 

ScrollingStateTree::attachNode
AsyncScrollingCoordinator::attachToStateTree
|
\_
   RenderLayerCompositor::attachScrollingNode
   RenderLayerCompositor::updateScrollCoordinationForThisFrame
   RenderLayerCompositor::updateScrollCoordinatedLayer
   RenderLayerCompositor::updateScrollCoordinatedStatus
   |
   \_
      RenderLayerBacking::updateGeometry
      RenderLayerCompositor::rebuildCompositingLayerTree
      RenderLayerCompositor::updateCompositingLayers
   |
   \_
      RenderLayerCompositor::updateBacking
      RenderLayerCompositor::computeCompositingRequirements
      RenderLayerCompositor::updateCompositingLayers
|
\_
   AsyncScrollingCoordinator::ensureRootStateNodeForFrameView
   AsyncScrollingCoordinator::frameViewRootLayerDidChange
|
\_
   RenderLayerCompositor::reattachSubframeScrollLayers
   RenderLayerCompositor::updateCompositingLayers

ScrollingStateTree::attachNode basically appends the node to the parent and does not change anything if the node is already a child. Functions like rebuildCompositingLayerTree and computeCompositingRequirements browse the tree in the z-order (i.e. negZOrderList, normalFlowList, posZOrderList). However, if new nodes are inserted later, the scrolling nodes will never be reordered to match the z-order.

I am not sure what would be the best way to handle this. Maybe we should pass the ID of the previous sibling to attachNode or somewhat force a re-ordering somewhere.
Comment 12 Simon Fraser (smfr) 2017-09-28 13:53:48 PDT
Yeah, I started on this and it's tricky. We need to pass a childIndex or previousSibling in when adding scrolling nodes to the scrolling tree, but it's hard to compute those when we're not in the middle of a tree walk.

I considered adding a new z-order RenderLayer tree walk to rebuild the scrolling tree after mutations. Hopefully that's not too common.
Comment 13 Frédéric Wang (:fredw) 2017-10-19 08:14:12 PDT
Created attachment 324232 [details]
WIP Patch
Comment 14 Frédéric Wang (:fredw) 2017-10-20 09:49:09 PDT
Created attachment 324410 [details]
WIP Patch

This applies on top of the one for bug 178508.
Comment 15 Frédéric Wang (:fredw) 2017-11-16 07:14:44 PST
Created attachment 327061 [details]
WIP Patch

Rebasing after bug 178508.
Comment 16 Simon Fraser (smfr) 2017-12-12 16:46:33 PST
I'm thinking of storing scrolling data in the GraphicsLayer tree. That would automatically give us the correct z-index.
Comment 17 Frédéric Wang (:fredw) 2017-12-19 09:02:42 PST
(In reply to Simon Fraser (smfr) from comment #16)
> I'm thinking of storing scrolling data in the GraphicsLayer tree. That would
> automatically give us the correct z-index.

@Simon: What are your plan & status on this? The latest patch on bug 173833 for iOS frame scrolling now works relatively well, but there are still issues to handle mouse events when clicking an element (I'm testing with the simulator, but I guess the same is true for touch events on a device). It looks like the Web process does not take into account the scroll position when doing hit testing. Maybe something like bug 172917 is required for iOS, as you suggested in the past? Anyway, I plan to do other adjustments and to update & write tests for bug 173833 when I'm back in January. That way I can submit something to review for bug 173833, even if hit testing does not work perfectly for now. WDYT?
Comment 18 Simon Fraser (smfr) 2017-12-19 10:43:54 PST
I'm working on this now. My plan is to push scrollingNodeIDs onto GraphicsLayers, then register scrolling nodes during a GraphicsLayer tree walk, where it's trivial to find the ancestor.
Comment 19 Frédéric Wang (:fredw) 2018-12-05 07:23:38 PST
Created attachment 356603 [details]
Patch 327061, rebased on top of bug 192398 (untested)
Comment 20 Simon Fraser (smfr) 2018-12-05 08:27:08 PST
*** Bug 192395 has been marked as a duplicate of this bug. ***
Comment 21 Simon Fraser (smfr) 2018-12-05 08:33:23 PST
Comment on attachment 356603 [details]
Patch 327061, rebased on top of bug 192398 (untested)

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

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:167
> +    virtual ScrollingNodeID attachToStateTree(ScrollingNodeType, ScrollingNodeID newNodeID, ScrollingNodeID /*parentID*/, size_t /*childIndex*/) { return newNodeID; }

Maybe pass a default for childIndex of "notFound" to allow the current code to say "don't care" and get an append.

> Source/WebCore/page/scrolling/ScrollingStateNode.h:236
> +    void setChildIndex(size_t);
> +    size_t childIndex() const { return m_childIndex; }

I don't think we should ever set the index. I think if a node moves, we remove and re-add.

> Source/WebCore/page/scrolling/ScrollingStateNode.h:240
> +    void insertChild(size_t position, Ref<ScrollingStateNode>&&);

I would put the size_t argument last.

> Source/WebCore/page/scrolling/ScrollingStateNode.h:251
> +    void prepareNewChild(ScrollingStateNode& childNode);

Not sure why you need a separate prepare function.

> Source/WebCore/page/scrolling/ScrollingStateNode.h:261
> +    size_t m_childIndex { 0 };

Storing childIndex here is wrong; child order is controlled by the order of the parent's m_children.

> Source/WebCore/page/scrolling/ScrollingStateTree.cpp:103
> +        if (nodeTypeAndParentAndChildIndexMatch(*node, nodeType, parentID, childIndex))
>              return newNodeID;

I did this a little differently in my patch, allowing a simple re-order here.

> Source/WebCore/page/scrolling/ScrollingStateTree.cpp:141
>          if (!newNode) {
>              auto stateNode = createNode(nodeType, newNodeID);
>              newNode = stateNode.ptr();
> -            parent->appendChild(WTFMove(stateNode));
> +            parent->insertChild(childIndex, WTFMove(stateNode));
>          }

Maybe deal with childIndex == notFound like I did.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3806
> +        size_t childIndex = 0; // TODO

Yeah, I wasn't sure how to deal with this part. That's why I allowed a default value of notFound.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3915
> +    size_t childIndex = 0; // TODO

This is why I allowed a default value of notFound.

> Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:86
> +    encoder << node.childIndex();

You shouldn't have to encode childIndex; the order of children in the child array governs child order.
Comment 22 Frédéric Wang (:fredw) 2018-12-05 10:07:53 PST
Created attachment 356615 [details]
Simon's patch (bug 192395)
Comment 23 Frédéric Wang (:fredw) 2018-12-06 01:21:46 PST
Created attachment 356722 [details]
Patch
Comment 24 Frédéric Wang (:fredw) 2018-12-06 03:23:02 PST
Comment on attachment 356615 [details]
Simon's patch (bug 192395)

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

OK, I checked again the code this morning and your approach looks better. I've done some minor changes to your initial patch (see comments inline).

> Source/WebCore/page/scrolling/ScrollingStateNode.h:-230
> -    ScrollingNodeID scrollingNodeID() const { return m_nodeID; }

I think I should have included this change in the refactoring of bug 192398 (I thought it was intended to change the visibility of the member, but it's just moving code around) :-/ I can keep this in the patch if you want.

> Source/WebCore/page/scrolling/ScrollingStateTree.cpp:148
> +                // FIXME: use childIndex

I've addressed this FIXME, as I guess it was not intended to stay in the final patch.

> Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:416
> +        m_scrollingStateTree->attachNode(nodeType, nodeID, parentNodeID, i);

Using i is incorrect as it's the node index for some ordering of the tree, not the child index. As I see, we only need to append the child.
Comment 25 WebKit Commit Bot 2018-12-06 17:47:12 PST
Comment on attachment 356722 [details]
Patch

Clearing flags on attachment: 356722

Committed r238947: <https://trac.webkit.org/changeset/238947>
Comment 26 WebKit Commit Bot 2018-12-06 17:47:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Radar WebKit Bug Importer 2018-12-06 17:48:39 PST
<rdar://problem/46542237>
Comment 28 Truitt Savell 2018-12-07 09:10:00 PST
The revision https://trac.webkit.org/changeset/238947/webkit

has caused the test fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state.html to begin failing near constantly. 

reproduced failure with:
run-webkit-tests --root testbuild-238947 fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state.html --iterations 100 -f


the test fails on r238947 and passes on 238946. 

Can this be fixed quickly?

Diff:
--- /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state-expected.txt
+++ /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state-actual.txt
@@ -21,6 +21,11 @@
       (viewport rect at last layout at (0,0) size 785x585)
     )
     (Fixed node
+      (anchor edges: AnchorEdgeRight AnchorEdgeTop)
+      (viewport rect at last layout at (0,0) size 785x585)
+      (layer position at last layout (685,0))
+    )
+    (Fixed node
       (anchor edges: AnchorEdgeLeft AnchorEdgeBottom)
       (viewport rect at last layout at (0,0) size 785x585)
       (layer position at last layout (0,485))
@@ -28,11 +33,6 @@
     (Fixed node
       (anchor edges: AnchorEdgeLeft AnchorEdgeTop)
       (viewport rect at last layout at (0,0) size 785x585)
-    )
-    (Fixed node
-      (anchor edges: AnchorEdgeRight AnchorEdgeTop)
-      (viewport rect at last layout at (0,0) size 785x585)
-      (layer position at last layout (685,0))
     )
   )
 )
Comment 29 Truitt Savell 2018-12-07 09:10:39 PST
fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state.html

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fvisual-viewport%2Ftiled-drawing%2Fzoomed-fixed-scrolling-layers-state.html
Comment 30 Simon Fraser (smfr) 2018-12-07 10:50:25 PST
I'll take a look.
Comment 31 Truitt Savell 2018-12-07 10:51:43 PST
Reverted r238947 for reason:

Revision caused fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state.html to constantly fail

Committed r238958: <https://trac.webkit.org/changeset/238958>
Comment 32 Simon Fraser (smfr) 2018-12-08 12:18:37 PST
Why did EWS not show this failure?
Comment 33 Simon Fraser (smfr) 2018-12-08 13:09:24 PST
Re-landed in https://trac.webkit.org/changeset/239010/webkit.
Comment 34 WebKit Commit Bot 2018-12-08 23:58:28 PST
Re-opened since this is blocked by bug 192537
Comment 35 Alexey Proskuryakov 2018-12-08 23:58:39 PST
Test started to fail most of the time again, rolling back.

> Why did EWS not show this failure?

Good question that will need to be answered before the patch can be re-landed. Could be sensitive to performance, or to what preceding tests do.
Comment 36 Alexey Proskuryakov 2018-12-10 12:22:52 PST
(In reply to Simon Fraser (smfr) from comment #33)
> Re-landed in https://trac.webkit.org/changeset/239010/webkit.

I didn't realize that the re-landed changed marked the regression as flaky. Not sure why that's acceptable in this case, but a clear indication of what changed (and what didn't) would have helped.
Comment 37 Simon Fraser (smfr) 2018-12-10 12:44:18 PST
(In reply to Alexey Proskuryakov from comment #36)
> (In reply to Simon Fraser (smfr) from comment #33)
> > Re-landed in https://trac.webkit.org/changeset/239010/webkit.
> 
> I didn't realize that the re-landed changed marked the regression as flaky.
> Not sure why that's acceptable in this case, but a clear indication of what
> changed (and what didn't) would have helped.

The Changelog had this information.

Landed again in https://trac.webkit.org/r239042.