Bug 29601 – Slow mouse wheel scrolling fails in message list in Yahoo! Mail

• Component: DOM
• Status: ASSIGNED

[David Kilzer (:ddkilzer)]

* SUMMARY
Scrolling slowly with the mouse wheel (using a wired Apple Mighty Mouse) in the message list in Yahoo! Mail fails on Safari and WebKit nightly builds.  Works in Firefox 3.5.3.

* STEPS TO REPRODUCE
1. Log into Yahoo! Mail.
2. Position mouse over list of messages (assuming you have enough to scroll).
3. Use the mouse wheel to scroll messages up or down.

* RESULTS
Scrolling slowly produces no mouse movement.  You must move the mouse wheel rapidly to get any effect.

* REGRESSION:
Unknown.  Only tested with Safari 4.0.3 in SnowLeopard10B504 (10.6.1).

* NOTES
Works fine in Firefox 3.5.3.

[Brian Weinstein]

Is this only with the mighty mouse?

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> Is this only with the mighty mouse?

I haven't tested with any other mice...because I don't own any others.  Also, I've only tested this on Mac OS X 10.6.1.

[David Kilzer (:ddkilzer)]

<rdar://problem/7453254>

[Adele Peterson]

I can reproduce with two fingered scrolling too.

[Adele Peterson]

Yahoo is checking for a wheelDelta greater or equal to 120 before scrolling.  IE only produces wheelDeltas in increments on 120, so I'm sure that's why.

[Andy Estes]

Created attachment 49177 [details]
Test case for mousewheel events

A simple test case; displays the wheelDelta value emitted by the mousewheel/DOMMouseScroll event.

[Andy Estes]

Here are the increments in which several browsers produce mousewheel events:

Chrome 5/IE 8: 120
Safari 4: 3
Firefox 3.6: 1 (Firefox also inverts the sign on the number)
Opera 10: 40

It's a bit of a mess.  Since Yahoo mail is filtering out wheel events with deltas less than 120, and IE/Chrome produce events in that increment, it sounds like Safari should match that behavior to maximize compatibility.

Thoughts?

[Andy Estes]

Safari's current behavior is due to http://trac.webkit.org/browser/trunk/WebCore/platform/mac/WheelEventMac.mm?rev=41746.

