Bug 192529

Summary: Add nodes to the scrolling tree in z-index order.
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Simon Fraser (smfr) <simon.fraser>
Severity: Normal CC: bfulgham, dino, ews-watchlist, fred.wang, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
dino: review+, ews-watchlist: commit-queue-
Archive of layout-test-results from ews121 for ios-simulator-wk2 none

Description Simon Fraser (smfr) 2018-12-08 13:02:03 PST
Bug 176914 adds the ability to add nodes to the scrolling tree in the right z-order, but we actually need to make use of that.
Comment 1 Simon Fraser (smfr) 2018-12-08 13:03:41 PST
Marking fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state.html as flakey until this is fixed; the current code attaches nodes to the scrolling tree by traversing a HashSet, which is not ordered.
Comment 2 Radar WebKit Bug Importer 2019-01-18 17:40:47 PST
Comment 3 Simon Fraser (smfr) 2019-01-27 22:24:28 PST
Created attachment 360318 [details]
Comment 4 Simon Fraser (smfr) 2019-01-27 23:15:31 PST
Created attachment 360322 [details]
Comment 5 Simon Fraser (smfr) 2019-01-27 23:15:52 PST
Comment on attachment 360322 [details]

Comment 6 Simon Fraser (smfr) 2019-01-29 15:28:17 PST
Created attachment 360510 [details]
Comment 7 EWS Watchlist 2019-01-29 17:25:37 PST
Comment on attachment 360510 [details]

Attachment 360510 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10944721

New failing tests:
Comment 8 EWS Watchlist 2019-01-29 17:25:38 PST
Created attachment 360530 [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.13.6
Comment 9 Dean Jackson 2019-01-29 17:41:37 PST
Comment on attachment 360510 [details]

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

> Source/WebCore/ChangeLog:21
> +        was hard because the backing was already disconnect from its owning RenderLayer, so I added RenderLayerBacking::willBeDestroyed()

typo: disconnected

> Source/WebCore/rendering/RenderLayerCompositor.cpp:627
> +        return { };

Is this the new nullopt?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3773
> +    ASSERT_IMPLIES(nodeType == ScrollingNodeType::MainFrame, !treeState.parentNodeID.value());

oooh. I didn't know ASSERT_IMPLIES existed :|

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3807
> +        // FIXME


> Source/WebCore/rendering/RenderLayerCompositor.cpp:3837
> +    bool isViewportConstained = roles.contains(ScrollCoordinationRole::ViewportConstrained);

I don't know what a con stain is.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3890
> +        ASSERT(layer.renderer().isFixedPositioned());

huh. i assumed this wouldn't compile in release.

> LayoutTests/scrollingcoordinator/scrolling-tree/scrolling-tree-is-z-order.html:45
> +            z-index: 3;

It might be worth explaining in a comment (assuming I'm correct) that this z-index value causes the third Fixed Node in the results to be the one with the top-left-most layout position.

i.e. the result shows that the scrolling layers are #second, #third, #first, because z-index: 3, auto, 1.

Also, I guess you could make this value 2 :)
Comment 10 Simon Fraser (smfr) 2019-01-29 19:31:29 PST