Martin Stránský [:stransky] (ni? me) Reporter
	Description • 11 years ago

I see those messages with the gtk3 build:

[Parent 20296] ###!!! ASSERTION: deltaX or deltaY must be non-zero: 'wheelEvent.deltaX || wheelEvent.deltaY', file /home/komat/tmp668-trunk/src/widget/gtk2/nsWindow.cpp, line 3088

	Morgane ”Sardem FF7” Glidic
	Comment 1 • 11 years ago

It seems that the only way to have the assert wrong is with GTK+ >=3.4 which have a smooth scrolling enum value. This patch just adds it in the switch.

	Josh Matthews [:jdm]
	Comment 2 • 11 years ago

Comment on attachment 8351594 [details] [diff] [review]
Patch to add GDK smooth scrolling support

According to hg log, Karl looks like a good choice for reviewer here.

	Karl Tomlinson (:karlt)
	Comment 3 • 11 years ago

(In reply to Martin Stránský from comment #0)
> I see those messages with the gtk3 build:
> 
> [Parent 20296] ###!!! ASSERTION: deltaX or deltaY must be non-zero:
> 'wheelEvent.deltaX || wheelEvent.deltaY', file
> /home/komat/tmp668-trunk/src/widget/gtk2/nsWindow.cpp, line 3088

What are the steps to reproduce?

Is there any unexpected behaviour?

Which version of GTK3?

	Karl Tomlinson (:karlt)
	Comment 4 • 11 years ago

Comment on attachment 8351594 [details] [diff] [review]
Patch to add GDK smooth scrolling support

>Bug 908102 - GTK3 - ASSERTION: deltaX or deltaY must be non-zero
>
>Support GDK smooth scrolling

Please adjust the comment to say how the patch changes what the code does, instead of the bug title.

This patch doesn't add smooth scrolling support, because the widget does not select GDK_SMOOTH_SCROLL_MASK events.
So why are GDK_SCROLL_SMOOTH events being processed here?
Is it because these events have propagated from plugin widgets?

>+#if GTK_CHECK_VERSION(3,4,0)
>+    gdouble deltaX, deltaY;
>+#endif

>+#if GTK_CHECK_VERSION(3,4,0)
>+    case GDK_SCROLL_SMOOTH:
>+        gdk_event_get_scroll_deltas(((GdkEvent *)aEvent), &deltaX, &deltaY);
>+        wheelEvent.deltaX = wheelEvent.lineOrPageDeltaX = deltaX;
>+        wheelEvent.deltaY = wheelEvent.lineOrPageDeltaY = deltaY;
>+        break;
>+#endif

Please add a block for this case, and declare the variables inside the block, instead of above.
See, for example
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp?rev=127070

It doesn't look correct to set lineOrPageDeltaX here because those are integer values and deltaX will likely be less than 1.
Should isPixelOnlyDevice be set here, instead?

Please include function names and context in future patches.
Mercurial can be configured to do this automatically as described at
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

	Karl Tomlinson (:karlt)
	Comment 5 • 11 years ago

>+        gdk_event_get_scroll_deltas(((GdkEvent *)aEvent), &deltaX, &deltaY);

Please remove the extra parentheses around (GdkEvent *)aEvent.

	Makoto Kato [:m_kato]
	Comment 7 • 10 years ago

If widget is top-level, both GDK_SCROLL_UP/DOWN and GDK_SCROLL_SMOOTH are received.  If non-top-level widget such as text view, GDK_SCROLL_SMOOTH is received only when Xi2 supports. GDK doesn't seem to filter out emulated old wheel event...

	Makoto Kato [:m_kato]
	Comment 8 • 10 years ago

filed as https://bugzilla.gnome.org/show_bug.cgi?id=726878 for signaling both messages on top-level widget only.

	Martin Stránský [:stransky] (ni? me) Reporter
	Comment 9 • 10 years ago

The patch actually does not fix the error messages.

	Martin Stránský [:stransky] (ni? me) Reporter
	Comment 10 • 10 years ago

I think it's better to ignore the bogus smooth scroll events unless we implement it.

	Karl Tomlinson (:karlt)
	Comment 11 • 10 years ago

Comment on attachment 8407441 [details] [diff] [review]
ignore smooth scroll event

I assume we need some version check here?

>+        // Gdk sends us GDK_SCROLL_SMOOTH even when it's not supported,

Please use upper case for "GDK".

I don't know what you mean by "supported".
Would "requested" be more appropriate?
We shouldn't get these unless GDK_SMOOTH_SCROLL_MASK is in the event mask, I assume?

	Martin Stránský [:stransky] (ni? me) Reporter
	Comment 12 • 10 years ago

(In reply to Karl Tomlinson (:karlt) from comment #11)
> Comment on attachment 8407441 [details] [diff] [review]
> ignore smooth scroll event
> 
> I assume we need some version check here?

I don't think so...who runs something older that 3.4?

> >+        // Gdk sends us GDK_SCROLL_SMOOTH even when it's not supported,
> 
> Please use upper case for "GDK".
> 
> I don't know what you mean by "supported".
> Would "requested" be more appropriate?
> We shouldn't get these unless GDK_SMOOTH_SCROLL_MASK is in the event mask, I
> assume?

With or without the GDK_SMOOTH_SCROLL_MASK, gdk_event_get_scroll_deltas() keeps return 0 or 1/-1, at least on my box.

	Karl Tomlinson (:karlt)
	Comment 13 • 10 years ago

(In reply to Martin Stránský from comment #12)
> I don't think so...who runs something older that 3.4?

I assumed this code was compiled in GTK2 builds.

	Emilio Pozuelo Monfort
	Comment 14 • 10 years ago

That patch would ignore _SMOOTH events, but there's something else going on here as I mentioned in bug #996678:

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

i.e. if we only get _SMOOTH events when they are forwarded from plugin-container, but we ignore them, then scrolling in plugin windows won't do anything, which is a regression.

	Martin Stránský [:stransky] (ni? me) Reporter
	Comment 15 • 10 years ago

(In reply to Emilio Pozuelo Monfort from comment #14)
> i.e. if we only get _SMOOTH events when they are forwarded from
> plugin-container, but we ignore them, then scrolling in plugin windows won't
> do anything, which is a regression.

Yes, it looks like only the plugin-container produces useful _SMOOTH events but not _UP/_DOWN events.

	Karl Tomlinson (:karlt)
	Comment 16 • 10 years ago

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #3)
> What are the steps to reproduce?
> 
> Is there any unexpected behaviour?

Is this only when the mouse is over a plugin?
i.e. is this the same bug as bug 996678?

	Martin Stránský [:stransky] (ni? me) Reporter
	Comment 17 • 10 years ago

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #16)
> (In reply to Karl Tomlinson (needinfo?:karlt) from comment #3)
> > What are the steps to reproduce?
> > 
> > Is there any unexpected behaviour?
> 
> Is this only when the mouse is over a plugin?

The _SMOOTH events are generated for all widgets in Gtk3 Top windows (and all widgets with explicit GDK_SCROLL_MASK flags) also get UP/DOWN evens. So it depends if the plugin sets GDK_SCROLL_MASK or not.

> i.e. is this the same bug as bug 996678?

This bug is about _SMOOTH scroll events with 0 deltas. I have no idea why Gtk3 generates those events...sems to be a Gtk3 bug.

	Makoto Kato [:m_kato]
	Comment 20 • 10 years ago

This was fixed by bug 958868 by supporting GDK_SCROLL_SMOOTH.