386687 - mouse wheel events propagated from plugin windows are dropped and so do not scroll

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Karl Tomlinson (:karlt back Feb 10)]

Plugins that use XEmbed or share Mozilla's gtk event processing through NPNVGtk2 propagate unhandled events to the parent window in Mozilla, but Mozilla ignores mouse scroll events (and other mouse events) that are not received on its own windows.

A good test plugin is diamondx-0.1, available from http://multimedia.cx/diamondx/

This bug is very similar to bug 102573 but I'm using a separate bug because this may not fix Xt plugins.  However it may enable Xt plugins to use XSendEvent to pass unhandled events to the parent window.

Comment 1

[Karl Tomlinson (:karlt back Feb 10)]

The event is being missed in scroll_even_cb, line 4552 of
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/gtk2/nsWindow.cpp&rev=1.220

Using get_window_for_gtk_widget to get the nsWindow as in key_press_event_cb instead of get_window_for_gdk_window, and adding the appropriate reference point transformations from coordinate system of the foreign window that received the event to that of the widget that received the signal window should fix this.  (ScreenToWidget would be useful.)

Comment 2

[Marco Cimmino]

thanx Karl to take attention to this so old bug, remember that firstly seen on 2001 not 2007 :)

Comment 3

[Stuart Parmenter]

this is a very long standing bug.. not going to block on it for firefox3..

Comment 4

[Marco Cimmino]

