Jesse Ruderman Reporter
	Description • 22 years ago

Splitting from bug 66834, which was fixed on Windows but not on gtk. 
gtk-related comments from bug 66834:


------- Additional Comment #65 From Aaron Leventhal  2002-11-04 17:08 -------

Sigh, this patch doesn't help with gtk -- we might need to file a separate bug
for that. In widget/src/gtk/nsWindow.cpp, it doesn't look like we do anything with
the aConsumeRollupEvent parameter in CaptureRollupEvents().

Are the events getting swallowed by  
nsWindow::OnButtonPressSignal(GdkEventButton *aGdkButtonEvent)
?

Perhaps that method needs to pay attention to a member variable set by the
aConsumeRollupEvent parameter in CaptureRollupEvents()?


------- Additional Comment #74 From Aaron Leventhal  2002-11-05 19:20 -------

->Akkana, for widget/src/gtk fixes

widget/src/gtk/OnButtonPressSignal should not always return early if
gRollupWidget is true. 

We need to remove the extra return statement and let it fall through to the last
statement, if a variable like gCaptureClicksOutsidePopup is set. We need to
invent that variable and set it from the nsWindow::CaptureRollupEvents()
method's last parameter. Then we can avoid the early return here if that's false.


------- Additional Comment #77 From Aaron Leventhal  2002-11-06 13:41 -------

...

Believe it or not, clicks in the Mozilla window are always supposed to be eaten
when a combobox is pulled down (they just roll the combobox up).

However, there's yet another problem in gtk where clicks outside the Mozilla
window are even getting eaten, when a combobox is pulled down.


------- Additional Comment #79 From Akkana  2002-11-08 11:56 -------

Blizzard, does gtk2 do this right?  Who owns menus in the gtk/gtk2 widget
library, or has some familiarity with the menu code?


------- Additional Comment #80 From Christopher Blizzard  2002-11-08 13:38 -------

This is easy to fix.  Don't make the autocomplete widget grab the pointer or the
keyboard.  Make it pop down on a focus out event.


------- Additional Comment #87 From Christopher Blizzard  2002-12-02 21:55 -------

I, for one, would love to see autocomplete not grab at all.  This is something
that has been asked for by lots of people that I talk to.  I don't really like
it either.

That being said, we need to emulate platform behaviour as closely as possible. 
With gtk/gtk2 popups and dropdowns always eat the mouse event and are popped
down.  As near as I can tell we're pretty close to platform behaviour, at least
with that particular platform.


------- Additional Comment #88 From Robin Lu 2002-12-05 19:19 -------

Created an attachment (id=108440)
proposal patch for gtk

This patch will enable gtk aware the aConsumeEvent. I did not touch the grab
code because I think it's more like just because the check_roll_up eats the
event. 

However, only with this patch the problem could not be sovled, at least in my
gtk mozilla, because the aConsumeEvent being sent to me is always TRUE. I found
the parentTag of the autocomplete in my mozilla is 'popupset' instead of
'textbox'. Could anyone explain it to me?

Blizzard, I don't mean to break the platform routing. I just provide a choice
here. :)

	Jesse Ruderman Reporter
	Comment 1 • 22 years ago

Patch from bug 66834 comment 88.

	Adam Guthrie
	Comment 2 • 19 years ago

*** Bug 301245 has been marked as a duplicate of this bug. ***

	Joe Drew (not getting mail)
	Comment 3 • 19 years ago

Just an amendment to a comment blizzard made earlier - Firefox's behaviour is no longer even close to the platform behaviour.

	Reed Loden [:reed]
	Comment 4 • 17 years ago

Is this patch still needed/wanted?

	Christopher Aillon (sabbatical, not receiving bugmail)
	Comment 5 • 17 years ago

The patch isn't since the gtk (GTK+ 1.2) port has been removed from the tree, but the bug is still valid for the gtk2 port.

	Reed Loden [:reed]
	Comment 6 • 17 years ago

(In reply to comment #5)
> The patch isn't since the gtk (GTK+ 1.2) port has been removed from the tree,
> but the bug is still valid for the gtk2 port.

Mind updating the patch to current CVS and requesting review then?

	Christopher Aillon (sabbatical, not receiving bugmail)
	Comment 7 • 17 years ago

I had a brief look just now; the code has completely changed and will require some more investigation, so it's not really possible to update the current patch too quickly.  Someone should figure out what is happening with the gtk2 widget impl.

	Matthew Gregan [:kinetik] Assignee
	Comment 8 • 17 years ago

This seems to work, but it's useless on its own because nsWindow::CaptureRollupEvents is only ever called with aConsumeRollupEvent == PR_TRUE.

	Matthew Gregan [:kinetik] Assignee
	Comment 9 • 17 years ago

I made these changes so that nsWindow::ConsumeRollupEvents would be called with aConsumeEvents == PR_FALSE for the autocomplete dropdown (for both the location bar and the search box).  They might be wrong, but the current code in nsMenuPopupFrame::ConsumeOutsideClicks looks wrong for trunk's browser.xul... somebody who knows this better than me needs to take a look.

	Matthew Gregan [:kinetik] Assignee
	Comment 10 • 17 years ago

> aConsumeEvents == PR_FALSE

I mean aConsumeRollupEvents.

	Matthew Gregan [:kinetik] Assignee
	Comment 11 • 17 years ago

Comment on attachment 296473 [details] [diff] [review]
Robin's patch reworked for gtk2

Requesting r+sr.  Even if callers aren't doing the right thing, we might as well take this code so we do the right thing with the aConsumeRollupEvent parameter of nsWindow::CaptureRollupEvents in the GTK2 widget code.  Then it's just a matter of fixing the callers.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 12 • 17 years ago

Maybe we can just check whether gRollupListener is null instead of adding a new variable.

	Matthew Gregan [:kinetik] Assignee
	Comment 13 • 17 years ago

nsWindow::CaptureRollupEvents can be passed a rollup listener both when we want to consume rollup events and when we don't (that is, the request to consume rollup events in independent of whether we are passed a rollup listener), so we can't share that variable for this piece of state.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 14 • 17 years ago

How about just checking "gConsumeRollupEvent && check_for_rollup"...?

	Matthew Gregan [:kinetik] Assignee
	Comment 15 • 17 years ago

Yeah, introducing that nested if is a bit dumb.  New patch attached that fixes that, and also takes the precaution of setting gConsumeRollupEvent to PR_FALSE in the aDoCapture == PR_FALSE path of nsWindow::CaptureRollupEvents.

Note that the test has to be |check_for_rollup && gConsumeRollupEvent|, because we must always call check_for_rollup (it has the side effect of causing an existing rollup to roll up).  To make it clearer, I've moved the check_for_rollup call out of the if statement and now save the result of calling it so the test becomes |gConsumeRollupEvent && rolledUp|.

	Matthew Gregan [:kinetik] Assignee
	Comment 16 • 17 years ago

Comment on attachment 296883 [details] [diff] [review]
patch v2

Requesting a1.9+.  Low risk change that makes progress towards fixing this bug by adding support to the GTK2 widget code.  Until the appropriate changes are made to nsMenuPopupFrame.cpp, this patch will not affect behaviour.

	Reed Loden [:reed]
	Comment 17 • 17 years ago

Patch was screwed up... had to manually patch parts of it.

Checking in widget/src/gtk2/nsWindow.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.250; previous revision: 1.249
done

	Matthew Gregan [:kinetik] Assignee
	Comment 18 • 17 years ago

My patch caused bug 412341.

	Matthew Gregan [:kinetik] Assignee
	Comment 19 • 16 years ago

Filed follow up bug 433051 for comment 9.