Closed Bug 537694 Opened 14 years ago Closed 14 years ago

scrolling occasionlly snaps to top of page (on pageload?)

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Linux
defect
Not set
normal

Tracking

(fennec1.0+)

VERIFIED FIXED
Tracking Status
fennec 1.0+ ---

People

(Reporter: ted, Assigned: vingtetun)

References

Details

Attachments

(1 file, 2 obsolete files)

While using Fennec, I noticed that occasionally the scroll position would snap to the top of the page. I think (but I'm not sure) that it corresponded to the pageload finishing. i.e., I would load a page, scroll down while it continued loading, then the page would snap back to the top when it finished.
This happen at the end of the page load because of http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#662 where pos.x and pos.y are at 0

Step to reproduce: 
 * Go to p.m.o
 * while loading pan a bit the bottom
 * wait until the end of page load

Actual result:
 * page is scrolled back at 0,0 at the end

Expected result:
 * nothing special
Attached patch Patch (obsolete) — Splinter Review
I place the scrollContentToBrowser call during the documentStop because this way the document is fully loaded and the position really reflect the content
Attachment #419924 - Flags: review?(webapps)
I've run into this as well, and find it to be a pretty annoying usability issue. A page is mostly loaded so I start to scroll, and then finally the last bit loads and snaps me back up to the top. Just an FYI as far as priority goes, this one drives me personally crazy :)
Comment on attachment 419924 [details] [diff] [review]
Patch

>   scrollContentToBrowser: function scrollContentToBrowser() {
>     let bv = this._browserView;
>     let pos = BrowserView.Util.getContentScrollOffset(this.selectedBrowser);
>-    pos.map(bv.browserToViewport);

Is this intentional?

Patch seems to work ok. Will test more.
Attachment #419924 - Flags: review+
(In reply to comment #4)
> (From update of attachment 419924 [details] [diff] [review])
> >   scrollContentToBrowser: function scrollContentToBrowser() {
> >     let bv = this._browserView;
> >     let pos = BrowserView.Util.getContentScrollOffset(this.selectedBrowser);
> >-    pos.map(bv.browserToViewport);
> 
> Is this intentional?
> 
> Patch seems to work ok. Will test more.


Yes it is. That's one of the reason why I want Ben2 input on that. I've remove this because the content was scrolling a bit to the top/bottom with it when doing a simple refresh. (a bit = related to the zoom factor)

Step to repro:
 * go to any website where you can scroll to the bottom
 * scroll to the bottom
 * show one of the sidebar
 * hit the refresh button

Actual result:
 * The content scroll to the top/bottom

Expected result:
 * Nothing

In fact it can be considered as an other bug if we really want to track that.
Attached patch Patch (obsolete) — Splinter Review
The patch here resolve this bug and also bug 538573.
This one should be more robust.
Assignee: nobody → 21
Attachment #419924 - Attachment is obsolete: true
Attachment #420731 - Flags: review?(webapps)
Attachment #420731 - Flags: review?(mark.finkle)
Attachment #419924 - Flags: review?(webapps)
tracking-fennec: --- → 1.0+
Comment on attachment 420731 [details] [diff] [review]
Patch

>   /** Update viewport to location of browser's scrollbars. */
>   scrollContentToBrowser: function scrollContentToBrowser() {
>-    let bv = this._browserView;
>     let pos = BrowserView.Util.getContentScrollOffset(this.selectedBrowser);
>-    pos.map(bv.browserToViewport);

Why is browserToViewport gone?  This *should* be necessary to scroll back to the proper place for websites with a zoom level of not 1.  I tested on CNN and it's fine though.  What's going on here?

>-    else
>-      Browser.pageScrollboxScroller.scrollTo(0, 0);

This was first used to make sure urlbar was visible when page was loaded, but then we got scrollContentToTop

>+    else if (aStateFlags & Ci.nsIWebProgressListener.STATE_IS_DOCUMENT) {
>+      if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
>+        this._documentStop();
>+      }
>+    }

So we have a document stop again.  Do you know if this impacts page loading times at all?  I think gavin had removed this because 1) it wasn't being used and 2) it added some time to page load.

>+  _documentStop: function _documentStop() {
>+    if (this._tab == Browser.selectedTab) {
>+      // XXX Sometimes MozScrollSizeChange has not occurred, so the scroll pane will not
>+      // be resized yet. We are assuming this event is on the queue, so scroll the pane
>+      // "soon."
>+      Util.executeSoon(function() {
>+        let scroll = Browser.getScrollboxPosition(Browser.contentScrollboxScroller);
>+        if (scroll.x == 0 && scroll.y == 0)

Use isZero.

>+          Browser.scrollContentToBrowser();
>+      });
>+    }
>+    else {
>+      let scroll = BrowserView.Util.getContentScrollOffset(this._tab.browser);
>+      this._tab.scrollOffset = { x: scroll.x, y: scroll.y };
>+    }

Use Point instead of custom object.

Would be r+ with nits, but I'm concerned about page load impact and browserToViewport (even though it seems to work fine).
(In reply to comment #7)
> (From update of attachment 420731 [details] [diff] [review])
> >   /** Update viewport to location of browser's scrollbars. */
> >   scrollContentToBrowser: function scrollContentToBrowser() {
> >-    let bv = this._browserView;
> >     let pos = BrowserView.Util.getContentScrollOffset(this.selectedBrowser);
> >-    pos.map(bv.browserToViewport);
> 
> Why is browserToViewport gone?  This *should* be necessary to scroll back to
> the proper place for websites with a zoom level of not 1.  I tested on CNN and
> it's fine though.  What's going on here?
> 

I guess something has changed somewhere because the behavior you described is the reason why i have removed this call, when reloading a webpage i observe it scrolling to the top more or less depending on the zoom factor.

But if we have any doubt removing this line is not necessary for the patch

> >+    else if (aStateFlags & Ci.nsIWebProgressListener.STATE_IS_DOCUMENT) {
> >+      if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
> >+        this._documentStop();
> >+      }
> >+    }
> So we have a document stop again.  Do you know if this impacts page loading
> times at all?  I think gavin had removed this because 1) it wasn't being used
> and 2) it added some time to page load.

In my test the perf impact are minimal. But i think the main reason why people has removed it was because it was pretty much unused. I can move the code in an other place if needed but it think this one is the right one.

> >+  _documentStop: function _documentStop() {
> >+    if (this._tab == Browser.selectedTab) {
> >+      // XXX Sometimes MozScrollSizeChange has not occurred, so the scroll pane will not
> >+      // be resized yet. We are assuming this event is on the queue, so scroll the pane
> >+      // "soon."
> >+      Util.executeSoon(function() {
> >+        let scroll = Browser.getScrollboxPosition(Browser.contentScrollboxScroller);
> >+        if (scroll.x == 0 && scroll.y == 0)
> 
> Use isZero.

I'll do it

> >+          Browser.scrollContentToBrowser();
> >+      });
> >+    }
> >+    else {
> >+      let scroll = BrowserView.Util.getContentScrollOffset(this._tab.browser);
> >+      this._tab.scrollOffset = { x: scroll.x, y: scroll.y };
> >+    }
> 
> Use Point instead of custom object.

Yeah, i've forgot it.

> Would be r+ with nits, but I'm concerned about page load impact and
> browserToViewport (even though it seems to work fine).

I will re-test that on monday to ensure everything is working fine
(In reply to comment #7)

> >+    else if (aStateFlags & Ci.nsIWebProgressListener.STATE_IS_DOCUMENT) {
> >+      if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
> >+        this._documentStop();
> >+      }
> >+    }
> 
> So we have a document stop again.  Do you know if this impacts page loading
> times at all?  I think gavin had removed this because 1) it wasn't being used
> and 2) it added some time to page load.

I removed it because we weren't using it. Adding it back to the WebProgressListener should not cause any slowdown.
Comment on attachment 420731 [details] [diff] [review]
Patch

r+ but let's answer Ben's question about:

-    pos.map(bv.browserToViewport);
Attachment #420731 - Flags: review?(mark.finkle) → review+
Comment on attachment 420731 [details] [diff] [review]
Patch

the two places you're only using 'bv' in one place, just use this._browserView.XXX


When does document stop get called, vs network, etc?
(In reply to comment #11)
> (From update of attachment 420731 [details] [diff] [review])
> 
> When does document stop get called, vs network, etc?

The main difference for me is that this happen when the document is *fully* loaded, which mean every image size are set, css applied. So this mean the scrollbars are well set when in networkStop it can stay a few difference which can result in a wrong x,y position for scrollbars.

You can see this by going to slashdot.org, pan a little, show one of the sidebar and refresh the page : if we keep the code in networkStop, depending on where you were, the position can change.


About the browserToViewport, as said previously I've removed it because i see the behavior described by stechz and removing this line is the remedy. I've test it and it works as expected.
Attachment #420731 - Attachment is obsolete: true
Attachment #421028 - Flags: review?(webapps)
Attachment #420731 - Flags: review?(webapps)
Attachment #421028 - Flags: review?(webapps) → review+
Comment on attachment 421028 [details] [diff] [review]
Patch v0.2 - adress Ben's comments

a=me for landing
Attachment #421028 - Flags: superreview+
http://hg.mozilla.org/mobile-browser/rev/0cce64ed08e7
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
This is fixed on mobile-1.9.2, but I'm still seeing the problem on trunk over the following nightly build:

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20100112 Firefox/3.7a1pre Fennec/1.1a1pre
verified FIXED on build:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2pre) Gecko/20100112 Firefox/3.6pre Fennec/1.1a1pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: