354694 - Consolidate NS_MOUSE_***_UP/DOWN/CLICK/DBLCLICK events

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

to make nsEvent structures to work more like dom events,
nsIDOMMouseEvent::type should always map to only one nsMouseEvent::message.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

I have a patch for this, but only for gtk2. Need to still convert other
toolkits.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for all toolkits

Need to still re-read the patch myself.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

roc, jst, perhaps you have some comments to the changes in nsGUIEvent.h?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks good. it might be nice to just make the button number be an integer because mice can have many more buttons.

Comment 5

[Johnny Stenback (:jst)]

Looks fine to me too. We *could* save ourselves 32-bits per mouse event if we'd be ok with a 16 bit clickCount member, but either way is fine with me as these event objects don't tend to live very long...

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for all toolkits 2

This is tested to work on linux/win/mac

I'm not quite sure how this should be reviewed.
Jst, perhaps you could check at least the content/ and layout/ and
say what you think.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 244096 [details] [diff] [review]
for all toolkits 2

I looked through the content and layout parts and that looks all good to me. Maybe get the peers for the various widget libraries to look over the OS specific parts?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 244096 [details] [diff] [review]
for all toolkits 2

josh, could you look at the mac code.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 244096 [details] [diff] [review]
for all toolkits 2

Roc, could you look at the gtk/gtk2 part?

Comment 10

[Sergei Dolgov]

Comment on attachment 244096 [details] [diff] [review]
for all toolkits 2

r=sergei_d
if number of arguments
here http://lxr.mozilla.org/seamonkey/source/widget/src/beos/nsWindow.cpp#3059 and here http://lxr.mozilla.org/seamonkey/source/widget/src/beos/nsWindow.cpp#3153 
will be chnaged from 5 to 6.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 244096 [details] [diff] [review]
for all toolkits 2

Ere, could you look at the Windows part?

Comment 12

[Doug Shelton]

This patch breaks BeOS mouse handling.  Left-click did not function even at startup profilemanager.  Partial console output of debug build:

CMA
TK-GI
###!!! ASSERTION: Wrong number of arguments to CallMethod: 'info->nArgs == 6', file /boot/home/develop/mozilla/widget/src/beos/nsWindow.cpp, line 1831
CMA
TK-GI
###!!! ASSERTION: Wrong number of arguments to CallMethod: 'info->nArgs == 6', file /boot/home/develop/mozilla/widget/src/beos/nsWindow.cpp, line 1831
CMA
TK-GI
###!!! ASSERTION: Wrong number of arguments to CallMethod: 'info->nArgs == 6', file /boot/home/develop/mozilla/widget/src/beos/nsWindow.cpp, line 1831
CMA
TK-GI
###!!! ASSERTION: Wrong number of arguments to CallMethod: 'info->nArgs == 6', file /boot/home/develop/mozilla/widget/src/beos/nsWindow.cpp, line 1831  

Comment 13

[Doug Shelton]

More specifically, clicking with left mouse button does not have intended result.  Example:  in profile manager, clicking left button on available profile, "start minefield" or "exit" button changes focus to the clicked item.  However, expected action (start or exit) never actually occurs, even with repeated clicking.  Also, assertion listed above disappears if "== 6" is replaced by "== 5".

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Doug, did you read Sergei's review comments ;)

Comment 15

[Ere Maijala (slow)]

Comment on attachment 244096 [details] [diff] [review]
for all toolkits 2

Windows part looks good to me.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 244096 [details] [diff] [review]
for all toolkits 2

Michael, could you look at os2 part.

Comment 17

[Andy Willis (abwillis)]

I built it into OS/2 and it seems to be working fine, is there something I should check in particular?  

Comment 18

[Mike Kaply [:mkaply]]

Looks OK from an OS/2 specific.

I'm curious as to why a mouse button is passed along at all in the context menu key case?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Checked in with some fixes for mac

Comment 20

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061120 Minefield/3.0a1 ID:2006112021 [cairo]

Is there any chance this has caused the context menu in both the add-ons manager and download manager to appear bottom left (regardless of pointer position) ?

Comment 21

[[e-mail]]

Comment on attachment 244096 [details] [diff] [review]
for all toolkits 2

>-    case NS_MOUSE_MIDDLE_BUTTON_UP:
>-    if(!gMiddlePref)
>+    case NS_MOUSE_BUTTON_UP:
>+      if (aEvent->eventStructType != NS_MOUSE_EVENT ||
>+          (NS_STATIC_CAST(nsMouseEvent*, aEvent)->button == nsMouseEvent::eMiddleButton &&
>+           !gMiddlePref)) {
>         break;
>+      }
> 
>-    case NS_MOUSE_LEFT_BUTTON_UP:
>-       // stop capturing
>-      //printf("stop capturing\n");
>-      AddListener();
>-      DragThumb(PR_FALSE);
>-      if (mChange) {
>-        nsRepeatService::GetInstance()->Stop();
>-        mChange = 0;
>+      if (NS_STATIC_CAST(nsMouseEvent*, aEvent)->button == nsMouseEvent::eLeftButton) {
>+         // stop capturing
>+        AddListener();
>+        DragThumb(PR_FALSE);
>+        if (mChange) {
>+          nsRepeatService::GetInstance()->Stop();
>+          mChange = 0;
>+        }
>+        mRedrawImmediate = PR_FALSE;//we MUST call nsFrame HandleEvent for mouse ups to maintain the selection state and capture state.
>+        return nsFrame::HandleEvent(aPresContext, aEvent, aEventStatus);
>       }
>-      mRedrawImmediate = PR_FALSE;//we MUST call nsFrame HandleEvent for mouse ups to maintain the selection state and capture state.
>-      return nsFrame::HandleEvent(aPresContext, aEvent, aEventStatus);
This change caused bug 364125 - the old code was equivalent to if ((button == 1 && gMiddlePref) || button == 0) but the new code doesn't do anything useful for middle buttons.