Bug 192529 - Add nodes to the scrolling tree in z-index order.
Summary: Add nodes to the scrolling tree in z-index order.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-08 13:02 PST by Simon Fraser (smfr)
Modified: 2019-01-29 19:31 PST (History)
7 users (show)

See Also:


Attachments
Patch (75.85 KB, patch)
2019-01-27 22:24 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (75.75 KB, patch)
2019-01-27 23:15 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (74.94 KB, patch)
2019-01-29 15:28 PST, Simon Fraser (smfr)
dino: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.43 MB, application/zip)
2019-01-29 17:25 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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
<rdar://problem/47402708>
Comment 3 Simon Fraser (smfr) 2019-01-27 22:24:28 PST
Created attachment 360318 [details]
Patch
Comment 4 Simon Fraser (smfr) 2019-01-27 23:15:31 PST
Created attachment 360322 [details]
Patch
Comment 5 Simon Fraser (smfr) 2019-01-27 23:15:52 PST
Comment on attachment 360322 [details]
Patch

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

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

New failing tests:
fast/scrolling/ios/hit-testing-iframe.html
Comment 8 Build Bot 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]
Patch

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

oops!

> 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
https://trac.webkit.org/changeset/240698/webkit