STR:

• set apz.allow_zoom to true
• Visit any website and zoom in via DesktopZooming (ideally somewhere in the bottom right corner)
• switch to a different tab, then switch back to the zoomed-in tab

ER: the tab content of the zoomed tab should remain in the same position
AR: the viewport doesn't maintain position and is moved further to the top left corner

Note: this seems to only happen on the first tab switch. Subsequent tab switches seem to work as expected

I'll investigate

So the first surprise to me here is that when we switch tabs, the layer transaction doesn't have the isFirstPaint flag set on it as I would expect. Seems like only the first non-suppressed paint on a presShell has that flag, and when we switch tabs, the presShell isn't destroyed and re-created, so it doesn't generate a new first-paint.

Since the paint isn't a first-paint, we just get a NotifyLayersUpdated call with some (seemingly correct) value in the visual viewport offset which we don't apply to the APZ's metrics. The APZ leaves its mScrollOffset at zero and then on the next repaint request it clobbers the main-thread visual viewport offset.

Still trying to wrap my head around some of the fields and their semantics at the moment. In particular the APZ copy of the metrics doesn't seem to ever use the visual viewport offset which seems a bit wrong, but maybe can be (ab)used to fix this problem somehow.

Presumably if the isFirstPaint flag was being set as I expect, then the metrics update sent to APZ would also be setting the visual viewport update type to eRestore due to this code and everything should in theory work. So maybe I should focus on fixing the isFirstPaint flag, but that will likely have all sorts of other side-effects.

But at least my memory of the isFirstPaint flag being set on tab changes is partly correct, per this bug comment (i.e. on Android at least it happens).

Even when the isFirstPaint flag is not set, the tab change causes a change in the layer tree that results in the APZC tree getting rebuilt. So the first call to NotifyLayersUpdated on the new APZC will have isDefault = true. If we use that to apply the visual scroll offset coming from the main thread then that fixes this bug.

This seems like a "safe" patch and maybe it's the thing to do for now. I'm not sure if it's worth pursuing setting isFirstPaint for tab switches as I expect that to open up some can of worms.

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=9a2136c7f326b42f4730fa250da9fbc7d7339bcb

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=206b14992c12aa6ca9a31c7ea264ba25203d6c4c

Seems almost totally green, but macOS opt verify builds are showing some intermittent failures. Trying to track that down. I can't reproduce it locally but I'm doing pushes with more logging.

I spun off bug 1640730 for an edge case that seemed like a separate bug. Worked around it in the test. Here's a green try push:

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=7dcf2d814e3489fac3a1351fc7189e61fc9cf576

SetScrollableFrameMetrics uses a bogus scrollable rect (-1, -1, -1, -1) if none
is provided. With the next patch, we start using this bogus rect for clamping
the scroll offset on some calls to NotifyLayersUpdated, and causes the gtest
to fail. If we provide some sane rect it all works fine.

Depends on D76810

This is the main fix for the bug. When APZ is getting metrics from the main
thread "for the first time" it should accept the visual scroll offset in the
main thread metrics. Normally one would expect the isFirstPaint flag to be
set on this call to NotifyLayersUpdated, but that seems to only ever be set
on the very first paint of a presShell. In particular it is not set after a
tabswitch because the brower's async tab switching mechanism means that the
layer tree is already cached in the compositor.

Depends on D76811

Depends on D76812

This is specifically needed because the new test I'm adding uses both
pinchZoomInWithTouch and forceLayerTreeToCompositor, and it seems hard/impossible
to make that work unless both are async functions or both are non-async generator
functions. And in general I think we want to move away from the generator-style
tests and towards async-style tests as they are cleaner and more flexible. So
this migrates helper_basic_zoom along with turning pinchZoomInWithTouch into
an async function.

Depends on D76813

This is a useful function for non-fission tests too.

Depends on D76814

Depends on D76815

https://hg.mozilla.org/mozilla-central/rev/d8421ae6242e
https://hg.mozilla.org/mozilla-central/rev/76db5106df72
https://hg.mozilla.org/mozilla-central/rev/47328d6c1b40
https://hg.mozilla.org/mozilla-central/rev/d829a9b89a5e
https://hg.mozilla.org/mozilla-central/rev/9bd88bca8644
https://hg.mozilla.org/mozilla-central/rev/cae3fa76f122
https://hg.mozilla.org/mozilla-central/rev/7042555d785f