496644 - when going back or forward, use the last pan x, y offsets

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Madhava Enros [:madhava]]

When a user clicks back or forward, they should end up at the position and zoom level on the previous (for next) page where the left it.  This is important for things like going through a list of links, backing up after each one.  To show page loading state, if that's required, we can superimpose the titlebar with a throbber going at the top of the screen.

Comment 1

[Benjamin Stover (:stechz)]

Patch for bug 514623 fixes this partially (it does not restore zoom).

Comment 2

[Aakash Desai [:aakashd]]

I saw this on the fennec b4 testday and i"m upgrading the severity to get some eyes on it.

Comment 3

[Benjamin Stover (:stechz)]

Attachment: WIP (aka: why isn't this working?)

This happens to fix the bug where anchors aren't jumped to on load, and also makes spacebar/up-down/pgup-pgdown scroll changes hide or show the titlebar appropriately.

But it does not fix this bug.  Docshell should set scroll position, but window utils is always returning 0,0.  I've tried flushing on getScrollXY and setTimeouts.

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7923

What am I doing wrong?

Comment 4

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

> But it does not fix this bug.  Docshell should set scroll position, but window
> utils is always returning 0,0.  I've tried flushing on getScrollXY and
> setTimeouts.

I bet getScrollXY returns 0 because we don't sync the browser scroll positions and the panning positions.

And the anchors probably works because we handle this case into handlePageScroll.

Comment 5

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

Comment on attachment 415746 [details] [diff] [review]
WIP (aka: why isn't this working?)


>   scrollContentToTop: function scrollContentToTop() {
 
>+  /** Update viewport to location of browser's scrollbars. */
>+  scrollViewportToBrowser: function scrollViewportToBrowser() {

scrollContentToBrowser() to match the method above

I am applying the patch and will test it out

Comment 6

[Benjamin Stover (:stechz)]

Attachment: Do what Vivien said

Vivien was totally correct.  I was very careful about scrolling the browser to the content pane since this invalidates the visible region--I only scroll when we start loading another page when the invalidation occurs anyway.

I had to push things around in resizeAndPaint so that 1) content scrolls to top before the page has finished loading but 2) content doesn't scroll to top before new page has rendered.

Comment 7

[Benjamin Stover (:stechz)]

Attachment: Hack

On testing, we discovered that hitting "back" from a smaller page to a larger page will not scroll the content pane to the correct position because MozScrollSizeChange has not occurred.

In these cases, the event to resize the scroll pane should be in the queue, so I used an executeSoon with an appropriate comment.  Other ideas:
1. Set a scroll flag somewhere.  On scroll size change, scroll the pane if the flag is set.  Fragile.
2. Have a method that allows you to set where the scroll pane *should* be.  On scroll change, scroll to these values if possible.  If user scrolls, reset the *should* values.  This is closer to the "right way."

Comment 8

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

It seems to work well! :)

Can we just prevent the rendering before scrolling?

Comment 9

[Benjamin Stover (:stechz)]

Attachment: Pause rendering until scrolled

Comment 10

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

Comment on attachment 416765 [details] [diff] [review]
Pause rendering until scrolled

>   endLoading: function() {
>     // Determine at what resolution the browser is rendered based on meta tag
>     let browser = this._browser;
>     let metaData = Util.contentIsHandheld(browser);
>+    let bv = this._browserView;

nit : change this line bt let bv = Browser._browserView; and move it next to the pauseRendering call, otherwise the patch won't work :)

Comment 11

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

Also i've the feeling to have seen the x,y positions to 0,0 when they should not be but I've not been able to repro. So for me this patch is a nice improvment and we can track the edge case (if it is real and not only my imagination) in a followup. That's a nice improvment!

Comment 12

[Benjamin Stover (:stechz)]

Attachment: Fix for google.com

I was testing on google and just noticed their page is now AJAXy, so back and forward are not going to do the appropriate thing.

This fixes an unsightly bug where the page would shrink on pressing "Back" after a google search, and there would suddenly be nothing visible.  It's a small fix and it is somewhat related to back/forward panning, so I tacked it on to this patch.

Comment 13

[Benjamin Stover (:stechz)]

Pushed: http://hg.mozilla.org/mobile-browser/rev/1fe9a7ef1495

Comment 14

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091211 Firefox/3.6b5pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091211 Firefox/3.7a1pre Fennec/1.0b5

Comment 15

[Aakash Desai [:aakashd]]

litmus testcase 9798 has been created for regression testing this bug.