Bug 198788

Summary: (Async scrolling) Handle 'position:fixed' inside 'position:sticky' correctly.
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: ScrollingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, simon.fraser
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
simon.fraser: review+
patch none

Description Antti Koivisto 2019-06-12 07:21:05 PDT
https://www.fandom.com/explore
Comment 1 Antti Koivisto 2019-06-12 07:21:22 PDT
<rdar://problem/51589759>
Comment 2 Antti Koivisto 2019-06-12 07:34:22 PDT
Created attachment 371955 [details]
patch
Comment 3 Simon Fraser (smfr) 2019-06-12 08:23:29 PDT
Comment on attachment 371955 [details]
patch

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

> Source/WebCore/ChangeLog:3
> +        REGRESSION(r245818): Async scrolling: Fixed position banner jumps around page

Change the title to make it clear this is about fixed inside sticky.

> Source/WebCore/page/scrolling/cocoa/ScrollingTreeFixedNode.mm:106
> +            if (is<ScrollingTreePositionedNode>(*ancestor)) {
> +                auto& positioningAncestor = downcast<ScrollingTreePositionedNode>(*ancestor);
> +                // See if sticky node already handled this positioning node.
> +                // FIXME: Include positioning node information to sticky/fixed node to avoid these tests.
> +                if (lastStickyNode && lastStickyNode->layer() == positioningAncestor.layer())
> +                    continue;
> +                if (positioningAncestor.layer() != m_layer)
> +                    overflowScrollDelta -= positioningAncestor.scrollDeltaSinceLastCommit();
> +                continue;
> +            }
> +
> +            if (is<ScrollingTreeStickyNode>(*ancestor)) {
> +                auto& stickyNode = downcast<ScrollingTreeStickyNode>(*ancestor);
> +                overflowScrollDelta += stickyNode.scrollDeltaSinceLastCommit();
> +                lastStickyNode = &stickyNode;
> +                continue;
> +            }

It seems weird for ScrollingTreeFixedNode to have to test all its ancestor node types. This was the problem that the old delta was trying to solve, I think; these ancestors should pass state down the tree for the applyLayerPositions() tree walk.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3057
> +        if (ancestor->hasCompositedScrollableOverflow())
> +            return true;

But this might not be in the ancestor paint order chain. A simple fixed -> non-stacking overflow -> fixed should not need to composite the inner fixed. I think we only need to composite the inner fixed if some paint-order ancestor has a scrolling tree node (but we can't do that check at this stage).
Comment 4 Antti Koivisto 2019-06-12 08:42:05 PDT
> It seems weird for ScrollingTreeFixedNode to have to test all its ancestor
> node types. This was the problem that the old delta was trying to solve, I
> think; these ancestors should pass state down the tree for the
> applyLayerPositions() tree walk.

The old approach had multiple problems. It didn't work unless update started from the root (which it doesn't in most cases). It couldn't handle sticky without additional parameters. 

This is much easier to reason about and when we have decent test coverage we can factor this differently.


> > Source/WebCore/rendering/RenderLayerCompositor.cpp:3057
> > +        if (ancestor->hasCompositedScrollableOverflow())
> > +            return true;
> 
> But this might not be in the ancestor paint order chain. A simple fixed ->
> non-stacking overflow -> fixed should not need to composite the inner fixed.
> I think we only need to composite the inner fixed if some paint-order
> ancestor has a scrolling tree node (but we can't do that check at this
> stage).

Unnecessary nodes should be harmless. You'll probably need to write the optimal version of this.
Comment 5 Simon Fraser (smfr) 2019-06-12 09:19:11 PDT
(In reply to Antti Koivisto from comment #4)

> Unnecessary nodes should be harmless. You'll probably need to write the
> optimal version of this.

They have memory cost, potentially significant (triggering jetsams).
Comment 6 Antti Koivisto 2019-06-12 10:44:31 PDT
Created attachment 371973 [details]
patch
Comment 7 WebKit Commit Bot 2019-06-12 11:27:42 PDT
Comment on attachment 371973 [details]
patch

Clearing flags on attachment: 371973

Committed r246367: <https://trac.webkit.org/changeset/246367>
Comment 8 WebKit Commit Bot 2019-06-12 11:27:43 PDT
All reviewed patches have been landed.  Closing bug.