Bug 198292 – [async scrolling] Fixed positioning inside stacking context overflow scroll is jumpy

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Antti Koivisto]

It is.

[Antti Koivisto]

Created attachment 370727 [details]
patch

[Antti Koivisto]

GTK/WPE build failures are in unused "nicosia" code. It is unreasonable to push maintenance burden of it to people who are working on async scrolling.

[Zan Dobersek]

Created attachment 370729 [details]
GTK/WPE build fixes

Here's the GTK/WPE build fixes in the Nicosia code. Feel free to apply it before landing, otherwise I can land it once your patch lands.

[EWS Watchlist]

Comment on attachment 370727 [details]
patch

Attachment 370727 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12307670

New failing tests:
scrollingcoordinator/mac/nested-sticky.html

[EWS Watchlist]

Created attachment 370734 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Antti Koivisto]

Created attachment 370738 [details]
patch

[Darin Adler]

Comment on attachment 370738 [details]
patch

Old code seems to have "cleverly" avoided a source of n^2 algorithm cost, by computing as we descend the tree. New code walks the parent list so is potentially slower for very deep trees. Makes me wonder if we should have some kind of stress test for a deeply nested tree. I’m sure many of our algorithms have such issues, not sure what our performance/scaling goals are for them.

[Simon Fraser (smfr)]

Comment on attachment 370738 [details]
patch

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

> Source/WebCore/page/scrolling/cocoa/ScrollingTreeFixedNode.mm:90
> +        FloatSize overflowScrollDelta;
> +        // FIXME: This code is wrong in complex cases where the fixed element is inside a positioned node as
> +        //        the scroll container order does not match the scrolling tree ancestor order.
> +        for (auto* node = parent(); node; node = node->parent()) {
> +            if (is<ScrollingTreeFrameScrollingNode>(*node)) {
> +                // Fixed nodes are positioned relative to the containing frame scrolling node.
> +                // We bail out after finding one.
> +                auto layoutViewport = downcast<ScrollingTreeFrameScrollingNode>(*node).layoutViewport();
> +                return m_constraints.layerPositionForViewportRect(layoutViewport) - overflowScrollDelta;
> +            }
> +
> +            if (is<ScrollingTreeOverflowScrollingNode>(*node)) {
> +                // To keep the layer still during async scrolling we adjust by how much the position has changed since layout.
> +                auto& overflowNode = downcast<ScrollingTreeOverflowScrollingNode>(*node);
> +                auto localDelta = overflowNode.lastCommittedScrollPosition() - overflowNode.currentScrollPosition();
> +                overflowScrollDelta += localDelta;
> +            }
> +        }
> +        ASSERT_NOT_REACHED();
> +        return FloatPoint();

Won't Sticky nodes need similar logic?

[Antti Koivisto]

> Won't Sticky nodes need similar logic?

Maybe. I'll add some tests and fixes as needed separately.

[WebKit Commit Bot]

Comment on attachment 370738 [details]
patch

Clearing flags on attachment: 370738

Committed r245818: <https://trac.webkit.org/changeset/245818>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51186119>

[Antti Koivisto]

Created attachment 370769 [details]
test