60703 - UIEvent button property is wrong

Status: VERIFIED FIXED

(Core :: DOM: Events, defect, P3)

Description

[Erik Arvidsson]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0; COM+ 1.0.2204)
BuildID:    2000111704

The property "button" for UIEvent returns the wrong value. It is off by one.

Reproducible: Always
Steps to Reproduce:
1. Add the following JS:
   document.onmouseup = function (e) {
      alert(e.button);
   };
2. Click anywhere in the document and you'll get an alert giving you a number. 
This number is wrong according to the W3C DOM level 2 specs


Actual Results:  Left button => 1
Middle button => 2
Right button => 3

Expected Results:  Left button => 0
Middle button => 1
Right button => 2

This is taken from the documentation at w3.org

button of type unsigned short, readonly 
During mouse events caused by the depression or release of a mouse button, 
button is used to indicate which mouse button changed state. The values for 
button range from zero to indicate the left button of the mouse, one to 
indicate the middle button if present, and two to indicate the right button. 
For mice configured for left handed use in which the button actions are 
reversed the values are instead read from right to left.

Comment 1

[Johnny Stenback (:jst)]

Reassigning to the owner of the DOM event code.

Comment 2

[joki (gone)]

Easy enough to fix.  Though the regression testing will be rough.

Comment 3

[Blake Ross]

Attachment: [patch] initial patches

Comment 4

[Blake Ross]

I may have missed some usages, this is just a first cut.  Any help testing 
these on Linux and Mac would be much appreciated.

Blizzard, I don't know enough about gtk event handling -- I shouldn't be 
touching anything in nsWindow (e.g. 
http://lxr.mozilla.org/seamonkey/source/widget/src/gtk/nsWidget.cpp#1895), 
right?

Comment 5

[Blake Ross]

cc'ing hyatt since this would entail a minor change to the xbl spec if we go 
through with this.

Comment 6

[Blake Ross]

Attachment: [patch] updated

Comment 7

[John Morrison]

I think I just heard the sound of the tiny brains of hundreds of web
developers exploding. ;-] Is it intentional that DOM L2's middlemouse
is mapped to Nav4 and IE4/5/6.. leftmouse?

Comment 8

[Blake Ross]

Where do you see that?  IE5.5 follows DOM2 -- 0 is left button, 1 is middle, 2 
is right.

Comment 9

[John Morrison]

I stand corrected. I was looking at Microsoft's (outdated) documentation, and
testing in IE5.0 on windows. Yes, Mac IE5.0 also does 0 for leftmouse. (I'll
shut up now :-\

Comment 10

[Blake Ross]

Attachment: [patch] updated again for more missed cases

Comment 11

[timeless]

r=timeless

Comment 12

[Johnny Stenback (:jst)]

I had a look at the patch and in general it looks very good but there are a few
things I don't agree with:

In nsDOMEvent::GetButton():

I'd suggest adding a NS_WARNING() in the if (!mEvent...) block since people
should not be calling that, right?

In nsEditorEventListeners.cpp, line ~380:

Why not initialize button to 0? I don't see a good reason to not do that since
you're not checking the return value from moseEvent->GetButton() or anything.
Leave the = 0 in there, even if it's potentially wrong, it's better than having
an uninitialized variable cause weird random behavior.

In outlinerBindings.xml:

You call b.selection.select(row.value); unconditionally where the old code
checked for if (event.button == 1), is this correct? If so please explain.

Comments? Thoughts?

Comment 13

[Johnny Stenback (:jst)]

Blake, I talked to joki about this and he sez you'll need this patch too (for
4.x compatibility):

Index: content/events/src/nsDOMEvent.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/events/src/nsDOMEvent.cpp,v
retrieving revision 1.102
diff -u -r1.102 nsDOMEvent.cpp
--- nsDOMEvent.cpp 2001/02/12 06:54:30 1.102
+++ nsDOMEvent.cpp 2001/03/01 11:19:02
@@ -860,7 +860,7 @@
{
PRUint16 button;
(void) GetButton(&button);
- *aWhich = button;
+ *aWhich = button + 1;
break;
}
default:

Comment 14

[Johnny Stenback (:jst)]

Hmm, sorry about the missing whitespace, but you get the idea.

Comment 15

[joki (gone)]

I have only two comments on the patch.  The first is event.which, which is an 
old 4.x property (which sucks) that was used for key values and button values.  
event.which was 1 based, as opposed to 0 based.  That's the reason that 
event.button was set the way it was.  But for compatibility with 4.x scripts we 
need to continue setting event.which to be 1 based.  So nsDOMEvent::GetWhich 
needs to increment the value of which after GetButton.

