Bug 214179 - [GTK][WPE] Scrolling with mouse wheel doesn't work on iframes with async scrolling enabled
Summary: [GTK][WPE] Scrolling with mouse wheel doesn't work on iframes with async scro...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2020-07-10 02:26 PDT by Carlos Garcia Campos
Modified: 2020-11-12 10:58 PST (History)
10 users (show)

See Also:


Attachments
Patch (13.02 KB, patch)
2020-10-22 04:36 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (13.49 KB, patch)
2020-11-02 06:11 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (13.49 KB, patch)
2020-11-02 06:34 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (13.57 KB, patch)
2020-11-02 07:43 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (13.57 KB, patch)
2020-11-09 01:50 PST, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2020-07-10 02:26:27 PDT
The main frame is still scrolled instead. I think this used to work, so it's probably a "recent" regression.
Comment 1 Simon Fraser (smfr) 2020-07-10 09:34:07 PDT
On Mac the scrolling thread now hit-tests CALayers to determine which thing to scroll. GTK needs an implementation of:
RefPtr<ScrollingTreeNode> ScrollingTreeMac::scrollingNodeForPoint(FloatPoint point)
Comment 2 Chris Lord 2020-10-22 04:36:55 PDT
Created attachment 412085 [details]
Patch
Comment 3 Carlos Garcia Campos 2020-10-22 04:54:07 PDT
Comment on attachment 412085 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        async scrolling is enabled on WPE and GTK backends.

backends -> ports

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:97
> +        auto nodeID = scrollingNodeID();
> +        compositionLayer.updateState([nodeID](Nicosia::CompositionLayer::LayerState& state) {

[nodeID = scrollingNodeID()]

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeNicosia.cpp:80
> +static void collectDescendantLayersAtPoint(Vector<RefPtr<CompositionLayer>>& layersAtPoint, RefPtr<CompositionLayer> parent, FloatPoint point)

Could we make Vector<RefPtr<CompositionLayer>> the return value instead? and point should be a const reference

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeOverflowScrollingNodeNicosia.cpp:69
> +        auto nodeID = scrollingNodeID();
> +        compositionLayer.updateState([nodeID](Nicosia::CompositionLayer::LayerState& state) {

[nodeID = scrollingNodeID()]
Comment 4 Chris Lord 2020-10-22 05:02:37 PDT
(In reply to Carlos Garcia Campos from comment #3)
> Comment on attachment 412085 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412085&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        async scrolling is enabled on WPE and GTK backends.
> 
> backends -> ports

Okidokes.

> > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:97
> > +        auto nodeID = scrollingNodeID();
> > +        compositionLayer.updateState([nodeID](Nicosia::CompositionLayer::LayerState& state) {
> 
> [nodeID = scrollingNodeID()]

I tried this originally and got some syntax error, but I'll have another go.

> > Source/WebCore/page/scrolling/nicosia/ScrollingTreeNicosia.cpp:80
> > +static void collectDescendantLayersAtPoint(Vector<RefPtr<CompositionLayer>>& layersAtPoint, RefPtr<CompositionLayer> parent, FloatPoint point)
> 
> Could we make Vector<RefPtr<CompositionLayer>> the return value instead? and
> point should be a const reference

Not with this being a static function really - I did it this way to mirror how it's done in ScrollingTreeMac. Good point about the const reference though, will change that.

> > Source/WebCore/page/scrolling/nicosia/ScrollingTreeOverflowScrollingNodeNicosia.cpp:69
> > +        auto nodeID = scrollingNodeID();
> > +        compositionLayer.updateState([nodeID](Nicosia::CompositionLayer::LayerState& state) {
> 
> [nodeID = scrollingNodeID()]

Same comment as above, but I'll try it again, maybe I just missed something.
Comment 5 Zan Dobersek 2020-10-23 03:13:46 PDT
Comment on attachment 412085 [details]
Patch

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

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeNicosia.cpp:84
> +    parent->accessCommitted([&](const CompositionLayer::LayerState& state) {

There's three stages in the CompositionLayer:
- pending, for storing local changes before they are flushed at the end of an update (layer flush or scrolling),
- staging, for storing the flushed changes,
- committed, which mirrors the staging state and applies it to the composition framework.

My perspective is that the committed state is just a (otherwise necessary) copy of the staging state. Think of it as a delayed, on-demand copy that's updated by the composition engine when possible (i.e. when a new frame can be rendered and there's pending changes to incorporate into the visually-presented scene). As such, I feel accessing the staging state here (and elsewhere) would guarantee retrieving more accurate and up-to-date information. But it would require a new accessor in the CompositionLayer.

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeOverflowScrollingNodeNicosia.cpp:73
> +            state.delta.scrollingNodeChanged = true;
> +        });
> +    }

How is this expected to be propagated from the local state over to the committed state? Right now it depends on whatever update comes next -- either a layer flush done on the main thread, or an update triggered through UpdateScope upon scrolling being handled on the scrolling thread.

Also applies to the change in ScrollingTreeFrameScrollingNodeNicosia::commitStateBeforeChildren().

> Source/WebCore/platform/graphics/nicosia/NicosiaPlatformLayer.h:204
> +
> +        WebCore::ScrollingNodeID scrollingNodeID;

This should have a sensible initialization value (0 probably).
Comment 6 Chris Lord 2020-11-02 06:11:06 PST
Created attachment 412910 [details]
Patch
Comment 7 Chris Lord 2020-11-02 06:34:39 PST
Created attachment 412913 [details]
Patch
Comment 8 Chris Lord 2020-11-02 07:43:27 PST
Created attachment 412918 [details]
Patch
Comment 9 Carlos Garcia Campos 2020-11-06 00:46:22 PST
Zan?
Comment 10 Chris Lord 2020-11-09 01:50:01 PST
Created attachment 413569 [details]
Patch
Comment 11 EWS 2020-11-09 02:21:18 PST
Committed r269579: <https://trac.webkit.org/changeset/269579>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413569 [details].