258193 - nsIDOMMouseEvent::GetButton() broken with contextmenu event

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Crispin Flowerday (not receiving bug mail)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a3) Gecko/20040903 Galeon/1.3.17.99
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a3) Gecko/20040903 Galeon/1.3.17.99

If you capture a contextmenu event, using the nsIDOMContextMenuListener
interface, and then call do:

PRUint16 btn;
rv = aMouseEvent->GetButton (&btn);
NS_ENSURE_SUCCESS (rv, rv);

the GetButton() call succeeds, but btn isn't initialized. I believe this is
because |mEvent->message| is NS_CONTEXTMENU, which isn't handled in the switch
statement.


Reproducible: Always
Steps to Reproduce:

Comment 1

[Boris Zbarsky [:bzbarsky]]

Wladimir, do you have time for this one?

Comment 2

[Wladimir Palant]

Attachment: Patch

NS_CONTEXTMENU could be created by a keyboard action, so we can't return a
mouse button here (unless changes are made in the event creation code, which
shouldn't be worth it). I made GetButton() return -1 for mouse events that
don't have a mouse button associated with them (e.g. NS_MOUSE_MOVE). Decided
that we don't need a warning here, for mouse events this call should be always
legal. Fixed the wrong mButton check while there, but mButton is never used
anyway.

Comment 3

[Crispin Flowerday (not receiving bug mail)]

Won't this make the (mouse generated) NS_CONTEXTMENU event have a mouse button
of -1 ? I would assume that it should be 2 (as it was generated by a right mouse
button) ?

Comment 4

[Wladimir Palant]

Attachment: Patch (mouse button set to 2 for NS_CONTEXTMENU now)

Just saw it - keyboard generated context menu events produce
NS_CONTEXTMENU_KEY, not NS_CONTEXTMENU. Looking through the plattform specific
code it seems to be safe to assume, that NS_CONTEXTMENU has been triggered by
the right mouse button (even on MacOS).

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 158091 [details] [diff] [review]
Patch (mouse button set to 2 for NS_CONTEXTMENU now)

Seems reasonable to me. r=jst

Bz, what say you?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 158091 [details] [diff] [review]
Patch (mouse button set to 2 for NS_CONTEXTMENU now)

sr=bzbarsky.  Seems ok.

Comment 7

[Ian Neal]

Checking in nsDOMMouseEvent.cpp;
/cvsroot/mozilla/content/events/src/nsDOMMouseEvent.cpp,v  <--  nsDOMMouseEvent.cpp
new revision: 1.4; previous revision: 1.3
done

Comment 8

[Martijn Wargers (dead)]

When I have this: document.onmousemove=function(e){window.status=e.button;}
then I get before this fix: 0 , after this fix: 65535
So the former behavior is wrong and the latter behavior is right, then?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> So the former behavior is wrong and the latter behavior is right, then?

The behavior of .button for non-button-related events like mousemove is undefined...

Comment 10

[Martijn Wargers (dead)]

Ok, does the same for apply e.which?
document.onmousemove=function(e){window.status=e.which;}
Before the fix returns 19, after the fix 65536.

Comment 11

[Boris Zbarsky [:bzbarsky]]

That seems odd... which should be button + 1.  That is, the 65536 is "correct",
but the 19 confuses me...  Or rather, it shouldn't have been consistently 19. 
It should have been a random number...

Comment 12

[Mike Kaply [:mkaply]]

I don't understand what this patch actually fixed.

Do we have a testcase for this?

This patch actually broke backwards compatibility with other browsers in the mouse move case.

Comment 13

[Mike Kaply [:mkaply]]

Incidentally, it was consistenly 19 in a release build, random in a debug build.

Comment 14

[Mike Kaply [:mkaply]]

And incidentally it is wrong to hardcode context menu to 2 - what if a platform displays the context menu using a different button?