User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130219 Firefox/21.0 Build ID: 20130219031055 Steps to reproduce: 1. Open https://bugzilla.mozilla.org/show_bug.cgi?id=718264#c1 2. Scroll to the bottom / top 3. Reload page Actual results: Page went back to the anchor position Expected results: Page should have stayed at the bottom / top (or wherever it was before)

Steps To Reproduce: 1. Open testcase 2. Click a link "Click to scroll to anchor" 3. Scroll up few times 4. Reload (F5) Actual Results: Scroll position is not restored. Always scroll to the anchor. Expected Results Scroll position should be restored at previous position. IE Chrome Opera and Firefox 18 and earlier restore the position. Regression window(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/335830f44719 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121115052751 Bad: http://hg.mozilla.org/mozilla-central/rev/a37525d304d9 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121115081852 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=335830f44719&tochange=a37525d304d9 Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/8072a58a9e86 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121114195550 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/34a4de5feafe Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121114224150 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8072a58a9e86&tochange=34a4de5feafe Suspected: Bug 811301

We restore the scroll position correctly from the state, but there's a ScrollToRef() call that stomps on it. There's a mDidHistoryRestore on the scroll frame that we could use to ignore the first ScrollToAnchor(). Seems to work locally... https://tbpl.mozilla.org/?tree=Try&rev=f6d2be7e4bc5

Not critical enough to track for upcoming releases since most scroll position is maintained. We'd accept a low risk uplift.

Comment on attachment 720430 [details] [diff] [review] fix+test Review of attachment 720430 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsPresShell.cpp @@ +2889,5 @@ > + nsIScrollableFrame* rootScroll = GetRootScrollFrameAsScrollable(); > + if (rootScroll && rootScroll->DidHistoryRestore()) { > + // Scroll position restored from history trumps scrolling to anchor. > + aScroll = false; > + rootScroll->ClearDidHistoryRestore(); What about non-root scroll frames? Seems like the same issue can happen there? Why do we need to call ClearDidHistoryRestore?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5) > What about non-root scroll frames? Seems like the same issue can happen > there? I'm not sure what you mean. Restoring the scroll position from history works fine for all scroll frames currently, it's just that GoToAnchor() mess it up for the root scroll frame. > Why do we need to call ClearDidHistoryRestore? It was needed at some point but it appears it's not in the final patch. I'll take it out if you want, assuming Try is green: https://tbpl.mozilla.org/?tree=Try&rev=254b5e28b45c

(In reply to Mats Palmgren [:mats] from comment #6) > I'm not sure what you mean. Restoring the scroll position from history > works fine for all scroll frames currently, it's just that GoToAnchor() > mess it up for the root scroll frame. Couldn't GoToAnchor mess up other scroll frames too? ScrollContentIntoView will scroll any nested scrollframes containing the anchor.

Yes, GoToAnchor's call to ScrollContentIntoView may scroll arbitrary scrollframes on the page. The patch suppresses that call to ScrollContentIntoView, leaving all scrollframes at their restored scroll position. Sorry for mentioning "root scroll frame" in my last comment, I can see how that's misleading. What I meant is that there's only this ScrollContentIntoView call to deal with, not any kind of direct calls on the nested scroll frames themselves.

Comment on attachment 720430 [details] [diff] [review] fix+test Review of attachment 720430 [details] [diff] [review]: ----------------------------------------------------------------- OK with ClearDidHistoryRestore removed.

It is not fully fixed. My 20130307 Nightly still loses scroll position after reload if I scroll up to the _very_ top of the page in comment 1 testcase.

(In reply to Sid from comment #12) > It is not fully fixed. My 20130307 Nightly still loses scroll position after > reload if I scroll up to the _very_ top of the page in comment 1 testcase. Filed Bug 849219

Good catch, thanks!

Filed follow up Bug 851485

Mats, FYI: the test added in this bug has this line: <link rel="stylesheet" href="842853.sjs"> but the sjs file is named "file_bug842853.sjs". Not sure if the test is still testing what it's meant to be testing?

I usually verify locally that tests fail without the code change so I hope I did so in this case too. I suspect that I developed the test using 842853.sjs then realized at the last moment that the file name didn't follow naming conventions and renamed it but forgot to update this href. Anyway, I pushed a fix to Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebef3c1e8382