1141728 - APZ computes wheel scroll deltas from the wrong event values

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

APZ currently multiples the line/page delta by the line scroll size. It should be multiplying the raw delta by the line scroll size instead.

Comment 1

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

Attachment: fix

Comment 2

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

Comment on attachment 8575533 [details] [diff] [review]
fix

This doesn't quite work right.

Comment 3

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

Attachment: better fix

This version inlines the relevant logic from WheelTransaction::AccelerateWheelDelta - grabbing the widget's scroll delta override. We skip the root scrollframe check since APZ has to do that part. Everything else in the WheelTransaction/ESM path we can ignore since it's accumulating scroll speed, which APZ has to handle itself.

Comment 4

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

Not a huge fan of this, let me think about it a bit.

Comment 5

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

Comment on attachment 8575620 [details] [diff] [review]
better fix

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

Clearing review as per IRC discussion, we'll just bail out to main-thread handling of the wheel event instead if these prefs are set.

Comment 6

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

Attachment: Outline of what I would be ok with

If it turns out that at some point in the future we do need to route this through the APZ then I would rather have it structured like the attached patch (just an outline, I didn't compile it).

Comment 7

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

Attachment: fix

Comment 8

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

Comment on attachment 8576175 [details] [diff] [review]
fix

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +830,5 @@
>    return result;
>  }
>  
> +// Note: currently, wheel events may only be sent to APZ if the controller
> +// thread is the main thread.

Remove this comment. WidgetWheelEvent is main-thread only so this is already implicit; if you want to explicitly state it then you can add an assertion but I don't think it's necessary.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/abd759f41a6d

w/ comment addressed

Comment 10

[Carsten Book [:Tomcat]]

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