we are always last one platform :(

Comment 5

[Michael Ventnor]

Attachment: Patch

Karl and I finally developed a patch which seems to work wonderfully. Whether or not this regresses plugins that do accept scroll events I don't know because such plugins are a rarity, but I have extreme doubt that it does due to code behaviour not undergoing a radical change.

Comment 6

[Reed Loden [:reed]]

Comment on attachment 291976 [details] [diff] [review]
Patch

>+        // GDK bubbles up wierdly. So instead of returning FALSE lets do our own bubbling.

weirdly*

Comment 7

[Karl Tomlinson (:karlt back Feb 10)]

Comment on attachment 291976 [details] [diff] [review]
Patch

Well done, Michael!

+    // XXX we're never sure which GdkWindow the event came from due to our custom bubbling
+    // in nsWindow.cpp, so use ScreenToWidget to translate the screen root coordinates into
+    // coordinates relative to this widget.
+    nsRect windowRect;
+    ScreenToWidget(nsRect(nscoord(aGdkEvent->x_root), nscoord(aGdkEvent->y_root), 1, 1), windowRect);
+
+    aEvent.refPoint.x = windowRect.x;
+    aEvent.refPoint.y = windowRect.y;

IIRC ScreenToWidget requires a few round trips with the X server, so it would
be nice to avoid this where possible.

If the initialization of event.refPoint is moved to nsWindow::OnScrollEvent(),
then aEvent->window can be compared with mDrawingarea->inner_window (I'm
guessing it's the inner window not the clip window - if we can't be sure then
get_window_for_gdk_window() could be used again.)

     nsRefPtr<nsWindow> window = get_window_for_gdk_window(event->window);
-    if (!window)
-        return FALSE;
+    GdkWindow *aGdkWindow = event->window;
+    while (!window) {
+        // GDK bubbles up wierdly. So instead of returning FALSE lets do our own bubbling.
+        aGdkWindow = gdk_window_get_parent(aGdkWindow);
+        if (!aGdkWindow)
+            return FALSE;
+        window = get_window_for_gdk_window(aGdkWindow);
+    }

The "a" prefix on variables should really be reserved for parameters.
aGdkWindow -> gdkWindow perhaps?

What do you think about condensing the two get_window_for_gdk_window() into one
by using

  while (!(window = get_window_for_gdk_window(aGdkWindow)))

or maybe clearer, a while(1) loop that breaks when the nsWindow is found?

It's actually GTK doing the bubbling.  And the weirdness is really us only
connecting to signals on moz_containers.  Perhaps change the comment to
something like:

 "The event has bubbled to the moz_container widget but this is an ancestor of
  the window that we need.  If we don't know about event->window, we need to
  find an ancestor window because event->window may be in a plugin."

Comment 8

[Michael Ventnor]

Attachment: Patch 2

This moves the initilisation of the mouse event to within nsWindow, so we could possibly avoid using ScreenToWidget. It also merges the window line within the while statement, and adds comments. I admit I slightly altered the suggested comments so that they seem a little bit more friendly to someone who would've never seen this code before, e.g. me at 9.00 this morning ;-)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 291989 [details] [diff] [review]
Patch 2

+    event.isShift   = (aEvent->state & GDK_SHIFT_MASK)
+        ? PR_TRUE : PR_FALSE;

Use != 0 instead of ? PR_TRUE : PR_FALSE. I know you're only moving the code, but still.

Comment 10

[Michael Ventnor]

Attachment: Patch 2.1

Patch that fixes a very, very, very long-standing annoyance in Linux, with no plugin regressions noticed so far. Since this small patch significantly improves the smoothness of the user experience it should be taken into Firefox 3 final.

Comment 11

[Marco Cimmino]

is there a way to test the patch?

thanx!

Comment 12

[Michael Ventnor]

(In reply to comment #11)
> is there a way to test the patch?
> 
> thanx!
> 

Just apply it to the files in the mozilla source and build it.

Comment 13

[Marco Cimmino]

> Just apply it to the files in the mozilla source and build it.

I know, but a snapshot already compiled?

Comment 14

[Reed Loden [:reed]]

Checking in widget/src/gtk2/nsCommonWidget.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsCommonWidget.cpp,v  <--  nsCommonWidget.cpp
new revision: 1.33; previous revision: 1.32
done
Checking in widget/src/gtk2/nsCommonWidget.h;
/cvsroot/mozilla/widget/src/gtk2/nsCommonWidget.h,v  <--  nsCommonWidget.h
new revision: 1.25; previous revision: 1.24
done
Checking in widget/src/gtk2/nsWindow.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.240; previous revision: 1.239
done

Comment 15

[Michael Ventnor]

(In reply to comment #13)
> > Just apply it to the files in the mozilla source and build it.
> 
> I know, but a snapshot already compiled?
> 

Now that its been checked in, tonight's nightly build should have the fix. I forget the link off hand but you can get builds from ftp.mozilla.org.

Comment 16

[Reed Loden [:reed]]

(In reply to comment #15)
> Now that its been checked in, tonight's nightly build should have the fix. I
> forget the link off hand but you can get builds from ftp.mozilla.org.

ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

Comment 17

[Marco Cimmino]

yeeeeeeah!
Tried: "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007121108 Minefield/3.0b2pre"

and it works perfectly! Finally!!!

THANX!

Comment 18

[Marco Cimmino]

Michael tried firefox 3.0-beta2 and it has still the bug, for one day the patch hasn't reached it?

Mozilla/5.0 (X11; U; Linux i686; it; rv:1.9b2) Gecko/2007121016 Firefox/3.0b2

Comment 19

[Aleksander Adamowski]

I can attest that with the recent Firefox nightlies (2007122004) and the latest Adobe Flash Player Beta (Shockwave Flash 9.0 r115) this bug has been fixed for most pages, but not all.

Example:

http://www.ikea.com/pl/pl/ - when mouse hovers over the large flash banner, mouse wheel doesn't scroll (but keyboard arrows DO scroll).

http://www.arkadia.com.pl/ - mouse wheel scrolling works fine.

It's hard to tell what causes the problem on the IKEA page, while the Arkadia page works fine. They both use JavaScript for creating the SWF object, although in a slightly different way.

Comment 20

[Aleksander Adamowski]

I've opened follow-up bug 409669 that covers the remaining issue. Please help finding a minimal testcase for that bug.