Closed Bug 1255634 Opened 8 years ago Closed 8 years ago

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


(Core :: Panning and Zooming, defect)

Not set



Tracking Status
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- fixed


(Reporter: mccr8, Assigned: kats)



(Keywords: regression, Whiteboard: [gfx-noted])


(1 file, 2 obsolete files)

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.
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.
This is my hacky work-in-progress. It makes tests pass, but obviously having a 1000 sitting in there is not great.
Blocks: 1252251
No longer blocks: 1252256
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.
Keywords: regression
Whiteboard: [gfx-noted]
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:

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.
Attachment #8733390 - Flags: review?(masayuki)
(In reply to Andrew McCreight [:mccr8] from comment #4)
> try run:
> 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.
Attachment #8729260 - Attachment is obsolete: true
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 =

>+  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) {

Attachment #8733390 - Flags: review?(masayuki) → review-
Assignee: continuation → bugmail.mozilla
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.
You need to log in before you can comment on or make changes to this bug.