462809 - Interpretation of scroll events on Windows and OS X

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

We seem to currently be treating scroll events differently between OS X and Windows, where on OS X a quick succession of events can trigger scroll by page, and on Windows we only do line scrolling.  For an completely unscientific example, it takes me ~7 flicks of the scroll wheel on my mouse to go from the top to the bottom of this document on OS X, and around ~30 similar flicks of the mouse wheel on Windows.

http://www.ietf.org/rfc/rfc1738.txt

I think we should use the native OS X behavior on all platforms, since it makes scrolling in large documents considerably more manageable, and also gives the user the very real perception that Firefox is faster (even though we haven't actually changed anything other than the interpretation of scroll events).

(Not entirely sure if Widget:Win32 is the correct location for this bug, please feel free to move to the correct location).

Comment 1

[Ria Klaassen (not reading all bugmail)]

Firefox appears to use the system settings on Windows. If I want to scroll one page in 7 rolls I need to set it to 4. I'm using an extension which makes the wheel scroll in 5 steps but it also scrolls very slow and smoothly (like an autocue). https://addons.mozilla.org/en-US/firefox/addon/5846

Comment 2

[Markus Stange [:mstange]]

Gecko doesn't do any mouse wheel acceleration on OS X. Maybe the behaviour you're seeing is a feature of your mouse driver?
Macbook touchpads also use acceleration, but that's not something that Gecko has any control over.

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

>Maybe the behaviour you're seeing is a feature of your mouse driver?

In that case I guess the bug shifts to "copy Apple's generic mouse driver behavior for scrolling on Windows."  Using Vista, IE8 and a Microsoft bluetooth mouse, it takes ~30 scrolls to go from the top to the bottom of the document.  I think this is a situation where we can take Apple's superior approach and apply it to all platforms to create a better experience relative to those platform's native browsers.

Comment 4

[Ria Klaassen (not reading all bugmail)]

I found some settings that Firefox can use to change this behaviour:

user_pref("mousewheel.withnokey.numlines", 5);
user_pref("mousewheel.withnokey.sysnumlines", false);

but I doubt if anyone will find this comfortable. It scrolls with huge leaps and I think it can only be used in combination with smooth scrolling.

Comment 5

[:Margaret Leibovic]

I tried changing these preferences and didn't notice a difference, even when I set mousewheel.withnokey.numlines to be very large (I tried various values ranging up to 30). This happened to me running Firefox 3.5 and Minefield 3.6a1pre on both Mac OSX and a Windows XP VM. Bug 405573 describes this problem. Has anyone else experienced this?

Comment 6

[:Margaret Leibovic]

Attachment: scroll-v1

I used a timeout function to detect a series of scroll wheel notifications (the original code handles each notification individually). Then I used the index of the notification within a given series to adjust for an acceleration effect. Thus if a notification is the first in a series, it does not scroll far, but each consecutive notification will result in an increasingly scaled scroll.

This is a very preliminary attempt at imitating the Mac OSX scroll model, but it seems to be an improvement.

Comment 7

[:Margaret Leibovic]

Attachment: scroll-v2

Updated patch to include preferences for scrolling acceleration. These prefs determine when acceleration starts, how much of an effect acceleration has, and how many lines we scroll by before acceleration starts. 

In order to restore previous scrolling model, "mousewheel.acceleration.start" must be set to -1 and "mousewheel.speed.numlines" must be set to 3. Maybe there should be one preference to restore the older behavior?

Comment 8

[:Margaret Leibovic]

Attachment: scroll-v3

Updated patch to modify default values for "mousewheel.withnokey.sysnumlines" and "mousewheel.withnokey.numlines" instead of changing number of lines scrolled with preference that modifies nsWindow::ComputeMouseWheelDelta.

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 390067 [details] [diff] [review]
scroll-v3

>+  nsresult rv;
>+  nsCOMPtr<nsIPrefBranch> prefBranch = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // point at which acceleration starts: integer refers to mousewheel click
>+  // acceleration can be turned off if pref is set to -1
>+  PRInt32 accelerationStart;
>+  rv = prefBranch->GetIntPref("mousewheel.acceleration.start", 
>+                              &accelerationStart);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // factor to be muliplied for constant acceleration
>+  PRInt32 accelerationFactor;
>+  rv = prefBranch->GetIntPref("mousewheel.acceleration.factor", 
>+                              &accelerationFactor);
You will likely want to cache these values and become a pref observer.  Hitting the pref service on every scroll event seems expensive.

With that change, you'll want to get review from either vlad or roc.

Comment 10

[:Margaret Leibovic]

Attachment: scroll-v4

Comment 11

[Dirkjan Ochtman (:djc)]

I've been using the SmoothWheel extension for years, might make for a good reference. http://smoothwheel.mozdev.org/

Comment 12

[:Margaret Leibovic]

Attachment: scroll-v5

I added a pref observer for acceleration prefs so that we don't check the prefs on every scroll. I also changed mousewheel.withnokey.sysnumlines and mousewheel.withnokey.numlines back to original defaults because flipping mousewheel.withnokey.sysnumlines to false causes code in nsEventStateManager.cpp to override the delta I computed. So for now slow scrolling (before acceleration kicks in) still moves 3 lines per scroll tick, as opposed to 6. We could use mousewheel.withnokey.numlines to change this value in nsWindow::ComputeMouseWheelDelta, but I don't know if we need to do that right now.

Comment 13

[Ria Klaassen (not reading all bugmail)]

I'm using the "Yet Another Smooth Scrolling" extension, which has more possibilities than the first one (can be fine tuned with a panel). Either you like it or you hate it, this way of scrolling. The behavior can even be applied to keys, but because keys has less feeling, you have to problem that if you press the down arrow quickly three times in succession, it passes the targeted spot. So you have to wait between each push. Sorry for my horrible English.

Comment 14

[:Margaret Leibovic]

Attachment: numlines-v1

Uses scrollwheel.withnokey.numlines pref to change number of lines of scrolling before acceleration computation. I changed the default to 6 lines, since that's what Alex and I discussed doing before. This patch also cleans up some of the computations from the previous patch to account for users possibility entering silly pref values (like negative acceleration), so if we don't want to include this numlines pref, I should probably go edit the previous patch.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 390968 [details] [diff] [review]
scroll-v5

Looks great, but a few problems with the observer pieces...

>+// for scroll acceleration
>+nsScrollPrefObserver* nsWindow::sScrollPrefObserver = nsnull;

>+
>+  // Init scroll pref observer for scroll acceleration
>+  sScrollPrefObserver = new nsScrollPrefObserver();

The scroll pref observer is static, but here you're creating a new one per nsWindow, and leaking the old one.  At the very least you should check if one already exists before creating it, but even that will leave a problem at shutdown -- this memory will be "leaked" and will probably show up on the tinderboxes.  It doesn't /really/ matter, but we should do some cleanup.  Unfortunately I don't have any good suggestions on where that cleanup should go.  robarnold might have some ideas, if we have a decent place to put in toolkit init/shutdown code.

>+/**
>+ * Scroll pref observer class for scroll acceleration
>+ */
>+class nsScrollPrefObserver : public nsIObserver
>+{
>+public:
>+  nsScrollPrefObserver();
>+  int GetScrollAccelerationStart();
>+  int GetScrollAccelerationFactor();
>+
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIOBSERVER
>+
>+private:
>+  nsCOMPtr<nsIPrefBranch2> mPrefBranch;
>+  int mScrollAccelerationStart;
>+  int mScrollAccelerationFactor;
>+};

This class doesn't need to be in the header file -- it's only really used in nsWindow, right?  So I'd move it into the .cpp file, and probably just make the observer a global variable in that file instead of static on nsWindow.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 391209 [details] [diff] [review]
numlines-v1

This part looks fine, though the observer changes will depend on how it's fixed in the previous patch.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Whoops, this is in the static init part of nsWindow::nsWindow, so that's fine -- we just need the corresponding delete in the static part of ~nsWindow (Global shutdown piece).

Comment 18

[:Margaret Leibovic]

Attachment: scroll-final

Made fixes and combined patches.

Comment 19

[:Margaret Leibovic]

Attachment: scroll-final-v2

Oops, I forgot to delete some #include statements from the header file.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 391496 [details] [diff] [review]
scroll-final-v2

Looks fine, thanks!

Comment 21

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

tryserver builds are up https://build.mozilla.org/tryserver-builds/[e-mail]-try-913c7c2eb5e6/

As a side note, using this in a VM with a mighty mouse, the acceleration is pretty extreme.

Comment 22

[Shawn Wilsher :sdwilsh]

Alex - if you can get to the ui-review soonish, zpao or I can get this landed in time for the alpha.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 391496 [details] [diff] [review]
scroll-final-v2

uir+; this is the right thing to be doing, we can tweak the pref in a followup bug if we think the specific values are wrong; I'd rather see this land sooner rather than later

Comment 24

[Ria Klaassen (not reading all bugmail)]

This code doesn't change the behavior of the YASS extension.

Comment 25

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

pushed http://hg.mozilla.org/mozilla-central/rev/d8143262d94d

Comment 26

[Alex Faaborg [:faaborg] (Firefox UX)]

>Alex - if you can get to the ui-review soonish, zpao or I can get this landed
>in time for the alpha.

sorry about the lag, looks like beltzner beat me to the review and I agree that getting feedback on this (or people just subconsciously thinking we made major perf improvements) is great.

Comment 27

[IU]

Ok I know I shouldn't be posting here, but something to consider would be exposing an option (in Tools --> Options) for controlling the scroll rate.  This is now annoyingly fast on Windows.

Comment 28

[:Margaret Leibovic]

There are prefs available that you can access from about:config. Here's documentation about what each of them do: https://wiki.mozilla.org/Firefox/Sprints/Perception_of_Performance#Design:_Acceleration_Model_for_Scrolling

Comment 29

[IU]

(In reply to comment #28)
> There are prefs available that you can access from about:config. Here's
> documentation about what each of them do:

Thanks.  That wiki page is in error.  The about:config options are not scrollwheel...

Instead, they should be mousewheel...

Comment 30

[:Margaret Leibovic]

Oops! Thanks for pointing that out. I just fixed the wiki page.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Why is the patch using int and not PRInt32?

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

When is scrollTimer released or does the patch cause shutdown leak?

Comment 33

[Shawn Wilsher :sdwilsh]

(In reply to comment #31)
> Why is the patch using int and not PRInt32?
probably overlooked in review - file a followup please?

(In reply to comment #32)
> When is scrollTimer released or does the patch cause shutdown leak?
That would show up in leak logs in automated testing, wouldn't it?  But, this is the important code to do that:
>@@ -406,16 +516,19 @@ nsWindow::~nsWindow()
>     if (sIsOleInitialized) {
>       ::OleFlushClipboard();
>       ::OleUninitialize();
>       sIsOleInitialized = FALSE;
>     }
>     // delete any of the IME structures that we allocated
>     nsIMM32Handler::Terminate();
> #endif // !defined(WINCE)
>+
>+    gScrollPrefObserver->RemoveObservers();
>+    NS_RELEASE(gScrollPrefObserver);

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

> (In reply to comment #32)
> That would show up in leak logs in automated testing, wouldn't it?
Why would it? Are there any automated tests for this code? (I don't think it is even possible to write tests for it.)

Comment 35

[Shawn Wilsher :sdwilsh]

I think you actually may be right, but I see you've already filed a bug on it.

Comment 36

[Sylvain Pasche]

That should explain the leaks I've seen recently:

   0 TOTAL                                          23      132   618101        2 ( 2578.12 +/-  2428.11)  1208798        2 ( 1664.83 +/-  2385.21)
 593 nsThread                                       72       72        8        1 (    4.27 +/-     2.25)     6457        1 ( 1130.19 +/-   621.12)
 597 nsTimerImpl                                    60       60      299        1 (   43.54 +/-    11.55)     1284        1 (   83.79 +/-    23.69)

It happens if you scroll on a page.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I suggest that the patch should be backed-out.

I think that the patch took wrong approach. You want to accelerate *only* the scrolling speed, right? But the patch doesn't so. The wheel delta value is just used for scrolling, not the meaning is scrolling amount.

I suggest that you should implement the accelerating code in nsMouseWheelTransaction which is implemented in content/events/src/nsEventStateManager.cpp. That manages the scrolling transaction by mouse wheel events. So, you can implement the app level acceleration in XP level. And you can escape form changing the wheel event specification changes.

If you do so, you can fix bug 508747 automatically.

And also I found two problems on the current acceleration implementation. You should be careful to them.

1. The delayed wheel messages do unexpected high speed scrolling.

The mouse messages comes for short term if the system or the process is busy in some reasons. Even if the actual user operation is slow.

2. The acceleration is not needed in small scrolling box, e.g., listbox.

I troubled to scroll some listboxes by wheel. The listbox doesn't have enough lines in the box. So, the 6 lines and the acceleration is too high speed for such small scrollable box. You should check the scrollbox's size before accelerating. You can check it if you implement it in nsMouseWheelTransaction.

Comment 38

[Peter Lairo]

I don't know if this is a mere matter of "tweaking". Could be, but the scroll speed is way too fast in recent nightly builds(1). I can't control scrolling anymore (despite trying various mouse driver settings). I can't read long web pages without speeding past where I wanted to go. 

The scroll speed should respect the OS's behavior and also respect the user-selected driver setting (e.g., 1 line, 3 lines, and acceleration). I do *not* think we should use the native OS X behavior on all platforms, as the OP requested.

Please back out the patch for this bug.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090807 Minefield/3.6a1pre (.NET CLR 3.5.30729) Creative ZENcast v1.02.10

Comment 39

[Alex Faaborg [:faaborg] (Firefox UX)]

>the scroll speed is way too fast in recent nightly builds(1).
>I can't control scrolling anymore

It would be really useful for people who find this change problematic to provide details on the physical mouse they are using, and the driver name/version number.  If your driver is providing it's own acceleration model, you may be experiencing double acceleration, which wasn't intended.  Obviously developing on Windows is a bit more complicated given the wide range of hardware and compatibility issues (which is why we landed the change, to test this approach in the wild, and figure out what the contingencies are).

For example, I've personally tested with these two (pretty standard and boring) configurations:

OS: XP
Mouse: Microsoft Intellimouse Optical
Driver Name: HID Compliant mouse
Driver Creator: Microsoft
Driver Version: 5.1.2600.0

OS: Vista
Mouse: Microsoft Bluetooth Notebook mouse 5000
Driver Name: HID Compliant mouse
Driver Creator: Microsoft
Driver Version: 6.0.6001.18000

Effects: In both cases I didn't experience any problems with the new acceleration model.

Comment 40

[Alex Faaborg [:faaborg] (Firefox UX)]

Here is a quick description of how to get this information: http://people.mozilla.com/~faaborg/files/daf/mouseDriver.png

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm using:

OS: Vista x64
Mouse: Microsoft Explorer Mouse
Driver Name: Microsoft USB Wireless Mouse (IntelliPoint)
Driver Creator: Microsoft
Driver Version: 6.30.189.0

I disabled the mouse driver owning accelerator. But the app level accelerated scrolling speed is very fast. I'm experienced in operation without acceleration. Therefore, I turn the wheel fast if I want to scroll many lines. But the current trunk build denies the my operation and my experience which is needed in other applications.

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I moved the code to XP level at bug 512235, so, bug 509257 might be gone, please check it (I cannot look the detail).

Comment 43

[Nochum Sossonko [:Natch]]

I think this needs some additional tweaking, the default levels seem to scroll way too fast (almost feels like Chrome when it first came out) and accelerates too eagerly. I found setting:

mousewheel.withnokey.numlines, 3
mousewheel.acceleration.factor, 8

to be slightly better.

Comment 44

[Gary [:streetwolf52]]

Just wanted to mention that I had to disable accelerated scrolling after I started using a Microsoft Explorer Mouse (wireless) because pages would scroll a few lines all by themselves.  This action is intermittent.  I set these two Config options to -1.

mousewheel.acceleration.factor
mousewheel.acceleration.start


Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100805 Minefield/4.0b4pre - Build ID: 20100805141418