347626 - Touchpad (two-finger) scrolling way too fast in trees

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060805 Minefield/3.0a1

Steps to reproduce:
1. Open bookmark manager.
2. Try scrolling up and down using two fingers on the touchpad.

Result: It moves way too fast, as if it's scrolling by a row every time it gets a "scroll by pixel" event.

This also happens in DOM Inspector, so I think this is a general tree widget problem.

I'm guessing this is a regression from bug 319078.

Comment 1

[Mark Mentovai]

Yup.  And this'll also affect any other widget that doesn't want to be scrolled by a pixel at a time, like autocomplete drop-downs.

Comment 2

[Mark Mentovai]

(And the mighty mouse too.)

Comment 3

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

New feature, not working as intended, so we'd like to get it right.

Comment 4

[Mark Mentovai]

Attachment: Possible patch

OK, here's the deal.  DOM events can't currently carry pixel scrolls.  (I guess they could, but we'd have to make that extension, and then we'd need to make anything that receives a DOM scroll event do something intelligent with pixel scrolls.)  And the tree widget and combo box don't want to scroll by pixels anyway.  (Grab their scroll bar thumbs to see what I mean.)

With that in mind, the theory here is to not attempt to do pixel-scrolling where it doesn't make sense, and instead report the event as unhandled, allowing the widget library to send in a fresh line-scroll event.  That's what this patch does.  In addition to rejecting pixel-scrolling for DOM events, it also rejects it for text size (control-scroll) and history (option-scroll), preventing those from scrolling too quickly, as well as forced single-line scrolling (command-scroll) achieved by setting usesysnumlines=false.

But there's a problem with this approach, that I'm hoping roc will deny, but I'm expecting he'll confirm.  In DoScrollText, we try to send the DOM event first, and only if its handlers don't call preventDefault will we try to scroll frames.  Because we get pixel-scroll events first, we'll NOT send the DOM event first, and instead will always try to pixel-scroll a frame.  Only if that can't be done will we then wind up with a line-scroll event that we'll try to send as a DOM event.

This can be a potential problem, but I'm not sure if it will cause trouble in the real world.  It doesn't have any ill effects in Firefox, as far as I can tell, but I suppose it would if someone stuck something like a tree widget inside a scrollable frame.  (But does anyone do that?)  So, I'm kind of expecting r- on this approach, but hoping for some helpful comments.

(Scroll events aren't real DOM events anyway, or so I've been told by some comments.)

Comment 5

[Mark Mentovai]

Seems like the xul tree never calls preventDefault on the scroll DOM event, so if one existed in a scroll frame, both would scroll anyway - so at least in that case, the possible patch on this bug wouldn't introduce any new bugs.

Comment 6

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

> Because we get pixel-scroll events first, we'll NOT send the DOM event
> first, and instead will always try to pixel-scroll a frame.  Only if that
> can't be done will we then wind up with a line-scroll event that we'll try to
> send as a DOM event.

So things that respond to DOMMouseScroll --- lists and trees, possibly more --- can't be scrolled by this two-finger scrolling method, it always just scrolls the innermost scrolling container? That doesn't seem too bad, but then, I'm not familiar with this UI.

Is there anywhere I can read about newfangled scrolling UI?

Comment 7

[Mark Mentovai]

Yeah, that's right.  But I have a feeling that we don't really have any cases like that.  At least for trees and lists, I wasn't seeing preventDefault set on the DOM event after dispatch, so scrolls over them would wind up scrolling BOTH the tree AND a containing scroll frame.  I assume someone would have noticed/filed a bug/fixed this if DOMMouseScroll responders ever lived inside scrollable frames.

If there were any cases like that, then with this patch, we'd wind up pixel-scrolling the scroll frame and not sending any DOMMouseScroll events.  The behavior would be different, but still wrong.

There's also the possibility that there are DOMMouseScroll responders out there that do live inside scroll frames and do call preventDefault.  (FWIW, the new Google Maps UI responds to DOMMouseScroll events and doesn't call preventDefault either, but Maps is careful to size itself to the enclosing window so there are no enclosing scroll views.  This patch does fix a "zooms too fast" problem with Maps.)

