Bug 218477

Summary: Scroll position can get reset after programmatic scroll
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: ScrollingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, ews-watchlist, fred.wang, jamesr, koivisto, luiz, simon.fraser, thorton, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch koivisto: review+

Description Simon Fraser (smfr) 2020-11-02 20:01:56 PST
Scroll position can get reset after programmatic scroll
Comment 1 Simon Fraser (smfr) 2020-11-02 20:10:12 PST
Created attachment 413001 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2020-11-02 20:10:39 PST
<rdar://problem/70978102>
Comment 3 Simon Fraser (smfr) 2020-11-02 22:39:44 PST
Created attachment 413005 [details]
Patch
Comment 4 Antti Koivisto 2020-11-03 02:52:37 PST
Comment on attachment 413005 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingTree.cpp:405
> +//    LOG(Scrolling, "\nScrollingTree %p applyLayerPositions (main thread %d)", this, isMainThread());

Stray //

> Source/WebCore/page/scrolling/ScrollingTree.cpp:582
> +    LockHolder locker(m_pendingScrollUpdatesLock);

I prefer

auto locker = holdLock(m_pendingScrollUpdatesLock);

> Source/WebCore/page/scrolling/ScrollingTree.h:221
> +        ScrollUpdate() = default;
> +        ScrollUpdate(ScrollingNodeID scrollingNodeID, FloatPoint point, Optional<FloatPoint> viewportOrigin, ScrollingLayerPositionAction udpateAction)
> +            : nodeID(scrollingNodeID)
> +            , scrollPosition(point)
> +            , layoutViewportOrigin(viewportOrigin)
> +            , updateLayerPositionAction(udpateAction)
> +        { }

You can probably delete these explicit constructors and the code will still compile as-is.

> Source/WebCore/page/scrolling/ScrollingTree.h:225
> +        FloatPoint scrollPosition;
> +        Optional<FloatPoint> layoutViewportOrigin;

...except these may need { }.
Comment 5 Antti Koivisto 2020-11-03 02:53:45 PST
Comment on attachment 413005 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingTree.h:216
> +        ScrollUpdate(ScrollingNodeID scrollingNodeID, FloatPoint point, Optional<FloatPoint> viewportOrigin, ScrollingLayerPositionAction udpateAction)

Spelling udpateAction
Comment 6 Simon Fraser (smfr) 2020-11-03 12:04:54 PST
https://trac.webkit.org/r269312