Closed Bug 574663 Opened 14 years ago Closed 14 years ago

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


(Core :: Widget: Cocoa, defect)

Not set



Tracking Status
blocking2.0 --- final+


(Reporter: ted, Assigned: mstange)


(Blocks 2 open bugs)



(4 files, 3 obsolete files)

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.
blocking2.0: --- → ?
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.
The momentum scrolling comes for free from the system - it's just repeatedly sending scroll events. 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.
Damon has one of these - maybe he can confirm for us?
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.
I can, maybe next week.
Yep.  Confirmed.
Assignee: nobody → mstange
blocking2.0: ? → -
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.
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.
blocking2.0: - → final+
Attached file 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.
Interesting. Can you add another log where you scroll down fast, and then while momentum scroll is still scrolling down, scroll up again?
Attached file test app log 2
as requested
Attached patch v1 (obsolete) — Splinter Review
Does this work?
It does! 

But it also causes momentum scrolling to stop while ctrl is pressed.
Attached patch v2 (obsolete) — Splinter Review
How about this?
Attachment #459112 - Attachment is obsolete: true
It works!
Attachment #459132 - Flags: review?(Olli.Pettay)
Attachment #459132 - Flags: review?(joshmoz)
Comment on attachment 459132 [details] [diff] [review]

>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.
Attachment #459132 - Flags: review?(Olli.Pettay) → review+
Attachment #459132 - Flags: review?(joshmoz) → review+
Attached patch with test (obsolete) — Splinter Review
Attachment #459132 - Attachment is obsolete: true
Closed: 14 years ago
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
Backed out because the test fails on Windows. Not sure why yet.
> 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
Resolution: FIXED → ---
Whiteboard: [backed out due to test failure on Windows]
Have you had a chance to look into why the test failed? Do you have a Windows environment to test in?
Attached patch with better testSplinter Review
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.
Attachment #461257 - Attachment is obsolete: true
(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!
Blocks: 605751

I'll file a new bug for the setTimeout/executeSoon ordering issue.
Closed: 14 years ago14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [backed out due to test failure on Windows]
Target Milestone: mozilla2.0b3 → mozilla2.0b8
Depends on: 613147
Blocks: 669979
Blocks: 767197
See Also: → 1199737
This bug still occurs in the latest Firefox version.
You need to log in before you can comment on or make changes to this bug.