1255634 - APZ does not let mousewheel.default.delta_multiplier_{x,y} scroll more than a page

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Andrew McCreight [:mccr8]]

The preferences mousewheel.default.delta_multiplier_x and _y can be changed to increase how much the mouse wheel scrolls at a time. Normally, this scrolling is capped to be one page at a time. However, bug 801101 made it so that if these prefs are 1000 or larger than the scroll can be more than a page at a time. This is implemented in EventStateManager::DoScrollText() in the section that starts with the comment "We shouldn't scroll more one page at once except when over".

APZ has not implemented this behavior. I think the equivalent location to put this code is in AsyncPanZoomController::GetScrollWheelDelta(). The constant MIN_MULTIPLIER_VALUE_ALLOWING_OVER_ONE_PAGE_SCROLL is private, but maybe it is okay to make it public.

Comment 1

[Andrew McCreight [:mccr8]]

This is responsible for the remaining test failures in test_wheel_default_action.html when e10s is enabled. That test sets the multiplier prefs to 99999999.

However, even with my patch for this bug, we get some test failures, where the results for a few tests are off by a few pixels. I think this is due to APZ doing some intermediate computations using floating point numbers instead of integers, or vice versa. If I reduce these prefs from 99999999 to 999999 then the tests pass. (Reducing them to 9999999 still causes failures.) I'm not sure if it is worth tracking down precisely how these multiplier prefs are rounded differently in APZ and normally, though I guess it does cause slight user-visible changes.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: APZ should scroll by more than a page with large values of mousewheel.default.delta_multiplier_{x,y}.

This is my hacky work-in-progress. It makes tests pass, but obviously having a 1000 sitting in there is not great.

Comment 3

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

The number of wheel-related prefs and behaviours really boggles my mind. But your patch looks ok from an APZ point of view, assuming we use a #define or constant or something, and add a comment.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: APZ should scroll by more than a page with large values of mousewheel.default.delta_multiplier_{x,y}.

This patch is a little ugly. It makes the constant MIN_MULTIPLIER_VALUE_ALLOWING_OVER_ONE_PAGE_SCROLL public, and simply duplicates the page scroll limit logic in AsyncPanZoomController::GetScrollWheelDelta(). It would be nice to avoid that, but the equivalent non-APZ function operates on different types, so I wasn't sure exactly how to do that. Masayuki, is this approach okay? Maybe in the long term it would be nice to share code better, but this at least gets it working now.

Another ugly thing in this patch is that the constants in doTestWholeScroll() have to be reduced because otherwise some subtests end up returning results that are off by a few pixels with e10s, presumably due to differences in floating point precision in APZ compared to non-APZ. I thought that this is such a niche feature that it wasn't worth tracing through exactly how the computations are done in each version, but let me know if you think I should look into that, Masayuki

I'll give kats a chance to review this, too, once Masayuki gives an r+ to some version of a patch.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37957a0f3db8

Although I just realized that apparently we aren't running mochitest-plain with e10s on any other platform besides Linux! I'll do an opt run to cover Windows and OSX.

Comment 5

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

(In reply to Andrew McCreight [:mccr8] from comment #4)
> try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37957a0f3db8
> 
> Although I just realized that apparently we aren't running mochitest-plain
> with e10s on any other platform besides Linux! I'll do an opt run to cover
> Windows and OSX.

We do, on win7, win10, and OS X 10.10. You can use the "Add jobs" on your try push to add those jobs.

Comment 6

[Andrew McCreight [:mccr8]]

OSX and Win7 was green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b581da69e94

Comment 7

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

Comment on attachment 8733390 [details] [diff] [review]
APZ should scroll by more than a page with large values of mousewheel.default.delta_multiplier_{x,y}.

>-  if (Abs(delta.x) > pageScrollSize.width) {
>+  // We shouldn't scroll more than one page at once except when the
>+  // user preference is large.
>+  const int32_t minAllowPageScroll = EventStateManager::MIN_MULTIPLIER_VALUE_ALLOWING_OVER_ONE_PAGE_SCROLL;

nit: too long line and const names should start with "k" prefix. So, should be:

  const int32_t kMinAllowPageScroll =
    EventStateManager::MIN_MULTIPLIER_VALUE_ALLOWING_OVER_ONE_PAGE_SCROLL;

>+
>+  if (!(Abs(aEvent.mUserDeltaMultiplierX) >= minAllowPageScroll) &&

I'd like you to create a method which explains what it checks by its name.

>+      Abs(delta.x) > pageScrollSize.width) {
>     delta.x = (delta.x >= 0)
>               ? pageScrollSize.width
>               : -pageScrollSize.width;
>   }
>-  if (Abs(delta.y) > pageScrollSize.height) {
>+  if (!(Abs(aEvent.mUserDeltaMultiplierY) >= minAllowPageScroll) &&
>+      Abs(delta.y) > pageScrollSize.height) {

Same.

Comment 8

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

Stealing

Comment 9

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

Attachment: APZ should scroll by more than a page with large values of mousewheel.default.delta_multiplier_{x,y}. (v2)

Updated with review comments.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/927fe582c13f

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/927fe582c13f

Comment 12

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

wontfix'ing for 47 from the APZ side because it's not important enough to be worth uplifting, specially since we're not going to release in 47. Andrew, feel free to request uplift anyway if you want the testing stuff uplifted to 47, the patch is certainly safe enough.