Bug 176914

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

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

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

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

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

I meant the scrolling tree, sorry.

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.

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.

Created attachment 324232 [details] WIP Patch

Created attachment 324410 [details] WIP Patch This applies on top of the one for bug 178508.

Created attachment 327061 [details] WIP Patch Rebasing after bug 178508.

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

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

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.

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

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

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.

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

Created attachment 356722 [details] Patch

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 on attachment 356722 [details] Patch Clearing flags on attachment: 356722 Committed r238947: <https://trac.webkit.org/changeset/238947>

All reviewed patches have been landed. Closing bug.

<rdar://problem/46542237>

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

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

I'll take a look.

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>

Why did EWS not show this failure?

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

Re-opened since this is blocked by bug 192537

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.

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

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