Closed Bug 1543387 Opened 6 months ago Closed 5 months ago

Restore scroll position

Categories

(GeckoView :: General, defect, P1)

Unspecified
Android
defect

Tracking

(firefox66 unaffected, firefox67 unaffected, firefox68 fixed, firefox69 fixed)

RESOLVED FIXED
mozilla69
Tracking Status
firefox66 --- unaffected
firefox67 --- unaffected
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: sebastian, Assigned: droeh)

Details

(Whiteboard: [geckoview:fenix:m7][qa-triaged])

Attachments

(1 file)

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

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.

Assignee: nobody → droeh
OS: All → Android
Priority: -- → P1
Whiteboard: [geckoview:fenix:m4]

FYI this works for me on my Samsung S10.

(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.

Flags: needinfo?(s.kaspari)

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

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.

Whiteboard: [geckoview:fenix:m4] → [geckoview:fenix:m5]

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.

Whiteboard: [geckoview:fenix:m5] → [geckoview:fenix:p3]

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?

Flags: needinfo?(s.kaspari)

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. :)

Flags: needinfo?(cpeterson)

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.

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

Flags: needinfo?(cpeterson)
Priority: P1 → P2
Whiteboard: [geckoview:fenix:p3] → [geckoview:fenix:m7]

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.

Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/35991bb82eb3
Ignore initial about:blank load when restoring scroll position. r=snorp
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/329fc8b82aeb
Ignore initial about:blank load when restoring scroll position: add missing semicolon. CLOSED TREE
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

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
Attachment #9066747 - Flags: approval-mozilla-beta?

(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?

Flags: needinfo?(droeh)

Fennec doesn't use this.

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

Flags: needinfo?(droeh)
Attachment #9066747 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Whiteboard: [geckoview:fenix:m7] → [geckoview:fenix:m7][qa-triaged]

[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

Priority: P2 → P1
You need to log in before you can comment on or make changes to this bug.