58589 - Enhance scrolling event to handle horizontal scroll

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Jeffrey S. Kobal]

Currently, the nsMouseScrollEvent only supports vertical scrolling; however, 
there are devices that have the capability of scrolling in both the vertical and 
horizontal directions.  This event structure can be extended to support this 
functionality (perhaps renaming it to nsScrollEvent to reflect the fact that 
non-mouse devices or even external programs can generate scroll messages), with 
a field that specifies whether the event is a horizontal or vertical scroll.

On OS/2, we handle WM_HSCROLL and WM_VSCROLL messages in nsWindow.cpp by calling 
OnHScroll and OnVScroll, and these methods construct and dispatch the 
appropriate nsMouseScrollEvent.

On Windows, I would expect a single-wheel mouse to send the WM_MOUSEWHEEL 
message, but a two-wheel mouse or trackpoint mouse would send probably 
WM_VSCROLL and WM_HSCROLL messages instead (the ones we have tested do this on 
OS/2).  Currently, the Windows code in nsWindow.cpp doesn't do anything with 
those messages; if you hook them up like we did in the OS/2 code, you can use 
the same nsMouseScrollEvent to implement both functions, with the WM_MOUSEWHEEL 
acting as a WM_VSCROLL.  Not only would this allow pointing devices to scroll in 
both directions, but it would also support any 3rd-party program that sends 
WM_HSCROLL messages to do scrolling.

Adding this capability to the XP event would also allow this functionality to be 
used by other platforms.

See bug #50597 for more background information on the code in question.

Comment 1

[Brian Ryner (not reading)]

My understanding was that WM_HSCOLL and WM_VSCROLL, on Windows, are sent for
scroll events on *native* scrollbars, and hence don't work with gfx scrollbars.
cc'ing rods since he knows more about that.

Comment 2

[Jeffrey S. Kobal]

WM_HSCROLL and WM_VSCROLL are normally sent to the window on behalf of the 
native scrollbar, but they are just messages that can come from other sources, 
such as a mouse driver or 3rd-party software.  Currently, these messages only 
work with the native scrollbar implementation (nsScrollbar) in Mozilla, but with 
my proposed enhancements, we can make them work with the XP scrollbars as well.

Comment 3

[Brian Ryner (not reading)]

Attachment: patch

Comment 4

[Brian Ryner (not reading)]

This patch adds a "flags" field to the mousewheel scroll event, and page scroll
and horizontal are the two flags present right now.

jkobal, you want to review?  scc, since you suggested this approach, can you sr=?

Comment 5

[Jeffrey S. Kobal]

Please make the following changes in widget/src/os2/nsWindow.cpp:

Remove the #if 0 and #endif in OnHScroll
Change deltaColumns to delta

---

I notice that you didn't actually implement the handling of the kHorizontal 
value on the XP side, so won't the horizontal scroll message cause a vertical 
scroll to happen with the current code?

Also, you didn't put in the code to handle the HSCROLL and VSCROLL on Windows, 
but I guess it would be up to that platform owner to decide how best to handle 
this.

Were the first set of changes in the gtk file intentional (looking at the button 
type)?  They don't seem to be relevant to this particular bug, though the code 
does have to do with mousewheel events.

Thanks again for doing this, by the way.

Comment 6

[Brian Ryner (not reading)]

I didn't intend to include the first part of the gtk patch... it's code I think
should be added, but not for this bug.

The fact that this isn't implemented on the XP side is precisely why I didn't
uncomment the section in the os2 code.  I'm just doing this as groundwork -- the
bug will remain open until the XP implementation is done.

Comment 7

[Jeffrey S. Kobal]

Ok, then removing the extraneous gtk mousewheel fix, and replacing 
"deltaColumns" with "delta" in the os2 file should do it.

Comment 8

[Brian Ryner (not reading)]

Attachment: patch #2

Comment 9

[Jeffrey S. Kobal]

Looks good to me.  r=[e-mail]

Comment 10

[Christopher Blizzard (:blizzard)]

I don't like the fact that a vertical scroll is the default assumption.  I would
rather have a flag that actually indicates that instead of it being assumed.  It
means that people reading the code have to know that 0 means vertical scroll.

Comment 11

[Scott Collins]

The code looks pretty good.  Name flag constants, though, as you would the
boolean variables they extract, e.g., |kIsPageScroll|, or |kIsFullPage|, etc.  I
don't know how exactly I would deal with blizzard's comment.  You _could_ adjust
the logic to |&| against, e.g., |kScrollDirectionMask| and compare the result to
|kIsVerticalScroll|, and |kIsHorizontalScroll|.  Perhaps a better answer would
be to pull the flag testing into tiny inline functions, e.g.,

  struct nsMouseScrollEvent {
    // ...
    PRBool IsFullPage() const { return scrollFlags & kIsFullPage; }

    PRBool IsVerticalScroll() const {
      return !(scrollFlags & kIsHorizontalScroll);
    }

    PRBool IsHorizontalScroll() const {
      return scrollFlags & kIsHorizontalScroll;
    }
    // ...
  }

