1292201 - [10.12] Line-based scrolling is too fast when trying to scroll slowly - lineOrPageDeltaY is always at least 1, because [event deltaX/Y] now report fractional values

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Markus Stange [:mstange]]

Trying to scroll a XUL tree slowly on 10.12 makes it scroll rather fast - every single "pixel" scroll event scrolls by a whole line.

It looks like 10.12 has changed the behavior of -[NSEvent deltaX/Y]. Those properties used to return integer values whenever a line threshold was crossed, and zero for events in between the thresholds. That no longer happens. Instead, every event comes with a fractional delta. (roughly scrollingDeltaX/Y / 21.5)

So this code here no longer does what it's supposed to:

>   int32_t lineOrPageDeltaX = RoundUp(-[theEvent deltaX]);
>   int32_t lineOrPageDeltaY = RoundUp(-[theEvent deltaY]);

We need to do our own line threshold detection somewhere. Masayuki, what would be the right place to do it? We could do it in ChildView, unless there already is cross-platform code for such a thing.

Comment 1

[Markus Stange [:mstange]]

This also triggers intermittent extreme zooming, for example if I try to Cmd+click a link with my magic mouse but accidentally move my finger across the surface during the click (which causes slight scrolling, and Cmd+scrolling zooms).

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

We need to do it in ChildView. On Windows, we have similar problem with high resolution scroll. Therefore, MouseScrollHandler does similar things:
https://dxr.mozilla.org/mozilla-central/source/widget/windows/WinMouseScrollHandler.cpp

However, unfortunately, the code is too complicated, so, it might be difficult to implement in a few days. (Resetting the state is the key of such implementation. When we should do it, for example.)

Comment 3

[Markus Stange [:mstange]]

We can reset the state whenever we receive an event with phase NSEventPhaseBegan. I don't think we need to care about old touchpads that do not send these events. (It looks like the modern trackpads were released in 2010.)

Comment 4

[Markus Stange [:mstange]]

I've filed rdar://27720346 about this.

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1292201 - Accumulate integer deltas for scroll wheel events.

Review commit: https://reviewboard.mozilla.org/r/69654/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69654/

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8778341 [details]
Bug 1292201 - Accumulate integer deltas for scroll wheel events.

https://reviewboard.mozilla.org/r/69654/#review66908

Looks good to me. Thanks!

Comment 7

[Tracy Walker [:tracy]]

Markus, will you be landing this soon?

Comment 8

[Markus Stange [:mstange]]

I still need to test it on a pre-10.12 machine. I was hoping that this would be fixed in a later 10.12 Beta, but at the moment it doesn't look like it'll be.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:mstange, do we still want to land this?  I have a 10.11.6 machine I can try it on, if you still need pre-10.12 testing.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8778341 [details]
Bug 1292201 - Accumulate integer deltas for scroll wheel events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69654/diff/1-2/

Comment 11

[Markus Stange [:mstange]]

Yes, I still want to land this.

I decided to just change the patch so that it falls back to the old code on pre-10.12. I don't think this patch needs to be re-reviewed.
I still need to run the wheel event tests locally on 10.12 before I land this.

Comment 12

[Markus Stange [:mstange]]

Tests seem ok.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d72a1ec07543
Accumulate integer deltas for scroll wheel events. r=masayuki

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d72a1ec07543