638170 - Scroll position isn't restored on www.mozilla.org

Status: RESOLVED DUPLICATE of bug 638164

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

More or less a follow-up from bug 622601.

Steps to reproduce:
1- Go to http://www.mozilla.org
2- Scroll downwards to the bottom of the page
3- Tap on the "Our Mission" link
4- Scroll down on the resulting page, e.g. "The Mozilla Mission" page
5- Go back in history
6- Go forward in history

Expected result:
The scroll position on http://www.mozilla.org should get restored, when going back in history, e.g. step 5.
When going forward in history, e.g. step 6, the "The Mozilla Mission" page should get also its scroll position restored.

Actual result:
Not after step 5, nor after step 6, the scroll position gets restored.

Comment 1

[Martijn Wargers (dead)]

I'm also seeing this on http://www.mountaindragon.com/html/marquee.htm
Perhaps it has something to do with dynamic content that is changing on unload or something?

Comment 2

[Martijn Wargers (dead)]

Mark Finkle mentioned on IRC this is probably related to bug 617952.

Comment 3

[Wesley Johnston (:wesj)]

I would suspect for these pages it is more of a timing problem. We attempt to go to the position three times using setTimeout(foo, 0).

Tends to work on desktop where things happen quickly. On device, the delays between events seem to be larger. If I change that to setTimeout(foo,100) it works more often (not that I'm proposing that is a great solution).

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Vivien pointed out to me some code that landed between my previous patch and this one which resets the scroll position. This has it reset to our current scroll position.

Helps fix this problem.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 516425 [details] [diff] [review]
Patch v1

I'm wondering if calling sendScroll() is necessary on pageshow for page coming from bfcache if the x/y scroll positions are sent this way? (Basically I wonder if the firstpaint of such page are not a full display of the page)

r+ with this question answered.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch v1.1

This seems to work fine as a replacement. Seeing some things related to 638164, but I will try to fix them in a separate bug.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Whoops. Forgot about scale in the previous patch. I think Bug 638164 is also being caused by the FirstPaint change, which can result in a late breaking change to the scale of the page, which does not necessarily reset the scroll.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 516647 [details] [diff] [review]
Patch v2

(In reply to comment #7)
> Created attachment 516647 [details] [diff] [review]
> Patch v2
> 
> I think Bug 638164 is also 
> being caused by the FirstPaint change, which can result in a late breaking
> change to the scale of the page, which does not necessarily reset the scroll.

I think that you're right.

>diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml
>+
>+          // ensure that we are scrolled within the page's viewable area
>+          view.scrollBy(0,0);
>+          view._updateCacheViewport();

rootView.scrollBy(0,0) could possibly trigger a call to updateCacheViewport so we could end with two calls.
I know that Ben would told me that it is cheap but I don't like that, having the feeling to do twice the work.
I wonder if we should refactor scrollBy to not call _updateCacheViewport by itself and add an API for knowing when there is a need for a refresh...
bah, just speaking out loud here, this is out of the scope of this bug.


>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>-    browser.messageManager.addMessageListener("Browser:FirstPaint", function(aMessage) {
>+    browser.messageManager.addMessageListener("Browser:FirstPaint", function firstPaintListener(aMessage) {

Thanks for naming the anonymous function, I tend to forgot when I used them that way!

>+        browser.getRootView().scrollTo(Math.floor(json.x * browser.scale),
>+                                       Math.floor(json.y * browser.scale));
>         Browser.pageScrollboxScroller.scrollTo(0, 0);
>       }
>-

Please keep the new line.

Comment 9

[Wesley Johnston (:wesj)]

Pushed:

http://hg.mozilla.org/mobile-browser/rev/cece41b37315

Comment 10

[Anna (Waverley)]

I can still reproduce it following the steps mentioned by Martijn.

I tried with nytimes.com, because the page is longer. I scrolled the page to the bottom, tapped on a link and when I went back (either by tapping the system menu back button or the "Back" button from the right side panel) the page position was not restored correctly (it was restored at the top of the page).
I am opening the bug.
 

Build Id: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110316 Firefox/4.0b13pre Fennec /4.0b6pre 

Devices: Motorola Droid 2(Android 2.2), HTC Desire (Android 2.2)

Comment 11

[Doug Turner (:dougt)]

is there a regression range?  Did this *just* stop working?

Comment 12

[Martijn Wargers (dead)]

Attachment: Screenshot

It seems to work for me in landscape mode, but in portrait mode it's broken.
I'm seeing a gray block at the "Our Mission" page ( http://www.mozilla.org/about/mission.html ), see screenshot. Maybe that has something to do with it?
Tested on the Droid, using yesterday's trunk build and on the N900, using yesterday's trunk build.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

This seems to be exactly like bug 638164. Patches that fix bug 638164 fix this
bug too.