Second, as a result the patch needs to change the code using event.which.  Since 
we've deprecated event.which it should really use event.button so I'd suggest 
leaving the numbers in the patch as is and changing the uses of event.which to 
event.button.

Other than that it looks great.

Comment 16

[Blake Ross]

Thanks for looking.

> I'd suggest adding a NS_WARNING() in the if (!mEvent...) block since people
> should not be calling that, right?

Sure, sounds good.

> Why not initialize button to 0? I don't see a good reason to not do that since
> you're not checking the return value from moseEvent->GetButton() or anything.
> Leave the = 0 in there, even if it's potentially wrong, it's better than 
> having an uninitialized variable cause weird random behavior.

Agreed.  Initial thinking was that if GetButton somehow failed (which can't 
currently happen anyways), the button would be mistakenly set to 0 (left 
button), which is a valid value.  But now that I think about it, an uninit'd 
variable would be worse than that anyways :-)  (We could also just set it to -1 
as we do in other places, but...*shrug*)

> You call b.selection.select(row.value); unconditionally where the old code
> checked for if (event.button == 1), is this correct? If so please explain.

Look closer :-)  The old code bailed immediately if it was anything but button 
1, and then had another redundant check for button 1 later on.  I got rid of 
both and just used xbl's |button| filter for handler (<handler button="0|1|2"/>)

Attaching a new patch to add the warning, leave the init, and preserve |which| 
backwards compatibility.

Comment 17

[Blake Ross]

Attachment: [patch] add warning, keep init, preserve |which| backwards compat.

Comment 18

[Johnny Stenback (:jst)]

sr=jst

Comment 19

[Blake Ross]

Fix checked in.

- Not landed on mailnews perf branch.  When those changes are merged with the 
tip, some things may need to be changed (outlinerBindings.xml especially).

- hyatt: the xbl spec needs updating.  Want a new bug as a reminder?

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, this change caused bug 70683 because some files were missed.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It also caused an additional regression breaking middle-mouse paste of URLs into
the content area by missing a change in contentAreaClick.js.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The fix I have is

Index: contentAreaClick.js
===================================================================
RCS file: /cvsroot/mozilla/xpfe/communicator/resources/content/contentAreaClick.js,v
retrieving revision 1.6
diff -u -d -r1.6 contentAreaClick.js
--- contentAreaClick.js	2001/03/02 03:07:23	1.6
+++ contentAreaClick.js	2001/03/02 19:14:16
@@ -125,7 +125,7 @@
       handleLinkClick(event, linkNode.href);
       return true;
     }
-    if (pref && event.button == 2 &&
+    if (pref && event.button == 1 /* middle click */ &&
         !findParentNode(event.originalTarget, "scrollbar") &&
         pref.GetBoolPref("middlemouse.contentLoadURL")) {
       if (middleMousePaste()) {


jag said r=jag on IRC

Comment 23

[David Hyatt]

The patches to the tree XBL are incorrect.  To quote Peter Linss,

READ THE SPEC.

You just added code to the tree handler that imposed a filter on the event
handler.  This means it will no longer be matched when CTRL or META are pressed.
 The correct code should have just patched the button == 1 check to become
button == 0.

A filter (the button attribute) should not have been placed on the handler.

Please fix this right now.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

hyatt:  Do your comments apply to all of treeEditable.xml, outlinerBindings.xml,
and treeBindings.xml, or just some of them?

Comment 25

[David Hyatt]

The code that jst had questions about is the code that was incorrectly modified.
 That is treeBindings.xml (and presumably outlinerBindings.xml) as well.

Comment 26

[David Hyatt]

If you need a bug, the regression caused is

"Ctrl+Click no longer works in trees."

Comment 27

[David Hyatt]

The fix is to revert the file back to the way it was and then just do a straight
substitution everywhere button is used with a value of 1.

Comment 28

[Brendan Eich [:brendan]]

Is there a spin-off bug yet?

/be

Comment 29

[David Hyatt]

Not that I know of.  SOmebody needs to file it on blake.

Comment 30

[Christopher Blizzard (:blizzard)]

bug #70755

Comment 31

[Vladimir Ermakov]

QA Contact Update

Comment 32

[Vladimir Ermakov]

Verifying fixed on 2001-05-29-08-trunk windows 98