1692707 - Odd interaction between scrollBy() and smooth scrolling

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

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.

Comment 1

[Frank Griffith Jr]

Updated Assignee. Part of Jira Epic FFXP-370

Comment 2

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Output scroll offsets and see the offsets

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: An example output by the patch in comment 4

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

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: 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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Scroll offsets with D128704

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.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1692707 - Add sampled scroll offsets field in APZTestData to evaluate scroll _smoothness_ in mochitests. r?botond

Comment 9

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

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".

Comment 11

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9863e27b04f
Add sampled scroll offsets field in APZTestData to evaluate scroll _smoothness_ in mochitests. r=botond,emilio

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/f9863e27b04f
https://hg.mozilla.org/mozilla-central/rev/9085cdd07acb