Bug 208654 – When using the scrolling thread, push ScrollingNodeIDs onto PlatformCALayers

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

[Simon Fraser (smfr)]

When using the scrolling thread, push ScrollingNodeIDs onto PlatformCALayers

[Simon Fraser (smfr)]

Created attachment 392597 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/60090331>

[Antti Koivisto]

Comment on attachment 392597 [details]
Patch

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

Still r+ to keep things moving.

> Source/WebCore/ChangeLog:12
> +        We only need one ScrollingNodeID per platform layer, since a given platform layer only
> +        ever has one scrolling role.

I don't think this is true. Specifically a single layer can meaningfully have both ViewportConstrained and Positioning roles and the scrolling tree code deals with this.

[Antti Koivisto]

We create a ScrollingTreePositionedNode with a ScrollingTreeFixedNode as its child in this case, both pointing to the same layer.

See this code in ScrollingTreeFixedNode::applyLayerPositions() dealing with it:

            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;
            }

[Simon Fraser (smfr)]

(In reply to Antti Koivisto from comment #4)
> We create a ScrollingTreePositionedNode with a ScrollingTreeFixedNode as its
> child in this case, both pointing to the same layer.
> 
> See this code in ScrollingTreeFixedNode::applyLayerPositions() dealing with
> it:
> 
>             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;
>             }

True, but for the purposes of overflow scroll hit-testing I don't care about fixed and positioned nodes.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/257949/webkit