1149128 - Support fractional scroll wheel events in APZ

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Apparently, some mice/touchpad drivers send lots of events with tiny fractional deltas, rather than one event with a normal delta. To accommodate for this, EventStateManager will round deltas to 0 and accumulate the remaining fraction, which then gets applied to the delta of the next event.

This is implemented in two functions:

EventStateManager::DeltaAccumulator::InitLineOrPageDelta is called from PreHandle(), and decides whether or not to reset the pending amount in response to a wheel event.

EventStateManager::DeltaAccumulator::ComputeScrollAmountForDefaultAction accumulates the fractional component of the delta.

For APZ, I'm mostly concerned about getting tests to pass and for making sure the behavior for these devices roughly matches what the main thread is doing. Masayuki points out that, unless APZ can precisely match the main thread, certain DOM wheel event fields will not match the final scroll amount. That's a pretty hard thing to guarantee for APZ, and the wheel event fields that expose the scroll amount are all proprietary to Firefox (and mostly undocumented). So I'd like to just roll with a rough approximation and see where it goes.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

This is a minimal reproduction of the main thread logic. The accumulated values are global in ESM, so I put them in APZCTM. But APZCs would be cleaner and wouldn't break tests, so I'd be fine with them there too.

Another small difference is that for simplicity, the "whoa, scroll delta is too large" check occurs before the delta accumulation, not after. That shouldn't really matter.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Did you see my comment on IRC? It's not clear to me why this is needed.

http://logs.glob.uno/?c=mozilla%23apz&s=27%20Mar%202015&e=27%20Mar%202015#c4574
http://logs.glob.uno/?c=mozilla%23apz&s=27%20Mar%202015&e=27%20Mar%202015#c4579

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Did you see my comment on IRC? It's not clear to me why this is needed.
> 
> http://logs.glob.uno/
> ?c=mozilla%23apz&s=27%20Mar%202015&e=27%20Mar%202015#c4574
> http://logs.glob.uno/
> ?c=mozilla%23apz&s=27%20Mar%202015&e=27%20Mar%202015#c4579

Sorry, I missed this comment. The difference is solely the rounding mechanism used. ScrollFrameHelper::ScrollToImpl() truncates, whereas ComputeScrollAmountForDefaultAction() rounds toward zero. Masayuki wasn't happy with that being different.

I don't really have an opinion. The difference seems subtle enough that just adjusting tests to be more lenient seems fine to me.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Just to make sure I understand fully: let's say you get a bunch of y-axis scrolls that are of magnitude 0.1, 0.2, 0.3, 0.4. At the end of this we'll be at y=1 with main-thread scrolling and with APZ scrolling. After the third scroll though (when the unrounded position is y=0.6) the main-thread scrolling code will be at y=1 and APZ scrolling will be at y=0. Am I understanding the problem correctly?

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Just to make sure I understand fully: let's say you get a bunch of y-axis
> scrolls that are of magnitude 0.1, 0.2, 0.3, 0.4. At the end of this we'll
> be at y=1 with main-thread scrolling and with APZ scrolling. After the third
> scroll though (when the unrounded position is y=0.6) the main-thread
> scrolling code will be at y=1 and APZ scrolling will be at y=0. Am I
> understanding the problem correctly?

Yeah, that is correct.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Do we know why ComputeScrollAmountForDefaultAction rounds differently? Pretty much all other scrolling goes through ScrollToImpl and gets rounded the same way so it seems to me that the right thing to do here is (1) change how ComputeScrollAmountForDefaultAction rounds and (2) update tests accordingly. Then everything should just work with APZ.

If changing ComputeScrollAmountForDefaultAction isn't an option then I think making the tests more lenient would be better than adding this giant hunk of code for what seems to be a very minor difference.

Comment 7

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

It's traditional behavior, i.e., not my idea. But I think that it's reasonable for native wheel event handling. Sometimes users could turn mouse wheel to wrong direction at starting to use it. E.g., when attempting to turn mouse wheel to the user's side, the user touches the wheel. At this time, wheel might turn to away from user only a few amount. Even if such unexpected operation occurs < 1 line, we can prevent the unexpected result.

