Closed Bug 1756529 Opened 2 years ago Closed 2 years ago

msdPhysics doesn't honor general.smoothScroll.(other|pages)

Categories

(Core :: Panning and Zooming, defect, P3)

Product:
Core
Component:
Panning and Zooming
Version:
Firefox 98
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
100 Branch
Tracking Flags:
Tracking Status
firefox100 --- fixed

People

(Reporter: tustamido, Assigned: matteo.ruello, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 1 obsolete file)

Bug 1756529 - Honor general.smoothScroll.[line|page|other] for keyboard scrolls with MSD physics r?botond
48 bytes, text/x-phabricator-request
Details | Review

general.smoothScroll.other and general.smoothScroll.pages allow to disable smoothScroll on keybinds that scroll large amounts at once: Home, End, PageUp and PageDown.

But the great general.smoothScroll.msdPhysics currently doesn't follow these two prefs.

Are you saying that if you set general.smoothScroll.msdPhysics.enabled to true, and general.smoothScroll.pages to false then you get undesired smooth scrolling when pressing pageup and pagedown?

Yes.

The non-MSD (Bezier) animation respects this setting here. (Its way of respecting the setting is interesting: it still runs an animation, but with the durations set to zero, thereby appearing instant.)

For MSD, the settings is respected in a different way by this check, which avoids runs the animation altogether. However, this check is only performed for mousewheel events, not keyboard events.

Fixing this should be fairly straightforward, would make a nice mentored bug.

Mentor: botond
Severity: -- → S3
Priority: -- → P3
Whiteboard: [lang=c++]

I am going to take a look at this, as discussed with @botond

Thanks Matteo! I assigned the bug to you.

Assignee: nobody → matteo.ruello
Attachment #9265650 - Attachment is obsolete: true
Attachment #9266127 - Attachment description: Bug 1756529 ScrollPage now honors general.smoothScroll.page preference with MSD physics r?botond → Bug 1756529 - Honor general.smoothScroll.[line|page|other] for keyboard scrolls with MSD physics r?botond
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58f10ceb16ba
Honor general.smoothScroll.[line|page|other] for keyboard scrolls with MSD physics r=botond,hiro
Flags: needinfo?(botond)

Based on the logs, it looks like there are some "carry-over" scroll offset updates from the previous test case. I have an idea for a fix; will do a Try push that runs the test on all platforms this time.

Flags: needinfo?(botond)

I'm not able to reproduce a test failure locally in repeated runs. Unfortunately, that means getting this test to be reliable enough to land is going to be an uphill battle...

Are you testing on Windows? Seems like it does not fail there, right?

It seems like the scroll counter is not incremented even on multiple scrolls (I see multiple redraws, but a single increment at the end) Is it possible that we don't parse the counter thread until the UI is completely redrawn?

