Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
42561:
Support 2d trackpad and mighty mouse scrolling on OS X. (Closed)
|
Created:
11 years, 9 months ago by nicothakis Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionSupport 2d trackpad and mighty mouse scrolling on OS X.
Patch Set 1 #Patch Set 2 : '' #
Total comments: 1
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 2
Patch Set 5 : '' #Messages
Total messages: 18 (0 generated)
http://codereview.chromium.org/42561/diff/1002/2001 File webkit/glue/webinputevent_mac.mm (right): http://codereview.chromium.org/42561/diff/1002/2001#newcode148 Line 148: delta_x = delta_lines_y; This isn't right. The desired behavior is: Vertical wheel = scroll vertical Shift + vertical wheel = scroll horizontal Horizontal wheel = scroll horizontal Shift + horizontal wheel = scroll horizontal Here you're just making shift swap the scroll axis. See code in WebKit's mac port or in Chromium's windows port for how horizontal scrolling is handled. (Note that shift + a 2d trackpad movement is wildly unclear... I suggest that perhaps shift should only convert vertical movement to horizontal when there is 0 horizontal component.)
Fantastic. A few comments: 1. Have you signed the CLA? I can't find a record that you've done so. Head over to http://code.google.com/legal/individual-cla-v1.0.html and fill in the forms. 2. Once you've signed the CLA, you're welcome to add yourself to src/AUTHORS. 3. I filed crbug.com/9006 about scrolling issues a week ago. What we're looking for is: What we're aiming for: 1. When using a mouse, scrolling the wheel should scroll vertically (up=up). 2. When using a mouse, shift+scrolling the wheel should scroll horizontally (up=left). 3. When using a trackpad/mighty mouse, both horizontal and vertical scrolling should work. 4. When using a trackpad/mighty mouse, pressing shift during a scroll should not affect the scrolling. Currently we fail 2, 3, and 4. I compiled your patch into my working copy and found that with it, we pass criterion 3, but still fail 2 and 4. Do you have an external mouse to work on this issue?
On 2009/03/24 15:17:22, pkasting wrote: > (Note that shift + a 2d trackpad movement is wildly unclear... I suggest that > perhaps shift should only convert vertical movement to horizontal when there is > 0 horizontal component.) Nope. Shift should not affect scrolling when the input device is capable of providing two-axis input. (See my bug/note, which is derived from empirical observations of behavior in Safari.)
On 2009/03/24 15:17:22, pkasting wrote: > http://codereview.chromium.org/42561/diff/1002/2001 > File webkit/glue/webinputevent_mac.mm (right): > > http://codereview.chromium.org/42561/diff/1002/2001#newcode148 > Line 148: delta_x = delta_lines_y; > This isn't right. The desired behavior is: > > Vertical wheel = scroll vertical > Shift + vertical wheel = scroll horizontal > Horizontal wheel = scroll horizontal > Shift + horizontal wheel = scroll horizontal > > Here you're just making shift swap the scroll axis. > > See code in WebKit's mac port or in Chromium's windows port for how horizontal > scrolling is handled. > > (Note that shift + a 2d trackpad movement is wildly unclear... I suggest that > perhaps shift should only convert vertical movement to horizontal when there is > 0 horizontal component.) Oh, I didn't think of those nudge-to-the-side mouse wheels. If shift-nudge should result in horizontal scrolling, then I need to do something else. So, important use cases: 1. Normal 2d trackpad/mighty mouse. This is done in the else branch and works. 2. Shift-mouse wheel should scroll horizontally (works as is). 3. Shift-side nudge should scroll horizontally (doesn't). What shift-trackpad should do is an academic issue, but it shouldn't be confusing. Your suggestion is potentially confusing: On a trackpad, it's very easy to accidentally scroll horizontally a bit when intending to scroll down. So there would be intermittent mode switches, which'd be strange. All other approaches I can think of are weird under certain circumstances as well: 1. Scroll horizontally by dx+dy (doesn't scroll on dy=-dx line) 2. Scroll by `if abs(dx) > abs(dy) then dx else dy` (weird if dx, dy have same absolute value but different sign) 3. Scroll by hypot(dx, dy), choose sign of value with larger absolute value Since they are all strange, I went with the simplest of those (`dx + dy`). For the three important use cases, all those methods work equally well.
On 2009/03/24 16:32:19, nicothakis wrote: > Oh, I didn't think of those nudge-to-the-side mouse wheels. Do we need to consider them? When I've used them on the Mac, the side nudge feature doesn't work by default. It should be the problem of whoever implements that at the driver level. > So, > important use cases: > > 1. Normal 2d trackpad/mighty mouse. This is done in the else branch and works. Correct. > 2. Shift-mouse wheel should scroll horizontally (works as is). This does not appear to work for me, either with or without your patch. > 3. Shift-side nudge should scroll horizontally (doesn't). I don't think side nudge should be addressed explicitly in this patch. > What shift-trackpad should do is an academic issue, but it shouldn't be > confusing. We should match what other apps do, which is ignore the shift key when handling input from two-axis devices.
That was supposed to be a comment, didn't intend to send just yet. Ignore.
> 1. Have you signed the CLA? I can't find a record that you've done so. Head over > to http://code.google.com/legal/individual-cla-v1.0.html and fill in the forms. I work at Google. But since that question comes up every time, I just signed a CLA anyway. > 3. I filed crbug.com/9006 about scrolling issues a week ago. What we're looking > for is: > > What we're aiming for: > > 1. When using a mouse, scrolling the wheel should scroll vertically (up=up). > 2. When using a mouse, shift+scrolling the wheel should scroll horizontally > (up=left). > 3. When using a trackpad/mighty mouse, both horizontal and vertical scrolling > should work. > 4. When using a trackpad/mighty mouse, pressing shift during a scroll should not > affect the scrolling. > > Currently we fail 2, 3, and 4. > > I compiled your patch into my working copy and found that with it, we pass > criterion 3, but still fail 2 and 4. Do you have an external mouse to work on > this issue? Just checked with an external mouse, 2 works for me. I implemented Peter's suggestion to ignore shift if delta_x != 0, this gives us all functionality necessary in practice. It fails on "shift-trackpad should scroll vertically", but I doubt anyone will notice. If you don't think that's acceptable, I'll check tonight if I can get a device identifier or similar from the event and explicitly ignore shift for trackpads and mighty mice.
On 2009/03/24 17:06:14, nicothakis wrote: > I work at Google. Oh! Sorry; saw your gmail account and didn't think to check. > Just checked with an external mouse, 2 works for me. That's really surprising. When I load a page and resize the window down, shift scroll doesn't work for me. I'm curious as to why. (I'm in the middle of something else but will look at it when I can.) > It fails on "shift-trackpad should scroll vertically", > but I doubt anyone will notice. > > If you don't think that's acceptable, I'll check tonight if I can get a device > identifier or similar from the event and explicitly ignore shift for trackpads > and mighty mice. It doesn't seem like Safari does device checking. If we look at WebCore/platform/mac/WheelEventMac.mm, we see that it is using an SPI called wkGetWheelEventDeltas, but surely we can do as well without it...
I have the solution. When you scroll-wheel on a mouse with the shift key down, Cocoa _automatically_ puts the delta in -[NSEvent deltaX] for you. Therefore it's wrong to have that if block at all. Try that.
Awesome, works.
...and thanks for the patch! http://codereview.chromium.org/42561/diff/9/4003 File webkit/glue/webinputevent_mac.mm (right): http://codereview.chromium.org/42561/diff/9/4003#newcode149 Line 149: delta_x = delta_lines_x; You don't need to set things up separately and then do straight assignments like this now that the conditional is gone. Just assign [event deltaX] right into wheel_ticks_x, then set delta_y to wheel_ticks_x * kScrollbarPixelsPerTick.
http://codereview.chromium.org/42561/diff/9/4003 File webkit/glue/webinputevent_mac.mm (right): http://codereview.chromium.org/42561/diff/9/4003#newcode149 Line 149: delta_x = delta_lines_x; On 2009/03/25 00:28:43, Mark Mentovai wrote: > You don't need to set things up separately and then do straight assignments like > this now that the conditional is gone. Just assign [event deltaX] right into > wheel_ticks_x, then set delta_y to wheel_ticks_x * kScrollbarPixelsPerTick. Done.
✔+
This is ready for checkin. How do you want to be attributed in the commit log? Usually it's "Patch by w <x@y.z>". Fill in the blanks.
On 2009/03/25 02:02:49, Mark Mentovai wrote: > This is ready for checkin. > > How do you want to be attributed in the commit log? Usually it's "Patch by w > <x@y.z>". Fill in the blanks. Patch by Nico Weber <nicolasweber@gmx.de>.
Committed r12433
Thanks all! |
