UIEvent button property is wrong

VERIFIED FIXED in mozilla0.9

Status

()

Core
DOM: Events
P3
major
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: Erik Arvidsson, Assigned: Blake Ross)

Tracking

({dom2})

Trunk
mozilla0.9
x86
Windows NT
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

17 years ago
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.
Reassigning to the owner of the DOM event code.
Assignee: jst → joki

Comment 2

17 years ago
Easy enough to fix.  Though the regression testing will be rough.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
Keywords: dom2
Component: DOM Level 2 → DOM Events
(Assignee)

Comment 3

17 years ago
Created attachment 26152 [details] [diff] [review]
[patch] initial patches
(Assignee)

Comment 4

17 years ago
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?
Assignee: joki → blakeross
Status: ASSIGNED → NEW
(Assignee)

Comment 5

17 years ago
cc'ing hyatt since this would entail a minor change to the xbl spec if we go 
through with this.
(Assignee)

Comment 6

17 years ago
Created attachment 26153 [details] [diff] [review]
[patch] updated

Comment 7

17 years ago
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?
(Assignee)

Comment 8

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

Comment 9

17 years ago
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 :-\
(Assignee)

Comment 10

17 years ago
Created attachment 26175 [details] [diff] [review]
[patch] updated again for more missed cases

Comment 11

17 years ago
r=timeless
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?
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:
Hmm, sorry about the missing whitespace, but you get the idea.

Comment 15

17 years ago
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.
(Assignee)

Comment 16

17 years ago
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.
(Assignee)

Comment 17

17 years ago
Created attachment 26598 [details] [diff] [review]
[patch] add warning, keep init, preserve |which| backwards compat.
sr=jst
(Assignee)

Comment 19

17 years ago
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?
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
FWIW, this change caused bug 70683 because some files were missed.
It also caused an additional regression breaking middle-mouse paste of URLs into
the content area by missing a change in contentAreaClick.js.
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

17 years ago
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.

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

Comment 25

17 years ago
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

17 years ago
If you need a bug, the regression caused is

"Ctrl+Click no longer works in trees."

Comment 27

17 years ago
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.
Is there a spin-off bug yet?

/be

Comment 29

17 years ago
Not that I know of.  SOmebody needs to file it on blake.
bug #70755

Comment 31

16 years ago
QA Contact Update
QA Contact: vidur → vladimire

Comment 32

16 years ago
Verifying fixed on 2001-05-29-08-trunk windows 98
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.