Bug 214179 – [GTK][WPE] Scrolling with mouse wheel doesn't work on iframes with async scrolling enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

The main frame is still scrolled instead. I think this used to work, so it's probably a "recent" regression.

[Simon Fraser (smfr)]

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)

[Chris Lord]

Created attachment 412085 [details]
Patch

[Carlos Garcia Campos]

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()]

[Chris Lord]

(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.

[Zan Dobersek]

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).

[Chris Lord]

Created attachment 412910 [details]
Patch

[Chris Lord]

Created attachment 412913 [details]
Patch

[Chris Lord]

Created attachment 412918 [details]
Patch

[Carlos Garcia Campos]

Zan?

[Chris Lord]

Created attachment 413569 [details]
Patch

[EWS]

Committed r269579: <https://trac.webkit.org/changeset/269579>

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