There isn't really any documentation on the UI, but implementation is covered at http://developer.apple.com/qa/qa2005/qa1453.html .  If you've got access to a 2005-or-later Apple laptop or a Mac with a Mighty Mouse running 10.4, you can play with the UI yourself.  Really, all it does is remove the "notchiness" of traditional mouse wheels, but the devil's in the details.  Instead of sending scroll events corresponding to lines of movement, it sends events for pixels, with appropriate acceleration and scaling.  If the application doesn't respond to or handle the pixel-scroll event, the system will store up pixel scrolls until they correspond to a line and then send a line-scroll - this is precisely what we'd take advantage of in the patch.  (When the system does this, it also considers things like direction and the time between wheel movements.)

Comment 8

[Mark Mentovai]

Attachment: Unnecessarily stressful and admittedly contrived testcase

Just to continue my previous thoughts, this puts Maps inside an iframe in a document large enough to be scrollable (downsize huge windows to make them scrollable if you don't see scrollbars when you load this).  As you can see, it's a problem even without considering pixel scrolling: both Maps and the enclosing page respond to mousewheel events simultaneously.  When you add this patch into the mix, we'll only scroll the enclosing page by pixel if we get a pixel scroll.  If we can't pixel scroll (because you try scrolling up when you're already at the top of the page), then Maps will get a scroll-up.

If Maps called preventDefault, everything in this example would behave properly with line-scrolling hardware, but if you have pixel-scrolling hardware, it would still be broken.

Comment 9

[Mark Mentovai]

One thought is to send a 0-line DOMMouseScroll when we get a pixel scroll, and if it comes back with preventDefault set, to avoid scrolling frames and instead report the event as unhandled so we'll get a fresh line-scroll event from widget.

Comment 10

[Mark Mentovai]

Attachment: Inner frame of preventDefault testcase

Comment 11

[Mark Mentovai]

Attachment: preventDefault testcase

The iframe accepts DOMMouseScroll events and calls preventDefault on them.

Comment 12

[Mark Mentovai]

Attachment: Better patch

This implements my previous suggestion.  When we get a pixel scroll, it will generate a DOMMouseScroll, but will set the detail field to 0 (meaning "don't scroll any lines.")  If the event has preventDefault called, then we'll assume that whateer handled the test event would have handled a real DOMMouseScroll as well, so we'll return eIgnore for the pixel-scroll event and expect to be called with a new line-scroll event.

It's possible, but unlikely, that handlers would not call preventDefault for detail=0 but would for nonzero values.

Unfortunately, this is broken with Maps, which seems to interpreting detail=0 as negative and is zooming in for all of the test events.  (Hmm...lemme see about that...)

Comment 13

[Mark Mentovai]

With a "fixed" maps, I'm seeing many events not making it all the way through.  The only patch that works consistently and reliably is the first one on this bug, although it does the wrong thing with the preventDefault testcase.

Comment 14

[Mark Mentovai]

roc, any thoughts?

Comment 15

[Mike Schroepfer]

Taking off the blocker list - but would happily take a low risk patch

Comment 16

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

1) What does Safari do?
2) Can we have a simple patch that just ignores the pixel-scroll events so that we fix this bug ASAP, and have a followup bug to actually support them properly?

Comment 17

[Mark Mentovai]

Safari doesn't send DOMMouseScroll events, it uses an IE-style onmousescroll handler.  It doesn't even send onmousescroll into an iframe.  Safari's onmousescroll event aren't cancelable, the event handler ALWAYS fires and the view ALWAYS scrolls.  So it seems that Safari's going to be about the same as what the first patch on this bug does.

Disabling pixel-scrolling for the time being would involve a single-line comment in widget/src/mac.

Comment 18

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

Let's do that first. I don't want to mess with the DOM mouse scrolling API without a clear plan. Maybe that plan should be to use onmousescroll instead.

Comment 19

[Mark Mentovai]

Attachment: Disable pixel scrolling

It pains me to have to do this.  I had grown accustomed to pixel-scrolling in content views...

Comment 20

[Mark Mentovai]

Patch checked in and pixel scrolling is disabled on the trunk.

Comment 21

[Mark Mentovai]

Filed bug 350471 to figure out a way to do pixel scrolling that doesn't cause problems.

Comment 22

[Mike Schroepfer]

Comment on attachment 235252 [details] [diff] [review]
Disable pixel scrolling

a=schrep ug.  Bummer that we can't keep this behavior for the content window....

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed on branch.