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

RESOLVED FIXED in Firefox 48

Status

()

Core
Panning and Zooming
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: kats)

Tracking

({regression})

Trunk
mozilla48
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 wontfix, firefox47 wontfix, firefox48 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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.
(Reporter)

Comment 2

2 years ago
Created attachment 8729260 [details] [diff] [review]
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.
(Reporter)

Updated

2 years ago
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]
status-firefox46: --- → wontfix
status-firefox47: --- → affected
(Reporter)

Comment 4

2 years ago
Created attachment 8733390 [details] [diff] [review]
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.
Attachment #8733390 - Flags: review?(masayuki)
(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.
(Reporter)

Updated

2 years ago
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 =
    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.
Attachment #8733390 - Flags: review?(masayuki) → review-
Stealing
Assignee: continuation → bugmail.mozilla
Created attachment 8740171 [details] [diff] [review]
APZ should scroll by more than a page with large values of mousewheel.default.delta_multiplier_{x,y}. (v2)

Updated with review comments.
Attachment #8733390 - Attachment is obsolete: true
Attachment #8740171 - Flags: review?(masayuki)
Attachment #8740171 - Flags: review?(masayuki) → review+

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/927fe582c13f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
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.
status-firefox47: affected → wontfix
You need to log in before you can comment on or make changes to this bug.