[iOS WK] REGRESSION (r242132): Fixed position banners flicker and move when scrolling (Apple, Tesla, YouTube, Reddit)
Created attachment 363839 [details] Patch
<rdar://problem/48518959>
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.
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.
(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.
Created attachment 363877 [details] Patch
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.
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.
Comment on attachment 363877 [details] Patch Clearing flags on attachment: 363877 Committed r242601: <https://trac.webkit.org/changeset/242601>
All reviewed patches have been landed. Closing bug.