617952 - when user goes back or forward, use the last pan x, y offsets

Status: VERIFIED FIXED

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

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.

If this looks familiar, it's because we identified and fixed this once before in Bug 496644.

Comment 1

[Benjamin Smedberg]

This would be fixed in the frontend, so switching flags around.

Comment 2

[Wesley Johnston (:wesj)]

This is fixed by the patch in bug 479862.

Comment 3

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

(In reply to comment #2)
> This is fixed by the patch in bug 479862.

Are you sure? I just landed a patch for bug 479862 and this doesn't seem fixed

Comment 4

[Wesley Johnston (:wesj)]

Whoops. I was seeing something else apparently.

We do seem to restore scrollTop and scrollLeft on the document, but that isn't reflected in the display port. For this to be done correctly, we may also need to restore the scale, so my new plan is to use an nsISHistoryListener to maintain a separate list with scroll and scale information.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: WIP

Here's a WIP. I'm fighting a bunch of race conditions here where the contentDocumentWidth in the parent process, the viewport width seen by the nsIContentViewManager, and the width of the document in the child process all don't agree, but this seems close to working on desktop.

Just a little more massaging to do....

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Still doing some testing, but this seems to work fairly well. Restores scroll position, zoom, and textZoom. We don't currently provide very good interfaces for dealing with those things though, so there's occasional glitches.

For instance, textZoom seems to rely on a parameter, _isZoomedToElement, which I'm not messing with. We could provider a way for things to register with this "saved-state" provider, and then send data that they want saved, receive data when we want to restore.

I'm going to try running with this over the weekend, and if everything is ok I'll put up for review. But wanted some feedback on whether or not we want something more advanced here for final.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v1.1

This works pretty well, but asking for feedback while I test it out more and try to narrow down some edge cases occasionally creep up.

Comment 8

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

I am worried about creating a second system to hold a list of scroll and scale values for a session history entry.

I think we could do this in the platform?

nsISHistory holds a list of nsIHistoryEntry and a nsIHistoryEntry can be QI'ed to a nsISHEntry. A nsISHEntry holds some state for a given page in history, found in the layoutHistoryState property:
http://mxr.mozilla.org/mozilla-central/source/docshell/shistory/public/nsISHEntry.idl#125

I am not sure if the nsILayoutHistoryState is scriptable or not, but we could try storing the scale in there. It would be even better if we could get the platorm to handle all aspects of this system, but I would be happy with Fennec using the the nsISHEntry to hold all the data.

Comment 9

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

Looks like scroll position is restored here:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8351

Comment 10

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

I followed the scroll restore code in comment 9 and found that we should end up firing a "scroll" event here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#1773

Comment 11

[Wesley Johnston (:wesj)]

nsILayoutHistoryState is not available to script at all as far as I can tell. Alternatively, there is "stateData" which is available to us and to web pages, but might open us to some strange attack vectors. I'm digging a little to see if there are any other interfaces around that already give us access to the current nsILayoutHistoryState.

It sounds like you would rather attack this entirely from the platform side though? In that case, we could expose a getter and a setter in nsISHEntry_MOZILLA_2_0 for the nsILayoutHistoryState, to store our scale and other info in for now.

Comment 12

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

Or just add a getScale method to nsISHEntry_MOZILLA_2_0 similar to the getScrollPosition method.

Also, why don't we get the "scroll" event fired when the docShell sets the scroll from history here:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8351

Comment 13

[Wesley Johnston (:wesj)]

"scroll" does fire sometimes, most noticeable by us hiding the sidebars when you hit back sometimes, but we also sometimes actually scroll to the right place. That's why I thought this was originally fixed by bug 479862.

It doesn't always seem to work. I suspect that is due to some race condition between the event firing and something like XULScrollAreaChanged also firing. Looking into that...

It also won't work if the user is zoomed, as that scroll position may not be available anymore. Similar problems happen if we don't also restore textZoom. Those are what make me think it would be better to create generic methods to modify nsILayoutHistoryState rather than a few particular fields.

Comment 14

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

Comment on attachment 511184 [details] [diff] [review]
Patch v1.1

feedback- based on our discussion about moving some of this into the platform

Comment 15

[Wesley Johnston (:wesj)]

Current plan is just to focus on restoring scroll position here, and work on restoring scale and textZoom separately.

I think the problem I am seeing with this not always happening is due to the bfcache. Pages loaded from it don't seem to hit the PressShell::RestoreRootScrollPosition() calls.

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Mostly working patch. Trouble here is mostly in keeping the parent and child in sync. In cases that are not coming from the bfcache, we post a scroll event and the parent is updated. In the bfcache case, we need to update on pageshow so that the parent's nsIContentView will be in sync with the child's scroll position.

In some of my initial testing I was using a shopping site, where each page was the same height, and I was scrolling to the bottom of each one. In that case we were bailing out of updating the scroll position, so I've added some code to reset the scroll cache on pagehide.

MozScrollAreaChanged events cause the rest of the problems, and I think they are independent of whether we are coming from the bfcache or not. They race with these "scroll" events to prevent/enable scrolling to a particular position, as well as causing size and layout changes during the initial page load.

To deal with that I am updating the scroll position again on MozScrollAreaChanged events, and checking to see if the document has scrolled to the correct position. If the document size changes, I also update the scrollPosition. If the user scrolls the page, I remove the saved data so that we don't continue trying to scroll the page ourselves.

This works fairly well. I'm going to test over the weekend, and make sure there aren't major regressions, but would appreciate feedback.

Comment 17

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

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

>diff --git a/chrome/content/bindings/browser.js b/chrome/content/bindings/browser.js

>+      case "pagehide":
>+        ContentScroll.reset();
>+      case "pageshow": {
>+        if (aEvent.type == "pageshow") {
>+          ContentScroll.doScroll();
>+        }

no {} needed

>+  doScroll: function() {
>+    let scrollOffset = this.getScrollOffset(content);
>+    if (this._scrollOffset.x == scrollOffset.x && this._scrollOffset.y == scrollOffset.y)
>+      return;
>+
>+    let msg = this._scrollOffset = scrollOffset;
>+    sendAsyncMessage("scroll", scrollOffset);
>+  },

"msg" is unused

>diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml

>               case "MozScrolledAreaChanged":

>+                let scroll = self._restoreScroll;
>+                if (scroll && self.scrollTo(scroll)) {
>+                     self._restoreScroll = null;
>+                } else {
>+                  let view = self.getRootView();
>+                  view.scrollBy(0, 0);

* No need for local "scroll" variable
* "_restoreScroll" confuses me. I don't fully understand the reason why "MozScrolledAreaChanged" is racing with "scroll"

>+      <method name="scrollTo">

We already have a "scrollTo" API on the view. I sense confusion ahead.

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>   dragStart: function dragStart(clientX, clientY, target, scroller) {
>     let browser = getBrowser();
>+    if (browser._restoreScroll)
>+      browser._restoreScroll = null;

This seems like a cry for help. I don't like the front-end updating the binding

f+ for getting a wip approach, but I think we need to find the bugs lurking in our impl instead of using a bandaid

Comment 18

[Wesley Johnston (:wesj)]

> * "_restoreScroll" confuses me. I don't fully understand the reason why
> "MozScrolledAreaChanged" is racing with "scroll"

There's no guarantee whether we get the “scroll” message before or after we get the “MozScrolledAreaChanged” message (which tells us that a particular scroll position is available). In one test on desktop I get:

Back Button Hit
Content:LocationChange - http://planet.mozilla.org/
ReceiveMessage:MozScrolledAreaChanged - {"width":800,"height":10316.216796875}
ReceiveMessage:Scroll - {"x":0,"y":14060}
ReceiveMessage:MozScrolledAreaChanged - {"width":800,"height":105542.484375}

We could force the page to scroll to this position, but on AJAXy pages the scroll position may never be available.

These same MozScrolledAreaChanged events also trigger window resizes between 800 and 980px wide viewports, so there is an additional race between whether browser.xml or browser.js handles the MozScrolledAreaChanged events first.

> This seems like a cry for help. I don't like the front-end updating the binding

Sorry :( That was a hack until I thought of a better way. Platform has some similar problems, and likely I will need to do something similar to what they do (i.e. store our last position as well, and make sure nothing changed):

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2100

> f+ for getting a wip approach, but I think we need to find the bugs lurking in
> our impl instead of using a bandaid

There are only two bugs here I'm aware of here:

1.) Size changes aren't maintaining scroll position. We have fancy code in our parent resize handler to deal with this, but nothing in our Tab.updateViewportSize code that I know of. I added some simple code to do that here, but will try to do something more advanced.

2.) bfcache doesn't restore the nsIContentView display port in the parent. I'll have to dig to discover what needs to be done to fix this, but I think it might be out of scope for 4.0.

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Patch based on talking to mfinkle today. Keeping this simple for now. This fires a scroll event from pageshow if the transition is from the bfcache. It also attempts to do the scroll a few times (3) when we receive "scroll" events with setTimeout zeros.

Comment 20

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

Comment on attachment 515791 [details] [diff] [review]
Patch v3

>diff --git a/chrome/content/bindings/browser.js b/chrome/content/bindings/browser.js

>+  doScroll: function doScroll() {
>+    let scrollOffset = this.getScrollOffset(content);
>+    if (this._scrollOffset.x == scrollOffset.x && this._scrollOffset.y == scrollOffset.y)
>+      return;
>+
>+    this._scrollOffset = scrollOffset;
>+    sendAsyncMessage("scroll", scrollOffset);
>   }
> };

Let's call it "sendScroll" instead of "doScroll" so it's more obvious that we are sending a message

>diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml

>       <field name="_messageListenerRemote"><![CDATA[
>         ({
>           self: this,
>+          attemptNum: 0,
>           receiveMessage: function receiveMessage(aMessage) {
>             let self = this.self;
>             let json = aMessage.json;
> 
>             switch (aMessage.name) {
>               case "scroll":
>                 if (!self.scrollSync)
>                   return;
>-
>-                // Use floor so that we always guarantee top-left corner of content is visible.
>-                let view = self.getRootView();
>-                view.scrollTo(Math.floor(json.x * self.scale), Math.floor(json.y * self.scale));
>+                this.doScroll(json.x, json.y);
>                 break;
>            }
>+         },
>+
>+         doScroll: function doScroll(aX, aY) {
>+            let self = this.self;
>+            // Use floor so that we always guarantee top-left corner of content is visible.

add a blank line before the comment

>+            let view = self.getRootView();
>+            view.scrollTo(Math.floor(aX * self.scale), Math.floor(aY * self.scale));
>+
>+            let position = view.getPosition();
>+            if ((position.x != aX*self.scale || position.y != aY*self.scale) && this.attemptNum < 3) {

add spaces around the "*"

>+              setTimeout((function() {
>+                 this.attemptNum++;
>+                 this.doScroll(aX, aY);
>+              }).bind(this), 0);
>+            } else {
>+               this.attemptNum = 0;
>+            }
>          }
>        })

I think we can remove the need for this.attemptNum if we make an additional param to doScroll 

           doScroll: function doScroll(aX, aY, aCount) {
             let self = this.self;

             // Use floor so that we always guarantee top-left corner of content is visible.
             let view = self.getRootView();
             view.scrollTo(Math.floor(aX * self.scale), Math.floor(aY * self.scale));

             let position = view.getPosition();
             if ((position.x != aX * self.scale || position.y != aY * self.scale) && aCount < 3) {
               setTimeout((function() {
                  this.doScroll(aX, aY, ++aCount);
               }).bind(this), 0);
             }
           }

if aCount is not passed in, then it acts like 0 (zero)

Test this and see if it works the same as before. I want to see the final patch in the bug, just for a final look.

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch v3.1

Not sure how to mark review, so I just carried over the r+. This addresses comments and works.

Comment 22

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

Looks good. Land this puppy.

Comment 23

[Wesley Johnston (:wesj)]

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

Comment 24

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

Just a note: This bug is only about restoring the scroll position, not zoom

Comment 25

[Tony Chung [:tchung]]

Reopening, as i still do not see this fix on nightly: Mozilla/5.0 (Android; Linux armv71; rv:2.0b13pre) Gecko/20110303 Firefox/4.0b13pre Fennec/4.0b6pre	

Repro
1) load planet.mozilla.org
2) In same tab, load slashdot. 
3) In slashdot, scroll down to some page position that you will remember.
4) hit fennec Back.  Then hit forward.
5) Verify slashdot redraws at the very top.
6) Do the same as steps 1-5, but replace the with back instead of forward.

Comment 26

[Wesley Johnston (:wesj)]

Some issues will be resolved in bug 638170. I think we should wait and see if that handles this issue. However, sites like planet lay out so slowly on device that there is still a chance this won't work even after.

Making this work 100% of the time is too general a problem to be a blocker. Its a metabug with a few small issues contributing to it. I should have renamed this to designate the real issue that we fixed, but unless there is an argument against, I'm going to rename to "Moving back or forward in the bfcache should fire scroll events." and close it later.

Comment 27

[Wesley Johnston (:wesj)]

Bug 638170 fixed this for me. Marking resolve fixed for now.

Comment 28

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Maemo; Linux armv7l; rv:2.0b13pre) Gecko/20110314
Firefox/4.0b13pre Fennec/4.0b6pre

and:
Mozilla/5.0 (Android; Linux armv7l; rv:2.0b13pre) Gecko/20110314
Firefox/4.0b13pre Fennec/4.0b6pre

I've verified this for the simple case for http://nu.nl
Steps I verified it with:
- Go to http://nu.nl
- Scroll down a bit
- Tap on a link
- Scroll down on the new page
- Go back and forward in history.

However, the case which Tony mentioned in comment 25 doesn't work, I've file bug 641787 for that.