121011 - widget misses mouse-up event after scrolling when mouse button is released outside the widget area

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GTK Widget, defect)

Description

[Ingo Koehne]

can be reproduced with galeon:

press the mouse button on the scroll bar.
start scolling to the top and do not release the button
until you are over the menu bar.
moving vertically over the widget window now scrolls the page.

(occasionally i also recognized similar behaviour without
leaving the widget window, but i have not been been able to 
reproduce it)

i found that this bug was already at
http://bugzilla.gnome.org/show_bug.cgi?id=55194
and the galeon folks seemed have the opinion that this is 
a problem with the widget.

however, i didnt find anything matching bug here hence this duplicate.

ingo

Comment 1

[Ingo Koehne]

oops i forgot: 
galeon 1.0.2 with libgtkembedmoz from mozilla 0.9.7

Comment 2

[Christopher Blizzard (:blizzard)]

Attachment: patch

This patch adds any motion widget to the grab list.  The motion widget is kept
track of whenever someone starts a mouse motion even.  X does a passive grab in
this case, so this really is a grab.  We just need to track this so that the
passive grab is noticed in handle_gdk_event.

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 67051 [details] [diff] [review]
patch

r=bryner

Comment 4

[Christopher Blizzard (:blizzard)]

Attachment: patch fixing brendan's comments

Comment 5

[Christopher Blizzard (:blizzard)]

Attachment: picky, picky!

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 67057 [details] [diff] [review]
picky, picky!

