[webkit-reviews] review granted: [Bug 198788] REGRESSION(r245818): Async scrolling: Fixed position banner jumps around page : [Attachment 371955] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jun 12 08:23:29 PDT 2019
Simon Fraser (smfr) <simon.fraser at apple.com> has granted Antti Koivisto
<koivisto at iki.fi>'s request for review:
Bug 198788: REGRESSION(r245818): Async scrolling: Fixed position banner jumps
around page
https://bugs.webkit.org/show_bug.cgi?id=198788
Attachment 371955: patch
https://bugs.webkit.org/attachment.cgi?id=371955&action=review
--- Comment #3 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 371955
--> https://bugs.webkit.org/attachment.cgi?id=371955
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).
More information about the webkit-reviews
mailing list