1509575 - Returning to a page that was zoomed in on the right side is opened on the left

Status: RESOLVED FIXED

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

Description

[csheany]

User Agent: Mozilla/5.0 (Android 7.1.1; Tablet; rv:65.0) Gecko/65.0 Firefox/65.0

Steps to reproduce:

1. Open https://www.mozilla.org/en-US/
2. Zoom in on the right side
3. Open a new tab
4. Return to the original tab


Actual results:

The left side of the page is in view.


Expected results:

The location should be restored.

Comment 1

[Botond Ballo [:botond]]

This is caused by bug 1498812.

Comment 2

[Jan Henning [:JanH]]

Ugh, so far I thought this only happens when the session store restores the scroll position (browser restart, restoring a recently closed tab, restoring a tab that was discarded to save memory), or when session history comes into play when navigating forwards/backwards through a tab's session history.

Apparently merely switching away from a tab and then back again is enough to trigger this as well, though.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

So do you think that may be solved in bug 1498812?

Comment 4

[Botond Ballo [:botond]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #3)
> So do you think that may be solved in bug 1498812?

I expect so, yes.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late to fix in 64. Marking this issue as fix-optional for 65; if you land a patch in nightly and think it's low-risk for beta, please request uplift.

Comment 7

[Jan Henning [:JanH]]

(In reply to Botond Ballo [:botond] from comment #4)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #3)
> > So do you think that may be solved in bug 1498812?
> 
> I expect so, yes.

1. Bug 1498812 won't help here, because session history isn't involved in this problem as far as I can tell. After all, this happens even when merely switching to a different tab and back again.
2. For completeness, this also happens when going back/forward restores a page from the bfcache, where session history (the PresState) is effectively bypassed as far as the scroll position is concerned, because we simply take the previously rendered page out of the bfcache.
3. I tried poking around a bit with a debugger - somehow the repainting/layer updating that happens when re-selecting a tab (or fishing a page out of the bfcache) clobbers APZ's scroll position (and therefore the visual viewport) with the scroll position of the layout viewport. Beyond that however it would probably be better for someone more familiar with APZ to take this over.

Comment 8

[Botond Ballo [:botond]]

Thanks for the analysis.

I did some debugging, and it looks like APZ is getting a "first paint" transaction when we switch back to the page, which is not particularly surprising seeing as browser.js sets that flag is contentDocumentChanged() [1]. The visual scroll position does indeed get clobbered during a "first paint" transaction.

I also see that the session history is being bypassed, because the scroll frame object itself remains alive.

I still think there's a relation to bug 1498812, in the following sense: whatever mechanism we use to restore the visual scroll position when restoring a page from the session history, we should use the same mechanism to restore the visual scroll position after (or during) a "first paint" transaction.

[1] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/mobile/android/chrome/content/browser.js#1082

Comment 9

[Jan Henning [:JanH]]

Attachment: preserve_visual_viewport.patch

Hmm. According to the documentation at [1], the isFirstPaint flag can be reset to effectively get a notification of when the next paint happens, and clobbering the visual viewport position seems a rather unfortunate side effect.
We're not resetting the pinch-zoom, either, when (re-)doing a first paint, so is it really necessary to reset the (visual) scroll offset?

To me, I have a feeling that getting session history (or even worse, the session store) involved with this could turn out rather awkward.

I hacked together a small patch, which at a first glance seems to work: Getting a page from the bfcache or manually resetting isFirstPaint (as Fennec does) no longer clobbers the visual viewport position, but navigating to a new page still correctly resets the visual viewport back to its default position.

[1] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/dom/interfaces/base/nsIDOMWindowUtils.idl#237-244

Comment 10

[Botond Ballo [:botond]]

(In reply to Jan Henning [:JanH] from comment #9)
> We're not resetting the pinch-zoom, either, when (re-)doing a first paint,
> so is it really necessary to reset the (visual) scroll offset?

Agreed, it's not necessary.

> To me, I have a feeling that getting session history (or even worse, the
> session store) involved with this could turn out rather awkward.

Sorry, I wasn't very clear. What I meant was, both session history and isFirstPaint should use something like scrollToVisual() (the API to be added in bug 1507279) to restore the visual scroll position.

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 9034205 [details] [diff] [review]
preserve_visual_viewport.patch

Review of attachment 9034205 [details] [diff] [review]:
-----------------------------------------------------------------

So this is actually starting to look like the implementation of bug 1507279, with just the details being a bit different.

Give me a chance to put some patches up in bug 1507279, and the fix here should become straightforward.

Comment 12

[Jan Henning [:JanH]]

(In reply to Botond Ballo [:botond] from comment #10)
> > To me, I have a feeling that getting session history (or even worse, the
> > session store) involved with this could turn out rather awkward.
> 
> Sorry, I wasn't very clear. What I meant was, both session history and
> isFirstPaint should use something like scrollToVisual() (the API to be added
> in bug 1507279) to restore the visual scroll position.

By "isFirstPaint", do you mean that this should "just work", i.e. front-end code could simply reset the flag without any ill effects on the (visual) scroll position, or that the front-end code that was setting isFirstPaint back to true should also be responsible for subsequently calling the new bug 1507279-API to restore the previous visual viewport scroll position?

It's the latter alternative that I'd potentially find rather awkward.

As for session history, I didn't debug this in detail, but just to clarify, this only affects the case when the page is loaded from the bfcache and the normal session history code for restoring the scroll position is bypassed. When the page doesn't come from the bfcache, the scroll restoration code in nsGfxScrollFrame runs and everything works fine, although of course that code will have to be switched to the new API from bug 1507279, too.

> Give me a chance to put some patches up in bug 1507279, and the fix here should become straightforward.

Okay, I was already working on getting a test case working for this.

One more thing, though - right now I just commented out the code in Fennec that was resetting isFirstPaint and while this does break the UI rather severely because we're never clearing the clear colour we're drawing front of the content until we've painted something, I can still reproduce this problem.
Through getting my test case to work I've confirmed that resetting the isFirstPaint flag on the chrome window (at least in Fennec - I still need to get the test working on Desktop with e10s) does indeed get APZ into the code path where it overwrites its own scroll position with the scroll position from the incoming FrameMetrics, which courtesy of ComputeScrollMetadata contains the layout viewport position.

However it seems that switching tabs or getting a page from the bfcache is sufficient to trigger the same issue even when aIsFirstPaint is false. So while my patch would be sufficient for Fennec (which one way or another apparently is resetting isFirstPaint for all the relevant cases), it doesn't yet cover all potential cases.

Comment 13

[Botond Ballo [:botond]]

(In reply to Jan Henning [:JanH] from comment #12)
> (In reply to Botond Ballo [:botond] from comment #10)
> > > To me, I have a feeling that getting session history (or even worse, the
> > > session store) involved with this could turn out rather awkward.
> > 
> > Sorry, I wasn't very clear. What I meant was, both session history and
> > isFirstPaint should use something like scrollToVisual() (the API to be added
> > in bug 1507279) to restore the visual scroll position.
> 
> By "isFirstPaint", do you mean that this should "just work", i.e. front-end
> code could simply reset the flag without any ill effects on the (visual)
> scroll position, or that the front-end code that was setting isFirstPaint
> back to true should also be responsible for subsequently calling the new bug
> 1507279-API to restore the previous visual viewport scroll position?
> 
> It's the latter alternative that I'd potentially find rather awkward.

I imagined something like this:

  - The JS API nsIDOMWindowUtils.scrollToVisual() would be backed by
    a C++ API on nsIPresShell.

  - When sending a transaction with isFirstPaint=true, the pres shell
    (which knows this via nsIPresShell::mIsFirstPaint) would invoke
    the C++ API directly.

I don't think there's a need for front-end code to be involved.

> As for session history, I didn't debug this in detail, but just to clarify,
> this only affects the case when the page is loaded from the bfcache and the
> normal session history code for restoring the scroll position is bypassed.
> When the page doesn't come from the bfcache, the scroll restoration code in
> nsGfxScrollFrame runs and everything works fine, although of course that
> code will have to be switched to the new API from bug 1507279, too.

Yup, understood.

> Okay, I was already working on getting a test case working for this.

Cool, thanks for working on a test case! We should be able to use it regardless of the exact fix.

> One more thing, though - right now I just commented out the code in Fennec
> that was resetting isFirstPaint and while this does break the UI rather
> severely because we're never clearing the clear colour we're drawing front
> of the content until we've painted something, I can still reproduce this
> problem.
> Through getting my test case to work I've confirmed that resetting the
> isFirstPaint flag on the chrome window (at least in Fennec - I still need to
> get the test working on Desktop with e10s) does indeed get APZ into the code
> path where it overwrites its own scroll position with the scroll position
> from the incoming FrameMetrics, which courtesy of ComputeScrollMetadata
> contains the layout viewport position.
> 
> However it seems that switching tabs or getting a page from the bfcache is
> sufficient to trigger the same issue even when aIsFirstPaint is false. So
> while my patch would be sufficient for Fennec (which one way or another
> apparently is resetting isFirstPaint for all the relevant cases), it doesn't
> yet cover all potential cases.

Interesting. We can investigate this as a follow-up.

Comment 14

[Botond Ballo [:botond]]

In this Try push, I have a WIP patch for bug 1507279, and on top of it patches for this bug which make the test from bug 1517976 pass.

Comment 15

[Botond Ballo [:botond]]

Attachment: Bug 1509575 - Expose the isFirstPaint flag on LayerManager. r?kats

So that it's easily available during painting.

The flag is set based on nsIPresShell::mIsFirstPaint, but the pres shell
flag is cleared at the beginning of the paint, so we can't query it from
the pres shell during the paint.

Comment 16

[Botond Ballo [:botond]]

Attachment: Bug 1509575 - Restore the visual viewport offset from the main thread during a first paint. r?kats

During a "first paint" transaction, compositor-side state such as APZ's copy
of the visual viewport offset is overwritten. However, the scroll frame may
persist on the main thread, and in such a case we want to restore the visual
viewport offset stored in the scroll frame. This comes into play during e.g.
navigation back to a page.

Depends on D16237

Comment 17

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #13)

One more thing, though - right now I just commented out the code in Fennec
that was resetting isFirstPaint and while this does break the UI rather
severely because we're never clearing the clear colour we're drawing front
of the content until we've painted something, I can still reproduce this
problem.
Through getting my test case to work I've confirmed that resetting the
isFirstPaint flag on the chrome window (at least in Fennec - I still need to
get the test working on Desktop with e10s) does indeed get APZ into the code
path where it overwrites its own scroll position with the scroll position
from the incoming FrameMetrics, which courtesy of ComputeScrollMetadata
contains the layout viewport position.

However it seems that switching tabs or getting a page from the bfcache is
sufficient to trigger the same issue even when aIsFirstPaint is false. So
while my patch would be sufficient for Fennec (which one way or another
apparently is resetting isFirstPaint for all the relevant cases), it doesn't
yet cover all potential cases.

Interesting. We can investigate this as a follow-up.

Filed bug 1519214 to track this.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7dcfa876f76c
Expose the isFirstPaint flag on LayerManager. r=kats
https://hg.mozilla.org/integration/autoland/rev/245d6855cd3b
Restore the visual viewport offset from the main thread during a first paint. r=kats

Comment 19

[Andreea Pavel [:apavel]]

Backed out for failing wpt at css/cssom-view/scroll-behavior-main-frame-root.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=221255649&searchStr=wpt5&revision=245d6855cd3bcb4ff5afd2ab9cc90ce2cedf8a00

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=221255649&repo=autoland&lineNumber=13227

Backout: https://hg.mozilla.org/integration/autoland/rev/c908cdfffe306ae3c461c54ae24c6c9882985592

Comment 20

[Botond Ballo [:botond]]

What appears to be happening here is that a visual scroll offset update is clobbering a smooth scroll being requested in the same transaction. The smooth scroll should probably take precedence.

Comment 21

[Botond Ballo [:botond]]

Attachment: Bug 1509575 - Extend the internal visual scroll API to allow specifying "restore" vs. regular priority. r?kats

Depends on D16238

Comment 22

[Botond Ballo [:botond]]

These patches seem to be causing test_session_scroll_position.html and test_session_scroll_visual_viewport.html to time out:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=396d148b35471a240f94e6641efca54b3a3b499f

Comment 23

[Jan Henning [:JanH]]

(In reply to Botond Ballo [:botond] from comment #22)

These patches seem to be causing test_session_scroll_position.html and test_session_scroll_visual_viewport.html to time out:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=396d148b35471a240f94e6641efca54b3a3b499f

Comment 24

[Botond Ballo [:botond]]

It looks like the isFirstPaint mechanism is interfering with the session store mechanism.

Comment 25

[Jan Henning [:JanH]]

Hmm, the thing that breaks seems to be session history not involving the bfcache.
I'm now seeing the same thing locally with my own patch as well, but unfortunately I cannot remember whether I never tested that scenario, or whether something else landed in the meantime that broke this again.

Comment 26

[Botond Ballo [:botond]]

I believe I have a fix for this:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=041b8c572c9d13f4876ad877c089f73e07a1f2bc

(The tests in question are passing in this push. The other failures are unrelated.)

Comment 27

[Botond Ballo [:botond]]

Attachment: Bug 1509575 - Make nsIPresShell::mVisualViewportOffset a Maybe so we can tell if it's ever been set. r=kats

This allows us to e.g. avoid sending a value that's (0,0) because it hasn't
been set yet in a visual scroll update.

Depends on D16346

Comment 28

[Botond Ballo [:botond]]

To expand a little more on what the problem was: the tests were exercising a case where the pres shell does not persist across the navigation, so the visual offset to be restored was not stored in nsIPresShell::mVisualViewportOffset (that variable was just at its default value of (0,0)). Nonetheless, the first-paint mechanism was sending that (0,0) over as a visual scroll update, and it rode the same transaction as the layout scroll update from ScrollToRestoredPosition(). They were both eRestore and incompatible, so the (0,0) won.

My solution was to track whether nsIPresShell::mVisualViewportOffset has been set, and avoid sending a visual update based on it if it has not been set.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ecba81cf5e74
Expose the isFirstPaint flag on LayerManager. r=kats
https://hg.mozilla.org/integration/autoland/rev/52501b577855
Extend the internal visual scroll API to allow specifying "restore" vs. regular priority. r=kats
https://hg.mozilla.org/integration/autoland/rev/eba0c068601d
Make nsIPresShell::mVisualViewportOffset a Maybe so we can tell if it's ever been set. r=kats
https://hg.mozilla.org/integration/autoland/rev/041f9c2bfce5
Restore the visual viewport offset from the main thread during a first paint. r=kats

Comment 30

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/ecba81cf5e74
https://hg.mozilla.org/mozilla-central/rev/52501b577855
https://hg.mozilla.org/mozilla-central/rev/eba0c068601d
https://hg.mozilla.org/mozilla-central/rev/041f9c2bfce5

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Given that we're less than a week away from the 65 RC and this isn't a new issue, I think this should just ride the trains to release. Feel free to nominate for approval if you feel strongly otherwise.

Comment 32

[Botond Ballo [:botond]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #31)

Given that we're less than a week away from the 65 RC and this isn't a new issue, I think this should just ride the trains to release.

+1