64485 - [Linux] Intellimouse Explorer Backwards and Forwards button support

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[Matt Schulkind]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.4.0-prerelease i686; en-US; 0.6)
Gecko/20001205
BuildID:    2001010415

I would like to be able to use mouse buttons 6 and 7 on my microsoft
intellimouse optical for back and forward respectivly. In windows the
intellimouse software provides this feature, but I would also like this to work
under linux and other operating systems.

Reproducible: Always
Steps to Reproduce:
N/A

Actual Results:  N/A


Expected Results:  N/A


N/A

Comment 1

[Matt Schulkind]

*** This bug has been marked as a duplicate of 30431 ***

Comment 2

[Loco]

Yup.

VERIFIED.

Comment 3

[Daniel Wang]

reopen

Comment 4

[Daniel Wang]

-> XP Toolkit

Comment 5

[lord_ion]

 30431 didn't fix this, it only fixed windows. Many linux users have their
intellimouse set up as described by deadman:
http://www.deadman.org/X/xbuttons.html and here is a nasty hack to get the
navigation working:
http://www.p-two.net/modules.php?op=modload&name=News&file=article&sid=2
xev output: 
ButtonPress event, serial 26, synthetic NO, window 0x1400001,
    root 0x78, subw 0x0, time 4206911804, (107,116), root:(108,138),
    state 0x10, button 6, same_screen YES

ButtonRelease event, serial 26, synthetic NO, window 0x1400001,
    root 0x78, subw 0x0, time 4206911924, (107,116), root:(108,138),
    state 0x10, button 6, same_screen YES

ButtonPress event, serial 26, synthetic NO, window 0x1400001,
    root 0x78, subw 0x0, time 4206912812, (107,116), root:(108,138),
    state 0x10, button 7, same_screen YES

ButtonRelease event, serial 26, synthetic NO, window 0x1400001,
    root 0x78, subw 0x0, time 4206912924, (107,116), root:(108,138),
    state 0x10, button 7, same_screen YES

With a USB Intellimouse Explorer under XFree86 4.2.1 on Gentoo. Button 6 is the
"Back" button, button 7 "Forward". These are the windows defaults anyway.

Comment 6

[Matt Larson]

FWIW, support for this mouse button 6/7 back/forward behavior under Linux is the
only thing keeping me from never using IE on Windows again.  I would love to see
this feature implemented.

Comment 7

[Darren Salt]

Attachment: Patch allowing the use of the back/forward buttons (GTK)

This patch seems to work (at least in Phoenix) for gtk. Something similar
should work for gtk2 and qt.

Comment 8

[Tom Harding]

Popping in here with a "me too" comment.  I hope those of us who don't build,
let alone patch, Mozilla can use our fancy mouse buttons soon.  Surf for hours
without touching the keyboard.

Comment 9

[timeless]

Attachment: that didn't compile

Comment 10

[Brian Ryner (not reading)]

I'd like to figure out a way to know at runtime which button numbers are mapped
to which buttons on the mouse.  For example, if we wanted to support mice which
have a horizontal scroll wheel on Linux, that scroll wheel is mapped to buttons
6 and 7.  So, it's ambiguous exactly how we should treat these buttons without
getting additional info.

Comment 11

[Tom Harding]

Mozilla can never know for sure what logical button corresponds to what physical
button on the mouse.  Shouldn't Mozilla just show which logical buttons are
active and let the user map functions to them?

Comment 12

[Ed Catmur]

Which mouse function is mapped to which button number is a matter for the
driver, and in any case can be altered with xmodmap -e "pointer=123458967".
FWIW, most high-end mice these days are 7-button; horizontal scrolling is still
relatively uncommon. 

Comment 13

[Stephen Williams]

Another "me too" here as well.  Although I do not have a problem with the
nightly binary builds, it only occurs to me when I build from source.  When
building from source I'm building for gtk2, I've not built for gtk as I'm trying
to get away from the older platform.

I'm a user, not a coder, but if there's anything I can do to help... 

Comment 14

[Darren Salt]

I should mention that I'm also using gtk2 these days (Phoenix, built from
source). It appears that side button clicks are passed on by gtk2 as scroll
events; consequently, they cause vertical scrolling (they're mapped as buttons 4
and 5), and I've been unable to find any way to tell gtk2, short of downloading
and patching the source, that buttons 6 and 7 are the scroll wheel :-|