(In reply to Matteo Ruello from comment #14)

Are you testing on Windows? Seems like it does not fail there, right?

There are test failures on all three desktop platforms; here is one on Windows. Locally I'm running Linux.

I've been continuing to investigate this using Try pushes with added logging. It seems like one issue is that the window.scrollTo(0, 0) call is not effective at restoring the scroll position to 0 for the next test case.

Part of the reason for this is that, since bug 1692708, the scrollTo() gets translated into a relative scroll (meaning, if the current scroll position is, say, 100, it gets intereprested as "scroll by -100" rather than "scroll to 0"). Relative scrolls do not interrupt ongoing smooth scroll animations; rather, they adjust the destination of animation by the relative delta (that happens here).

Unfortunately, it looks like this adjustment is not implemented correctly for MSD scroll physics. Specifically, while ScrollAnimationMSDPhysics::ApplyContentShift() (called from here) adjusts ScrollAnimationMSDPhysics::mDestination, that value is never actually read; the copy of the destination stored internally by the AxisPhysicsMSDModel is what actually needs to adjusted.

(In reply to Botond Ballo [:botond] from comment #17)

Unfortunately, it looks like this adjustment is not implemented correctly for MSD scroll physics. Specifically, while ScrollAnimationMSDPhysics::ApplyContentShift() (called from here) adjusts ScrollAnimationMSDPhysics::mDestination, that value is never actually read; the copy of the destination stored internally by the AxisPhysicsMSDModel is what actually needs to adjusted.

Filed bug 1759958 for this.

Depends on: 1759958

Here's a Try push with the patches from bug 1759958: https://treeherder.mozilla.org/jobs?repo=try&revision=543f65cdd0c17748c6bbfba9a4374af08ce70bf1.

The number of failures is significantly reduced, but there are still some, most of them on Mac.

At least some of the Mac failures are due a line scroll only getting as far as 48px when the test is expecting at least 50px.

The test uses line-height: 100px;, and seems to expect a line scroll to scroll by that amount (I assume the 50px is just a conservative bound).

The actual distance of a line scroll is three lines, with the height of a line determined by the scroll frame's "line scroll amount". The "line scroll amount" seems to be 19px on Linux and 16px on Mac, resulting in total scroll distances of 57px and 48px, respectively.

The line-height: 100px; seems to have no impact on the "line scroll amount".

Updated the patch to use a different method of controlling the distance of the line scroll, by using the pref toolkit.scrollbox.verticalScrollDistance to increase the number of lines scrolled from 3 to 5.

Here's a new Try push: https://treeherder.mozilla.org/jobs?repo=try&revision=6d7075e02159d84a7bef9fc3136fb2ffbaf05fe9. There are now fewer failures on Mac, but there are still some remaining ones.

The remaining Mac failures were caused by the fact that even if the relative scroll update from window.scrollTo(0, 0) is handled correctly, if the smooth scroll animation is still in progress, we're still going to get residual scrolling from it that interferes with the next test case. We really want to do something that cancels any in-progress animation to properly clean up the current testcase.

I looked around for what would do that, and settled on sending a synthesized touch-tap event. The touch-down interrupts all in-progress animations, since a touch is supposed to "hold" the page in place.

This then caused another complication, which is that in the case of instant scrolling test cases, two touch-taps could be sent in quick enough succession that they'd be gesture-detected as a double-tap and trigger double-tap-zooming, which also interferes with subsequent test cases. I worked around this by running the tests with apz.allow_double_tap_zooming=false.

With these changes, the test seems to be green on Try: https://treeherder.mozilla.org/jobs?repo=try&revision=d68664e752783d90dd16dfcb8a3e0f90123d2d14.

(Having to synthesize a touch-tap to interrupt ongoing scrolling animations feels a bit hackish. It would probably be nicer to add a new test API specifically for the purpose of cancelling ongoing animations.)

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4be195c46471
Honor general.smoothScroll.[line|page|other] for keyboard scrolls with MSD physics r=botond,hiro
Regressions: 1760402

Backed out for causing mochitest failures in test_group_keyboard.html

  • Backout link
  • Push with failures
  • Failure Log
  • Failure line: TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_group_keyboard.html | Expected to be scrolled to origin, actually scrolled to 1 - got +0, expected 1
Flags: needinfo?(matteo.ruello)

Hmm. The failures that I see on the autoland push are:

  • test-windows10-32-2004-qr/debug-mochitest-plain-fis-e10s-3 - intermittent failure, on 4/7 runs
  • test-windows10-64-2004-qr/debug-mochitest-plain-spi-nw-fis-e10s-3 - intermittent failure, on 4/6 runs

But on my Try push, I've retriggered both of the above jobs 10 times and they pass 10/10 times.

I did another Try push from the autoland branch, and this time the failures are readily reproducible (they happened on the first try).

The issue seems to be that while the test synthesized a touch-tap to interrupt the animation, it wasn't correctly waiting for the consequences to be propagated to the main thread. (It was calling promiseApzFlushedRepaints(), but the notification that sends to the compositor can get there before the synthesized touch events are processed, and therefore before any repaints have been enqueued.)

When this happens, the test can get to the scrollTo(0, 0) while there is still a repaint pending from the last time the animation was sampled prior to being cancelled. In such a situation, the relative delta from the scrollTo(0, 0) will be combined with the scroll from that last animation sample (often (0, 1)), and result in the scroll offset ending up at (0, 1).

This can be solved by waiting for the synthesized touch event to be dispatched to the page (then we know it has been processed by APZ), then calling promiseApzFlushedRepaints() and finally the scrollTo().

Patch with that change is green on Try, 10/10 retriggers of the jobs in question: https://treeherder.mozilla.org/jobs?repo=try&revision=8be920025304eaab944d6db1459ec39165b2b90a

Unfortunately, while passing on Windows, the revised test is failing (intermittently but frequently) on Linux, though I can't reproduce the failure locally: https://treeherder.mozilla.org/jobs?repo=try&revision=9f6549f0968e3451003897a6456aa973e293556e

I can't really justify spending more time investigating this test. It seems to pass reliably on platforms other than Linux, so let's land the patch with the test disabled on Linux.

Flags: needinfo?(matteo.ruello)
Blocks: 1760731
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb898843abd1
Honor general.smoothScroll.[line|page|other] for keyboard scrolls with MSD physics r=botond,hiro
Regressions: 1760744

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

Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
status-firefox100: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 100 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: