Closed Bug 496644 Opened 13 years ago Closed 12 years ago

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

Categories

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

defect
Not set
major

Tracking

(fennec1.0+)

VERIFIED FIXED
Tracking Status
fennec 1.0+ ---

People

(Reporter: madhava, Assigned: stechz)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
tracking-fennec: --- → ?
tracking-fennec: ? → 1.0+
Assignee: nobody → rfrostig
Depends on: 514623
Patch for bug 514623 fixes this partially (it does not restore zoom).
I saw this on the fennec b4 testday and i"m upgrading the severity to get some eyes on it.
Severity: normal → major
Assignee: froystig → webapps
Status: NEW → ASSIGNED
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?
Attachment #415746 - Flags: review?(mark.finkle)
> 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 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
Attached patch Do what Vivien said (obsolete) — Splinter Review
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.
Attachment #415746 - Attachment is obsolete: true
Attachment #416196 - Flags: review?(21)
Attachment #415746 - Flags: review?(mark.finkle)
Attached patch Hack (obsolete) — Splinter Review
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."
Attachment #416196 - Attachment is obsolete: true
Attachment #416659 - Flags: review?(21)
Attachment #416196 - Flags: review?(21)
It seems to work well! :)

Can we just prevent the rendering before scrolling?
Attached patch Pause rendering until scrolled (obsolete) — Splinter Review
Attachment #416659 - Attachment is obsolete: true
Attachment #416765 - Flags: review?(21)
Attachment #416659 - Flags: review?(21)
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 :)
Attachment #416765 - Flags: review?(21) → review+
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!
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.
Attachment #416765 - Attachment is obsolete: true
Attachment #416795 - Flags: review?(21)
Pushed: http://hg.mozilla.org/mobile-browser/rev/1fe9a7ef1495
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Flags: in-litmus?
litmus testcase 9798 has been created for regression testing this bug.
Flags: in-litmus? → in-litmus+
Component: General → Panning/Zooming
You need to log in before you can comment on or make changes to this bug.