I /could/ reset the mapping (and reconfigure X appropriately), but doing so
will, IME, cause the side buttons to be completely ignored. Not good...

Comment 15

[Sam Rowe]

Use xmodmap as indicated in the deadman.org link above. There's no reason for
Mozilla (or Phoenix in your case) to be compensating for XFree's odd placement
of those descriptors.

Comment 16

[Darren Salt]

I'm using that X/xmodmap configuration anyway; the details in my above comment
aren't quite right :-(

Anyway, having updated my copy of the source and rebuilt Phoenix, I find that
the side buttons are being treated as a /horizontal/ scroll wheel, which of
course would be useful were it the case that my mouse had two scroll wheels.

I might attempt to patch widget/src/gtk2/nsWindow.cpp to implement a function to
(try to) determine whether the mouse has two wheels - i.e. 8 buttons or more
rather than 7 or fewer; are there any single-wheel mice with more than 7 buttons?

Comment 17

[Sébastien Delahaye]

*** Bug 201981 has been marked as a duplicate of this bug. ***

Comment 18

[timeless]

.

Comment 19

[personseb]

** me too **

Comment 20

[personseb]

In fact I have noticed that it doesn't work with gtk2. the buttons are viewed as
horizontal scroll button.
I tried to use imwheel to translate button 6 and 7 to respectively ALT_L|Left
and ALT_L|Right but didn't solve it (this trick was working with gtk1.x I believe).

Anybody has idea ?

Comment 21

[Graham Dennis]

Attachment: Configurable support for forward/back buttons in gtk/2

This patch adds support for gtk and gtk2 in different ways. I don't know which
is the better solution, so I'd be grateful is someone could enlighten me.
gtk gets all button/scroll events as buttons and treats buttons 4/5 as the
scrollwheel. This patch makes buttons 6/7 be treated as 'ExtendedMouseEvents'
which can be configured under the preferences key: 'mousebuttonsextended'
gtk2 treats 4/5 as vertical scrollwheel and 6/7 as horizontal scrollwheel
regardless of whether a horizonal scrollwheel is on the mouse. The fix for gtk2
was to enable independed configuring of the horizontal / vertical scrollwheels
(formerly they were both connected).
But from my testing the patch works in its current form for both gtk and gtk2.
Comments welcome.

Comment 22

[Thomas Kleffel]

This patch has one small problem:

if the "sysnumlines"-config-option is true, then numLines gets the value of
msEvent->delta.

So after executing the following line:
if ((msEvent->delta < 0)) numLines = -numLines;

numLines is always positive. This means, that scrolling only works in one
direction if sysnumlines is true.

This can be fixed by exchanging

        if (aBool) {
          numLines = msEvent->delta;
          if (msEvent->scrollFlags & nsMouseScrollEvent::kIsFullPage)
            action = MOUSE_SCROLL_PAGE;
        }

with

        if (aBool) {
          numLines = abs(msEvent->delta);
          if (msEvent->scrollFlags & nsMouseScrollEvent::kIsFullPage)
            action = MOUSE_SCROLL_PAGE;
        }


Notice that this part appears four times.

I did not create a new attachment, because I thought that it would be easier if
the supplier of the patch would edit his patch and add the few letters.

Tommy

Comment 23

[Thomas Kleffel]

Attachment: Enhanced Patch for using Buttons 6 and 7 with gtk and gtk2

I rewrote the section for gtk2 (horizontal scrollwheel stuff) to make it more
straightforward. It works now even when sysnumlines is true.

Comment 24

[personseb]

hi, I would like to test this patch ...
fisrt I applied the lastest submited but there was an error about a structure
that wasn't declared so I also applied the previous one.
This time I got another error :

g++ -o nsEventStateManager.o -c -DOSTYPE=\"Linux2.4\" -DOSARCH=\"Linux\"
-I./../../html/base/src -I./../../xul/content/src
 -I../../../dist/include/xpcom -I../../../dist/include/string
-I../../../dist/include/dom -I../../../dist/include/js
-I../../../dist/include/locale -I../../../dist/include/gfx
-I../../../dist/include/layout -I../../../dist/include/widget
-I../../../dist/include/caps -I../../../dist/include/xpconnect
-I../../../dist/include/docshell -I../../../dist/include/pref
-I../../../dist/include/htmlparser -I../../../dist/include/view
-I../../../dist/include/necko -I../../../dist/include/unicharutil
-I../../../dist/include/content -I../../../dist/include
-I/var/tmp/portage/mozilla-firebird-0.6-r6/work/mozilla/dist/include/nspr     
-I/usr/X11R6/include   -fPIC  -I/usr/X11R6/include -frtti -fno-handle-exceptions
 -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth
