Summary: | REGRESSION(r246496): [GTK] Tapping the web view scrolls up a bit | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alice Mikhaylenko <alicem> | ||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bugs-noreply, cgarcia, commit-queue, mcatanzaro | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Alice Mikhaylenko
2019-07-24 09:58:26 PDT
Correction. It also looks like that patch for phosh is faulty. :) It only scrolls up once at the beginning, i.e. on any tap. Ok, this is really a GTK bug. If scroll direction of a GdkEvent is GDK_SCROLL_SMOOTH, then gdk_event_get_scroll_direction() returns false and sets direction to GDK_SCROLL_UP, because that's the first value in the enum. Well, might also be the expected behavior, since it does return false after all. See https://gitlab.gnome.org/GNOME/gtk/issues/2048 Created attachment 374890 [details]
Patch
Comment on attachment 374890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374890&action=review > Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:64 > GdkScrollDirection direction; I think we should initialize it here instead. > Source/WebKit/Shared/gtk/WebEventFactory.cpp:206 > GdkScrollDirection direction; Ditto. Not really. GTK does set the value even if returns false. (In reply to Alexander Mikhaylenko from comment #2) > Ok, this is really a GTK bug. If scroll direction of a GdkEvent is > GDK_SCROLL_SMOOTH, then gdk_event_get_scroll_direction() returns false and > sets direction to GDK_SCROLL_UP, because that's the first value in the enum. > > Well, might also be the expected behavior, since it does return false after > all. > > See https://gitlab.gnome.org/GNOME/gtk/issues/2048 gboolean gdk_event_get_scroll_direction (const GdkEvent *event, GdkScrollDirection *direction) { gboolean fetched = TRUE; GdkScrollDirection dir = 0; switch (event->type) { case GDK_SCROLL: if (event->scroll.direction == GDK_SCROLL_SMOOTH) fetched = FALSE; else dir = event->scroll.direction; break; default: fetched = FALSE; break; } if (direction) *direction = dir; return fetched; } So... WTF? The function is documented to get the GdkScrollDirection, not get the GdkScrollDirection except fail if the scroll direction is GDK_SCROLL_SMOOTH. Shouldn't it be: gboolean gdk_event_get_scroll_direction (const GdkEvent *event, GdkScrollDirection *direction) { if (event->type == GDK_SCROLL) { *direction = event->scroll.direction; return TRUE; *direction = 0; return FALSE; } Yeah, seems weird to me too. Tbh not a fan of the whole GDK_SCROLL_SMOOTH thing. IMO it would be a lot saner to just have deltas for discrete scrolling too. Comment on attachment 374890 [details]
Patch
I'll accept this if you add a link to your GTK bug report, because upstream seems hesitant but it's confusing and not good....
Created attachment 374953 [details]
Patch
Added the link to https://gitlab.gnome.org/GNOME/gtk/issues/2048 in the changelog Comment on attachment 374953 [details] Patch Clearing flags on attachment: 374953 Committed r247862: <https://trac.webkit.org/changeset/247862> All reviewed patches have been landed. Closing bug. |