Bug 222900

Summary: [GTK][WPE] Wrong frame scrolled when view is horizontally scrolled with async scrolling enabled
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Alejandro G. Castro <alex>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alex, bugs-noreply, clord, cmarcelo, ews-watchlist, fred.wang, jamesr, luiz, mrobinson, simon.fraser, tonikitoo, zan, zdobersek
Priority: P2 Keywords: Gtk
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Carlos Garcia Campos
Reported 2021-03-08 02:48:25 PST
It seems we are not taking into account the horizontal scrolling while computing the point that receives the wheel events. To reproduce: 1- Go to https://gitlab.gnome.org/GNOME/glib/-/boards 2- Scroll horizontally and try to scroll any of the frames with the mouse wheel
Attachments
Patch (2.15 KB, patch)
2021-03-19 13:50 PDT, Alejandro G. Castro
no flags
Patch (8.25 KB, patch)
2021-03-30 11:44 PDT, Alejandro G. Castro
no flags
Patch (8.29 KB, patch)
2021-03-30 11:52 PDT, Alejandro G. Castro
no flags
Patch (9.06 KB, patch)
2021-04-06 03:55 PDT, Alejandro G. Castro
no flags
Alejandro G. Castro
Comment 1 2021-03-19 13:50:41 PDT
Alex Christensen
Comment 2 2021-03-19 15:30:42 PDT
Comment on attachment 423771 [details] Patch Does this make a test pass?
Alejandro G. Castro
Comment 3 2021-03-22 02:39:36 PDT
(In reply to Alex Christensen from comment #2) > Comment on attachment 423771 [details] > Patch > > Does this make a test pass? Great question, I tested gtk and unfourtunately we are not using async in the testing, it is a bummer but it is currently the default so makes sense we still test the other scrolling, that is why we do not get failures in these cases. Hopefully when we have all the situation with AC solved we can release this as a default. Anyway, I'm going to check with wpe because it should be used there and maybe there is some test skipped, we need to do a lot of cleaning there, if there is no test I'll add one. Thanks for the comment.
Carlos Garcia Campos
Comment 4 2021-03-25 03:40:59 PDT
Comment on attachment 423771 [details] Patch LGTM, we just need a test
Alejandro G. Castro
Comment 5 2021-03-30 11:44:18 PDT
Alejandro G. Castro
Comment 6 2021-03-30 11:52:55 PDT
Alejandro G. Castro
Comment 7 2021-03-30 12:00:42 PDT
Uploaded new patch with the test because apparently we were not testing the async code path in the WPE test runner. I've activated the async by default, this can cause some unrelated tests that were passing to fail, so we have to handle this patch with care for WPE. Considering this I've requested a new review from Carlos, I've added also Martin to the CC to add more eyes to check new failures. Regarding the question is we were checking this before I've tested locally the whole fast/scrolling directory and other scrolling tests trying to check if this situation was tested before but just not tested and I could not find it so I added a test for this use case.
Alejandro G. Castro
Comment 8 2021-03-31 08:09:45 PDT
Apparently the test times out in IOS, probably the scrolling event reaches the external scrollable area instead of the children? I can make it a glib test only because maybe the pixels used for the events in the test do not work well in every platform. I can also skip in that case for IOS if it is interesting. I'm not sure what is the preferred solution here, I'm adding Simon to check the best option in this case.
Alejandro G. Castro
Comment 9 2021-04-06 03:55:47 PDT
Alejandro G. Castro
Comment 10 2021-04-06 03:57:05 PDT
Carlos pointed out EventSender's mouseMoveTo is not supported in IOS, so I've skipped it, thanks!
EWS
Comment 11 2021-04-07 00:23:37 PDT
EWS
Comment 12 2021-04-07 01:32:37 PDT
Committed r275593: <https://commits.webkit.org/r275593> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425262 [details].
Note You need to log in before you can comment on or make changes to this bug.