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

RESOLVED FIXED in mozilla1.9beta3

Status

()

Core
Widget: Gtk
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: karlt, Assigned: Michael Ventnor)

Tracking

Trunk
mozilla1.9beta3
x86
Linux
Points:
---
Bug Flags:
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
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.)

Updated

11 years ago
Flags: blocking1.9?

Comment 2

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

Comment 3

11 years ago
this is a very long standing bug.. not going to block on it for firefox3..
Flags: blocking1.9? → blocking1.9-

Comment 4

11 years ago
we are always last one platform :(
(Assignee)

Comment 5

11 years ago
Created attachment 291976 [details] [diff] [review]
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.
Assignee: nobody → ventnor.bugzilla
Status: NEW → ASSIGNED
Attachment #291976 - Flags: superreview?(roc)
Attachment #291976 - Flags: review?(roc)
Comment on attachment 291976 [details] [diff] [review]
Patch

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

weirdly*
(Reporter)

Comment 7

11 years ago
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."
(Assignee)

Comment 8

11 years ago
Created attachment 291989 [details] [diff] [review]
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 ;-)
Attachment #291976 - Attachment is obsolete: true
Attachment #291989 - Flags: superreview?(roc)
Attachment #291989 - Flags: review?(roc)
Attachment #291976 - Flags: superreview?(roc)
Attachment #291976 - Flags: review?(roc)
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.
Attachment #291989 - Flags: superreview?(roc)
Attachment #291989 - Flags: superreview+
Attachment #291989 - Flags: review?(roc)
Attachment #291989 - Flags: review+
(Assignee)

Comment 10

11 years ago
Created attachment 292002 [details] [diff] [review]
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.
Attachment #291989 - Attachment is obsolete: true
Attachment #292002 - Flags: approval1.9?

Comment 11

11 years ago
is there a way to test the patch?

thanx!
(Assignee)

Comment 12

11 years ago
(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.

Updated

11 years ago
Attachment #292002 - Flags: approval1.9? → approval1.9+

Comment 13

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

I know, but a snapshot already compiled?
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
(Assignee)

Comment 15

11 years ago
(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.
(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

11 years ago
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

11 years ago
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

11 years ago
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

11 years ago
I've opened follow-up bug 409669 that covers the remaining issue. Please help finding a minimal testcase for that bug.
You need to log in before you can comment on or make changes to this bug.