227769 - Mouse is "sticking" at scrolling

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Sergei Dolgov]

Steps to reproduce.
1)Grab scrolling knob, scroll up and down, while scrolling, move mouse a bit
inside window, so it leaves scrollbar (don't release buttons).
2)Release buttons and move mouse up/down.
Page is still scrolling

http://bebits.com/talkback/2715 - comment by  By procosm - Posted on October 6,
2003 - 17:43:57   (#9090) "Reliable Way to Reproduce Mouse Scolbar Release Bug"

Reason - in widget/src/beos/nsWindow.cpp
in metod nsViewBeOS::MouseMoved()
we are sending in mouse event number of buttons same as it was in MouseDown.

Fix - number of buttons should be reset - most elegant way seems to call again 
BWindow::CurrentMessage(), as it is done in MouseDown,

Comment 1

[Sergei Dolgov]

Attachment: patch

Fixing bug, 
adding NS_IMETHOD CaptureMouse(PRBool aCapture);
separating mouse clicks and mouse moves.

Comment 2

[Sergei Dolgov]

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

review request

Comment 3

[Sergei Dolgov]

reassigning to myself

Comment 4

[timeless]

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

Raistlin is in transit from the east coast to the west coast, it should arrive
in two weeks.

>RCS file: /cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v
>@@ -1745,8 +1756,6 @@
> 					rollup = true;
> 				mView->UnlockLooper();
> 			}
>-			if (rollup)
>-				break;

please explain this change.

>@@ -2434,24 +2444,24 @@
> 			switch(aEventType)
> 			{
> 			case NS_MOUSE_MOVE :
>-				mMouseListener->MouseMoved(event);
>+			//	result = ConvertStatus(mMouseListener->MouseMoved(event));
> 				break;

Can you explain why this is safe?

> 			case NS_MOUSE_LEFT_BUTTON_UP :
> 			case NS_MOUSE_MIDDLE_BUTTON_UP :
> 			case NS_MOUSE_RIGHT_BUTTON_UP :
>-				mMouseListener->MouseReleased(event);
>-				mMouseListener->MouseClicked(event);
>+				result = ConvertStatus(mMouseListener->MouseReleased(event));

you're going to lose this result ^^^^^ here:

>+				result = ConvertStatus(mMouseListener->MouseClicked(event));

is there something you might want to do with it?

>@@ -2817,7 +2826,7 @@
> 			args[2] = (uint32)point.y;
> 			args[3] = clicks;
> 			args[4] = modifiers();
>-			MethodInfo *info = new MethodInfo(w, w, nsWindow::ONMOUSE, 5, args);
>+			MethodInfo *info = new MethodInfo(w, w, nsWindow::BTNCLICK, 5, args);
> 			t->CallMethodAsync(info);
> 		}
> 		NS_RELEASE(t);

I know it isn't your code, but please add:
if (!info)
{do something reasonable}

this comment applies to the entire file (or at least the two patched instances)

>@@ -2882,7 +2886,7 @@
...
>-			MethodInfo *info = new MethodInfo(w, w, nsWindow::ONMOUSE, 5, args);
>+			MethodInfo *info = new MethodInfo(w, w, nsWindow::BTNCLICK, 5, args);
> 			t->CallMethodAsync(info);
...

In the future, please add -p to your diff flags

Comment 5

[Sergei Dolgov]

>@@ -1745,8 +1756,6 @@
> 					rollup = true;
> 				mView->UnlockLooper();
> 			}
>-			if (rollup)
>-				break;
Those changes above allow more native context menu behaviour - like in Net+ or
GobeProductive or wherelse - with open context menu right click outside menu
closes old one and opens new menu.
------------------------------
Now code in DispatchMouseEvent. Reasonable questions but i rather need your
advice. I copied that code from gtk-port, it is working, but big question is if
we reach at all this changed part where
nsnull == mEventCallback and nsnull != mMouseListener.
Because all calls for DispatchMouseEvent in beos widget code are made via
CallMethod, and i cannot figure situation when CallMethod is reached, but
mEventCallback is null. But maybe i'm wrong, so any advise is welcomed.

Now details. I noticed that stupidy
result=
result=
return result;
which i copied from gtk after i submitted patch.
So maybe 
result = ConvertStatus(mMouseListener->MouseReleased(event)) &&
ConvertStatus(mMouseListener->MouseClicked(event));
break;

is more logical.
----------------------------
>I know it isn't your code, but please add:
>if (!info)
>{do something reasonable}

Hmm, do you think it may happen? At which condition? When heap is totally exhausted?
Do you think we should just release widget and do nothing in this case?
--------------------------

Comment 6

[timeless]

i'm not sure what you should do when i run out of memory. i just know i run out
of memory w/ bezilla a lot more than anywhere else (something about 512mb of ram
and no vm hosting a debug bezilla). if there are things which won't be usable
once you fail but will be nullchecked by anything that might plan to use them
then it would be good to free them.

  > result = ConvertStatus(mMouseListener->MouseReleased(event)) &&
  >          ConvertStatus(mMouseListener->MouseClicked(event));
  > break;
> is more logical.

yeah, i like that.

> Those changes above allow more native context menu behaviour - like in Net+ or
> GobeProductive or wherelse - with open context menu right click outside menu
> closes old one and opens new menu.

ok (that's also standard behavior on windows fwiw...), i just didn't have enough
context or energy to trace the code to understand the change.

Comment 7

[Sergei Dolgov]

Preparing new patch.
Though, i still don't know what to do in case if it cannot allocate those 20
bytes for "info". As BeOS hook methods where we're using "info" are mostly of
void type. But if the weren't void, all the same, CallMethodAsync(info) seems
only way to report to mozilla something, even failure.

About memleaks. This is mainly from unreleased BBitmaps of nsSurface in GFX
code, but also related to improper destroy process for Mozilla children in
widget code.

I have some changes for GFX, but still there are problems with proper
destruction  in widget code.

Comment 8

[Sergei Dolgov]

Attachment: Patch.Second take

Added "new" test-paranoia by timeless request for all "new MethodInfo"
occurencies in nsWindow.cpp.
Restored statement in DispatchMouseEvent() - until i know for sure what happens
there.
Corrected return value in DispatachMouseEvents()
Removed test for ONMOUSE in ConsumeRedundantMouseEvents() in nsAppShell (as
clicks and moves are separated now).

Comment 9

[Sergei Dolgov]

Comment on attachment 138311 [details] [diff] [review]
Patch.Second take

review request

Comment 10

[tqh]

This seems to work just fine in my mozilla-tree.

Comment 11

[Sergei Dolgov]

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

removing review request from older version

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in