Bug 200084 - REGRESSION(r246496): [GTK] Tapping the web view scrolls up a bit
Summary: REGRESSION(r246496): [GTK] Tapping the web view scrolls up a bit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-24 09:58 PDT by Alexander Mikhaylenko
Modified: 2019-07-26 02:00 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.17 KB, patch)
2019-07-25 09:06 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (4.29 KB, patch)
2019-07-26 01:19 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Mikhaylenko 2019-07-24 09:58:26 PDT
Probably another fallout from r246033 and/or r246496, but I don't know for sure. Not going to bisect it because of overheating.

It should probably have been a part of https://bugs.webkit.org/show_bug.cgi?id=199322, but I didn't have a way to reproduce it until very recently.
Comment 1 Alexander Mikhaylenko 2019-07-25 08:20:21 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.
Comment 2 Alexander Mikhaylenko 2019-07-25 08:53:10 PDT
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
Comment 3 Alexander Mikhaylenko 2019-07-25 09:06:47 PDT
Created attachment 374890 [details]
Patch
Comment 4 Michael Catanzaro 2019-07-25 09:40:44 PDT
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.
Comment 5 Alexander Mikhaylenko 2019-07-25 10:08:40 PDT
Not really. GTK does set the value even if returns false.
Comment 6 Michael Catanzaro 2019-07-25 10:39:18 PDT
(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;
}
Comment 7 Alexander Mikhaylenko 2019-07-25 10:50:24 PDT
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 8 Michael Catanzaro 2019-07-25 16:34:49 PDT
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....
Comment 9 Alexander Mikhaylenko 2019-07-26 01:19:24 PDT
Created attachment 374953 [details]
Patch
Comment 10 Alexander Mikhaylenko 2019-07-26 01:19:56 PDT
Added the link to https://gitlab.gnome.org/GNOME/gtk/issues/2048 in the changelog
Comment 11 WebKit Commit Bot 2019-07-26 02:00:39 PDT
Comment on attachment 374953 [details]
Patch

Clearing flags on attachment: 374953

Committed r247862: <https://trac.webkit.org/changeset/247862>
Comment 12 WebKit Commit Bot 2019-07-26 02:00:40 PDT
All reviewed patches have been landed.  Closing bug.