574663 - When two-finger Scrolling with the trackpad (with momentum), pressing Ctrl while scrolling finishes causes page zoom

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[(not currently active) Ted Mielczarek]

On newer MacBooks with the multi-touch trackpad, there's a setting (on by default) to make two-finger scrolling scroll with momentum. I'm not sure if we have special code to handle this, or if we get it for free from the system.

If you two-finger scroll, the scroll continues for a bit after you remove your fingers (the momentum bit). However, if you press the Ctrl key while this scrolling continues, we translate it into page zoom, even if your fingers are no longer touching the trackpad. I find myself accidentally doing this all the time because I'll scroll down a page, then hit Ctrl+PgUp/PgDn to switch tabs.

Comment 1

[(not currently active) Ted Mielczarek]

I'm using a 32-bit trunk nightly on OS X 10.6.3. I just got this MacBook Pro about a week ago, it's one of the new Core i7 models.

Comment 2

[Markus Stange [:mstange]]

The momentum scrolling comes for free from the system - it's just repeatedly sending scroll events.
http://stackoverflow.com/questions/1849023/how-does-the-momentum-inertial-scroll-work-with-the-magic-mouse-on-nsscrollview#answer-1849870 says we discern normal scroll NSEvents from automatic repeated NSEvents by checking the scrollPhase field. Unfortunately I haven't found any documentation of that, and I don't have a device to test it.

Comment 3

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

Damon has one of these - maybe he can confirm for us?

Comment 4

[(not currently active) Ted Mielczarek]

Markus: if you can put together a sample xcode project or something, I'd be happy to build and run it and give you the results.

Comment 5

[Markus Stange [:mstange]]

I can, maybe next week.

Comment 6

[Damon Sicore (:damons)]

Yep.  Confirmed.

Comment 7

[(not currently active) Ted Mielczarek]

Joe: this is really visible and really annoying on newer MBPs. If you haven't seen it, go find someone that has one and try it out. I really think we should block on it. I hit it almost constantly while using my MBP, althought my workflow could be unique.

Comment 8

[Joe Drew (not getting mail)]

I have one of these MacBooks too! And a Magic Mouse which does the same thing. But I don't run into this at all.

However, I understand what you're saying, so let's say we'll block on this for now, since we always have the possibility to unblock on it later.

Comment 9

[Markus Stange [:mstange]]

Attachment: test app

Here's a test app that dumps the _scrollPhase and _continuousScroll fields to the console.
Can somebody with such a device run this for me, please?
1. Save as momentum.m
2. Go to the directory with Terminal
3. Run gcc -framework Cocoa -lobjc momentum.m -o momentum && ./momentum
   A gray window will open in the lower left corner of the screen.
4. Scroll over that window. Stuff will be dumped to the console.
5. Terminate with Ctrl+C.
6. Paste the output here.

Comment 10

[Joe Drew (not getting mail)]

Attachment: test app log

Comment 11

[Markus Stange [:mstange]]

Interesting. Can you add another log where you scroll down fast, and then while momentum scroll is still scrolling down, scroll up again?

Comment 12

[Joe Drew (not getting mail)]

Attachment: test app log 2

as requested

Comment 13

[Markus Stange [:mstange]]

Attachment: v1

Does this work?

Comment 14

[Joe Drew (not getting mail)]

It does! 

But it also causes momentum scrolling to stop while ctrl is pressed.

Comment 15

[Markus Stange [:mstange]]

Attachment: v2

How about this?

Comment 16

[Joe Drew (not getting mail)]

It works!

Comment 17

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

Comment on attachment 459132 [details] [diff] [review]
v2

