Summary: | [GTK][WPE] Scrolling with mouse wheel doesn't work on iframes with async scrolling enabled | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||||
Component: | WebKitGTK | Assignee: | Chris Lord <clord> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bugs-noreply, clord, cmarcelo, ews-watchlist, fred.wang, jamesr, luiz, simon.fraser, tonikitoo, zan | ||||||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=218859 https://bugs.webkit.org/show_bug.cgi?id=219594 |
||||||||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2020-07-10 02:26:27 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) Created attachment 412085 [details]
Patch
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()] (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 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). Created attachment 412910 [details]
Patch
Created attachment 412913 [details]
Patch
Created attachment 412918 [details]
Patch
Zan? Created attachment 413569 [details]
Patch
Committed r269579: <https://trac.webkit.org/changeset/269579> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413569 [details]. |