-Wno-ctor-dtor-privacy -Wno-long-long -O3 -mcpu=athlon-tbird -pipe -mmmx -m3dnow
-Wno-return-type -w -Wno-return-type -w -Wno-return-type -w -Wno-return-type -w
-Wno-return-type -w -s -fforce-addr -fshort-wchar -pthread -pipe  -DNDEBUG
-DTRIMMED -ffunction-sections -O2  -I/usr/X11R6/include -DMOZILLA_CLIENT
-include ../../../mozilla-config.h -Wp,-MD,.deps/nsEventStateManager.pp
nsEventStateManager.cpp
nsEventStateManager.cpp: Dans member function « virtual nsresult
   nsEventStateManager::PostHandleEvent(nsIPresContext*, nsEvent*, nsIFrame*,
   nsEventStatus*, nsIView*) »:
nsEventStateManager.cpp:2077: valeur de « case » double
nsEventStateManager.cpp:1973: précédemment utilisé ici
nsEventStateManager.cpp:2120: conversion invalide de « void* » vers « char* »
nsEventStateManager.cpp:2121: conversion invalide de « void* » vers « char* »


So what is the way to test this patch ? 
apply first the 2003-06-25 patch and then the last ?

which version of mozilla should I must use to apply the patch ? is it possible
to apply on firebird ?

I hope I didn't borrying you. Thanks for your help ;)

Comment 25

[Stian Jordet]

What is happening with this bug? Why isn't the patch commited? Or any activity
at all? I got a compile error when compiling so I can't test it. But in the end
of this bug it's only mentioned horizontal scrolling. Nothing about back/forward
buttons... Does it work for both?

Comment 26

[Ed Catmur]

Attachment: Updated for bitrot and Firebird

This patch applies, compiles and tests against CVS sources as of 20030907.

In reply to previous comment: the idea is that as GTK2 treats side button
events as horizontal scroll events we provide a set of preferences to allow the
user to bind horizontal scroll to the same events (scroll, page, history, zoom)
as are available for vertical scroll, binding unmodified horizontal scroll to
history by default:

mousewheel.horizscroll.withnokey.action = 2

The GTK code uses 'extended mouse button events' to handle buttons 6 and 7 as
mouse click events.

Comment 27

[Stian Jordet]

This works perfect :) What must be done to get this checked in..? Thanks for
your great work, all of you :)

Comment 28

[Thanos Kyritsis]

I tried this latest patch on the mozilla 1.4 sources. It got patched and
compiled flawlessly! The sidebuttons' behaviour works as expected.

I have one question though: The GTK1-enabled mozilla needs imwheel in order to
assign to the sidebuttons Alt-Left and Alt-Right. The sidebuttons at the
GTK2-enabled mozilla only work correctly if imwheel is *not* loaded. In this
case the sidebuttons are limited only to work on mozilla. Other programs require
imwheel so that the user can assign specific keyboard shortcuts for each
application. Yes, I know that the correct way would be every program to support
the events sent by X when pressing button 6 and 7 (and so on), but is there any
possibility of making a patch that would let imwheel do it's job without
affecting mozilla ?

Comment 29

[Stian Jordet]

Now that 0.7 is out, why isn't anyone trying to get this patch committed? It
doesn't seem to harm anything, and I really, really, really find it useful..

Comment 30

[Stian Jordet]

Attachment: forward port of patch to enable forward and back buttons on newer mouses

This is _not_ my patch, I have just rediffed Ed Catmur's patch agains cvs of
today, and hoping to get review on it.

Comment 31

[shwag]

Yah, i'd really like to see this make it into Firebird 0.8.

Comment 32

[Stian Jordet]

Btw. this patch has been in the Debian packages of Mozilla Firebird since early
october, and noone has filed any bugreport against it there. 

Comment 33

[timeless]

Comment on attachment 134724 [details] [diff] [review]
forward port of patch to enable forward and back buttons on newer mouses

>Index: content/events/src/nsEventStateManager.cpp
>@@ -1909,6 +1912,110 @@
>+      mPrefBranch->GetCharPref("mousebuttonsextended.buttonlist", &string);
A. you don't correctly check for failure. this can result in an unintialized
parameter instead of the behavior you expect. please read about xpcom.

