Fix async scroll event propagation for GTK4 #8663
Conversation
|
EWS run on previous version of this PR (hash d247dfa)
|
moggiesir
force-pushed
the
eng/GTK4-Infinite-loop-when-using-the-mouse-wheel
branch
from
d247dfa to
b827d61
Compare
|
EWS run on previous version of this PR (hash b827d61)
|
moggiesir
force-pushed
the
eng/GTK4-Infinite-loop-when-using-the-mouse-wheel
branch
from
b827d61 to
924f20f
Compare
|
EWS run on previous version of this PR (hash 924f20f)
|
I don't have much time this week to look at this one. @Exalm could you review it, please? |
moggiesir
force-pushed
the
eng/GTK4-Infinite-loop-when-using-the-mouse-wheel
branch
from
924f20f to
15e3f79
Compare
|
EWS run on previous version of this PR (hash 15e3f79)
|
moggiesir
force-pushed
the
eng/GTK4-Infinite-loop-when-using-the-mouse-wheel
branch
from
15e3f79 to
cf40646
Compare
|
EWS run on previous version of this PR (hash cf40646)
|
moggiesir
force-pushed
the
eng/GTK4-Infinite-loop-when-using-the-mouse-wheel
branch
from
cf40646 to
519d520
Compare
|
EWS run on previous version of this PR (hash 519d520)
|
moggiesir
force-pushed
the
eng/GTK4-Infinite-loop-when-using-the-mouse-wheel
branch
2 times, most recently
from
d2119c0 to
3725068
Compare
|
EWS run on previous version of this PR (hash 3725068)
|
|
EWS run on previous version of this PR (hash d2119c0) |
moggiesir
force-pushed
the
eng/GTK4-Infinite-loop-when-using-the-mouse-wheel
branch
from
3725068 to
1f239b9
Compare
|
EWS run on previous version of this PR (hash 1f239b9)
|
🧪 mac-AS-debug-wk2
|
Any further comments? |
moggiesir
force-pushed
the
eng/GTK4-Infinite-loop-when-using-the-mouse-wheel
branch
from
1f239b9 to
362e4b8
Compare
|
EWS run on previous version of this PR (hash 362e4b8) |
|
I think it looks good, as in I don't see obvious regressions as before, but I haven't tried running it - I don't have an up to date webkit checkout atm and not going to pull/build it on a weekend. |
moggiesir
force-pushed
the
eng/GTK4-Infinite-loop-when-using-the-mouse-wheel
branch
from
362e4b8 to
35c4094
Compare
|
EWS run on current version of this PR (hash 35c4094) |
https://bugs.webkit.org/show_bug.cgi?id=243924 https://bugs.webkit.org/show_bug.cgi?id=245616 Reviewed by Michael Catanzaro. Since GTK offers no way to asynchronously decide on whether an event should be propagated or stopped and webkit doesn't want to block the UiProcess on event handing in the WebProcess, webkit2gtk needs to use a bit of a hacky workaround. This involves returning GDK_EVENT_STOP to the original event handling, sending the event to the WebProcess for handling, which then asynchronously determines whether the event was handled. If not, the webkit scroll handler is informed that the "next" scroll event should be propagated instead of handled+stopped, then re-emits the event for handling by GTK's run loop. This works fine in GTK3 because there's an API to immediately run the event loop for a given event (ensuring that the next event that the handler sees is the intended one). For GTK4, the API for re-inserting an event appends the event to the end of the event queue (despite what the documentation says), meaning the next event the handler sees may not be the intended event - thus the wrong event gets propagated and the just re-added event gets sent to the WebProcess again. This coupled with the fact that GDK4 will synthesize scroll-start/scroll-end events in some cases to ensure scroll events allows have the scroll-start, scroll, scroll-end sequence can easily cause loops since it's not just 1 event that needs to be propagated, it's up to 3. To work better with the APIs that exist for GTK4, it seems like the best option is to track the set of events that are "pending propagation" instead of relying on the tight interlock of just ignoring the "next" event. The key handling potentially also needs to be addressed similarly. Unfortunately, even tracking the set of events to ignore is challenging because GTK4 can compress/coalesce scroll events, meaning there will inevitably be some cases where we won't propagate events that otherwise maybe should have been propagated (and "unhandled" scroll events may cause some faster/slower than expected scrolling when they get compressed into an not-yet-handled events). Canonical link: https://commits.webkit.org/259530@main
webkit-early-warning-system
force-pushed
the
eng/GTK4-Infinite-loop-when-using-the-mouse-wheel
branch
from
35c4094 to
dbc31d0
Compare
|
Committed 259530@main (dbc31d0): https://commits.webkit.org/259530@main Reviewed commits have been landed. Closing PR #8663 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
|
Planning to remove all of this code in https://bugs.webkit.org/show_bug.cgi?id=261348 and just never propagate the events... |
This function is deprecated, but we should still document it properly. It appends, not prepends. This is clear enough from its implementation, but also we have practical experience with WebKit in: WebKit/WebKit#8663
|
If event re-injection is removed, won’t scroll propagation break? I.e if a scrollable webview is in a scrollable gtk panel, without further work in WebKit, it seems like you wouldn’t be able to scroll the gtk panel if your cursor is over the webview? |
|
Hm, good question. it depends: if the GTK panel handles the event in the capture phase, then it would not be possible to scroll the web view. If the panel handles the event in the bubble phase, then it would not be possible to scroll the panel. But I assume the status quo is: not possible to scroll the panel unless the web view does not handle the scroll event (right?). That doesn't seem useful either? If we need to, then we could emit a signal to indicate that the scroll event was unhandled, although without |
Actually I think this is exactly what email clients like Geary expect. You're probably right; removing this might be a problem. |
|
Left a comment here. I'm still tempted to remove the wheel event reinjection now to avoid applications beginning to depend on it when porting to GTK 4, but I fear this might prevent a few applications from porting. |
This function is deprecated, but we should still document it properly. It appends, not prepends. This is clear enough from its implementation, but also we have practical experience with WebKit in: WebKit/WebKit#8663 Matthias prefers to avoid the prepend, append, start, and end terminology altogether.
This function is deprecated, but we should still document it properly. It appends, not prepends. This is clear enough from its implementation, but also we have practical experience with WebKit in: WebKit/WebKit#8663 Matthias prefers to avoid the prepend, append, start, and end terminology altogether.
This function is deprecated, but we should still document it properly. It appends, not prepends. This is clear enough from its implementation, but also we have practical experience with WebKit in: WebKit/WebKit#8663 Matthias prefers to avoid the prepend, append, start, and end terminology altogether.
6 participants
dbc31d0
35c4094
🛠 wincairo🧪 mac-AS-debug-wk2