Closed Bug 1269539 Opened 6 years ago Closed 6 years ago

Scroll position lost when reload page

Categories

(Core :: Layout, defect)

49 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- unaffected
firefox48 --- fixed
firefox49 + fixed

People

(Reporter: alice0775, Assigned: kats)

References

Details

(Keywords: regression)

Attachments

(1 file)

[Tracking Requested - why for this release]:

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/77cead2cd20300623eea2416bc9bce4d5021df09
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0 ID:20160502030207

Original report, see http://forums.mozillazine.org/viewtopic.php?p=14591285#p14591285

I can reproduce the problem with/without e10s.


Reproducible : always

Steps To Reproduce:
1. Open http://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-win32/
2. Scroll to the bottom
3. Reload page(F5)

Actual Results:
Scroll position is lost. Scroll position is restored at random.

Expected Results:
Scroll position should be restored.


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=774c838c19e217e177ba22876d57a344b54c33e4&tochange=a12cdfe43e929dfa151e3ea1c35982cef674ff66

Regressed by: a12cdfe43e92	Kartikaya Gupta — Bug 1268195 - When restoring a scroll position outside of incremental load, don't keep trying in a loop - just do it once and stop. r=tnikkel
Flags: needinfo?(tnikkel)
Flags: needinfo?(bugmail.mozilla)
We create the (new) root scroll frame before LoadComplete is called, and we restore the previous state (mRestoringHistoryScrollPosition is false). We try to restore once and then clear the restore pos. Then EndLoad is called and sets mRestoringHistoryScrollPosition to true, but its too late, the restore pos is gone.

We should simplify this to: if the document is loading, keep trying to restore. When the scroll frame is created we can check the loading status of the document.

I also noticed that PresShell::LoadComplete isn't quite what we want to be tracking as it only gets called on success. We also want to stop restoring on error or if the document loading was stopped. nsDocumentViewer::LoadComplete sets mLoaded, which is what I think we should track as for "loading/not loading".
Thanks for investigating! I'll write a patch that does what you suggested and see how that works. Also I'm amazed we don't have a test that covers this scenario, makes me feel less bad about the state of APZ tests :)
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(bugmail.mozilla)
^ This is mostly a backout of the previous patch, combined with the new implementation. I left the old test in place, and added a new one to cover this new scenario.
Attachment #8748175 - Flags: review?(tnikkel) → review+
Comment on attachment 8748175 [details]
MozReview Request: Bug 1269539 - Ensure that the scroll position is restored properly on reloading a page which loads incrementally. r?tnikkel

https://reviewboard.mozilla.org/r/50173/#review47061

::: docshell/base/nsIContentViewer.idl:44
(Diff revision 1)
>  
>    attribute nsIDocShell container;
>  
>    [noscript,notxpcom,nostdcall] void loadStart(in nsIDocument aDoc);
>    void loadComplete(in nsresult aStatus);
> +  [noscript] readonly attribute boolean loadCompleted;

Can we make this notxpcom as well? I'm not sure what nostdcall implies, but if it lets us get rid of cruft I'm for it.
Flags: needinfo?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #5)
> Can we make this notxpcom as well? I'm not sure what nostdcall implies, but
> if it lets us get rid of cruft I'm for it.

notxpcom is illegal on attributes, and nostdcall is a no-op for them, I believe.
https://hg.mozilla.org/mozilla-central/rev/9489784a0ea0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8748175 [details]
MozReview Request: Bug 1269539 - Ensure that the scroll position is restored properly on reloading a page which loads incrementally. r?tnikkel

Approval Request Comment
[Feature/regressing bug #]: bug 1268195
[User impact if declined]: reloading long pages that load incrementally can fail to restore the scroll position properly.
[Describe test coverage new/current, TreeHerder]: patch includes a test.
[Risks and why]: low/medium risk. See also risk assessment on bug 1268195; the two patches must be uplifted together. This patch backs out some of the changes from that bug so the overall footprint of them combined is smaller than either one in isolation.
[String/UUID change made/needed]: none
Attachment #8748175 - Flags: approval-mozilla-aurora?
47 is unaffected, and 48 will be affected if bug 1268195 is uplifted to 48. I'll flip it directly to fixed if we uplift both patches.
Comment on attachment 8748175 [details]
MozReview Request: Bug 1269539 - Ensure that the scroll position is restored properly on reloading a page which loads incrementally. r?tnikkel

This should uplift to aurora along with the patch in bug 1268195
Good for 48 since we aim to start rolling out apz/e10s in that release.
Attachment #8748175 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1274571
Blocks: 1286179
Depends on: 1305579
You need to log in before you can comment on or make changes to this bug.