Switching tabs while being zoomed changes the position of the page
Categories
(Core :: Panning and Zooming, defect, P3)
Tracking
()
People
(Reporter: mbalfanz, Assigned: kats)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: apz-planning [apz:dtz:10:M])
Attachments
(7 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
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
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
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.
Assignee | ||
Comment 3•4 years ago
|
||
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).
Assignee | ||
Comment 4•4 years ago
|
||
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.
Assignee | ||
Comment 5•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 6•4 years ago
|
||
Assignee | ||
Comment 7•4 years ago
|
||
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.
Assignee | ||
Comment 8•4 years ago
|
||
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:
Assignee | ||
Comment 9•4 years ago
|
||
Assignee | ||
Comment 10•4 years ago
|
||
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
Assignee | ||
Comment 11•4 years ago
|
||
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
Assignee | ||
Comment 12•4 years ago
|
||
Depends on D76812
Assignee | ||
Comment 13•4 years ago
|
||
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
Assignee | ||
Comment 14•4 years ago
|
||
This is a useful function for non-fission tests too.
Depends on D76814
Assignee | ||
Comment 15•4 years ago
|
||
Depends on D76815
Comment 16•4 years ago
|
||
Comment 17•4 years ago
|
||
bugherder |
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
Description
•