scroll position can not be retained, moving forward and backward between the nested pages

RESOLVED FIXED

Status

()

Core
Document Navigation
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: Hideo Saito, Assigned: Brian Ryner (not reading))

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

13 years ago
latest from 20050629/trunk but not 20050628/trunk

Scroll position can not be retained, when moving forward and backward between the nested pages.

steps are:
1. load a large page and scroll down
2. click a link and go to new page
3. repeat from 1 to 2 of the step(about 5 times)
4. click Back button until return to the first page
5. click Forward button until return to the last page
6. repeat from 4 to 5 of the step

actual result:
the first page or last page is reloaded and scroll positions are reset.

expected result:
scroll position is retained in spite of the number of nests.

Comment 1

13 years ago
After the patch for bug 292965 landed, the number of cached pages stored depends on the amount of memory available. If you want to have more pages stored, then change browser.sessionhistory.max_total_viewers to the number of pages you want, instead of leaving it at -1 which causes it to be based on the available memory.


Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → INVALID
now wait a second, whether the scroll position is restored is unrelated to bfcache. reopening.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---

Comment 3

13 years ago
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051227 Firefox/1.6a1

I tried reproducing this in a couple of ways: first I tried it with bfcache off, and couldn't reproduce it, then I tried it with bfcache based on the amount of memory available (max_total_viewers at -1) -- same thing. Then I tried with max_total_viewers at 5, and still couldn't reproduce it even if there were > 5 pages in the history. So, this works for me.

Hideo, could you provide more detailed steps to reproduce? Such as what page you start out and, and end up at, etc.?
(Reporter)

Comment 4

13 years ago
Created attachment 207088 [details]
testcase holder

some files(link.html, link2.html, ...) in testcase holder

I can reproduce it first loading link.html on testcase.
I can not reproduce it, if browser.sessionhistory.max_total_viewers is changed to zero, however, I can reproduce it, if it is -1, 1 or 10.
(Reporter)

Comment 5

13 years ago
Created attachment 207255 [details] [diff] [review]
patch

Before swaping the viewer, the layout state is captured since the scroll position will be restored after exceeding cached viewers of session history.
Attachment #207255 - Flags: review?(bzbarsky)
Comment on attachment 207255 [details] [diff] [review]
patch

I don't really know this code. Try bryner.  That said, does this also fix bug 321778 by any chance?
(Reporter)

Comment 7

13 years ago
Comment on attachment 207255 [details] [diff] [review]
patch

This patch is effective only about restoration of a scroll position.
Attachment #207255 - Attachment is obsolete: true
Attachment #207255 - Flags: review?(bzbarsky)
(Reporter)

Comment 8

13 years ago
Created attachment 207304 [details] [diff] [review]
patch for scroll position
Attachment #207304 - Flags: review?(bryner)
(Reporter)

Updated

13 years ago
Attachment #207304 - Attachment is obsolete: true
Attachment #207304 - Flags: review?(bryner)
(Reporter)

Comment 9

13 years ago
Created attachment 207341 [details] [diff] [review]
patch

This patch is effective about restoration of a scroll position and loss of text on text control area.
Attachment #207341 - Flags: superreview?(bryner)
Attachment #207341 - Flags: review?(bryner)
Assignee: nobody → saito
Status: REOPENED → NEW
(Reporter)

Comment 10

13 years ago
Created attachment 207342 [details]
testcase holder for patch

I tested setting the browser.sessionhistory.max_total_viewers preference to default(-1), scroll down and type text in the text area, click a link, and click Back button.
(Reporter)

Updated

13 years ago
Blocks: 321778
So why does that patch help?  I recommend putting the answer in comments in the code...
(Assignee)

Comment 12

13 years ago
Comment on attachment 207341 [details] [diff] [review]
patch

I think _probably_ what needs to happen is just to uncomment the "XXX PersistLayoutHistoryState();" up towards the beginning of RestoreFromHistory.  That's the equivalent ordering for what happens in Embed().

I'm confused by the EvictWindowContentViewer change, what problem is this fixing?
(Assignee)

