Bug 218477 - Scroll position can get reset after programmatic scroll
Summary: Scroll position can get reset after programmatic scroll
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-02 20:01 PST by Simon Fraser (smfr)
Modified: 2020-11-03 12:04 PST (History)
10 users (show)

See Also:


Attachments
Patch (21.15 KB, patch)
2020-11-02 20:10 PST, Simon Fraser (smfr)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (21.18 KB, patch)
2020-11-02 22:39 PST, Simon Fraser (smfr)
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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