>diff --git a/content/events/src/nsEventStateManager.cpp b/content/events/src/nsEventStateManager.cpp
>--- a/content/events/src/nsEventStateManager.cpp
>+++ b/content/events/src/nsEventStateManager.cpp
>@@ -2939,26 +2939,30 @@ nsEventStateManager::PostHandleEvent(nsP
>           nsContentUtils::GetBoolPref(sysNumLinesKey.get());
> 
>         if (useSysNumLines) {
>           if (msEvent->scrollFlags & nsMouseScrollEvent::kIsFullPage)
>             action = MOUSE_SCROLL_PAGE;
>         }
> 
>         if (aEvent->message == NS_MOUSE_PIXEL_SCROLL) {
>-          if (action == MOUSE_SCROLL_N_LINES) {
>+          if (action == MOUSE_SCROLL_N_LINES ||
>+              (msEvent->scrollFlags & nsMouseScrollEvent::kIsMomentum)) {
>              action = MOUSE_SCROLL_PIXELS;
>           } else {
>             // Do not scroll pixels when zooming
>             action = -1;
>           }
>         } else if (msEvent->scrollFlags & nsMouseScrollEvent::kHasPixels) {
>           if (action == MOUSE_SCROLL_N_LINES) {
>             // We shouldn't scroll lines when a pixel scroll event will follow.
>             action = -1;
>+          } else if (msEvent->scrollFlags & nsMouseScrollEvent::kIsMomentum) {
>+            // Don't do history scrolling or zooming for momentum scrolls.
>+            action = -1;
>           }
Why not combine 'if' and 'else if'? They both set action to -1

I don't know too much about OSX specific bits.

Comment 18

[Markus Stange [:mstange]]

Attachment: with test

Comment 19

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/cd5c28912351

Comment 20

[Markus Stange [:mstange]]

Backed out because the test fails on Windows. Not sure why yet.

http://hg.mozilla.org/mozilla-central/rev/bd4713e47e5e
http://hg.mozilla.org/mozilla-central/rev/8fbcb029ca0a

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280759437.1280762835.24943.gz#err0
> 36128 INFO TEST-PASS | /tests/content/events/test/test_bug574663.html | Normal scrolling shouldn't change zoom - 1 should equal 1
> 36129 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug574663.html | Normal scrolling should scroll - didn't expect 0, but got it
> 36130 INFO TEST-PASS | /tests/content/events/test/test_bug574663.html | Normal scrolling shouldn't change zoom, even after releasing the touchpad - 1 should equal 1
> 36131 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug574663.html | Normal scrolling should scroll, even after releasing the touchpad - didn't expect 0, but got it

Comment 23

[Justin Dolske [:Dolske]]

Have you had a chance to look into why the test failed? Do you have a Windows environment to test in?

Comment 24

[Markus Stange [:mstange]]

Attachment: with better test

I've looked into it now and I think I've found out what was wrong: I was using SimpleTest.executeSoon(f) instead of setTimeout(f, 0) when waiting for the scroll position to change. Mouse scroll events start a zero-interval timer (in nsGfxScrollFrameInner::ScrollTo) which updates the scroll position, and my check function was firing before the scroll position timer had a chance to fire.
The test seems to pass reliably now that I'm using setTimeout(f, 0). I've pushed the new patch to the tryserver (c80be47c7b8e); let's see what it says.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #24)
> Mouse scroll events start a zero-interval timer (in
> nsGfxScrollFrameInner::ScrollTo) which updates the scroll position, and my
> check function was firing before the scroll position timer had a chance to
> fire.

That's pretty odd. I would expect that since bug 512645 landed, |setTimeout(f1, 0); executeSoon(f2);| would result in f1 always being called first. setTimeout uses nsITimers under the hood, so that shouldn't be any different than your situation here. Worth adding a comment to your test ("can't depend on ordering of timers and executeSoon") if that really isn't the case. I'm kind of surprised it wouldn't have affected us elsewhere already - maybe it's the cause of some of our orange!

Comment 26

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/0eac6a562867

I'll file a new bug for the setTimeout/executeSoon ordering issue.

Comment 32

[pcampbell0]

This bug still occurs in the latest Firefox version.