[webkit-reviews] review denied: [Bug 195396] [iOS WK] REGRESSION (r242132): Fixed position banners flicker and move when scrolling (Apple, Tesla, YouTube, Reddit) : [Attachment 363839] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 195396: [iOS WK] REGRESSION (r242132): Fixed position banners flicker and
move when scrolling (Apple, Tesla, YouTube, Reddit)
https://bugs.webkit.org/show_bug.cgi?id=195396

Attachment 363839: Patch

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




--- Comment #4 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 363839
  --> https://bugs.webkit.org/attachment.cgi?id=363839
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.