Updated

13 years ago
Attachment #207341 - Flags: superreview?(bryner)
Attachment #207341 - Flags: superreview-
Attachment #207341 - Flags: review?(bryner)
Attachment #207341 - Flags: review-
(Assignee)

Comment 13

13 years ago
Created attachment 207400 [details] [diff] [review]
patch

I think this is all it needs... not sure why I didn't catch that XXX comment before checking this in originally.
Assignee: saito → bryner
Status: NEW → ASSIGNED
Attachment #207400 - Flags: review?(cbiesinger)
(Reporter)

Comment 14

13 years ago
Created attachment 207408 [details] [diff] [review]
patch

bryner, please check this patch again, because your patch is failed for my testing.

The cause of loss of added text on text control area is that added text is not synchronized when closing and destruction of the DocumentViewer frame issue from exceeding the number of cached viewers of session history.

I think that viewer->Destroy() is called after ownerEntry->SyncPresentationState is called.
Attachment #207341 - Attachment is obsolete: true
Attachment #207408 - Flags: review?(bryner)
(Reporter)

Comment 15

13 years ago
Comment on attachment 207408 [details] [diff] [review]
patch

bryner, I am sorry for my excessive request. My testing for your patch is successful as to restored scroll position.
Attachment #207408 - Flags: review?(bryner)
(Reporter)

Updated

13 years ago
Attachment #207408 - Attachment is obsolete: true
Attachment #207400 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 16

13 years ago
checked in on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 17

13 years ago
Comment on attachment 207400 [details] [diff] [review]
patch

We should get this in for FF2.  It probably doesn't fit the criteria for 1.5.0.x but I'll go ahead and nominate it in case branch drivers want it.
Attachment #207400 - Flags: approval1.8.1?
Attachment #207400 - Flags: approval1.8.0.2?

Updated

13 years ago
Attachment #207400 - Flags: approval1.8.1? → branch-1.8.1+
Comment on attachment 207400 [details] [diff] [review]
patch

not approved for 1.8.0 branch
Attachment #207400 - Flags: approval1.8.0.2? → approval1.8.0.2-

Updated

10 years ago
Component: History: Session → Document Navigation
QA Contact: history.session → docshell
Created attachment 387027 [details] [diff] [review]
mochitest test case
Attachment #387027 - Flags: review?(bzbarsky)
Jonathan, I think the key for this bug was that the first page loaded got evicted from bfcache after being initially placed into it.  So the testcase needs to assert that (at the very least) the last back() call executed leads to a load that's not coming from bfcache.  That way if we change the cache limit to more than 5 the test will fail and we'll know we need to bump the limit.

To make that easier, you should probably make the 5 a const declared up top in the script.

And for a spelling nit, s/it's/its/.  ;)
Oh, and it might be good to also assert that each of the pages does go into bfcache as we navigate initially (so assert that the pagehides all have persisted == true).
Oh, and also nix the random whitespace change in the makefile?
Created attachment 387729 [details] [diff] [review]
mochitest test case

Thanks for the review Boris.  I'm attaching a new version which addresses all your comments.  Instead of making a constant for 5 (the number of pages we cycle through in the test), I made one for MAX_BFCACHE_PAGES = 3 (the max number of pages stored in the bfcache for the current session history).  Test passes on trunk.
Attachment #387027 - Attachment is obsolete: true
Attachment #387729 - Flags: review?(bzbarsky)
Comment on attachment 387729 [details] [diff] [review]
mochitest test case

Yeah, that looks much better.  I just realized that this test would pass if the scrolling just did nothing (e.g. if the window were too big for some reason).  Can you add a check that the scroll position after scrolling is nonzero and before scrolling is 0?

With that, looks great.
Attachment #387729 - Flags: review?(bzbarsky) → review+
Created attachment 387785 [details] [diff] [review]
mochitest test case

Thanks for the suggestion Boris; I've added the pre- and post-scroll checks as you recommended.
Attachment #387729 - Attachment is obsolete: true
Attachment #387785 - Flags: review?(bzbarsky)
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.