996678 - [gtk3] Unexpected GDK_SCROLL_SMOOTH events when scroll button events are propagated from plugins

Status: RESOLVED DUPLICATE of bug 958868

(Core :: Widget: Gtk, defect)

Description

[Emilio Pozuelo Monfort]

When libxul is built against gtk3, we can receive GDK_SCROLL_SMOOTH events in addition to GDK_SCROLL_{UP,DOWN,LEFT,RIGHT}, but widget/gtk/nsWindow.cpp:nsWindow::OnScrollEvent() doesn't handle them.

This isn't a problem when scrolling on the firefox window, but when scrolling inside a plugin window (e.g. a flash video) plugin-container re-sends the event to the firefox X window (see dom/plugins/ipc/PluginModuleChild.cpp:gtk_plug_scroll_event()), and for some reason we only get a _SMOOTH event but no _UP/_DOWN events.

There's something odd here because we're not adding GDK_SMOOTH_SCROLL_MASK to the event mask, yet we're receiving _SMOOTH events.

Comment 1

[Daniel Colascione]

Attachment: hacky-smooth-scroll-support.diff

This patch works for me. It's a hacky workaround for GTK delivering both synthetic and regular scroll events without providing a clean way to distinguish them.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8408918 [details] [diff] [review]
hacky-smooth-scroll-support.diff

Karl, can you check this one please?

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8408918 [details] [diff] [review]
hacky-smooth-scroll-support.diff

IIUC this is more a patch for bug 958868 than this bug.  I'll retitle this bug
as the current title is a bit misleading.

>-    NS_ASSERTION(wheelEvent.deltaX || wheelEvent.deltaY,
>-                 "deltaX or deltaY must be non-zero");

Would it make sense to keep this assertion?

>+        if (((deltaX == 1.0 || deltaX == -1.0) && deltaY == 0.0) ||
>+            ((deltaY == 1.0 || deltaY == -1.0) && deltaX == 0.0))
>+        {
>+            // HACK: if this event looks like it's being synthesized
>+            // from a non-pixel-scroll event, then process the next
>+            // regular scroll event normally.

Is this necessary?  What are the consequences of proceeding with the smooth
scroll.

>+        wheelEvent.scrollType = WidgetWheelEvent::SCROLL_SYNCHRONOUSLY;

Is there a reason to force a synchronous scroll?

>+        wheelEvent.deltaX = deltaX;
>+        wheelEvent.deltaY = deltaY;

What are the units of the scroll deltas?
Is nsIDOMWheelEvent::DOM_DELTA_LINE appropriate for smooth scrolling?
Should isPixelOnlyDevice be set?

>+    /* Work around GNOME bug#726878 by discarding regular scroll
>+     * events synthesized from pixel scroll events. */

It would be helpful to know what is causing the problem here, so we can know
what is the correct fix.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #3)
> >+        wheelEvent.deltaX = deltaX;
> >+        wheelEvent.deltaY = deltaY;
> 
> What are the units of the scroll deltas?
> Is nsIDOMWheelEvent::DOM_DELTA_LINE appropriate for smooth scrolling?
> Should isPixelOnlyDevice be set?

AFAIK The delta is a fraction of regular scroll size. So 1(-1) means a normal scroll size, 2(-2) means double scroll size and if the system supports the smooth events you can get a value smaller than 1.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

More details about smooth vs. old scroll events - http://thread.gmane.org/gmane.comp.gnome.desktop/46513

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Attachment: smooth scroll patch

Karl, what about this one?

According to gnome folks (http://thread.gmane.org/gmane.comp.gnome.desktop/46513) all Gtk3 apps should use smooth scroll only. The old one is there just for a compatibility reasons.

IMHO nsIDOMWheelEvent::DOM_DELTA_LINE is ok here as far as wheelEvent.deltaX/wheelEvent.deltaY can be smaller than 1.

This patch discards od UP/DOWN scroll so we don't have to care about them. I tested the patch on Fedora 20 (gtk3-3.10.8) and works as expected.

If we process both events (UP/DOWN and SMOOTH) the scrolling is two times faster, the UP/DOWN carry the same values (at least on my box).

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Martin Stránský from comment #6)
> According to gnome folks
> (http://thread.gmane.org/gmane.comp.gnome.desktop/46513) all Gtk3 apps
> should use smooth scroll only. The old one is there just for a compatibility
> reasons.

I couldn't find anything saying that apps should stop using UP/DOWN.
Are you able to point me at the particular post you saw this, please,
and quote the relevant sentence?

Comment 8

[Martin Stránský [:stransky] (ni? me)]

http://thread.gmane.org/gmane.comp.gnome.desktop/46513

[...]

"* if you are on xserver 1.12 with xinput 2.2 your application stop 
receiving GDK_SCROLL_DOWN and GDK_SCROLL_UP events, but receive 
GDK_SCROLL_SMOOTH with an increment instead

that change seems to create quite some issue, it breaks for example 
mouse wheel scrolling over sound sliders in the control center, or 
scrolling in nautilus compact view

Grepping around in my work tree I see there are quite a lot of GNOME 
components using GDK_SCROLL_UP,DOWN, I guess those will stop working as 
they should."

[...]

But I can ask gnome team for further info.

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to Martin Stránský from comment #8)

http://article.gmane.org/gmane.comp.gnome.desktop/46513 was a post from
someone affected by the changes, and I think the part quoted on comment 8 was
an observation rather than a description of how things were meant to work.

That may be a bug report and
http://article.gmane.org/gmane.comp.gnome.desktop/46529 seems to confirm that
as a bug, where Matthias Clasen wrote:

  On Fri, Mar 16, 2012 at 8:50 AM, Patryk Zawadzki <patrys <at> pld-linux.org> wrote:
  > W dniu 16 marca 2012 13:33 użytkownik Sebastien Bacher
  > <seb128 <at> ubuntu.com> napisał:
  >> Le 16/03/2012 12:56, Bastien Nocera a écrit :
  >>> So gtk_widget_add_events (widget, GDK_SCROLL_MASK); for every widget for
  >>> which we want the old behaviour back, correct? Sounds doable by 3.4.
  >> Well "old behaviour back", you will still not get GDK_SCROLL_UP,DOWN events
  >> so any code checking for those will need updating.
  >
  > Shouldn't that be the case unless you explicitly ask for GDK_SMOOTH_SCROLL_MASK?

  Yes, that should be the case

It's not clear (from that thread) whether the bug is in GTK or Nautilus.

It's been hard to work out what is an appropriate solution without
documentation on how these events interact, and the bugs mean we cannot determine the intended behavior by observation, but I'm pretty sure that GTK will
not give us smooth scroll events if the mouse driver does not send them, so we
still need to handle the discrete (button) scroll events.

See also discussion in bug 958868.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8423977 [details] [diff] [review]
smooth scroll patch

Still need to handle button scroll events and lineOrPageDeltaX/Y is not right here.  See bug 958868.

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Ahh, okay, we can close it as a dupe of Bug 958868 then.

Comment 12

[Karl Tomlinson (:karlt)]

We can dupe this to Bug 958868 if the fix there works around this bug, but wait until something lands.  I suspect this bug is just the GTK bug
https://bugzilla.gnome.org/show_bug.cgi?id=726878
which would mean this report is technically invalid.

Comment 13

[AnAkkk]

I've been using the GTK3 version of firefox for a long time now, and I've always had a very annoying bug: I just can't scroll at all in the window, I have to alt-tab for scrolling to work again every time this happens. 
Would the issue be the same as reported in this bug report?

Comment 14

[Karl Tomlinson (:karlt)]

See bug 1182700 re comment 13.