319078 - Handle smooth mousewheel (and two-finger touchpad) scrolling

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Mark Mentovai]

Bug 314856 comment 16 refers to a new DTS Q&A, http://developer.apple.com/qa/qa2005/qa1453.html .

Under Mac OS X 10.4 ("Tiger"), we can register for a non-notchy scroll event (kEventMouseScroll) that fires for hardware that supports smooth scrolling (including recent touchpads and Mighty Mice).  The deltas that this event provides are pixels to scroll.  Traditional mice with notchy wheels will continue to send kEventMouseWheelMoved events with line deltas.

In addition to providing an improved user experience, this will fix bug 309344 (confirmed in proof-of-concept, see that bug for more details).

Comment 1

[Mark Mentovai]

Attachment: Checkpoint

This is just a snapshot, it's still rough around the edges and isn't ready for review yet.  I haven't implemented anything to provide fallback carbon events for plug-ins.  As a result, while it fixes bug 309344, it breaks scrolling in things like the PDF Browser for now.

Comment 2

[Mark Mentovai]

Attachment: Smooth mousewheel scrolling with fallback event for plug-ins

This implements a kEventMouseScroll handler.  On receipt of a kEventMouseScroll event, the handler will synthesize a kEventMouseWheelMoved event with a new kEventParamMouseWheelSynthetic parameter set and send it back to the window.  This preserves compatibility with plug-ins that don't yet understand kEventMouseWheelScroll events.  We will need to notify plug-in developers that if they ever do update their plug-ins to handle kEventMouseWheelScroll, they will simultaneously need to begin dropping any kEventMouseWheelMoved events with the kEventParamMouseWheelSynthetic set, in order to avoid responding multiple times to a single user action.  Because kEventMouseWheelScroll was only documented in the past two weeks, there are probably no adversely affected plug-ins in the wild yet.

Steven, I'd like you to test this patch as well.  If you don't have a patchable Mozilla build available, I could provide one for you.

Comment 3

[Mark Mentovai]

Attachment: v3, smooth mousewheel scrolling with fallback event for plug-ins

v2 was missing a #define.

Comment 4

[Mark Mentovai]

This isn't yet handling the Java problem (bug 309344) as I thought it would - my initial tests were flawed (ugh).  More to come.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Attachment: Possible bug 309344 fix for JEP

Mark, here's a patch for JavaEmbeddingPlugin.bundle that _might_ fix bug
309344, with or without the changes you're working on here.

I can't (of course) test this patch myself ... since I don't have the "right"
kind of laptop.  Nor (for the same reason) do I think that there's much point
in my testing your own work, at least until you're almost ready to land it.

I thought about releasing a new JEP "nightly" that includes this change.  But
that'd be silly unless I had some way to know whether or not it works.

So whoever wants to test this patch will need to be able to build
JavaEmbeddingPlugin.bundle themselves.  See Building.txt in the JEP distro's
Source directory for instructions.  Testers should probably build only for
their own OS version -- it's _much_ simpler that way.

Please let me know your results, both with released versions of Firefox (or
Seamonkey) and with versions that include your latest patch for this bug
(319078).

Comment 6

[Mark Mentovai]

Comment on attachment 205493 [details] [diff] [review]
Possible bug 309344 fix for JEP

Steven, I'm unable to build a working JavaEmbeddingPlugin.bundle, patched or unpatched.

I'm wondering whether the proposed patch here will do the right thing and allow everyone to pick up the events.  While I was poking around with the debugger, I found that if I remove and reinstall the event handler in Mozilla, I can restore scrolling function, suggesting that the true problem is that someone's not calling CallNextEventHandler.

Comment 7

[Mark Mentovai]

Comment on attachment 205493 [details] [diff] [review]
Possible bug 309344 fix for JEP

Never mind, I was able to build the binary portion of the JEP bundle and use the prebuilt .jar.  The patch here is a suitable workaround and also functions with my proposed smooth-scrolling implementation.

This patch works whether it returns eventNotHandledErr or the result of CallNextEventHandler.  As I mentioned, I'm a little concerned about making sure that kEventMouseScroll events are getting through to Java if it (ever?) wants them.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

