Support for continuous scrolling devices on the Mac.

Support for continuous scrolling devices on the Mac.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12488

[Avi (use Gerrit), 2009-03-25 17:12:20]

Peter: You did some scroll wheel work recently, so here you go.
Mark: FYI, for adapting to other projects.

[Peter Kasting, 2009-03-25 17:23:33]

Great comments.  Mostly LGTM, only one real note below.  I wouldn't call this
"smooth scrolling" as it's more like "handle continuous scrolling devices more
accurately"... true smooth-scrolling is going to need some WebKit modifications.

http://codereview.chromium.org/42607/diff/4/5
File webkit/glue/webinputevent_mac.mm (right):

http://codereview.chromium.org/42607/diff/4/5#newcode116
Line 116: WebMouseWheelEvent::WebMouseWheelEvent(NSEvent *event, NSView* view) {
Nit: Hey, while you're here, could you fix NSEvent * to be NSEvent*?

http://codereview.chromium.org/42607/diff/4/5#newcode189
Line 189: // For trackpads/Mighty Mouses (kCGScrollWheelEventIsContinuous != 0)
Nit: Mice?

http://codereview.chromium.org/42607/diff/4/5#newcode215
Line 215: // [1]
<http://developer.apple.com/documentation/Carbon/Reference/QuartzEventServices...>
Nit: Go ahead and break this URL at some path separator to avoid running over 80
columns.

http://codereview.chromium.org/42607/diff/4/5#newcode222
Line 222: wheel_ticks_x = round([event deltaX] / kCocoaTicksPerPhysicalTick);
Don't we want kCGScrollWheelEventDeltaAxis* here?  If we want raw physical
ticks...

http://codereview.chromium.org/42607/diff/4/5#newcode227
Line 227: CGEventGetIntegerValueField(cg_event,
kCGScrollWheelEventIsContinuous);
Nit: I would probably just put this call directly into the condition check...

[Avi (use Gerrit), 2009-03-25 17:33:54]

http://codereview.chromium.org/42607/diff/4/5
File webkit/glue/webinputevent_mac.mm (right):

http://codereview.chromium.org/42607/diff/4/5#newcode189
Line 189: // For trackpads/Mighty Mouses (kCGScrollWheelEventIsContinuous != 0)
On 2009/03/25 17:23:34, pkasting wrote:
> Nit: Mice?

The product is the Mighty Mouse; I wasn't sure how to pluralize it...

http://codereview.chromium.org/42607/diff/4/5#newcode222
Line 222: wheel_ticks_x = round([event deltaX] / kCocoaTicksPerPhysicalTick);
On 2009/03/25 17:23:34, pkasting wrote:
> Don't we want kCGScrollWheelEventDeltaAxis* here?  If we want raw physical
> ticks...

No we can't. For wheel mice, sure (does the spec say we have to disregard
acceleration?). The problem in in a continuous device, where we're scrolling by
pixels and there are no physical ticks.

This is a compromise. Can you point me to the relevant part of the spec?

[Avi (use Gerrit), 2009-03-25 17:54:28]

The fun begins...

http://adomas.org/javascript-mouse-wheel/ points out that many Mac browsers send
±0.1 rather than ±1. http://trac.dojotoolkit.org/ticket/3763 has entirely
different values.

Who are we trying to be like? Is there a clear spec?

[Peter Kasting, 2009-03-25 18:00:45]

On 2009/03/25 17:54:28, Avi wrote:
> The fun begins...
> 
> http://adomas.org/javascript-mouse-wheel/ points out that many Mac browsers
send
> ±0.1 rather than ±1. http://trac.dojotoolkit.org/ticket/3763 has entirely
> different values.
> 
> Who are we trying to be like? Is there a clear spec?

Basically, the DOM wheelEvent amount is supposed to match how Windows native
deltas work.  On Windows, a single wheel tick should give you a delta of +/-120.
 In WebCore, we multiply the wheelTicks field (of the event that we're setting
here) by 120 to obtain this.  So the goal is that one physical wheel tick should
give +/-1.

The end result is that web pages will have access, via JS, to the physical mouse
movement, but not to the logical (e.g. accelerated) scroll values.  Hyatt and I
have briefly talked about extending this event to provide both values to script
since one is useful when building scrolling UIs and the other is useful when
building something like a game that uses wheel ticks as an input.

[Peter Kasting, 2009-03-25 18:03:57]

On 2009/03/25 17:33:54, Avi wrote:
> http://codereview.chromium.org/42607/diff/4/5#newcode222
> Line 222: wheel_ticks_x = round([event deltaX] / kCocoaTicksPerPhysicalTick);
> On 2009/03/25 17:23:34, pkasting wrote:
> > Don't we want kCGScrollWheelEventDeltaAxis* here?  If we want raw physical
> > ticks...
> 
> No we can't. For wheel mice, sure (does the spec say we have to disregard
> acceleration?). The problem in in a continuous device, where we're scrolling
by
> pixels and there are no physical ticks.

