Last Comment Bug 60703 - UIEvent button property is wrong
: UIEvent button property is wrong
Status: VERIFIED FIXED
: dom2
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: x86 Windows NT
: P3 major (vote)
: mozilla0.9
Assigned To: Blake Ross
: Vladimir Ermakov
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2000-11-19 18:58 PST by Erik Arvidsson
Modified: 2001-05-31 15:52 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
[patch] initial patches (25.50 KB, patch)
2001-02-24 21:53 PST, Blake Ross
no flags Details | Diff | Splinter Review
[patch] updated (24.69 KB, patch)
2001-02-24 22:28 PST, Blake Ross
no flags Details | Diff | Splinter Review
[patch] updated again for more missed cases (25.84 KB, patch)
2001-02-25 16:12 PST, Blake Ross
no flags Details | Diff | Splinter Review
[patch] add warning, keep init, preserve |which| backwards compat. (27.20 KB, patch)
2001-03-01 18:35 PST, Blake Ross
no flags Details | Diff | Splinter Review

Description Erik Arvidsson 2000-11-19 18:58:01 PST
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, jst@mozilla.com) 2000-11-29 00:55:05 PST
Reassigning to the owner of the DOM event code.
Comment 2 joki (gone) 2001-01-22 13:36:09 PST
Easy enough to fix.  Though the regression testing will be rough.
Comment 3 Blake Ross 2001-02-24 21:53:08 PST
Created attachment 26152 [details] [diff] [review]
[patch] initial patches
Comment 4 Blake Ross 2001-02-24 21:55:44 PST
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 2001-02-24 22:17:53 PST
cc'ing hyatt since this would entail a minor change to the xbl spec if we go 
through with this.
Comment 6 Blake Ross 2001-02-24 22:28:33 PST
Created attachment 26153 [details] [diff] [review]
[patch] updated
Comment 7 John Morrison 2001-02-24 23:13:55 PST
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 2001-02-24 23:20:02 PST
Where do you see that?  IE5.5 follows DOM2 -- 0 is left button, 1 is middle, 2 
is right.
Comment 9 John Morrison 2001-02-24 23:31:52 PST
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 2001-02-25 16:12:11 PST
Created attachment 26175 [details] [diff] [review]
[patch] updated again for more missed cases
Comment 11 timeless 2001-02-28 18:36:05 PST
r=timeless
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2001-02-28 23:27:04 PST
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, jst@mozilla.com) 2001-03-01 03:20:22 PST
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, jst@mozilla.com) 2001-03-01 03:21:15 PST
Hmm, sorry about the missing whitespace, but you get the idea.
Comment 15 joki (gone) 2001-03-01 03:26:20 PST
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 2001-03-01 12:54:25 PST
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 2001-03-01 18:35:35 PST
Created attachment 26598 [details] [diff] [review]
[patch] add warning, keep init, preserve |which| backwards compat.
Comment 18 Johnny Stenback (:jst, jst@mozilla.com) 2001-03-01 18:44:36 PST
sr=jst
Comment 19 Blake Ross 2001-03-01 19:27:39 PST
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+2 (mostly busy through August 4; review requests must explain patch) 2001-03-02 10:58:13 PST
FWIW, this change caused bug 70683 because some files were missed.
Comment 21 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-03-02 11:13:08 PST
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+2 (mostly busy through August 4; review requests must explain patch) 2001-03-02 11:31:07 PST
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 2001-03-02 11:41:08 PST
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+2 (mostly busy through August 4; review requests must explain patch) 2001-03-02 12:45:56 PST
hyatt:  Do your comments apply to all of treeEditable.xml, outlinerBindings.xml,
and treeBindings.xml, or just some of them?
Comment 25 David Hyatt 2001-03-02 13:19:09 PST
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 2001-03-02 13:23:01 PST
If you need a bug, the regression caused is

"Ctrl+Click no longer works in trees."
Comment 27 David Hyatt 2001-03-02 13:27:54 PST
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] 2001-03-02 15:42:37 PST
Is there a spin-off bug yet?

/be
Comment 29 David Hyatt 2001-03-02 15:49:09 PST
Not that I know of.  SOmebody needs to file it on blake.
Comment 30 Christopher Blizzard (:blizzard) 2001-03-02 16:15:24 PST
bug #70755
Comment 31 Vladimir Ermakov 2001-05-29 15:49:57 PDT
QA Contact Update
Comment 32 Vladimir Ermakov 2001-05-31 15:52:42 PDT
Verifying fixed on 2001-05-29-08-trunk windows 98

Note You need to log in before you can comment on or make changes to this bug.