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

Description Carlos Garcia Campos 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
Comment 1 Alejandro G. Castro 2021-03-19 13:50:41 PDT
Created attachment 423771 [details]
Patch
Comment 2 Alex Christensen 2021-03-19 15:30:42 PDT
Comment on attachment 423771 [details]
Patch

Does this make a test pass?
Comment 3 Alejandro G. Castro 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.
Comment 4 Carlos Garcia Campos 2021-03-25 03:40:59 PDT
Comment on attachment 423771 [details]
Patch

LGTM, we just need a test
Comment 5 Alejandro G. Castro 2021-03-30 11:44:18 PDT
Created attachment 424667 [details]
Patch
Comment 6 Alejandro G. Castro 2021-03-30 11:52:55 PDT
Created attachment 424671 [details]
Patch
Comment 7 Alejandro G. Castro 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.
Comment 8 Alejandro G. Castro 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.
Comment 9 Alejandro G. Castro 2021-04-06 03:55:47 PDT
Created attachment 425262 [details]
Patch
Comment 10 Alejandro G. Castro 2021-04-06 03:57:05 PDT
Carlos pointed out EventSender's mouseMoveTo is not supported in IOS, so I've skipped it, thanks!
Comment 11 EWS 2021-04-07 00:23:37 PDT
zan@falconsigh.net does not have reviewer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 425262 [details] from commit queue.
Comment 12 EWS 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].