(Assuming, of course, you changed your names like this)  These inlines will
generate exactly the same code you have now ... but may ease comprehension, and
if you use them appropriately, could answer blizzards concerns.

Comment 12

[Brian Ryner (not reading)]

Attachment: patch #3

Comment 13

[Brian Ryner (not reading)]

ok, trying this again.  this should address all of the concerns here... look
good to everyone?

Comment 14

[Scott Collins]

names fixed, blizzards concerns addressed.  good.  sr=scc

Comment 15

[Brian Ryner (not reading)]

jkobal, can you review this third patch also?

Comment 16

[Jeffrey S. Kobal]

I'm not sure why you'd define kIsHorizonal and kIsVertical to two distinct 
values instead of setting kIsVertical to zero, since the two flags are mutually 
exclusive.  Unless, of course, you want to beef up the scrollevent so that it
can take a delta in each direction?

Doesn't really matter, though.  r=[e-mail]

Comment 17

[Brian Ryner (not reading)]

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

Comment 18

[Michael Collette]

Been a while since I went back and looked over this bug.  I was the fella that
submitted Bug 21902 concerning my Logitech trakball under NT.  Back then it was
a little flaky, and the horizontal scroll would have been quite nice.  Today,
scrolling is totally inoperative with this device using a relatively current
version of the driver.  Outside of Mozilla, all my Windows application seem to
properly respond to this software based scroller.

Comment 19

[Matthias Versen [:Matti]]

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

Comment 20

[Brian Ryner (not reading)]

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

Comment 21

[Matthias Versen [:Matti]]

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

Comment 22

[Vaclav Dvorak]

Hmmm... The patch's second anniversary is approaching. Is anything new?

I have an interesting issue with my Memorex MX4350RF mouse that I'm not sure is
related. The description will probably be a little bit lengthy, sorry about that.

The mouse has the common two buttons, a clickable wheel in the middle, plus two
more buttons, one on each side, that are programmable. Among the functions that
can be programmed for them are "Horizontal Wheel" and "Auto Scroll". (I'm
talking about the Memorex driver in Windows 98.) The former is supposed to
toggle the wheel between vertical (the default) and horizontal scrolling. The
latter displays a NSEW artifact, which is then a reference point for
auto-scrolling in the direction the mouse cursor is placed with respect to it.
Ugh... that is probably not a good description, but hopefully most of you know
what auto-scroll means. You activate it, move the mouse a bit down and the
document magically scrolls down until either you move the mouse back up or you
deactivate the auto-scroll.

The mouse driver also has a "Wheel Mode" setting that has two radio buttons:
"System default (Intelli-Mouse) mode" and "Enhance scroll mode".

Now, the weird part. Horizontal scrolling in Mozilla works perfectly in both
modes, both with Horizontal Wheel and with Auto Scroll (although this might be
just because I have not encountered a horizontally scrollable widget yet).
However, vertical scrolling shows mixed results:
1) System default (Intelli-Mouse) mode: the wheel works fine, but AutoScroll
doesn't (although the very same AutoScroll works for horizontal scrolling).
2) "Enhance scroll mode": both the wheel and AutoScroll work, UNLESS there is a
vertically scrollable combo box on the page (perhaps it also applies to other
v-scrollable widgets). If yes, it picks one (the first one?) of such widgets and
it scrolls only this one widget, always. No matter where the cursor is when you
roll the wheel, and no matter if you click somewhere before scrolling, and no
matter if you use the wheel or AutoScroll.

Thanks for your attention!

Comment 23

[R. Pao]

FYI: Mozilla 1.2 under XP Pro on a Thinkpad with TrackPoint does not do
horizontal scroll with the "horizontal wheel."  Vertical scrolling works fine.

Comment 24

[Mike Kaply [:mkaply]]

bryner, any reason why this patch never went in?

Comment 25

[Brian Ryner (not reading)]

It did, I just never got around to marking it fixed...

Comment 26

[Luca Olivetti]

FIXED but not in 1.2.1: it isn't working (it randomly selects text in the page
instead of scrolling horizontally).

I have

Option "ZaxisMapping" "4 5 6 7"

in my XF86Config-4 file and xev confirms that the horizontal wheel is mapped to
buttons 6 and 7.

Comment 27

[Luca Olivetti]

tested 1.2.1 on windows and doesn't scroll horizontally either.

Comment 28

[Brian Ryner (not reading)]

If you look at what this bug is actually about, this only means that the
infrastructure is there for platforms where we have code present to handle
horizontal scrolling.  Currently, the only such platform is OS/2. On windows,
I'm not sure we have any way at all to get these events.  On Linux, we can
definitely look for the button presses but I'm not sure how to determine which
buttons are mapped to the horizontal wheel.

Comment 29

[Luca Olivetti]

In this case bug 98940 (general horizontal scrolling) and/or bug 21902 (logitech
trackball specific) have been incorrectly marked as a duplicate of this one and
should be reopened, while bug 120778 and bug 149086 should be duplicates of bug
98940 instead of this one.