241646 - Shift+Mousewheel (scrollwheel) behavior backwards, unintuitive not correctable with configuration

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Luke]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a) Gecko/20040424 Firefox/0.8.0+ (BlueFyre)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a) Gecko/20040424 Firefox/0.8.0+ (BlueFyre)

Prior to this build i was using a build from 
2004/04/14
and from around 0.6/0.7 -> that build 
shift+mousewheel up, was forward
shift+mousewheel down was backwards
the build starting on the date above does this backwards, opposite to other
windows software and is unintuitive.
Changing the mousewheel.withshiftkey.numline property to negative, makes "back"
work in the intuitive manner, but breaks forward (with mousewheel).

Reproducible: Always
Steps to Reproduce:
1. any URL
2. Navigate to new url
3. Use shift+mousewheel keyboard hotkey to navigate back to the original page
4. Use shift+mousewheel keyboard hotkey to navigate forward to the second page

Actual Results:  
to go back, the user must roll the mousewheel up while holding shift. and vice
versa for forward

Expected Results:  
a downward mousescroll should make the browser go back while shift is pressed.
And a forward mousescroll should make the browser go forward.

Using a microsoft wireless 5 button mouse

Comment 1

[Mike Connor [:mconnor]]

appears to be fallout from bug 64485

tested on 04/19 Mozilla (working) and 04/24 Mozilla (broken) and Firefox 04/24
(broken)

Comment 2

[Derek Upham]

Looks like this has been the behavior from the original Debian patch.

What are some other Windows software that demonstrate the opposite behavior?

Comment 3

[Mike Connor [:mconnor]]

this is a longstanding behaviour exhibited by Mozilla if you use
mousewheel.withshiftkey.action = 2.  This is a cross-platform issue and has
absolutely nothing to do with Windows vs. Linux.

If the debian patch breaks this, we need to either fix it or back it out, since
this is breaking default functionality in a major embedding app (Firefox)

Comment 4

[Derek Upham]

Attachment: Fix to numLines support

Here's a patch that fixes the handling of numLines.  From the comments:

// If the scroll event's delta isn't to our liking, we can
// override it with the "numlines" parameter.  There are two
// things we can do:
//
// (1) Pick a different number.  Instead of scrolling 3
//     lines ("delta" in Gtk2), we would scroll 1 line.
// (2) Swap directions.  Instead of scrolling down, scroll up.
//
// For the first item, the magnitude of the parameter is
// used instead of the magnitude of the delta.	For the
// second item, if the parameter is negative we swap
// directions.

Setting mousewheel.withshiftkey.numlines to -1 now works as expected.

Comment 5

[Mike Connor [:mconnor]]

so this works with the back/forward navigation through history as well? (sorry,
haven't read the code thoroughly, so I'm not sure how numlines affects this.

Comment 6

[Derek Upham]

Attachment: Fix numlines config support, fix default history direction

It makes it possible to configure the behavior in one's preferences.  That was
the low-hanging fruit in this bug, and getting it to work will let me set
things up the way I like.

The root bug was a flip from ">" to "<" in DoScrollHistory, which I probably
put in to get the other "horizontal" buttons to work by default like they do on
Windows.  Whatever Windows code is mapping buttons 6 and 7 to scroll through
the page history, it uses the opposite directions from the scroll wheel
"buttons".

So the attached patch:

(1) Interprets the scroll wheel history "direction" as before.
(2) Correctly supports negative "numLines" preferences, allowing you to flip
the meaning of the mousewheel direction.
(3) Sets *all* the horizontal mousewheel preferences to match Windows by
overriding the event's delta.  (Windows doesn't seem to pay attention to Alt,
Control or Shift there, so neither do the preferences.)
(4) Removes the default preference in which Shift-Mouse-4 and Shift-Mouse-5 to
not use the event's delta.  The event's delta now has the correct sign.

Comment 7

[[e-mail]]

Comment on attachment 147030 [details] [diff] [review]
Fix numlines config support, fix default history direction

>+          bool swapDirs = (numLines < 0);
>+          PRInt32 userSize = swapDirs ? -numLines : numLines;
>+
>+          PRInt32 deltaUp = (msEvent->delta < 0);
>+          if (swapDirs) {
>+            deltaUp = ! deltaUp;
>+          }
>+
>+          numLines = deltaUp ? -userSize : userSize;

Lets look at this code more closely.

First, take the case where numLines >= 0.
swapDirs = [false]
userSize = numLines
deltaUp = msEvent->delta < 0
numLines = deltaUp ? -userSize : userSize
	 = msEvent->delta < 0 ? -numLines : numLines

Now see what happens where numLines < 0.
swapDirs = [true]
userSize = -numLines
deltaUp = !(msEvent->delta < 0)
numLines = deltaUp ? -userSize : userSize
	 = !(msEvent->delta < 0) ? numLines : -numLines
	 = msEvent->delta < 0 ? -numLines : numLines

So you could have just written this:
if (msEvent->delta < 0)
  numLines = -numLines;

Comment 8

[Derek Upham]

The original code was:

     if ((msEvent->delta < 0) && (numLines > 0))
       numLines = -numLines;

with no comments.  I had to figure out what it was doing, and why it was doing
it wrong.  The new code uses many more temporary variables, but when I read it I
can immediately see where it corresponds to the elements of the comment,
extracting properties and applying them.

Comment 9

[Luke]

this bug seems to be resolved into yesterday's build

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a) Gecko/20040429
Firefox/0.8.0+ (BlueFyre)

Comment 10

[Steffen Wilberg]

Yes, it works fine again: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a)
Gecko/20040430 Firefox/0.8.0+ (Steffen). And comment 8 looks like there'll be no
corrections to this.
-> Derek.

Comment 11

[Steffen Wilberg]

-> fixed.
OS = All btw.

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 147030 [details] [diff] [review]
Fix numlines config support, fix default history direction

a=asa (on behalf of drivers) for checkin to 1.7

Comment 13

[timeless]

mozilla/content/events/src/nsEventStateManager.cpp 	1.495, 1.487.2.1
mozilla/modules/libpref/src/init/all.js 	3.519, 3.515.2.2