On the other hand, on Windows, scrolls first in such scenarios. The reason is that Window sends us WM_MOUSEWHEEL message first. At receiving it, we set lineOrPageDelta as larger than 1. This is compatible behavior with the behavior when Windows didn't support high resolution mouse wheel. (Starting Vista, it's supported. XP or earlier, WM_MOUSEWHEEL always has 120 * n delta value. The system settings of Windows define how much line should be scrolled per 120)

Comment 8

[Kartikaya Gupta (email:[e-mail])]

So let me restate the issue to make sure I understand it: let's say the user is on a page at y=50 and they grab the mouse. The wheel is a high-resolution mousewheel and in the process of grabbing the mouse they get a scroll event for -0.1 pixels or something. With the current main-thread code this delta will get rounded towards zero, and so the page will remain at y=50. With the APZ codepath though the scroll position will change to y=49.9, which will get rounded to y=49 and so they will see the page scroll a bit before they "really" start scrolling downwards which is what they wanted to do. Except obviously it would be scrolling by a line amount rather than a single pixel - but the general idea is that the current main-thread scrolling also serves as a "debouncer" for this sort of scenario.

Honestly, I'm not sure how much this scenario will still occur in the APZ code, because APZ will always keep the full fractional scroll offset for all scroll frames. So in my above example, the "bad case" will only occur if the scroll offset was initially at 50 <= y < 50.1 (or more generally n <= y < n+scrolldelta). Probalistically speaking the chance of that happening is |scrolldelta| (in this example, a 0.1 probability) and as the mousewheel becomes higher resolution with smaller scrolldeltas the probability reduces to zero. I'm not sure which tests exercise this behaviour but I'm guessing they are already starting at an integer scroll position which is rather unlikely in a real-world case. Does that make sense?

Given this I don't think this stuff is worth doing in the APZ code because it adds quite a lot of complexity to solve a rather small problem. I would rather get wheel handling working without it - and then if users actually start complaining about this issue, we can fix it. Masayuki, what do you think?

Comment 9

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Honestly, I'm not sure how much this scenario will still occur in the APZ
> code, because APZ will always keep the full fractional scroll offset for all
> scroll frames. So in my above example, the "bad case" will only occur if the
> scroll offset was initially at 50 <= y < 50.1 (or more generally n <= y <
> n+scrolldelta). Probalistically speaking the chance of that happening is
> |scrolldelta| (in this example, a 0.1 probability) and as the mousewheel
> becomes higher resolution with smaller scrolldeltas the probability reduces
> to zero. I'm not sure which tests exercise this behaviour but I'm guessing
> they are already starting at an integer scroll position which is rather
> unlikely in a real-world case. Does that make sense?

Hmm, I don't understand well what you said here... Sorry.

> Given this I don't think this stuff is worth doing in the APZ code because
> it adds quite a lot of complexity to solve a rather small problem. I would
> rather get wheel handling working without it - and then if users actually
> start complaining about this issue, we can fix it. Masayuki, what do you
> think?

Sounds okay to me.

The important points are:

1. Don't change when DOMMouseScroll and MozMousePixelScroll events are fired.
2. Don't do super speed scroll with the rounding problem.

In my understanding, #1 will be handled ESM even with APZ, right? And #2 doesn't occur because APZ automatically stores scroll offset in sub pixels (i.e., per < 1px).

