Closed Bug 121011 Opened 23 years ago Closed 23 years ago

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


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

Not set


(Not tracked)



(Reporter: ingok, Assigned: blizzard)



(Whiteboard: has patch)


(2 files, 11 obsolete files)

25.37 KB, patch
: review+
: superreview+
Details | Diff | Splinter Review
25.37 KB, patch
: review+
Details | Diff | Splinter Review
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
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.

oops i forgot: 
galeon 1.0.2 with libgtkembedmoz from mozilla 0.9.7
Ever confirmed: true
Attached patch patch (obsolete) — Splinter Review
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.
Blocks: 115520
Whiteboard: has patch
Target Milestone: --- → mozilla0.9.8
Comment on attachment 67051 [details] [diff] [review]

Attachment #67051 - Flags: review+
Attached patch patch fixing brendan's comments (obsolete) — Splinter Review
Attachment #67051 - Attachment is obsolete: true
Attached patch picky, picky! (obsolete) — Splinter Review
Attachment #67054 - Attachment is obsolete: true
Comment on attachment 67057 [details] [diff] [review]
picky, picky!, 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)?

Attachment #67057 - Flags: superreview+
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.
Attachment #67057 - Flags: review+
Attachment #67057 - Flags: approval+
Checked into the 0.9.8 branch.
Checked into the trunk.
Closed: 23 years ago
Resolution: --- → FIXED
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

I've got a plan for a better way to fix this.  Patch in a few mins.
Resolution: FIXED → ---
Attached patch non-invasive patch (obsolete) — Splinter Review
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.
Attachment #67057 - Attachment is obsolete: true
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.
Attached patch couple of nits fixed (obsolete) — Splinter Review
Attachment #67139 - Attachment is obsolete: true
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.
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
Attached patch shaver's nits fixed (obsolete) — Splinter Review
Add a comment about why I'm commenting out the listening for button press
signals by default in |nsWidget|.
Attachment #67205 - Attachment is obsolete: true
Comment on attachment 67222 [details] [diff] [review]
shaver's nits fixed

Attachment #67222 - Flags: superreview+
I also had an r=bryner from IRC.
Comment on attachment 67222 [details] [diff] [review]
shaver's nits fixed

r=bryner from IRC
Attachment #67222 - Flags: review+
Checked into 0.9.8
Blocks: 122796
Backed out of 0.9.8.
No longer blocks: 115520
Blocks: 123426
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.
Attached patch newer, shinier patch (obsolete) — Splinter Review
Rewrite the gtk mainloop code, just in time for 0.9.9!
Attachment #67126 - Attachment is obsolete: true
Attachment #67222 - Attachment is obsolete: true
to disable a window:

evald w = getXULWindowFromWindow(window); w.enabled = false;
Attachment #70446 - Attachment is obsolete: true
Attachment #71042 - Attachment is obsolete: true
Comment on attachment 71106 [details] [diff] [review]
fix problems with menus not popping down

Attachment #71106 - Flags: review+
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!
Attached patch updated patchSplinter Review
Patch that fixes shaver's nits.
Attachment #71106 - Attachment is obsolete: true
Comment on attachment 72264 [details] [diff] [review]
updated patch

sr=shaver, moving r=ahead.
Attachment #72264 - Flags: superreview+
Attachment #72264 - Flags: review+
Attached patch final patchSplinter Review
This is the final patch.  It only has a couple renamed vars, reviews should
still apply.
Comment on attachment 72325 [details] [diff] [review]
final patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #72325 - Flags: superreview+
Attachment #72325 - Flags: review+
Attachment #72325 - Flags: approval+
Seems like this was checked in into the trunk. Are the statuses correct for this
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.