Bug 176914 – Allow control over child order when adding nodes to the scrolling tree

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Frédéric Wang (:fredw)]

See bug 172917 comment 15.

[Frédéric Wang (:fredw)]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Frédéric Wang (:fredw)]

I meant the scrolling tree, sorry.

[Frédéric Wang (:fredw)]

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.

[Simon Fraser (smfr)]

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.

[Frédéric Wang (:fredw)]

Created attachment 324232 [details]
WIP Patch

[Frédéric Wang (:fredw)]

Created attachment 324410 [details]
WIP Patch

This applies on top of the one for bug 178508.

[Frédéric Wang (:fredw)]

Created attachment 327061 [details]
WIP Patch

Rebasing after bug 178508.

[Simon Fraser (smfr)]

I'm thinking of storing scrolling data in the GraphicsLayer tree. That would automatically give us the correct z-index.

[Frédéric Wang (:fredw)]

(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?

[Simon Fraser (smfr)]

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.

[Frédéric Wang (:fredw)]

Created attachment 356603 [details]
Patch 327061, rebased on top of bug 192398 (untested)

[Simon Fraser (smfr)]

*** Bug 192395 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

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.

[Frédéric Wang (:fredw)]

Created attachment 356615 [details]
Simon's patch (bug 192395)

[Frédéric Wang (:fredw)]

Created attachment 356722 [details]
Patch

[Frédéric Wang (:fredw)]

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.

[WebKit Commit Bot]

Comment on attachment 356722 [details]
Patch

Clearing flags on attachment: 356722

Committed r238947: <https://trac.webkit.org/changeset/238947>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46542237>

[Truitt Savell]

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))
     )
   )
 )

[Truitt Savell]

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

[Simon Fraser (smfr)]

I'll take a look.

[Truitt Savell]

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>

[Simon Fraser (smfr)]

Why did EWS not show this failure?

[Simon Fraser (smfr)]

Re-landed in https://trac.webkit.org/changeset/239010/webkit.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 192537

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

(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.

[Simon Fraser (smfr)]

(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.