Bug 200084 – REGRESSION(r246496): [GTK] Tapping the web view scrolls up a bit

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

Correction. It also looks like that patch for phosh is faulty. :) It only scrolls up once at the beginning, i.e. on any tap.

[Alice Mikhaylenko]

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

[Alice Mikhaylenko]

Created attachment 374890 [details]
Patch

[Michael Catanzaro]

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.

[Alice Mikhaylenko]

Not really. GTK does set the value even if returns false.

[Michael Catanzaro]

(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;
}

[Alice Mikhaylenko]

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.

[Michael Catanzaro]

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....

[Alice Mikhaylenko]

Created attachment 374953 [details]
Patch

[Alice Mikhaylenko]

Added the link to https://gitlab.gnome.org/GNOME/gtk/issues/2048 in the changelog

[WebKit Commit Bot]

Comment on attachment 374953 [details]
Patch

Clearing flags on attachment: 374953

Committed r247862: <https://trac.webkit.org/changeset/247862>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.