Bug 195396 – [iOS WK] REGRESSION (r242132): Fixed position banners flicker and move when scrolling (Apple, Tesla, YouTube, Reddit)

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

[iOS WK] REGRESSION (r242132): Fixed position banners flicker and move when scrolling (Apple, Tesla, YouTube, Reddit)

[Simon Fraser (smfr)]

Created attachment 363839 [details]
Patch

[Simon Fraser (smfr)]

<rdar://problem/48518959>

[Frédéric Wang (:fredw)]

Comment on attachment 363839 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:187
> +    bool scrollingStateChanged = scrollPositionAndLayoutViewportMatch(position, overrideLayoutViewport);

Can we have both scroll position change and state change happening?
Maybe this should be renamed scrollingStateChangeOnly or scrollPositionOrLayoutViewportChange?

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:197
> +        scrollingTree().scrollingTreeNodeDidScroll(*this);

I don't understand this. Shoudn't it be !scrollingStateChanged? When position != m_currentScrollPosition, we are no longer doing this call compared to the former code.

[Antti Koivisto]

Comment on attachment 363839 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        First, ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling() would early return if the scroll position
> +        hadn't changed. It also needs to check the supplied layoutViewport (if any), but in some cases running the
> +        notifyRelatedNodesAfterScrollPositionChange() code is necessary even without a scroll position change:
> +        if the web process has committed new scrolling tree state (e.g. with new fixed constraints) since
> +        the last call, we have to run the layer positioning code to have fixed layers re-adjust their position relative
> +        to the root. This was the primary bug fix.
> +
> +        Secondly, a layer tree commit can give ScrollingTreeFrameScrollingNode a new layout viewport, but we need to
> +        adjust this by the scrolling tree's current scroll position in case it gets used before the next scroll.

Architectural fix for these sort of issues is to stop using web process supplied scroll positions for anything. Not having them in scrolling tree in the first place is the most robust way to achieve that.

>> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:187
>> +    bool scrollingStateChanged = scrollPositionAndLayoutViewportMatch(position, overrideLayoutViewport);
> 
> Can we have both scroll position change and state change happening?
> Maybe this should be renamed scrollingStateChangeOnly or scrollPositionOrLayoutViewportChange?

State changed if position and viewport matches? This makes no logical sense.

[Simon Fraser (smfr)]

(In reply to Antti Koivisto from comment #4)
> Comment on attachment 363839 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363839&action=review
> 
> > Source/WebCore/ChangeLog:19
> > +        First, ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling() would early return if the scroll position
> > +        hadn't changed. It also needs to check the supplied layoutViewport (if any), but in some cases running the
> > +        notifyRelatedNodesAfterScrollPositionChange() code is necessary even without a scroll position change:
> > +        if the web process has committed new scrolling tree state (e.g. with new fixed constraints) since
> > +        the last call, we have to run the layer positioning code to have fixed layers re-adjust their position relative
> > +        to the root. This was the primary bug fix.
> > +
> > +        Secondly, a layer tree commit can give ScrollingTreeFrameScrollingNode a new layout viewport, but we need to
> > +        adjust this by the scrolling tree's current scroll position in case it gets used before the next scroll.
> 
> Architectural fix for these sort of issues is to stop using web process
> supplied scroll positions for anything. Not having them in scrolling tree in
> the first place is the most robust way to achieve that.

Web process viewport layout rect is baked into FixedPositionViewportConstraints.

The geometry of fixed layers is based on the geometry of layers in the z-order ancestor chain (because the layer tree is unavoidably in z-order, because that's the only way to have CA do the right back-to-front ordering. That means that a fixed layer can be deeply nested in the z-order tree; even though its containing block is trivially the RenderView, we must compute the position for that fixed layer relative to its parent in z-order, which can be some arbitrary layer, even inside of overflow:scroll.

That, in turn, means that layout information is baked into FixedPositionViewportConstraints::m_layerPositionAtLastLayout, and we need pass m_viewportRectAtLastLayout in order to compute deltas in the scrolling tree.

What this means is that I don't think bug 194289 is achievable with our current architecture, where layer positions, and layer z-order have to be in the same hierarchy. Hence the need for patches like this. 

> >> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:187
> >> +    bool scrollingStateChanged = scrollPositionAndLayoutViewportMatch(position, overrideLayoutViewport);
> > 
> > Can we have both scroll position change and state change happening?
> > Maybe this should be renamed scrollingStateChangeOnly or scrollPositionOrLayoutViewportChange?
> 
> State changed if position and viewport matches? This makes no logical sense.

scrollingStateChanged was mis-named. It should be scrollingStateUnchanged.

[Simon Fraser (smfr)]

Created attachment 363877 [details]
Patch

[Antti Koivisto]

Comment on attachment 363877 [details]
Patch

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

Ok but please test well before landing.

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:187
> +    bool scrollPositionChanged = !scrollPositionAndLayoutViewportMatch(position, overrideLayoutViewport);

This is not just a rename, you invert the logic from the previous patch. This raises questions about testing.

[Simon Fraser (smfr)]

Comment on attachment 363877 [details]
Patch

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

>> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:187
>> +    bool scrollPositionChanged = !scrollPositionAndLayoutViewportMatch(position, overrideLayoutViewport);
> 
> This is not just a rename, you invert the logic from the previous patch. This raises questions about testing.

I'll check what scrollingTreeNodeDidScroll() does if you all it when nothing really changed, but it might be a no-op. I tested this patch on sim and hardware.

[WebKit Commit Bot]

Comment on attachment 363877 [details]
Patch

Clearing flags on attachment: 363877

Committed r242601: <https://trac.webkit.org/changeset/242601>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.