1453425 - Implement a relative scroll offset update mechanism

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

APZ and the main thread both keep track of a scroll offset for each scroll frame, and we have a mechanism to keep them synchronized.

Most changes to the scroll offset (e.g. due to input events, and most animations) originate in APZ, and are then propagated to the main thread; that is the usual flow of data.

However, sometimes the main thread independently changes the scroll offset, usually triggered by JS code (e.g. that calls scrollBy() or scrollTo(), or sets scrollTop). In such cases, we send a message to APZ telling APZ about the modified scroll offset; this is called a "scroll offset update". On the APZ side, the modified offset usually clobbers any recent changes made on the APZ side.

The scroll offset updates are currently *absolute*, in that they contain an absolute scroll offset (relative to the scrolled content's origin) which overwrites the scroll offset on the APZ side.

We would like to replace this with *relative* scroll offset updates (as proposed originally in bug 1268140 comment 2), where the update contains a delta from the previous scroll offset that was known to the main thread, and the delta is then applied to the APZ scroll offset. This avoids the clobbering of async changes mentioned above.

As this code is somewhat fragile / hard to reason about all cases, we'd like to implement a relative scroll offset update mechanism _alongside_ the existing absolute one, and gradually transition sources of main thread scrolling over to the relative one (e.g. we could start with scrollBy(), for which we have bug 959793 on file). Eventually, once all sources of main thread scrolling are transitioned, the absolute update mechanism can be removed.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1453425 - use openWebLinkIn in devtools/client/shared/link;

Review commit: https://reviewboard.mozilla.org/r/235756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235756/

Comment 2

[Ryan Hunt [:rhunt]]

I've started to take a look at this. I've created a test page here [1] that reproduces the issue we're trying to solve.

The page works by setting an interval that:
1. Busy waits for 1000ms
2. Inserts content to the top of the scroll frame
3. Increments the scroll height by the height of the newly added content 

With these three things, the apz scroll offset is effectively clobbered every repaint.

[1] https://eqrion.github.io/apz/relative-update.html

Comment 3

[Ryan Hunt [:rhunt]]

Attachment: relative-update.patch

I got a prototype working last night, here's the WIP patch.

Comment 4

[Ryan Hunt [:rhunt]]

Attachment: Bug 1453425 - Remove mScrollPosAtLastPaint from nsGfxScrollFrame. r?botond

This field appears to be only ever used as a local variable, and can be removed.

Comment 5

[Ryan Hunt [:rhunt]]

Attachment: Bug 1453425 - Add RepaintRequest for use of FrameMetrics in repaint requests. r?botond

FrameMetrics is currently used in about three ways.
  1. Main thread to APZ transactions
  2. Storing information in AsyncPanZoomController
  3. APZ to main thread repaint requests

There's overlap in the use of fields in all these use cases, but it's not perfect. In a
following commit, I'd like to change the information used for (1) to support relative
scroll offset updates. This information isn't needed for (2) or (3), so it would be
good to refactor FrameMetrics out into these use cases.

This commit refactors out (3) as it is fairly easy to do. I'd like to refactor (2) out
as well, but that is trickier. I'd like to leave that for a future followup.

Comment 6

[Ryan Hunt [:rhunt]]

Attachment: Bug 1453425 - Add relative scroll offset update mechanism to APZ. r?botond

This commit adds a relative scroll offset update mechanism to APZ. This allows
the main thread and APZ to simultaneously update the scroll offset and have the
results merged together. This feature is added behind a pref in order to help
detect regressions from this change, as this code path is tricky.

Relative updates are implemented by adding a 'base' scroll offset to the
FrameMetrics sent to APZ which is used to compute a delta. This delta is
then applied to the APZ scroll update. APZ then sends an absolute scroll
offset in repaint requests. The standard offset update filtering mechanism
in APZCallbackHelper is still used to filter stale updates.

The key part of this commit is maintaining a base scroll offset. A new field
is added to ScrollFrameHelper to maintain this. This field is reset to the
current scroll offset when a new scroll metadata is computed and sent in a
transaction. Additionally, when an apz scroll happens from a repaint request
we reset the offset to the current offset.

Comment 7

[Ryan Hunt [:rhunt]]

Sorry, I actually meant to do a try run with this first before posting it for review. I'll post the link here.

Comment 8

[Ryan Hunt [:rhunt]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=637d3862ee68fd110260dd0425387432902cfb63

Note, the patches here transition all sources of main thread scrolling to use relative updates. My understanding is that we'd like all updates to be relative eventually so I think it makes sense to try and get there right away.

If desired I can update the patches to support absolute and relative updates. It involves adding a new scroll origin and an additional flag, but isn't terrible.

This behavior change is also behind a live pref, 'apz.relative-update', so we can use that to isolate potential regressions.

Comment 9

[Ryan Hunt [:rhunt]]

Looks like some scroll reftest failures, which isn't unexpected. I'll look into them.

Comment 10

[Ryan Hunt [:rhunt]]

New patches here. Seems like there's some QR only failures, which is odd..

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=203213194

Comment 11

[Ryan Hunt [:rhunt]]

(In reply to Ryan Hunt [:rhunt] from comment #10)
> New patches here. Seems like there's some QR only failures, which is odd..
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&author=rhunt@eqrion.
> net&selectedJob=203213194

So it turns out this was caused by the fact that ComputeScrollMetadata can be called multiple times for the same scroll frame in QR, but not with Gecko. This was relied upon to compute proper deltas. I'll try and fix this assumption as that seems fragile.

Here's a new try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=941ae39f718cc164b366d89eecd7bd0622392b20

Comment 12

[Ryan Hunt [:rhunt]]

That run had some bustage, here's the fixed run. Looks green to me.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=edc8f4dc1de4283501f094e5a133fc696fb331c5

Comment 13

[Ryan Hunt [:rhunt]]

So it looks like this code is very sensitive to changes and a lot of things are tangled together. It doesn't look like we'll be able to support all main thread scroll changes becoming relative, as some tests seem to rely on concurrent mouse scrolls being overridden by main thread scroll updates. And some other untriaged test failures, I'm sure.

Additionally, adding a new scroll origin seems to be the simplest way of tracking this on the main thread. I tried adding a parallel field, mScrollUpdateLevel, but it made things much more complicated. I also had an unfortunate detour into nsGfxFrame::SaveState and RestoreState for frame reconstruction that turned out to not be necessary if you go the scroll origin + persist absolute updates route.

All that being said, I have patches that implement this and make scrolling the above test case work now. They also are green on try, so I'll put them up for review. [1]

It's probably a good idea to add a test for this code, because the existing tests saved me from a bunch of mistakes. I'd like to either add that on this bug, or in another.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e364813e818d506f6ccf12e135dea749808de9c3

Comment 14

[Ryan Hunt [:rhunt]]

Attachment: Bug 1453425 - Use ScrollByCSSPixels in relative scrolling DOM API's. r?botond

This commit changes `Window::ScrollBy` and `Element::ScrollBy` to use
ScrollByCSSPixels in preparation for marking ScrollByCSSPixels as a
relative scroll API.

Comment 15

[Ryan Hunt [:rhunt]]

Attachment: Bug 1453425 - Add relative scroll offset updates using nsGkAtoms::relative. r?botond

This commit adds a scroll origin, nsGkAtoms::relative, which can be used to
mark main thread scrolling that can be combined with a concurrent APZ scroll.

The behavior of this is controlled by a pref, apz.relative-update. This pref
is initially activated and is intended as an aid to narrowing down causes
of regressions for users in bug reports.

Relative scroll updates work by tracking the last sent or accepted APZ
scroll offset. This is sent along with every FrameMetrics. Additionally,
a flag is added to FrameMetrics, mIsRelative, indicating whether the
scroll offset can be combined with a potential APZ scroll. When this
flag is set, AsyncPanZoomController will apply the delta between the sent
base scroll offset, and sent new scroll offset.

This flag is controlled by the last scroll origin on nsGfxScrollFrame. The
new origin, `relative`, is marked as being able to clobber APZ updates,
but can only be set if all scrolls since the last repaint request or
layers transaction have been relative.

Comment 16

[Ryan Hunt [:rhunt]]

I've updated the url to the example I've used to test this [1].

(My Github site at some point lost its subfolder for examples, and I just added it back.)

[1] https://eqrion.github.io/web-tests/apz/relative-update.html

Comment 17

[Ryan Hunt [:rhunt]]

Attachment: Bug 1453425 - Add test for relative scroll offset updates. r?botond

Comment 18

[Ryan Hunt [:rhunt]]

I caught one change needed to make the test pass. (Which is why tests are good!)

I've kicked off a try run to make sure it doesn't affect other tests. [1]

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=faac3f9ed567e01cfd5df0dc65aee8b971b82483

Comment 19

[Ryan Hunt [:rhunt]]

Hmm. The test is failing on try, but passing locally to me. I'll look into it.

Comment 20

[Ryan Hunt [:rhunt]]

So it looks like the issue was with the scroll animation triggered by the wheel input. When the APZC receives a relative update, it will cancel all active animations. This was being hit in the test somehow on try, but not locally. I've resolved this by disabling smooth scrolling for this test.

We should be able to continue animations after receiving a relative update, but that should be a follow up. The new behavior of merging the scroll offsets, but still cancelling animations is strictly an improvement.

Here's a new try run to confirm everything is okay. [1]

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae713bd002c6bdcf9b75698e71b2b1ac0be371d5

Comment 21

[Ryan Hunt [:rhunt]]

I forgot to add back the SimpleTest.finish() calls to that run. This run has it and is green [1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=78a1361eacf83991edc68065b1f6ec4acb9972f1

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42961627bc9a
Remove mScrollPosAtLastPaint from nsGfxScrollFrame. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/d73aa682bf9a
Add RepaintRequest for use of FrameMetrics in repaint requests. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/214cc7e7efb6
Use ScrollByCSSPixels in relative scrolling DOM API's. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b8e2732f2a5
Add relative scroll offset updates using nsGkAtoms::relative. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab584824a073
Add test for relative scroll offset updates. r=botond

Comment 23

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/42961627bc9a
https://hg.mozilla.org/mozilla-central/rev/d73aa682bf9a
https://hg.mozilla.org/mozilla-central/rev/214cc7e7efb6
https://hg.mozilla.org/mozilla-central/rev/0b8e2732f2a5
https://hg.mozilla.org/mozilla-central/rev/ab584824a073