> Steven, I'm unable to build a working JavaEmbeddingPlugin.bundle, patched or
> unpatched.

> Never mind, I was able to build the binary portion of the JEP bundle and use
> the prebuilt .jar.

Take a look at Building.txt in the JEP distro's Source directory.  When
rebuilding from scratch, you need to specify certain OS-version-specific
*.java files (to cause them to be built).

> The patch here is a suitable workaround and also functions with my proposed
> smooth-scrolling implementation.

Very glad to hear it!  (There's also someone at bug 309344 who's had good
results with this patch ... though just with released browser versions.)

> As I mentioned, I'm a little concerned about making sure that
> kEventMouseScroll events are getting through to Java if it (ever?) wants
> them.

They're definitely getting through to Java ... and they always were (even
before my patch).

The problem is that, before my patch, the Carbon handlers installed for/by
Apple's Cocoa-Carbon interface were swallowing these events (i.e. marking them
as handled) -- _causing them not to be passed on to the browser_.  The new
drivers saw this and stopped sending kEventMouseWheelMoved events.

Without my intervention (in Handlers.m's HandleDispatcher()), _all_ mouse and
keyboard events get swallowed by Apple's Cocoa-Carbon interface.  I'd already
made provisions for kEventMouseWheelMoved events (and others) to be "resent"
(if need be) to the browser (using SendEventToTargetWithOptions() or
PostEventToQueue()).  But I hadn't yet (of course) made any provisions for the
new kEventMouseScroll events, which Apple only just documented in QA1453.

(Apple's Java implementation uses Cocoa, and sits on the "other side"
(opposite the Carbon-based browser) of Apple's Cocoa-Carbon interface.  It
doesn't have any problem receiving keyboard and mouse events.)

> This patch works whether it returns eventNotHandledErr or the result of
> CallNextEventHandler.

Interesting ... and possibly puzzling.  Do you know of any case where
CallNextEventHandler returns anything other than eventNotHandledErr on a
kEventMouseScroll event?

I've ordered a Mighty Mouse, and should get it in the next few days.  With
luck, its scroll wheel will demonstrate the same kind of problems that people
have had with Apple's newer trackpads.  Then I'll finally be able to start
doing my own tests on this stuff.

Comment 9

[Mark Mentovai]

(In reply to comment #8)
> Take a look at Building.txt in the JEP distro's Source directory.  When
> rebuilding from scratch, you need to specify certain OS-version-specific
> *.java files (to cause them to be built).

I had tried a few combinations of turning files on and off to no avail.  I'm working on 10.4.3 with Xcode 2.2.  We probably shouldn't clutter this bug with this problem, though - let's take it to another bug or private e-mail.

> Without my intervention (in Handlers.m's HandleDispatcher()), _all_ mouse and
> keyboard events get swallowed by Apple's Cocoa-Carbon interface.  I'd already
> made provisions for kEventMouseWheelMoved events (and others) to be "resent"
> (if need be) to the browser (using SendEventToTargetWithOptions() or
> PostEventToQueue()).  But I hadn't yet (of course) made any provisions for the
> new kEventMouseScroll events, which Apple only just documented in QA1453.

I hadn't realized that Apple was eating other event types too.

> (Apple's Java implementation uses Cocoa, and sits on the "other side"
> (opposite the Carbon-based browser) of Apple's Cocoa-Carbon interface.  It
> doesn't have any problem receiving keyboard and mouse events.)

OK, good, then this won't be a problem.

> Interesting ... and possibly puzzling.  Do you know of any case where
> CallNextEventHandler returns anything other than eventNotHandledErr on a
> kEventMouseScroll event?

Sure - if there's another kEventMouseScroll handler below on the stack (or attached to a less-specific target, like the application), and it handles the event.

With my smooth-scrolling patch applied, my handler is attached to the window, on the stack beneath yours.  CallNextEventHandler calls my event handler, which returns noErr, and therefore so will you.  On the other side of your handler, the system sees noErr, so no further action is taken.  That's fine, because you already called through to my handler.

> I've ordered a Mighty Mouse, and should get it in the next few days.  With
> luck, its scroll wheel will demonstrate the same kind of problems that people
> have had with Apple's newer trackpads.  Then I'll finally be able to start
> doing my own tests on this stuff.

I'll have a Mighty in a few days too, so I'll be able to work on this on the G5 instead of my PowerBook.

Comment 10

[Josh Aas]

// Plug-ins
// that are updated to understand kEventMouseScroll should also be modified
// to ignore kEventMouseWheelMoved events carrying the new parameter,
// to avoid responding multiple times to a single user operation.

If plugins understand kEventMouseScroll and don't know about this little hack in Gecko, they'll do bad things. If possible, we should try not to put the burden of having to know this on them.

What exactly are you waiting to test with your mighty mouse? Comment #9 indicates that you still have things to work on.

Comment 11

[Mark Mentovai]

The strategy under proposal was described in an e-mail to macplugin-dev a few weeks ago.  No responses were received.

Comment 12

[Josh Aas]

Comment on attachment 205461 [details] [diff] [review]
v3, smooth mousewheel scrolling with fallback event for plug-ins

Patch does not apply.

Comment 13

[Mark Mentovai]

Attachment: v4, v3 + up-to-date

v3 didn't apply after bug 111432 touched nsEventStateManager.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

I still haven't tested your patch, Mark (sorry I'm taking so long, but I was
away from my computer over Christmas, and I no longer have as much time to
devote to the Java Embedding Plugin as I used to).  But I'm now preparing
myself to do so (I've updated my copy of the Mozilla.org source from CVS and
am rebuilding Firefox).  So I should be able to test your patch sometime this
week (or possibly next weekend).

In the meantime I've revised my own patch for the Java Embedding Plugin.  Once
again, I hope to release a new JEP "nightly" sometime this week that includes
my workaround for what Apple did to trigger bug 309344 ... or possibly next
weekend.

The reason I'm writing this comment, now, is that I've found that Apple's
QA1453 seems to be partially inaccurate:  The following two paragraphs from it
seem to be largely false (at least as you and I have been interpreting them):

  When a kEventMouseScroll event is dispatched by the event dispatcher, it is
  sent to the window target that would have received the kEventMouseWheelMoved
  event in Panther (Mac OS X v10.3). However, if the window target does not
  handle the event, then the default HIObject handler will retrieve the
  kEventMouseWheelMoved event from the kEventParamEventRef parameter of the
  Scroll event, and send the MouseWheelMoved event to the window. This
  preserves compatibility with existing applications that only handle the
  kEventMouseWheelMoved event at the window level.

  If neither the kEventMouseScrolled nor the kEventMouseWheelMoved event is
  handled by the window event target, then the standard window event handler
  will send the kEventMouseScrolled event to the view under the mouse. If the
  view does not handle the event, then the default HIObject handler will send
  the corresponding kEventMouseWheelMoved event to the view under the
  mouse. This preserves compatibility with applications that only handle the
  kEventMouseWheelMoved event at the view level. If neither event is handled,
  then each event will be sent to the view's parent view, and so on up the
  control hierarchy.

To put it more succinctly, whether or not an event handler "handles" a
kEventMouseScrolled or kEventMouseWheelMoved event seems to have no effect on
which ones get sent to the various event targets and their handlers.  Instead,
it seems that Apple's "new" hardware (i.e. its Mighty Mouse and new trackpads)
always and only sends kEventMouseScrolled events, and that older hardware
(such as a Microsoft mouse with a scroll wheel) always and only sends
kEventMouseWheelMoved events.  (This is, of course, what happens with Apple's
newer driver(s) on OS X 10.4.X.  On 10.3 (and earlier?) both devices always
send kEventMouseWheelMoved events.)

Part of your patch assumes that it's (sometimes) important to indicate that
kEventMouseScrolled events haven't been handled -- even when they have been
(in these cases the handler would return nsEventNotHandledErr instead of
noErr).  My old patch (the one posted here as attachment 205493 [details] [diff] [review]) makes the
same assumption.

It's likely that following this assumption won't cause any harm, even if it's
false -- i.e. in many cases it won't matter whether a handler returns
nsEventNotHandledErr or noErr on a kEventMouseScrolled event.  But there might
be some cases in which it _does_ matter -- leading (perhaps) to the same event
being handled two or more times.

Why do I think that Apple's drivers don't pay any attention to whether or not
kEventMouseScrolled events have been "handled"?

1) My (revised) JEP patch still "works" even when I always return "noErr" on a
   kEventMouseScrolledEvent.

2) When I observe the flow of events through my JEP mouse event handler (on OS
   X 10.4.3), I see only kEventMouseScrolled events when I move the scroll
   ball of my MightMouse, and only kEventMouseWheelMoved events when I move
   the scroll wheel of my Microsoft mouse.  This is regardless of whether I
   have both mice attached at the same time, or just one, or just the other (I
   rebooted between each of these tests).

(These tests were done using Firefox 1.5.)

Comment 15

[Steven Michaud [:smichaud] (Retired)]

I've (finally) released a new version of the Java Embedding Plugin (0.9.5+c)
that fixes bug 309344:

http://javaplugin.sourceforge.net/

The patch being worked on here is still needed.  Or at least part of it is,
since Mozilla.org browsers don't yet support all aspects of Apple's new
"smooth scrolling".  But I hope that my fix for bug 309344 has made this work
less urgent.

Comment 16

[monkeypox37]

Any chance of rolling back the patch to 314856 ("Touchpad (two-finger) scrolling way too fast...") when you get this checked in?

I've been avoiding the nightlies post-314856 due to the much slower mousewheel scrolling but decided to see if it was still as annoying as ever in anticipation of Cairo-ization: yep. There's another bug which makes recent nightlies unusable, but I decided to dig out VPC and see how browsers on Windows handled mousewheel scrolling.

Win IE6 - about 3 lines per notch
Win FF 1.5 - about 3 lines per notch
Mac Safari - about 3 lines per notch
Mac FF nightlies - 1 line per notch

So... if this patch fixes the touchpad scrolling issues that users were having with 10.4 and FF then it'd be nice not to have Firefox on the Mac have much worse perceived scrolling than every other browser on the planet.

Comment 17

[Mark Mentovai]

Might want to do this for 1.8.1 platform polish?

Comment 18

[Darin Fisher]

Not going to block 1.8.1 for this, but we'll happily consider a baked-on-trunk patch.

Comment 19

[Mark Mentovai]

Attachment: v5

I decided that it was overkill to craft the kEventMouseWheelMoved event from a kEventMouseScroll event myself.  Plug-ins that take Carbon events themselves will either handle or not handle kEventMouseScroll - if they handle it, there's no reason for them to pass the event on to us, so we'll never know about it and won't turn it into a kEventMouseWheelMoved.  With that in mind, the nsMacWindow portion here is much simpler than in v4: I'll let the system turn kEventMouseScroll into kEventMouseWheelMoved, which we'll ignore.  I might even decide to drop that bit altogether.

Does anyone know of any Java applets that make use of the scroll wheel?  I used to, but I don't know where I found them.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

> Does anyone know of any Java applets that make use of the scroll
> wheel?

There's a SwingSet2 applet (very complex), one of Sun's JDK 1.4 demo
applets, which (in effect) runs other applets inside each of its
panes.

http://java.sun.com/products/plugin/1.4/demos/plugin/jfc/SwingSet2/SwingSet2.html

Each of these "panes" lets you display either an applet or its source
code.  All of the source code windows make use of the scroll wheel.
And so does the applet itself (the JEditorPane demo) in the fifth pane
from the left.

Comment 21

[Mark Mentovai]

Thanks.  I can't not handle the mouseWheelMoved at all after a mouseScroll, because after a Java applet is loaded in a window, JEP's handlers will cause both events to be delivered anyway.  The implementation in v5 avoids double-scrolling in this case.  Applets themselves are scrolled on Gecko events and don't rely on Carbon events, so there's no problem here.  Good.  Check.

v5 has bugs with scrolling PDFs displayed by Manfred's plugin in iframes: both the parent document and the iframe are scrolled.  This bug also exists on the 1.8.0 branch when only mouseWheelMoved is handled.  No worse now (and actually slightly better, because the parent document might scroll less).  Not bad.  Check.

The last thing I'd like to test is Flash, but I've been unable to get flash movies to respond to scroll events at all, with or without this change.  (Yes, I've clicked on the Flash movie to give it focus.)

I'm still considering tagging the event somehow before passing it on, to allow other consumers who need to know (or would like to know) the difference between hardware and converted mouseWheelMoved events to do so.

Comment 22

[Steven Michaud [:smichaud] (Retired)]

One thing you should be aware of (amd maybe are already):

When the mouse cursor is over a Java applet, the JEP doesn't change
the default behavior of Apple's Cocoa-Carbon interface -- it allows
the Carbon event handlers which run the Cocoa-Carbon interface to
translate both kinds of scrolling events into Cocoa events and swallow
them.

It's only when the mouse cursor isn't over any Java applet that the
JEP tries to redirect these events to the (Carbon-based) browser.

Comment 23

[Mark Mentovai]

Yup, thanks for the note.  I'm aware.

One thing you should know (but probably do already) is that when you've loaded Java in a window and then scroll the wheel over anything other than an applet, these messages are produced and wind up in the console:

2006-07-10 22:21:38.492 firefox-bin[3062] ERROR: carbonAppWindowMouseHandler(): could not create NSEvent for EventRef 0x44374ab0

I know it's not JEP's fault, it's coming from the conversion done by the Carbon-Cocoa interface's handlers, but I'll buy you the drink of your choice if you can make them stop!  (We probably owe you a few drinks already, if not entire meals.)

Comment 24

[Steven Michaud [:smichaud] (Retired)]

> 2006-07-10 22:21:38.492 firefox-bin[3062] ERROR: carbonAppWindowMouseHandler():
> could not create NSEvent for EventRef 0x44374ab0

Yeah, I know about those.  They're annoying, but they only appear on
OS X Tiger and (as far as I can tell) they're completely benign, so I
haven't tried to do anything about them.  Maybe Apple will eventually
do something to make them go away.

I'll still take a rain check on that drink, though :-)

Comment 25

[Steven Michaud [:smichaud] (Retired)]

> they only appear on OS X Tiger

Actually, they only appear on _Intel_ versions of OS X Tiger.

Bizarre.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

>> they only appear on OS X Tiger
>
>Actually, they only appear on _Intel_ versions of OS X Tiger.

Oops, I was wrong.  They only appear when I use the scroll ball on my
Mighty Mouse, or two-finger scrolling on my MacBook Pro (both only on
OS X Tiger) -- not when I use a conventional scroll wheel.

I suppose this is a clue about what the problem is.  But it'd probably
be quite difficult to solve (to find a workaround for), and probably
isn't worth much trouble.

Comment 27

[Michelle Sintov]

The Flash Player doesn't have scrolling turned on at all right now on the Mac. The plug-in currently doesn't process any Carbon events directly and instead takes all events from the browser. Is the fix for plug-ins to pick up Carbon event kEventMouseScroll, and will this cause no problem for Safari and Mozilla browsers? We can implement this in a future release.

Comment 28

[Mark Mentovai]

Michelle, I don't know if you can get the scroll events directly from the browser, because we wrap them up as EventRecords and there's no EventRecord that corresponds to mouse wheel scrolling.  If you can't see NS_MOUSE_SCROLL, you'll need to check for the Carbon events yourself.  When you implement the handlers, you should handle BOTH kEventMouseWheelMoved and kEventMouseScroll, because the latter is only generated with "smooth" (pixel-precision) scrolling hardware on Tiger (mighty mice and trackpads).  When you process either event, you should handle it and swallow it if Flash wants the scroll (if the pointer is over the movie, or if Flash is focused), otherwise, you should pass it into the handler chain to give the browser a crack.

Comment 29

[Josh Aas]

Comment on attachment 228734 [details] [diff] [review]
v5

looks fine to me, we should get it baking on the trunk asap.

if we're going to try to get this on the 1.8 branch, can we change the interface in nsIScrollableView.h?

Comment 30

[Darin Fisher]

Comment on attachment 228734 [details] [diff] [review]
v5

>Index: mozilla/widget/public/nsGUIEvent.h

> class nsMouseScrollEvent : public nsInputEvent
> {
> public:
>   enum nsMouseScrollFlags {
>     kIsFullPage =   1 << 0,
>     kIsVertical =   1 << 1,
>-    kIsHorizontal = 1 << 2
>+    kIsHorizontal = 1 << 2,
>+    kIsPixels =     1 << 3
>   };

It seems like roc or one of the "official" widget peers ought to
review widget API changes, right?


>Index: mozilla/widget/src/mac/nsMacEventHandler.cpp

>+  if (!resY && !resX)
>+    return PR_FALSE;
>+
>+  return PR_TRUE;
>+} // Scroll

    return (resY || resX);


I also think the scrollview changes should be reviewed by roc.

sr=me fwiw

Comment 31

[Mark Mentovai]

Comment on attachment 228734 [details] [diff] [review]
v5

roc, could you take a look?

I'd like this for 1.8 - do we need to maintain the nsIScrollableView interface there, even though it's defined in an .h?  I'm assuming that we do, and I'll come up with an alternative for that branch unless consensus is that it's not needed.

Comment 32

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

> enum nsMouseScrollEvent::nsMouseScrollFlags aAxis,

Lose the 'enum'

+  if (!resY && !resX)
+    return PR_FALSE;
+
+  return PR_TRUE;

"return resY || resX;"?

+// on 10.4.  #defines should override what's present in the SDK when defined,
+// because uses enums.

missing noun?

+  return ScrollTo(mOffsetX + dx, mOffsetY + dy, NS_VMREFRESH_SMOOTHSCROLL);

Do you really need SMOOTHSCROLL here?

 nsEventStateManager::DoScrollText(nsPresContext* aPresContext,
                                   nsIFrame* aTargetFrame,
                                   nsInputEvent* aEvent,
                                   PRInt32 aNumLines,
                                   PRBool aScrollHorizontal,
-                                  PRBool aScrollPage)
+                                  PRBool aScrollPage,
+                                  PRBool aScrollPixels)

I'd prefer an enum of (pages, pixels, lines) instead of two booleans. Or just name the enum with  MOUSE_SCROLL_* and use that.

I do think you should define nsIScrollableView_MOZILLA_1_8_BRANCH for branch. QI to it using CallQueryInterface since views aren't refcounted.

Comment 33

[Mark Mentovai]

Attachment: v6, address roc's comments

This version addresses roc's comments.  I added a new enum for nsEventStateManager::DoScrollText's argument, because the existing enum didn't exactly match DoScrollText's capabilities.

Comment 34

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

Comment on attachment 229866 [details] [diff] [review]
v6, address roc's comments

looks good. You just need to fix this comment:

+// on 10.4.  #defines should override what's present in the SDK when defined,
+// because uses enums.

Comment 35

[Mark Mentovai]

Comment fixed.  Checked in on the trunk.

Comment 36

[Mark Mentovai]

Attachment: 1.8 branch version

Comment 37

[Mark Mentovai]

Comment on attachment 229974 [details] [diff] [review]
1.8 branch version

This improves the Mac user experience on Tiger with recent laptops or Mighty Mice.  Once you've used pixel-scrolling for a while (as anyone who's used any of Apple's apps will have), notchy line-scrolling feels barbaric.

Comment 38

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

Comment on attachment 229974 [details] [diff] [review]
1.8 branch version

a=drivers, please land on the 181branch

Comment 39

[Mark Mentovai]

Checked in on MOZILLA_1_8_BRANCH before 1.8.1b2.

Comment 40

[Jesse Ruderman]

Smooth two-finger touchpad scrolling was disabled due to bug 347626.  Bug 350471 covers re-enabling it.