Closed Bug 842853 Opened 7 years ago Closed 7 years ago

scrolling position lost after page reload on url with anchor

Categories

(Core :: Layout, defect)

19 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox19 - wontfix
firefox20 - affected
firefox21 - affected
firefox22 - fixed

People

(Reporter: rodrigozeh, Assigned: mats)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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)
Attached file testcase
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
Blocks: 811301
Component: Untriaged → Layout
Keywords: regression
Product: Firefox → Core
Version: 21 Branch → 19 Branch
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch wip (obsolete) — Splinter Review
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.
Attached patch fix+testSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=94f7aedd87ad
Attachment #715883 - Attachment is obsolete: true
Attachment #720430 - Flags: review?(roc)
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.
Attachment #720430 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd280c980a1b
Flags: in-testsuite+
OS: Windows 7 → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/bd280c980a1b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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
Duplicate of this bug: 718264
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?
Flags: needinfo?(mats)
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
Flags: needinfo?(mats)
You need to log in before you can comment on or make changes to this bug.