>+      free(string);
B. free is not the correct match. use an xpidlcstring and avoid the problem
entirely.
>+          suffix=strdup(".up");

>+      straction= (char *)memset(malloc(100), 0, 100);
C. you'll crash when malloc fails
>+      strnumlines=(char *)memset(malloc(100), 0, 100);
(C)

>+      strcpy(straction+strlen(straction), suffix);
D. i'm fairly certain this will crash when the earlier strdup failed
>+      strcpy(strnumlines+strlen(strnumlines), suffix); 
(D)

please don't use char* strings. nsCAutoString is relatively usable and you can
easily do things like appending a bunch of strings...

>+      mPrefBranch->GetIntPref(straction, &action);
(A)
>+      mPrefBranch->GetIntPref(strnumlines, &numLines);
(A)

E. A comment here:
>+              if (numLines < 0)
would be nice
>+                 webNav->GoBack();
>+              else
>+                 webNav->GoForward();

>@@ -1918,56 +2025,53 @@
>       nsMouseScrollEvent *msEvent = (nsMouseScrollEvent*) aEvent;
>       PRInt32 action = 0;
>       PRInt32 numLines = 0;

And no, i don't care if the code you're replacing has the bugs I'm complaining
about.

>+      char *tempstring=  (char *)memset(malloc(200), 0, 200);
(C)

>+      sprintf(tempstring,"%s%s%s%s", prefmousewheel, prefDirection, prefModifier, prefaction);
F. nsPrintfCString?
>+      mPrefBranch->GetIntPref(tempstring, &action);
(A)

>+      sprintf(tempstring,"%s%s%s%s", prefmousewheel, prefDirection, prefModifier, prefnumlines);
(F)
>+      mPrefBranch->GetIntPref(tempstring, &numLines);
(A)

>+      sprintf(tempstring,"%s%s%s%s", prefmousewheel, prefDirection, prefModifier, prefsysnumlines);
(F)

>+      mPrefBranch->GetBoolPref(tempstring, &useSysNumLines);
(A)

>+      //if configures numlines==0 we take the system-Value
G. the comment should be human readable. please try again

>+      //Now invert this Value if the Scroll-Lines provided by System are negative.
I think you want 'is' negative

>+      if(msEvent->delta < 0) numLines*=-1;
Would numLines=-numLines; be better? asking the compiler to consider using
multiplication might not be a good idea.

>@@ -1988,10 +2092,10 @@
(E)
>+              if (numLines > 0)
>                  webNav->GoForward();
>+              else
>+                 webNav->GoBack();

>@@ -2005,7 +2109,8 @@
>+              // Same idea as before
(E)
>+              ChangeTextSize((numLines > 0) ? 1 : -1);

>Index: widget/src/gtk/nsWidget.cpp


>-    eventType = NS_MOUSE_LEFT_BUTTON_UP;
>+    //      eventType = NS_MOUSE_LEFT_BUTTON_UP;
i'm not fond of the random indenting you used after //, it doesn't match code
in this file afaik from patch context.
>@@ -2096,22 +2116,39 @@
>     anEvent.isMeta = PR_FALSE;  // GTK+ doesn't support the meta key
*sigh* I knew there was a reason we couldn't fix some other bug (neil/bz/...
will understand this comment)

