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

RESOLVED FIXED in mozilla2.0b8

Status

()

defect
RESOLVED FIXED
9 years ago
9 months ago

People

(Reporter: ted, Assigned: mstange)

Tracking

(Blocks 2 bugs)

Trunk
mozilla2.0b8
All
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(4 attachments, 3 obsolete attachments)

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.
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.
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+
Posted 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?
Posted file test app log 2
as requested
Posted patch v1 (obsolete) — Splinter Review
Does this work?
It does! 

But it also causes momentum scrolling to stop while ctrl is pressed.
Posted 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]
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.
Attachment #459132 - Flags: review?(Olli.Pettay) → review+

Updated

9 years ago
Attachment #459132 - Flags: review?(joshmoz) → review+
Posted patch with test (obsolete) — Splinter Review
Attachment #459132 - Attachment is obsolete: true
Status: NEW → ASSIGNED
http://hg.mozilla.org/mozilla-central/rev/cd5c28912351
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

9 years ago
Duplicate of this bug: 582609
Whiteboard: [backed out due to test failure on Windows]
Duplicate of this bug: 606500
Have you had a chance to look into why the test failed? Do you have a Windows environment to test in?
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!
http://hg.mozilla.org/mozilla-central/rev/0eac6a562867

I'll file a new bug for the setTimeout/executeSoon ordering issue.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 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
Duplicate of this bug: 549669
Duplicate of this bug: 629362
Duplicate of this bug: 562413

Updated

6 years ago
Duplicate of this bug: 591587

Updated

6 years ago
Blocks: 767197

Updated

4 years ago
Duplicate of this bug: 600281
See Also: → 1199737

Comment 32

9 months ago
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.