Closed Bug 354694 Opened 16 years ago Closed 16 years ago

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

Categories

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

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(1 file, 1 obsolete file)

to make nsEvent structures to work more like dom events,
nsIDOMMouseEvent::type should always map to only one nsMouseEvent::message.
I have a patch for this, but only for gtk2. Need to still convert other
toolkits.
Attached patch for all toolkits (obsolete) — Splinter Review
Need to still re-read the patch myself.
roc, jst, perhaps you have some comments to the changes in nsGUIEvent.h?
Looks good. it might be nice to just make the button number be an integer because mice can have many more buttons.
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...
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.
Attachment #242383 - Attachment is obsolete: true
Attachment #244096 - Flags: review?(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?
Attachment #244096 - Flags: review?(jst) → review+
Comment on attachment 244096 [details] [diff] [review]
for all toolkits 2

josh, could you look at the mac code.
Attachment #244096 - Flags: review?(joshmoz)
Attachment #244096 - Flags: review?(joshmoz) → review+
Comment on attachment 244096 [details] [diff] [review]
for all toolkits 2

Roc, could you look at the gtk/gtk2 part?
Attachment #244096 - Flags: review?(roc)
Attachment #244096 - Flags: review?(sergei_d)
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.
Attachment #244096 - Flags: review?(sergei_d) → review+
Comment on attachment 244096 [details] [diff] [review]
for all toolkits 2

Ere, could you look at the Windows part?
Attachment #244096 - Flags: review?(emaijala)
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  

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".
Doug, did you read Sergei's review comments ;)
Comment on attachment 244096 [details] [diff] [review]
for all toolkits 2

Windows part looks good to me.
Attachment #244096 - Flags: review?(emaijala) → review+
Comment on attachment 244096 [details] [diff] [review]
for all toolkits 2

Michael, could you look at os2 part.
Attachment #244096 - Flags: review?(mozilla)
I built it into OS/2 and it seems to be working fine, is there something I should check in particular?  
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?
Attachment #244096 - Flags: review?(mozilla) → review+
Checked in with some fixes for mac
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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) ?
Depends on: 361376
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.
Depends on: 373483
Depends on: 395426
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.