>+    if (aEventType!=NS_USER_DEFINED_EVENT) {
for fun you could use a function for this block, or perhaps a reference to the
result field, anything to avoid duplicating the switch statement. I'd probably
just have the switch statement assign to a local variable and then decide which
member field to use.

>+      switch(aGdkButtonEvent->type)
>+        case GDK_BUTTON_PRESS:

>+      switch(aGdkButtonEvent->type)
>+        case GDK_BUTTON_PRESS:

Note that this is not a gtk level review.

Comment 34

[Ed Catmur]

Attachment: Updated (NOT tested) after timeless' review

This patch should cover all of timeless' comments.
It was made against today's CVS and compiles with default configure flags to
give a working Mozilla build.
However, I have NOT tested it as I lack the necessary hardware. (I'm typing
this on a Windows machine - thank goodness for VNC!)
I will be back at my workstation in a week so hopefully can properly test it
before the new year. However if anyone wants to test it now I will try to keep
watching this bug.

Comment 35

[Brian Ryner (not reading)]

Comment on attachment 137756 [details] [diff] [review]
Updated (NOT tested) after timeless' review

>Index: browser/app/profile/all.js
>===================================================================

These changes can probably go away after bsmedberg's pref landing.

>Index: widget/src/gtk/nsWidget.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/gtk/nsWidget.cpp,v
>retrieving revision 1.288
>diff -u -r1.288 nsWidget.cpp
>--- widget/src/gtk/nsWidget.cpp	25 Sep 2003 05:34:25 -0000	1.288
>+++ widget/src/gtk/nsWidget.cpp	20 Dec 2003 16:11:27 -0000
>@@ -20,6 +20,8 @@
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>+ *   Andrew Wellington <[e-mail]>
>+ *   Graham Dennis <[e-mail]>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or 
>@@ -1884,9 +1886,17 @@
>       Release();
>       return;
> 
>-      // Single-click default.
>+      // Single-click default. <-- Perhaps the default should be
>+      //                           decided elsewhere?
>     default:
>-      eventType = NS_MOUSE_LEFT_BUTTON_DOWN;
>+      // was: eventType = NS_MOUSE_LEFT_BUTTON_DOWN;
>+      {
>+        nsExtendedMouseEventStatus *eMEStatus;
>+        eMEStatus = (nsExtendedMouseEventStatus*) &event.clickCount;
>+        eventType = NS_USER_DEFINED_EVENT;
>+        eMEStatus->event = nsExtendedMouseEventStatus_down;
>+        eMEStatus->button = aGdkButtonEvent->button;
>+      }
>       break;
>     }
>     break;

I think I'd prefer if we used a new event message for this. 
NS_MOUSE_EXTENDED_BUTTON, perhaps.


>Index: widget/public/nsGUIEvent.h
>===================================================================
>RCS file: /cvsroot/mozilla/widget/public/nsGUIEvent.h,v
>retrieving revision 3.98
>diff -u -r3.98 nsGUIEvent.h
>--- widget/public/nsGUIEvent.h	1 May 2003 01:01:03 -0000	3.98
>+++ widget/public/nsGUIEvent.h	20 Dec 2003 16:11:27 -0000
>@@ -416,6 +418,22 @@
>   nsDragDropEventStatus_eDrop  
> };
> 
>+/**
>+ * Event status for an extended mouse button event
>+ * The event (called clickCount) is type PRUint32:
>+ * hence this struct will be of the same size
>+ */
>+typedef struct {
>+  PRUint16 realClickCount;
>+  PRUint8  button;
>+  PRUint8  event;  // eg button went up / down
>+} nsExtendedMouseEventStatus;
>+

Our click counts will never need to be 32-bit integers.  I would vote for just
making this change in the nsMouseEvent struct.

