1543387 - Restore scroll position

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

AC: https://github.com/mozilla-mobile/android-components/issues/2700
Fenix: https://github.com/mozilla-mobile/fenix/issues/1185

When restoring the state in Reference Browser or Fenix then the scroll position is not being restored.

Looking at the recent state refactoring it seems like this is something that is supposed to happen:
https://hg.mozilla.org/mozilla-central/rev/51b9ea359763#l2.242

Comment 1

[Chris Peterson [:cpeterson]]

This sounds like a regression in GV 68 if it used to work in Fenix and RB with GV 67.

Dylan said he would investigate.

Comment 2

[[ex-Mozilla] Agi Sferro | :agi]

FYI this works for me on my Samsung S10.

Comment 3

[Dylan Roeh (:droeh) (he/him)]

(In reply to Agi | :agi | ⏰ EST | he/him from comment #2)

FYI this works for me on my Samsung S10.

Yeah, this seems to be working for me as well (on a Pixel 2). Sebastian, is there a particular site that reliably causes trouble, or is it just a blanket failure to restore scroll position on any site? In either case, I'll play around a bit with further testing tomorrow to see if I can get it to reproduce.

Comment 4

[Jan Henning [:JanH]]

Pages with frames could be problematic because of bug 1483141, but that's only an edge case.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

I tried a few things and can see it sometimes working. We may be saving an older state in some cases. I need to debug whether that's caused by us not saving a newer state or by GV not providing us with a newer state.

Comment 6

[Chris Peterson [:cpeterson]]

Dylan says this problem was probably caused by out of date save state. He recommends we move this bug to the GV backlog unless Sebastian can reproduce.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

I debugged this a bit today and it looks like we are saving and restoring the last state we got from GeckoView.

With the following STR I can reproduce this in RB and the sample browser:

---

1. Open a website; e.g. The Verge
2. Scroll down a bit
3. Go to the Recent Apps screen and swipe the browser app away
4. Open the app again

Expected:

The state is restored. The Verge is displayed with the same scroll position as before.

Actual:

The state is restored. The Verge is displayed but in initial scroll position.

---

At first I was expecting we may not receive the latest state from GeckoView (or not in time). But even if I wait a long time (and confirm that we received a new state from GV after scrolling) the scrolling state is not restored after performing the steps above.

An interesting observation: If you follow the steps above and insert the following step, then the scroll position is in the state and correctly restored:

• 1-2: As above
• 2.5: Open and switch to a different tab
• 3-4: As Above

It seems like the state we get from GeckoView does not always contain the (latest) scrolling state?

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

See steps above. The issue is currently tracked as P2 in the AC repo. We are starting to run out of P1 issues at the moment. :)

Comment 9

[Dylan Roeh (:droeh) (he/him)]

Interesting. I'm trying to wrap up some Focus stuff at the moment, I'll try to reproduce this locally on Monday. It should be pretty easy to tell if the state you're restoring from has the scroll data or not, the output of SessionState.toString() should include a scrolldata field if it does. I'm a bit worried that it reproduces only after a swipe-to-kill, the session restore process is a bit precarious and that could hint at some kind of raciness that only occurs when restoring during a cold start.

Comment 10

[Chris Peterson [:cpeterson]]

Adding [geckoview:fenix:m7] whiteboard tag because this is a usability issue but not a Fenix MVP blocker.

Comment 11

[Dylan Roeh (:droeh) (he/him)]

Attachment: Bug 1543387 - Ignore initial about:blank load when restoring scroll position. r=snorp!

When restoreState is called early in a GeckoSession's life, the scroll position restore code can catch the pageshow event for the initial about:blank load rather than for the page being restored, resulting in a failure to restore scroll position.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35991bb82eb3
Ignore initial about:blank load when restoring scroll position. r=snorp

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/329fc8b82aeb
Ignore initial about:blank load when restoring scroll position: add missing semicolon. CLOSED TREE

Comment 14

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/35991bb82eb3
https://hg.mozilla.org/mozilla-central/rev/329fc8b82aeb

Comment 15

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 9066747 [details]
Bug 1543387 - Ignore initial about:blank load when restoring scroll position. r=snorp!

Beta/Release Uplift Approval Request

• User impact if declined: May fail to restore scroll position when GeckoView.restoreState() is called during startup.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just guards against trying to restore scroll position on the initial about:blank load.
• String changes made/needed: N/A

Comment 16

[Julien Cristau [:jcristau]]

(In reply to Dylan Roeh (:droeh) (he/him) from comment #15)

• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No

Could this affect fennec? Any chance we can flip one of the above to "yes" before uplifting?

Comment 17

[Jan Henning [:JanH]]

Fennec doesn't use this.

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 9066747 [details]
Bug 1543387 - Ignore initial about:blank load when restoring scroll position. r=snorp!

fix GV scroll issue, approved for 68.0b5

Comment 19

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dbf2d7493d85
https://hg.mozilla.org/releases/mozilla-beta/rev/d37e9d3c46d7

Comment 20

[Chris Peterson [:cpeterson]]

[geckoview:fenix:m7] bugs should be priority P1.

I'm editing a bunch of GeckoView bugs. If you'd like to filter all this bugmail, search and destroy emails containing this UUID:

e88a5094-0fc0-4b7c-b7c5-aef00a11dbc9