Closed Bug 1692707 Opened 1 year ago Closed 3 months ago

Odd interaction between scrollBy() and smooth scrolling

Categories

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

defect

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox95 --- fixed

People

(Reporter: mstange, Assigned: hiro)

References

Details

Attachments

(5 files)

See bug 1692705 for the motivation.

Testcase: attachment 9203072 [details]

Steps to reproduce:

  1. Make sure smooth scrolling is enabled.
  2. Load attachment 9203072 [details].
  3. Make sure it is using scrollBy (the first radio button should be checked).
  4. Scroll up and down using the arrow keys, or using an external mouse with discrete mouse wheel "notches".

Expected results:
The page should feel stable, no jumps should occur.

Actual results:
At the end of the smooth scroll animation, the page sometimes jumps downwards / scrolls back up a bit.

Severity: -- → S3
Priority: -- → P3

Updated Assignee. Part of Jira Epic FFXP-370

Assignee: nobody → tnikkel

I've started looking at this issue. So far what I've suspected is GenericScrollAnimation::HandleScrollOffsetUpdate. This GenericScrollAnimation is being used for keyboard scrolling and this HandleScrollOffsetUpdate is called when we update the relative position change triggered by scrollBy(). As you can see, we don't update GenericScrollAnimation::mFinalDestination.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

I've started looking at this issue. So far what I've suspected is GenericScrollAnimation::HandleScrollOffsetUpdate. This GenericScrollAnimation is being used for keyboard scrolling and this HandleScrollOffsetUpdate is called when we update the relative position change triggered by scrollBy().

I was wrong. HandleScrollOffsetUpdate is not directly related to this issue. A directly related issue is this sampledState.UpdateScrollProperties(Metrics()), it gets called in response to each scrollBy call, thus if there are multiple already-sampled scroll offsets, they will be same value at the moment when NotifyLayersUpdated is called. So in subsequent compositions, we will use the same value as scroll offsets for a while. That's what I've observed so far. That means scroll offsets are not back to smaller values, it sometimes stays at a position, as far as I can tell at least on Linux.

This patch adds a new MOZ_LOG outputting scroll offsets all active APZC and includes a tool to see the offsets visually. It's very primitive but sufficient for my purpose for now.

EDITED:
the MOZ_LOG name is "apz.smoothness", you can get the log by doing something like this;

MOZ_LOG="apz.smoothness:5" ./mach run "https://bugzilla.mozilla.org/attachment.cgi?id=9203072" 2>&1 | tee log

As you can see there are multiple flat offset changes that's what I commented in comment 3.

Assignee: tnikkel → hikezoe.birchill

With D128704 there's no flat changes at all. Though it looks much smoother in this graph, honestly I don't feel any differences with/without D128704. I do actually feel sometimes non-smooth scrolls in both cases.

Attachment #9246356 - Attachment description: WIP: Bug 1692707 - Shift sampled scroll offsets with the delta in the case of relative scroll updates. → Bug 1692707 - Shift sampled scroll offsets with the delta rather than clobbering them with the latest one in the case of relative scroll updates. r?botond

I've finished adding a mochitest for this issue and pushed a try run. Though the change fixes the flat scroll offset changes, but there was a failure where a scroll position got backward jump from 634.199951171875 to 151.99993896484375. I do want to suppose it's a pre-existing issue in our relative scroll update machinery and it's not the issue what Markus originally saw in comment 0, but I am unsure honestly.

I am holding this change not to land now since the change makes document-has-system-focus.html near perma.

The reason why the test fails I am supposing is that on our CI multiple wpts run at the same time, with the change the test in question runs in wpt10 (in the case of Linux opt) whereas the test runs in wpt9 (see this try run for example), so in the wpt10 there's probably other tests which cause bad interactions with document-has-system-focus.html. I don't know why my change moved the test into wpt10.

Note that the test in question fails locally with/without my change by running it with "./mach wpt --processes=2 testing/web-platform/tests/html/interaction/focus".

In a different try run, the document-has-system-focus.html ran in wpt6, and it didn't fail there. See https://treeherder.mozilla.org/jobs?repo=try&test_paths=interaction&revision=622921db6a1a0696a0f5503bba6e3866092d0dc0

So I am going to land my change as-it-is, hopefully it won't make the test run in wpt10.

Pushed by hikezoe.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f9863e27b04f
Add sampled scroll offsets field in APZTestData to evaluate scroll _smoothness_ in mochitests. r=botond,emilio
Pushed by hikezoe.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9085cdd07acb
Shift sampled scroll offsets with the delta rather than clobbering them with the latest one in the case of relative scroll updates. r=botond
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch
You need to log in before you can comment on or make changes to this bug.