>Index: content/events/src/nsEventStateManager.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v
>retrieving revision 1.466
>diff -u -r1.466 nsEventStateManager.cpp
>--- content/events/src/nsEventStateManager.cpp	19 Nov 2003 02:23:25 -0000	1.466
>+++ content/events/src/nsEventStateManager.cpp	20 Dec 2003 16:11:28 -0000
>@@ -22,6 +22,9 @@
>  * Contributor(s):
>  *   Makoto Kato  <[e-mail]>
>  *   Dean Tessman <[e-mail]>
>+ *   Andrew Wellington <[e-mail]>
>+ *   Graham Dennis <[e-mail]>
>+ *   Thomas Kleffel <[e-mail]>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>@@ -1911,6 +1914,96 @@
>       }
>     }
>     break;
>+  case NS_USER_DEFINED_EVENT: // In reality an extended mouse event
>+    {
>+      nsExtendedMouseEventStatus *eMEStatus;
>+      nsMouseEvent *mEvent = (nsMouseEvent* )aEvent;
>+      eMEStatus = (nsExtendedMouseEventStatus *)&mEvent->clickCount;
>+      nsresult rv;
>+      nsXPIDLCString buttonList;
>+      char ourbutton=0;
>+      nsCAutoString keyBase, keySuffix;
>+      PRInt32 action;
>+      PRInt32 numLines;
>+
>+      rv = getPrefBranch();
>+      if (NS_FAILED(rv)) return rv;
>+      
>+      // adding 2 below so the maximum number of mouse buttons we can
>+      // support is 99 but to represent the buttons in buttonlist
>+      // we're going to map 10->35 onto 'a'->'z'
>+      // the storage actually supports 256
>+      if(eMEStatus->button > 35) break;
>+      if(eMEStatus->button > 9)
>+        ourbutton = 'a' + eMEStatus->button -9;
>+      else
>+        ourbutton = '0' + eMEStatus->button;
>+      rv = mPrefBranch->GetCharPref("mousebuttonsextended.buttonlist", 
>+		                    getter_Copies(buttonList));
>+      if(NS_FAILED(rv)) break;
>+      if(!PL_strchr(buttonList.get(), ourbutton)) break;

This all seems overkill to me.	Why have this button list at all; why not just
check for the pref for the button we received and bail if it's not present?

>+#if 0   // Because we would have to create a msEvent
>+      case MOUSE_SCROLL_N_LINES:
>+      case MOUSE_SCROLL_PAGE:
>+        {
>+          DoWheelScroll(aPresContext, aTargetFrame, msEvent, numLines,
>+                        (msEvent->scrollFlags & nsMouseScrollEvent::kIsHorizontal),
>+                        (action == MOUSE_SCROLL_PAGE), PR_FALSE);
>+
>+        }
>+        break;
>+#endif 

What's this #if 0 block about?

>+      case MOUSE_SCROLL_HISTORY:
>+        {
>+          nsCOMPtr<nsISupports> pcContainer;
>+          mPresContext->GetContainer(getter_AddRefs(pcContainer));
>+          if (pcContainer) {
>+            nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(pcContainer));
>+            if (webNav) {
>+              // negative numLines to go back one step, nonneg to go forward
>+              if (numLines < 0)
>+                 webNav->GoBack();
>+              else
>+                 webNav->GoForward();
>+            }
>+          }
>+        }
>+        break;
>+
>+      case MOUSE_SCROLL_TEXTSIZE:
>+        {
>+          // Exclude form controls and XUL content.
>+          nsIContent *content = aTargetFrame->GetContent();
>+          if (content &&
>+              !content->IsContentOfType(nsIContent::eHTML_FORM_CONTROL) &&
>+              !content->IsContentOfType(nsIContent::eXUL))
>+            {
>+              ChangeTextSize((numLines > 0) ? 1 : -1);
>+            }
>+        }
>+        break;
>+
>+      default:  // Including -1 (do nothing)
>+        break;
>+      }          
>+    }
>+    break;

Please don't duplicate all of the code for handling these cases... factor it so
that the same code is used for both normal mouse wheel and the extended
buttons.

Comment 36

[Derek Upham]

Attachment: New patch, with fixes addressing Brian Ryner's comments

New patch added that addresses Brian Ryner's comments.

* New NS_MOUSE_EXTENDED_BUTTON event message.
* Extended event fields merged into nsMouseEvent.
* Button list config parameter removed.  We just look for the pref with the
button's number.
* Code to scroll history, text size and text have been pulled out into their
own methods.

It turned out that, while scrolling text took a scroll event (msEvent), it
didn't need one.  I've adjusted the code to use an ordinary mouse event, which
means that the Gtk1 extended buttons can now generate scroll events (enabling
the "#if 0" block).

I've gotten this to compile with Gtk1 and Gtk2 against stock firefox 0.8
sources.

Comment 37

[Steffen Wilberg]

Comment on attachment 144473 [details] [diff] [review]
New patch, with fixes addressing Brian Ryner's comments

>diff -u -r mozilla-orig/browser/app/profile/all.js mozilla/browser/app/profile/all.js
>--- mozilla-orig/browser/app/profile/all.js	2004-02-09 00:24:53.000000000 -0800
>+++ mozilla/browser/app/profile/all.js	2004-03-21 15:15:14.000000000 -0800

1. mozilla/browser/app/profile/all.js was replaced by firebird.js in January,
see bug 231200, which was replaced by firefox.js in February.
2. Please edit the files in your "mozilla" directory and use "cvs diff -u [list
of changed files]" to create patches.

Comment 38

[Derek Upham]

Attachment: New patch, with fixes addressing Steffen Wilberg's comments

Here's a new patch with the extra button support, against a recent CVS tree.

Comment 39

[Brian Ryner (not reading)]

Comment on attachment 145530 [details] [diff] [review]
New patch, with fixes addressing Steffen Wilberg's comments

reminding myself to review this later

Comment 40

[Brian Ryner (not reading)]

Comment on attachment 145530 [details] [diff] [review]
New patch, with fixes addressing Steffen Wilberg's comments

>--- content/events/src/nsEventStateManager.cpp	25 Mar 2004 09:10:53 -0000	1.487
>+++ content/events/src/nsEventStateManager.cpp	2 Apr 2004 16:36:26 -0000
>@@ -1525,14 +1528,48 @@
>   return NS_OK;
> }
> 
>+nsresult
>+nsEventStateManager::DoScrollHistory(PRInt32 direction)

Just have this method return void.

>+nsresult
>+nsEventStateManager::DoScrollTextsize(nsIFrame *aTargetFrame,
>+                                      PRInt32 adjustment)

Same.

>+      nsCAutoString prefstatus(((mEvent->status == nsMouseEventStatus_up)
>+                                ? ".up" : ".down"));

Do we really need separate actions for button up and down?  This seems like
overkill to me.  Though see comments below about dispatching these events.

>+      actionKey      = prefbase;
>+      numLinesKey    = prefbase;
>+      sysNumLinesKey = prefbase;

Rather than building up 3 separate pref keys, how about just obtaining the
nsIPrefBranch for the root you're accessing, i.e. "mousewheel" or
"mousewheel.horizscroll"?  Then once you have the pref branch you can just ask
for pref names relative to that.  You could actually then use the same code for
extended button and mousewheel events.

>+      // Extract the preferences
>+      PRInt32 action = 0;
>+      PRInt32 numLines = 0;
>+      PRBool aBool;

I know you're just shuffling this part around, but please take the opportunity
to rename 'aBool' to something more meaningful like "useSysNumLines".

>--- widget/public/nsGUIEvent.h	6 Mar 2004 15:00:37 -0000	3.107
>+++ widget/public/nsGUIEvent.h	2 Apr 2004 16:38:37 -0000
>@@ -204,6 +206,7 @@
> #define NS_MOUSE_ACTIVATE               (NS_MOUSE_MESSAGE_START + 30)
> #define NS_MOUSE_ENTER_SYNTH            (NS_MOUSE_MESSAGE_START + 31)
> #define NS_MOUSE_EXIT_SYNTH             (NS_MOUSE_MESSAGE_START + 32)
>+#define NS_MOUSE_EXTENDED_BUTTON        (NS_MOUSE_MESSAGE_START + 33)

It seems better to have separate messages for _UP and _DOWN for consistency
with the LEFT, RIGHT, and MIDDLE button events.

>@@ -551,21 +554,65 @@
>  * Mouse event
>  */
> 
>+/*
>+ * Mozilla has to track a bunch of different information about mouse
>+ * events:
>+ *
>+ * 1. The fact that it's a mouse event rather than some other input event.
>+ * 2. The button that was pressed.
>+ * 3. Whether the button went up or down.
>+ * 4. The number of times the button has been clicked in close succession.
>+ *
>+ * The typical representation encodes everything into subtle
>+ * variations on item 1.  A symbol like NS_MOUSE_LEFT_BUTTON_DOWN
>+ * expresses 1, 2 and 3, while NS_MOUSE_RIGHT_DOUBLECLICK expresses 1,
>+ * 2 and 4.
>+ *
>+ * The symbol NS_MOUSE_EXTENDED_BUTTON (used by Gtk1 to handle mouse
>+ * buttons 6, 7 and beyond) only expresses item 1.  We have to store
>+ * the other data about the event in an auxiliary structure.
>+ *

It's not a structure so much as just a field (button).

Also, I'd really like to see a way to get this working for gtk2, since we're
moving towards that as our primary toolkit for Linux.  That doesn't need to be
addressed in this patch, just thought I'd point it out.

So I'm minusing this based on my earlier comments.  Please don't take this as
me trying to hold up this patch; I appreciate the work you're doing here and am
just trying to help refine it some.

Comment 41

[Derek Upham]

Attachment: New new patch, with fixes addressing Brian Ryner's comments

The "extended" mouse buttons are *gone*.  In Gtk1, buttons 6 and 7 now generate
horizontal scroll wheel events, just like they do in Gtk2.  That gets rid of
many patches.  Gtk1 and Gtk2 can now use the same config entries.

I've simplified the pref key construction, but haven't switched the code to be
pref-branch-centric.  The API doesn't seem to allow building sub-branch
objects; you have to construct an entire new top-level object, and that looks
like it would be expensive (given the IDL interface).

Comment 42

[Brian Ryner (not reading)]

Comment on attachment 145986 [details] [diff] [review]
New new patch, with fixes addressing Brian Ryner's comments

--- browser/app/profile/firefox.js	17 Mar 2004 00:22:20 -0000	1.7
+++ browser/app/profile/firefox.js	13 Apr 2004 05:02:24 -0000

You can get rid of the pref changes to firefox.js.  Firefox picks up the all.js
prefs; it only has a few overrides.

--- content/events/src/nsEventStateManager.cpp	25 Mar 2004 09:10:53 -0000     
1.487
+++ content/events/src/nsEventStateManager.cpp	13 Apr 2004 05:02:24 -0000

>+      mPrefBranch->GetIntPref(PromiseFlatCString(actionKey).get(), &action);
>+      mPrefBranch->GetBoolPref(PromiseFlatCString(sysNumLinesKey).get(),
>+                               &useSysNumLines);


>+
>+          mPrefBranch->GetIntPref(PromiseFlatCString(numLinesKey).get(),
>+                                  &numLines);
>+        }

You can get rid of the PromiseFlatCString calls here.  Just call .get() on the
nsCAutoString.

>--- widget/src/gtk/nsWidget.cpp	6 Mar 2004 15:00:37 -0000	1.293
>+++ widget/src/gtk/nsWidget.cpp	13 Apr 2004 05:02:28 -0000
>+      if (aGdkButtonEvent->button == 4 || aGdkButtonEvent->button == 6)
>         scrollEvent.delta = -3;
>       else
>         scrollEvent.delta = 3;

I actually have no idea whether 3 is an appropriate delta for horizontal
scroll.  We can tweak it later though if needed.

r=bryner with those changes (no need to attach a new patch).

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 145986 [details] [diff] [review]
New new patch, with fixes addressing Brian Ryner's comments

Looks great.

Comment 44

[:deanis74]

Why did the firefox.js changes get checked in?  Comment 42 said they didn't need to.

Comment 45

[Mike Connor [:mconnor]]

this appears to have caused bug 241646

Comment 46

[Asa Dotzler [:asa]]

Comment on attachment 145986 [details] [diff] [review]
New new patch, with fixes addressing Brian Ryner's comments

I don't think we want to take this if it's causing regressions. Please
re-request approval when any problems have been sorted out (or if I've
misunderstood that last comment).

Comment 47

[Mike Connor [:mconnor]]

bug 241646 has r+sr now, someone needs to land that so we can verify its fixed.

Comment 48

[:deanis74]

And someone still needs to back out the firefox.js changes from this checkin.

Comment 49

[Mike Connor [:mconnor]]

actually, as far as I can tell, those changes didn't go in (I don't know enough
about CVS to know exactly what happened, but checking lxr/cvs log shows that the
prefs don't exist in firefox.js, and timeless isn't in the CVS log history)

Comment 50

[Steffen Wilberg]

Comment on attachment 145986 [details] [diff] [review]
New new patch, with fixes addressing Brian Ryner's comments

Rerequesting approval1.7 per comment 46. The regression (bug 241646) has been
fixed.

Comment 51

[:deanis74]

Huh.  I could have sworn I saw those go in, otherwise I wouldnt have raised a
stink.  But you're right, I don't see them in there anywhere either.

Comment 52

[Asa Dotzler [:asa]]

Comment on attachment 145986 [details] [diff] [review]
New new patch, with fixes addressing Brian Ryner's comments

a=asa (on behalf of drivers) for checking to 1.7. We have to get the fix for
bug 241646 too, if we're going to take this one.

Comment 53

[Stian Jordet]

Someone with the right permissions, please close this bug. The patch has been
checkin in on both the trunk and branch now :)

Comment 54

[Christian :Biesinger (don't email me, ping me on IRC)]

marking so

Comment 55

[Steffen Wilberg]

Worth an addition to the 1.7 release notes. Already listed on 1.8a release notes.
1.7-branch checkin was on 2004-05-03 15:49.

Comment 56

[Steffen Wilberg]

(Note to myself for future reference)
Open /etc/X11/XF86-Config-4 and change "Protocol" and "Buttons" to these values:

Section "InputDevice"
        Identifier      "USB Mouse"
        Driver          "mouse"
        Option          "Device"                "/dev/input/mice"
	Option		"SendCoreEvents"	"true"
        Option          "Protocol"              "ExplorerPS/2"
        Option          "ZAxisMapping"          "4 5"
        Option          "Buttons"               "7"
EndSection

Comment 57

[timeless]

*** Bug 241021 has been marked as a duplicate of this bug. ***