So, the only problem here is, scrolling first vs. scrolling after delta is enough accumulated may not be matter in usual case (if it's in pixel, not line or page).

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> Hmm, I don't understand well what you said here... Sorry.

Any particular part you didn't understand? I can't really clarify unless I know what part you're having trouble with.

> 1. Don't change when DOMMouseScroll and MozMousePixelScroll events are fired.
> 2. Don't do super speed scroll with the rounding problem.
> 
> In my understanding, #1 will be handled ESM even with APZ, right? And #2
> doesn't occur because APZ automatically stores scroll offset in sub pixels
> (i.e., per < 1px).

Yes, I believe those statements are correct.

> So, the only problem here is, scrolling first vs. scrolling after delta is
> enough accumulated may not be matter in usual case (if it's in pixel, not
> line or page).

Not sure I'm understanding this correctly, but it sounds like you're agreeing with what I said earlier - that is, in the usual case, there is no problem. There is only a problem some of the time, and it should be infrequent enough to not worry about for now.

@dvander: which tests exercise this behaviour? I'd like to take a look at them, as it may help my understanding of the problem.

Comment 11

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Honestly, I'm not sure how much this scenario will still occur in the APZ
> code, because APZ will always keep the full fractional scroll offset for all
> scroll frames. So in my above example, the "bad case" will only occur if the
> scroll offset was initially at 50 <= y < 50.1 (or more generally n <= y <
> n+scrolldelta). Probalistically speaking the chance of that happening is
> |scrolldelta| (in this example, a 0.1 probability) and as the mousewheel
> becomes higher resolution with smaller scrolldeltas the probability reduces
> to zero. I'm not sure which tests exercise this behaviour but I'm guessing
> they are already starting at an integer scroll position which is rather
> unlikely in a real-world case.

Does scroll snapping change things in this regard, by making integer scroll positions much more common?

Comment 12

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #11)
> Does scroll snapping change things in this regard, by making integer scroll
> positions much more common?

I wouldn't say "much more" common - it depends on how frequently scroll snapping is used in the wild, which will be "not very much" initially. Forcing a particular scroll position using scrollTo is much more likely IMO.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> @dvander: which tests exercise this behaviour? I'd like to take a look at
> them, as it may help my understanding of the problem.

dom/events/test/test_bug422132.html which might be the only one we're failing because of this. It's also testing that a fractional pixel scroll accumulates into a fractional line scroll, which won't be true if one is handled by APZ and the other by ESM. But mstange is landing pixel scroll support to APZ soon.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

So am I correct in that if you change this test to:
1) start off by setting target to a nonzero scroll position
2) have -0.5 deltaX and deltaY values, and
3) change the target.scrollXXX > scrollXXX tests to be < instead of >

then the test passes with APZ?

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> So am I correct in that if you change this test to:
> 1) start off by setting target to a nonzero scroll position
> 2) have -0.5 deltaX and deltaY values, and
> 3) change the target.scrollXXX > scrollXXX tests to be < instead of >
> 
> then the test passes with APZ?

Well, even if it passes in APZ, it wouldn't pass without it... since one of the checks would fail, unless they do >= or something. I'll just simplify it to test that the accumulated value is 1 rather than testing that each fractional scroll potentially does nothing or something.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, refactor test

Just making the control flow sane and linear.

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, pass on apz

This changes the test to check that two 0.5 line scrolls == a total of 1.0 scroll change.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8589313 [details] [diff] [review]
part 1, refactor test

Review of attachment 8589313 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/test/test_bug422132.html
@@ +106,5 @@
> +
> +  var nextTest = function() {
> +    var test = tests.shift();
> +    synthesizeWheel(target, 10, 10, test.event);
> +    hitEventLoop(function() {

The hitEventLoop call is missing the ,20 argument at the end. (I know it gets removed in the next patch but since this one is a refactoring you should keep it here).

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8589314 [details] [diff] [review]
part 2, pass on apz

Review of attachment 8589314 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/test/test_bug422132.html
@@ +70,5 @@
>        },
>        check: function() {
>          ok(target.scrollLeft > scrollLeft,
>             "not scrolled to right by 0.5px delta value with pending 0.5px delta");
>          ok(target.scrollTop > scrollTop,

For consistency then can you change these ok checks to also check for 1 pixel difference?

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8732f95f12f0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e4d3753af8a2

w/ comments addressed

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8732f95f12f0
https://hg.mozilla.org/mozilla-central/rev/e4d3753af8a2