Bug 198788 – (Async scrolling) Handle 'position:fixed' inside 'position:sticky' correctly.

• Component: Scrolling
• Status: RESOLVED FIXED

[Antti Koivisto]

https://www.fandom.com/explore

[Antti Koivisto]

<rdar://problem/51589759>

[Antti Koivisto]

Created attachment 371955 [details]
patch

[Simon Fraser (smfr)]

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

[Antti Koivisto]

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

[Simon Fraser (smfr)]

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

[Antti Koivisto]

Created attachment 371973 [details]
patch

[WebKit Commit Bot]

Comment on attachment 371973 [details]
patch

Clearing flags on attachment: 371973

Committed r246367: <https://trac.webkit.org/changeset/246367>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.