Sure, but the "chunked values with no acceleration" description sounds like the
closest analog to physical ticks.  This matches what you would do on Windows
with devices that scrolled by smaller increments than 120; you accumulate the
values until your delta is >=120 and then send a chunked delta along.

[Peter Kasting, 2009-03-25 18:04:53]

On 2009/03/25 18:03:57, pkasting wrote:
> Sure, but the "chunked values with no acceleration" description sounds like
the
> closest analog to physical ticks.  This matches what you would do on Windows
> with devices that scrolled by smaller increments than 120; you accumulate the
> values until your delta is >=120 and then send a chunked delta along.

Though actually, that's no longer true... now, we would just send fractional
ticks here.

Argh.  What we really want are unchunked values without acceleration :(.

[Mark Mentovai, 2009-03-25 18:11:33]

LG

CGEvents make this doable from within Cocoa.  I like.  This used to be
horrendous.

http://codereview.chromium.org/42607/diff/2004/2005
File webkit/glue/webinputevent_mac.mm (right):

http://codereview.chromium.org/42607/diff/2004/2005#newcode164
Line 164: // For wheel mice (kCGScrollWheelEventIsContinuous == 0)
I'd say "notchy" or "line scrolling" or something in this heading in addition to
what you've already written.

http://codereview.chromium.org/42607/diff/2004/2005#newcode189
Line 189: // For trackpads/Mighty Mouses (kCGScrollWheelEventIsContinuous != 0)
I'd say "pixel" or "smooth scrolling" for this one.  If you were looking for a
place in your comment to say "balls," this might be it.

http://codereview.chromium.org/42607/diff/2004/2005#newcode215
Line 215: // [1] <http://developer.apple.com/documentation/Carbon/Reference/
Keep it on one line.  URLs are pretty much the only time you're allowed to
exceed 80 characters, and it's nice to be able to copy and paste.

http://codereview.chromium.org/42607/diff/2004/2005#newcode220
Line 220: // P.S. The "smooth scrolling" option in the system preferences is
utterly
Thanks for saying so, this confuses a lot of people (from experience working
with scroll balls).

http://codereview.chromium.org/42607/diff/2004/2005#newcode224
Line 224: static const float kCocoaTicksPerPhysicalTick = 0.1f;
double and no f

http://codereview.chromium.org/42607/diff/2004/2005#newcode229
Line 229: CGEventRef cg_event = [event CGEvent];
Can you #include something to make sure we have access to this?  <Cocoa/Cocoa.h>
only directly brings in Foundation, AppKit, and CoreData, none of which directly
brings in CoreGraphics.

http://codereview.chromium.org/42607/diff/2004/2005#newcode229
Line 229: CGEventRef cg_event = [event CGEvent];
Also, DCHECK this.

http://codereview.chromium.org/42607/diff/2004/2005#newcode238
Line 238: static const float kScrollbarPixelsPerCocoaTick = 40.0f;
double and no f again

http://codereview.chromium.org/42607/diff/2004/2005#newcode262
Line 262: static inline bool isKeyUpEvent(NSEvent* event)
Thanks :)

[Avi (use Gerrit), 2009-03-25 18:46:37]

http://codereview.chromium.org/42607/diff/2004/2005
File webkit/glue/webinputevent_mac.mm (right):

http://codereview.chromium.org/42607/diff/2004/2005#newcode164
Line 164: // For wheel mice (kCGScrollWheelEventIsContinuous == 0)
On 2009/03/25 18:11:33, Mark Mentovai wrote:
> I'd say "notchy" or "line scrolling" or something in this heading in addition
to
> what you've already written.

I'm changing the comment to refer to the physical unit of wheeling as a "notch".

[Avi (use Gerrit), 2009-03-25 18:48:39]

On 2009/03/25 18:04:53, pkasting wrote:
> Argh.  What we really want are unchunked values without acceleration :(.

The delta values will have to do. Take a look.

[Mark Mentovai, 2009-03-25 18:52:39]

http://www.lge.com/

[Peter Kasting, 2009-03-25 18:58:46]

LGTM.  Could you also send an upstream patch to modify Safari/Mac's calculation
of the wheel tick amounts to match this?  I wrote that code, it should probably
be kept in sync with this so we don't differ for web developers.

[Avi (use Gerrit), 2009-03-25 19:05:39]

On 2009/03/25 18:58:46, pkasting wrote:
> LGTM.  Could you also send an upstream patch to modify Safari/Mac's
calculation
> of the wheel tick amounts to match this?  I wrote that code, it should
probably
> be kept in sync with this so we don't differ for web developers.

Oh. Upstream they actually use an SPI to get the data I get, so I don't know
what to do there.

[Peter Kasting, 2009-03-25 20:06:19]

On 2009/03/25 19:05:39, Avi wrote:
> Oh. Upstream they actually use an SPI to get the data I get, so I don't know
> what to do there.

Can we do that here?  (We're in the browser process, it should be doable...
right?)  I just want to make sure web authors don't get different kinds of DOM
events from Mac Chrome vs. Mac Safari...

[Avi (use Gerrit), 2009-03-25 20:23:25]

Bug filed: http://bugs.webkit.org/show_bug.cgi?id=24813 .