[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