Closed Bug 638170 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec-)

RESOLVED DUPLICATE of bug 638164
Tracking Status
fennec - ---

People

(Reporter: martijn.martijn, Assigned: wesj)

References

Details

(Whiteboard: [has patch])

Attachments

(2 files, 2 obsolete files)

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.
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?
Mark Finkle mentioned on IRC this is probably related to bug 617952.
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).
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #516425 - Flags: review?(21)
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.
Attachment #516425 - Flags: review?(21) → review+
Assignee: nobody → wjohnston
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
Depends on: 617952, 622601
OS: Windows 7 → All
Hardware: x86 → All
Whiteboard: [has patch]
Attached patch Patch v1.1 (obsolete) — Splinter Review
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.
Attachment #516425 - Attachment is obsolete: true
Attachment #516614 - Flags: review+
Attachment #516614 - Flags: approval2.0?
Attachment #516614 - Flags: approval2.0?
Attached patch Patch v2Splinter Review
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.
Attachment #516614 - Attachment is obsolete: true
Attachment #516647 - Flags: review?(21)
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.
Attachment #516647 - Flags: review?(21) → review+
tracking-fennec: ? → 2.0+
Pushed:

http://hg.mozilla.org/mobile-browser/rev/cece41b37315
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
tracking-fennec: 2.0+ → ?
is there a regression range?  Did this *just* stop working?
Attached image 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.
This seems to be exactly like bug 638164. Patches that fix bug 638164 fix this
bug too.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 638164
tracking-fennec: ? → -
You need to log in before you can comment on or make changes to this bug.