607464 - Smooth scrolling + pixel scrolling + inertia = fail

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Tommy XP]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6) Gecko/20100101 Firefox/4.0b6
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6) Gecko/20100101 Firefox/4.0b6

scrolling with the page up and page down with smooth scrolling is very nice, but this causes problems with the magic mouse since it already does smooth scrolling automatically.
Turning smooth scrolling off fixes the magic mouse problems, but the keyboards scrolling no longer has smooth scrolling.

need option to turn them on/off individually

Reproducible: Always

Steps to Reproduce:
1.turn on smooth scrolling
2.scroll with magic mouse up and down
3.
Actual Results:  
scrolling with the magic mouse not fluid.

Expected Results:  
more fluid scrolling.

Comment 2

[Markus Stange [:mstange]]

This is a regression from bug 526394. At the moment there's nothing that checks for nsIScrollableFrame::SMOOTH at all...

Comment 3

[Markus Stange [:mstange]]

Oh, because INSTANT is now considered the inverse of SMOOTH.
We need a third scroll mode.

Comment 4

[Markus Stange [:mstange]]

Attachment: v1

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 505785 [details] [diff] [review]
v1

Thanks Markus!

     PRBool noDefer = aMouseEvent->scrollFlags & nsMouseScrollEvent::kNoDefer;
+    PRBool smooth = aScrollQuantity != nsIScrollableFrame::DEVICE_PIXELS;
     nsIntPoint overflow;
     frameToScroll->ScrollBy(nsIntPoint(scrollX, scrollY), aScrollQuantity,
                             noDefer ? nsIScrollableFrame::INSTANT
-                                    : nsIScrollableFrame::SMOOTH,
+                                    : (smooth ? nsIScrollableFrame::SMOOTH
+                                              : nsIScrollableFrame::NORMAL),

Instead of this nested ternary operator, how about something more like
  PRUint32 flags;
  if (aMouseEvent->scrollFlags & nsMouseScrollEvent::kNoDefer) {
    flags = nsIScrollableFrame::INSTANT;
  } else if (aScrollQuantity != nsIScrollableFrame::DEVICE_PIXELS) {
    flags = nsIScrollableFrame::SMOOTH;
  } else {
    flags = nsIScrollableFrame::NORMAL;
  }

Comment 6

[Markus Stange [:mstange]]

Attachment: for checkin

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 506350 [details] [diff] [review]
for checkin

I think we're better off taking this after 2.0 is out the door. If you strongly disagree please renominate and make a case for taking this change this late in this release.

Comment 8

[Markus Stange [:mstange]]

Attachment: with test

Re-requesting approval: This patch fixes a regression from Firefox 3.6 that makes scrolling with Mac touchpads needlessly sluggish if smooth scrolling is turned on. It's very small and only restores logic that was inadvertently removed in bug 526394, so it's very low-risk. It also has a test (as of now).

Comment 9

[Johnny Stenback (:jst)]

Roc, you cool with approving this for 2.0?

Comment 10

[Johnny Stenback (:jst)]

... and actually cc:ing roc. roc, please see previous comments.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 509370 [details] [diff] [review]
with test

Yes.

Comment 12

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/59642a869419