sr=[e-mail], but the brace style is not coherent (picky^3 :-).  You
use { on same line as if for the new if-then in GetGrabWindow, but the existing
uses { on its own line.  Fix that for me (flutters eyelashes)?

/be

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 67057 [details] [diff] [review]
picky, picky!

Sliding bryner's r= forward and approving for 0.9.8 on behalf of The Regents of
the University of California.

Comment 8

[Christopher Blizzard (:blizzard)]

Checked into the 0.9.8 branch.

Comment 9

[Christopher Blizzard (:blizzard)]

Checked into the trunk.

Comment 10

[Christopher Blizzard (:blizzard)]

Unfortunately, this caused a regression and I need to figure out a better way to
do this.  Gtk dialogs thrown as a result of a mouse down cause a situation where
mozilla still thinks it has the mouse and suddenly, native windows won't refresh
anymore.

I've got a plan for a better way to fix this.  Patch in a few mins.

Comment 11

[Christopher Blizzard (:blizzard)]

Attachment: non-invasive patch

This is a pretty non-invasive patch that works in all the test cases I could
find, and the test case that I missed.

I'm going to put together another patch that fixes this problem the right way
as well.

Comment 12

[Christopher Blizzard (:blizzard)]

Attachment: more invasive, but more correct patch

This is a much more invasive patch, but it actually has the correct behaviour. 
It adds a gtk_grab_add for the mozilla area any time there's a passive grab in
the mozilla widget.  This means that the behaviour for windows outside of the
mozilla drawing areas is correct, button press events are properly sent to the
right windows, and widgets outside the area don't get mouse over events, etc.

This patch is pretty well tested and I haven't been able to find any problems
with it.

Comment 13

[Christopher Blizzard (:blizzard)]

Attachment: couple of nits fixed

Comment 14

[Christopher Blizzard (:blizzard)]

Here's the description for how this patch works:

1. Changed nsWindow::GetMozArea() to nsWidget::GetOwningWidget() that is a
virtual method and can be overridden by subclasses.  This allows you to get the
owning GtkWidget for any of the widgets so that you can get a widget that you
might need to grab on.  This is only implemented for nsWindow at the moment
since it's the only place that it's needed.

There's a lot of noise generated by this part of the patch, mostly renaming calls.

2.  In handle_gdk_event, add an else clause after the code that redirects events
for real pointer grabs.  This code is triggered whenever there's a an event over
a GtkWidget * that isn't part of the Mozilla heirarchy.  It gives the mozilla
code a crack at the event before Gtk gets it.

Mozilla needs to see some events if there's a passive grab in effect.  The code
in nsWidget::ProcessButtonEvent() will return PR_TRUE if the event was consumed.
 If the event was consumed, gtk doesn't do default processing on the event.

3.  Whenever the Mozilla code gets a ButtonPress event, it sets a gtk grab on
the owning widget of the window where the event takes place.  This means that
within the normal Gtk mainloop, the Mozilla widget is respected as holding the
grab and any mouse or pointer events that take place outside of the Mozilla
widget will be ignored.

4.  In nsWidget::DropMotionTarget make sure to release the gtk_grab (see the
comment above.)

5.  Comment out attaching to the ButtonPress and ButtonRelease event for all
widgets that have an mWidget element.  The only widget that this mattered on was
nsWindow, but we switched away from using mWidget a long, long time ago.  This
was just screwing up grabbing on the scrollbar, generating extra events that we
didn't need to process.

Comment 15

[Christopher Blizzard (:blizzard)]

6.  Make sure to drop the motion target before delivering the button release
event.  If you don't do that and the button release event triggers another
window, the events for that new window will be incorrectly delivered to the old
window.  This is because the motion target is still set, even though the drag
has been released.  Dropping the target before delivering the event fixes the
problem.

Comment 16

[Christopher Blizzard (:blizzard)]

Attachment: shaver's nits fixed

Add a comment about why I'm commenting out the listening for button press
signals by default in |nsWidget|.

Comment 17

[Mike Shaver (:shaver emeritus)]

Comment on attachment 67222 [details] [diff] [review]
shaver's nits fixed

sold.
sr=shaver

Comment 18

[Christopher Blizzard (:blizzard)]

I also had an r=bryner from IRC.

Comment 19

[Christopher Blizzard (:blizzard)]

Comment on attachment 67222 [details] [diff] [review]
shaver's nits fixed

r=bryner from IRC

Comment 20

[Christopher Blizzard (:blizzard)]

Checked into 0.9.8

Comment 21

[Christopher Blizzard (:blizzard)]

Backed out of 0.9.8.

Comment 22

[Robert Ginda]

nsWindow->Enable(PR_FALSE); doesn't do anything on gtk.  I've got a patch but...

Chris Blizzard wrote:
> Do me a favor, and link to [bug 121011]
> I'll fix it when I do the code to fix that the right way.  (Put a reminder in
> there, too.)  They might not seem related, but they are.

For the record, my expectations are that nsWindow->Enable(PR_FALSE); on a 
top-level window would prevent the window from responding to user events, and
window-manager widgets would not dismiss the window.

Comment 23

[Christopher Blizzard (:blizzard)]

Attachment: newer, shinier patch

Rewrite the gtk mainloop code, just in time for 0.9.9!

Comment 24

[Christopher Blizzard (:blizzard)]

to disable a window:

evald w = getXULWindowFromWindow(window); w.enabled = false;

Comment 25

[Christopher Blizzard (:blizzard)]

Attachment: fix problems with modal gtk windows

Comment 26

[Christopher Blizzard (:blizzard)]

Attachment: patch that fixes double clicks on scrollbars missing mouse up

Comment 27

[Christopher Blizzard (:blizzard)]

Attachment: fix problems with menus not popping down

Comment 28

[Brian Ryner (not reading)]

Comment on attachment 71106 [details] [diff] [review]
fix problems with menus not popping down

r=bryner

Comment 29

[Mike Shaver (:shaver emeritus)]

4 things:

 - naming: the patch mixes temp_widget with grabIsRegularWidget.  I'll only buy
   your claim that under_scores are for GTK things, and interCaps are for 
   Mozilla things if you apply it consistently!  (c.f. GtkWidget *tempWidget)

 - save a branch and a store:
   PRBool grabIsRegularWidget = (grabW_widget && !GTK_IS_MOZAREA(grab_widget));

 - //printf(...) lines should go

 - use nsWindow::sFocusWindow rather than window->sFocusWindow, to make it 
   clearer that it's a class-static member.

Clean up these nits, and I'll gladly stamp!

Comment 30

[Christopher Blizzard (:blizzard)]

Attachment: updated patch

Patch that fixes shaver's nits.

Comment 31

[Mike Shaver (:shaver emeritus)]

Comment on attachment 72264 [details] [diff] [review]
updated patch

sr=shaver, moving r=ahead.

Comment 32

[Christopher Blizzard (:blizzard)]

Attachment: final patch

This is the final patch.  It only has a couple renamed vars, reviews should
still apply.

Comment 33

[Asa Dotzler [:asa]]

Comment on attachment 72325 [details] [diff] [review]
final patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 34

[Jerome Lacoste]

Seems like this was checked in into the trunk. Are the statuses correct for this
bug?

Comment 35

[Christopher Blizzard (:blizzard)]

Fixed.