This generates fractional wheel ticks on continuous input devices, but since the deltas are not cumulative over a single scrolling session, a slow but continuous scrolling motion will never generate a full wheel tick.  The solution is to set pixels scrolled equal to wheel ticks (this matches Chrome's behavior on continuous devices).

[Andy Estes]

Created attachment 50476 [details]
Patch with test cases

[Andy Estes]

Created attachment 50477 [details]
Patch with test cases (v2)

A tab character snuck into the last patch causing check-webkit-style to fail.  Oops!

[WebKit Review Bot]

Attachment 50476 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Created attachment 50719 [details]
Patch without WKSI binaries

[Andy Estes]

I removed the WKSI binaries from the patch to reduce its size.  They will be committed along with this patch when it is r+'d.

[John Sullivan]

Comment on attachment 50719 [details]
Patch without WKSI binaries

Now that there's no gigantic binary slogging down my poor remote network connection, I find that I can review this simple change.

[Andy Estes]

Committed revision 56012.

[Dave Hyatt]

Reopening now that this has been backed out.

[Dave Hyatt]

My take on this is that we should evangelize Yahoo to fix this, since what we're generating is not wrong.  If we are unable to get them to fix it, then we could consider a clamp of fractional lines to 1 or some kind of accumulation strategy, but really Yahoo should just fix their site.

[Peter Kasting]

(In reply to comment #17)
> My take on this is that we should evangelize Yahoo to fix this, since what we're generating is not wrong.

After much investigation, I believe the backed-out behavior was somewhat more correct than the current behavior, but both are wrong.

The backed-out behavior, which matches Chrome/Mac, produced a wheel delta of 120 when a standard wheel mouse (e.g. Logitech) was wheel-scrolled by 1 tick.  This is the "correct" wheel delta by the standard of matching IE's default behavior (when users haven't modified the "lines per wheel tick" setting), which has long been the goal for DOM wheel events.  Safari 4/the current behavior, by contrast, generates a delta of 3, which is very wrong.

The complexity arises when scrolling more rapidly, as OS X applies acceleration to the values reported.  wheelDelta is supposed to represent the physical distance traveled by the mouse wheel _without_ acceleration.  Thus, three ticks should == 360, no matter how fast or slow they arrive.  Chrome and both Safari behavior variants don't do this; the wheelDelta values are accelerated, meaning that in the Chrome/backed-out-Safari behavior the delta quickly becomes ludicrously large, while in the Safari 4/current-trunk behavior, the deltas rise to around 100-150, which is still too small.

The desired behavior should be that wheelDelta uses non-accelerated values and is scaled as roughly "120 * number of physical wheel ticks", but the amount the scrollable region scrolls (and I believe the event "delta") _are_ accelerated.

[Peter Kasting]

In short, I think the desired behavior would be something akin to this (haven't tested it):

CGEventRef cgEvent = [event CGEvent];
// The wheelDelta values here probably need scaling so one tick = 120, the scale value depends
// on where this code lives and what the API calls below return.  The critical bit is that these
// values will not be accelerated.
result.wheelDeltaY = CGEventGetIntegerValueField(cgEvent, kCGScrollWheelEventDeltaAxis1);
result.wheelDeltaX = CGEventGetIntegerValueField(cgEvent, kCGScrollWheelEventDeltaAxis2);
if (CGEventGetIntegerValueField(cgEvent, kCGScrollWheelEventIsContinuous)) {
    result.deltaY = CGEventGetIntegerValueField(cgEvent, kCGScrollWheelEventPointDeltaAxis1);
    result.deltaX = CGEventGetIntegerValueField(cgEvent, kCGScrollWheelEventPointDeltaAxis2);
} else {
    result.deltaX = [event deltaX] * Scrollbar::pixelsPerLineStep();
    result.deltaY = [event deltaY] * Scrollbar::pixelsPerLineStep();
}

[Avi Drissman]

(In reply to comment #19)

For reference, what Chrome does can be seen in http://trac.webkit.org/browser/trunk/WebKit/chromium/src/mac/WebInputEventFactory.mm . (BTW, I wrote that code, so direct questions to me.)

We use unaccelerated scroll data only for non-continuous devices. For continuous devices we're forced to use accelerated scroll data due to limitations of the scroll API.

The previous version of Safari used a private WK function. I don't know how that worked. You're welcome to the code that I wrote if you want it.

[Dave Hyatt]

Can you clarify why you believe wheelDelta should not be accelerated for continuous events?  I would think you'd want to match the platform behavior, which is to accelerate...

[Dave Hyatt]

The whole point of continuous events is acceleration is it not, so just ignoring that doesn't seem right to me.

[Dave Hyatt]

Also, we need to distinguish between the platform wheeling and what is reported to the DOM.  I'd expect platform wheel events to match the OS behavior, since those events might be used internally to implement stuff like overflow:auto mouse wheeling.

If the DOM needs to report something different we can patch it there, but removing acceleration from continuous wheel events at the platform event level doesn't seem right to me.

[Avi Drissman]

(In reply to comment #21)
> Can you clarify why you believe wheelDelta should not be accelerated for continuous events?  I would think you'd want to match the platform behavior, which is to accelerate...

I was told that's what the spec said the data should be. That's how I coded the non-continuous devices, and only made the continuous devices accelerated due to API limitations.

Was I misinformed?

[Dave Hyatt]

Do we only use the wheel ticks x and y field in the platform event for DOM events?  If so, then having them be non-accelerated (assuming that's what the spec says) seems fine.  We just need to make sure that those values are only used by the DOM and not when processing the wheel event internally, since we don't want to break overflow:auto mouse wheeling.

[Dave Hyatt]

Can someone point me to what spec indicates that the value has to correspond directly to the physical movement of the mouse (and can't be accelerated)?

[Andy Estes]

(In reply to comment #26)
> Can someone point me to what spec indicates that the value has to correspond directly to the physical movement of the mouse (and can't be accelerated)?

From <http://www.w3.org/TR/DOM-Level-3-Events/#events-Events-MouseWheelEvent-wheelDelta>:

"The distance the wheel has rotated around the y-axis. Because this is a legacy event type, the units or number of these units in wheelDelta are implementation-specific."

A strict interpretation of that first sentence would say that acceleration should be ignored, but it sounds like this was written with traditional wheel mice in mind, not continuous scrolling devices.

[Peter Kasting]

Note, however, that the DOM3 WheelEvent is not the same event as the existing "mousewheel".

But yes, hyatt and I sort of concluded over IRC that ideally we'd expose both types of values, and we'd put the unaccelerated ones in wheelDelta (with the others on a new property), but we're not actually sure we can get bug-free unaccelerated values from the underlying APIs.  Someone needs to write a test app to definitively document how some of these APIs behave.

Also, if someone could tell the DOM3 folks that their wheel event really needs to understand acceleration (e.g. by providing both accelerated and unaccelerated values), that'd be great.

[Andy Estes]

(In reply to comment #17)
> My take on this is that we should evangelize Yahoo to fix this, since what we're generating is not wrong.  If we are unable to get them to fix it, then we could consider a clamp of fractional lines to 1 or some kind of accumulation strategy, but really Yahoo should just fix their site.

After some consideration, I agree with Hyatt's take here.  This patch made continuous scrolling too fast on sites designed to work with Safari 4's behavior, and developer attempts to compensate for this by dividing out the extra multiplication make non-continuous scrolling too slow.  This is a no-win situation.

To fix this, I'm a fan of one of these two solutions:

1) Revert to Safari 4's behavior with continuous devices (perhaps without acceleration?), then fix the bug with non-continuous devices (where a wheel tick emits a wheelDelta of 3 instead of 120).  Evangelize sites that drop wheelDeltas less than 120.  Sites designed to take advantage of fine-grained scrolling will continue to work.

2) Accumulate fractional wheelDeltas and emit them each time they cross the 120 boundary.  Also implement DOM Level 3 WheelEvent [1] and evangelize that event to developers who need access to fine-grained scrolling deltas.  Sites that depend on wheelEvents in increments of 120 continue to work without changes, and developers can access fine-grained events using new API.

I think I might be partial to #2, since it fixes the original Yahoo! bug without requiring evangelization and gives us a reason to implement more of the DOM Level 3 Events spec.

[1] http://www.w3.org/TR/DOM-Level-3-Events/#events-wheelevents

[Andy Estes]

(In reply to comment #28)
> Note, however, that the DOM3 WheelEvent is not the same event as the existing "mousewheel".

I was quoting from the MouseWheelEvent section, not the WheelEvent section.

[Avi Drissman]

(In reply to comment #28)
> Someone needs to write a test app to definitively document how some of these APIs behave.

Someone meaning Apple, or someone meaning anyone? Because that's exactly what I did. I wrote a test app, and the documentation I wrote that lives in WebKit/chromium/src/mac/WebInputEventFactory.mm remains the most complete documentation of scrolling events that I know of.

[Peter Kasting]

(In reply to comment #31)
> (In reply to comment #28)
> > Someone needs to write a test app to definitively document how some of these APIs behave.
> 
> Someone meaning Apple, or someone meaning anyone? Because that's exactly what I did. I wrote a test app, and the documentation I wrote that lives in WebKit/chromium/src/mac/WebInputEventFactory.mm remains the most complete documentation of scrolling events that I know of.

Can you post that app somewhere?  hyatt and I have both read your comments, but the app would still be very useful.

Also, in the end we should make Chromium and Safari converge on the events they're sending.  This is not currently true (with the revert).

[Avi Drissman]

(In reply to comment #32)
> Can you post that app somewhere?  hyatt and I have both read your comments, but the app would still be very useful.

Sure thing. It's the simplest thing you can imagine, but it works well. It's at http://homepage.mac.com/avidrissman/archives/scrollbug.zip ; download it, compile it, run it, and scroll with as many mouse devices as you can steal from coworkers' desks.
 
> Also, in the end we should make Chromium and Safari converge on the events they're sending.  This is not currently true (with the revert).

Absolutely. First we need to decide what they should be (accelerated vs non-accelerated, etc), and then we need to wean Safari off the SPI. I'm pretty sure that the equivalent data is available via the route we take, at least for 10.5 and greater. (The SPI is likely still needed for the Tiger builds.)

[Peter Kasting]

(In reply to comment #33)
> First we need to decide what they should be (accelerated vs non-accelerated, etc)

Here is some text from a thread I wrote to the folks defining Pepper (the next-gen replacement for NPAPI).  Their wheel event is basically a passthrough of the relevant PlatformWheelEvent, so this applies to WebKit too.  hyatt et al., what do you think of this idea, assuming we can actually get the appropriate unaccelerated values from OS X?:

***

Recommendations:

* Explicitly say that m_wheelTicks{X,Y} is an unaccelerated value that represents how far the wheel has physically rotated, or the equivalent scrolling gesture has moved, since the last event.  (The gesture bit is for trackpads on the Mac, which send wheel events for two-finger scrolls.)  Specify that for mice without detents, or with exceptionally closely spaced detents (e.g. a Mighty Mouse), the value should be comparable to the value generated by a "normal" mouse wheel moved the semantically-equivalent distance.  (In other words, a single Mighty Mouse tick should reported as a small fraction of a Logitech MX5xx tick.)  Specify the base unit as one of the following two options: "1.0", for one physical tick on a normal mouse, or "120", to be directly compatible with the DOM mousewheel wheelDelta value.

* Explicitly say that m_delta{X,Y} is a possibly-accelerated value that represents the actual scroll distance requested by this event.  Say that the units are given by the m_granularity field.

The result of this would be that, on a standard Windows system with a typical mouse, moving one tick down would generate an event that looks like:

{
  m_deltaX: 0;
  m_deltaY: -3;
  m_wheelTicksX: 0;
  m_wheelTicksY: 1 [or 120];
  m_granularity: ScrollByLineWheelEvent;
  ...
}

[Nathan Vander Wilt]

(In reply to comment #29)
> (In reply to comment #17)
> > My take on this is that we should evangelize Yahoo to fix this, since what we're generating is not wrong.  If we are unable to get them to fix it, then we could consider a clamp of fractional lines to 1 or some kind of accumulation strategy, but really Yahoo should just fix their site.
> 
> After some consideration, I agree with Hyatt's take here.  This patch made continuous scrolling too fast on sites designed to work with Safari 4's behavior, and developer attempts to compensate for this by dividing out the extra multiplication make non-continuous scrolling too slow.  This is a no-win situation.
> 
> To fix this, I'm a fan of one of these two solutions:
> 
> 1) Revert to Safari 4's behavior with continuous devices (perhaps without acceleration?), then fix the bug with non-continuous devices (where a wheel tick emits a wheelDelta of 3 instead of 120).  Evangelize sites that drop wheelDeltas less than 120.  Sites designed to take advantage of fine-grained scrolling will continue to work.
> 
> 2) Accumulate fractional wheelDeltas and emit them each time they cross the 120 boundary.  Also implement DOM Level 3 WheelEvent [1] and evangelize that event to developers who need access to fine-grained scrolling deltas.  Sites that depend on wheelEvents in increments of 120 continue to work without changes, and developers can access fine-grained events using new API.
> 
> I think I might be partial to #2, since it fixes the original Yahoo! bug without requiring evangelization and gives us a reason to implement more of the DOM Level 3 Events spec.
> 
> [1] http://www.w3.org/TR/DOM-Level-3-Events/#events-wheelevents

I'd prefer Option 1 as the immediate fix, as it would fix both fine-grained continuous events and discrete clicks until the new API can be implemented. Long term, I can see the merit in Option 2 but there are issues with the new standard as well (deltaMode will be a major pain for clients even if its per-axis contradiction is worked out).

I am obviously biased, but I'd love to see the magnitudes fixed a.s.a.p. for sites that are trying use them to full potential, rather than waiting for a workaround for code that shoots itself in the foot by explicitly ignoring fine-grained events.

[Nathan Vander Wilt]

Created attachment 60125 [details]
Shows wrong magnitude of continuous scrolling in recent WebKit builds.

The attached testcase will need some massaging to work in IE, but shows the problem. Scrolling down so "Line 4." is at the top of the page yields a deltaSum of -7200 (!). Interpreted according to MSDN documentation (http://msdn.microsoft.com/en-us/library/ms536951(VS.85).aspx) this would mean -60 "increments" (which IIRC are 3-line chunks each) and over seven thousand pixels according to Quirksmode's interpretation (http://www.quirksmode.org/dom/w3c_events.html, grep wheelDelta) of that property. Basically, the wheelDelta values are 60 times what they should be.

On Safari 4, the deltaSum ends up at -120 as expected when scrolling down to "Line 4", although quite helpfully the sum can be reached gradually when the mouse has smooth scrolling.

[Nathan Vander Wilt]

Ah, sorry I missed that the specific large scroll delta issue was being tracked at https://bugs.webkit.org/show_bug.cgi?id=40441 and has already been reverted in the nightlies.

[Nathan Vander Wilt]

Ah, sorry I missed that the specific large scroll delta issue was being tracked at https://bugs.webkit.org/show_bug.cgi?id=40441 and has already been reverted in the nightlies.

[Darin Adler]

Comment on attachment 50719 [details]
Patch without WKSI binaries

Clearing the review flag from this since we landed it and then rolled it out. It’s showing up in my “patches that need to be committed” query.