622601 - Just before the new page is getting loaded, the old page is scrolled to the top and zoom reset

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Martijn Wargers (dead)]

I've seen this on Maemo and Android nightly builds.

Steps to reproduce:
- Go to nu.nl
- Scroll down
- Tap on a link

Expected result:
- The new page should be loaded without the old page getting scrolled up inbetween.

Actual result:
- Just before the new page is getting loaded the old page is shown at the start scroll position.

I see this bug happening on all pages, basically. Nu.nl is just an example.

Comment 1

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

We also reset the zoom before the new page content is loaded. This makes a "zoom" flash too.

Comment 2

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

Attachment: WIP

The wip address the bug, there is still some problems if the url does not changed and also I'm not sure the patch restore the scrollX/Y position if the page restore from page.

The patch also move the scrollContentToTop() out of the webProgressListener.onLocationChanged callback which is probably better for Tp.

Comment 3

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

For this bug at least 2 things happened to the old page but are normally dedicated for the new one, like scrolling to top and scaling.
Those are done because the browser.xml binding and a handler in chrome/content/browser.js receive a MozScrolledAreaChanged once the new page has loaded, and in consequence the front-end tries to adjust the displayport. But it looks like the new page content has not been painted yet (not receive by ShadowLayersParent.cpp afaict)

My actual workaround is to wait for a MozAfterPaint event on a new page load on the content side, then dispatch a message to the chrome side saying the displayport can now be updated. I've added some printf to http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayers.cpp#374 and to http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayersParent.cpp#151 show me that those method are done before the MozAfterPaint event, but I'm not 100% sure I can rely on that?

If that's not the case it could be nice to have:
 * a way to clear the basic layer with a background color (let's say white)
 * an event, message, anything else that will tell us a repaint has occured on the ShadowLayer

Even if MozAfterPaint is always fired after the repaint has occured having a way to filled the ShadowLayer with a color will be fine.

Do you guys have any thoughts on that?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #3)
> My actual workaround is to wait for a MozAfterPaint event on a new page load on
> the content side, then dispatch a message to the chrome side saying the
> displayport can now be updated.

That sounds pretty good.

> I've added some printf to
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayers.cpp#374
> and to
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayersParent.cpp#151
> show me that those method are done before the MozAfterPaint event, but I'm not
> 100% sure I can rely on that?

Hmm.  That didn't used to be true, but I think Rob may have changed that recently.  Lucky! :)

> 
> If that's not the case it could be nice to have:
>  * a way to clear the basic layer with a background color (let's say white)

I don't fully understand what's desired here, but see below.

>  * an event, message, anything else that will tell us a repaint has occured on
> the ShadowLayer

This is easy-peasy.  I've thought about firing an event after http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp#527, but AIUI that won't solve the problem here because that code isn't aware of page navigation.  (It doesn't know or care who drew the pixels in the shadow layers.)

> Do you guys have any thoughts on that?

I think listening for AfterPaint in content sounds like a good idea.  Let's worry about harder solutions only if that approach doesn't work well enough.

Comment 5

[Benjamin Stover (:stechz)]

Will background-color: white override the checkerboard pattern on a browser element?

That would only the page to be white on a new page load.

Comment 6

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

(In reply to comment #4)
> (In reply to comment #3)
> 
> >  * an event, message, anything else that will tell us a repaint has occured on
> > the ShadowLayer
> 
> This is easy-peasy.  I've thought about firing an event after
> http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp#527,

It will be nice

> but AIUI that won't solve the problem here because that code isn't aware of
> page navigation.  (It doesn't know or care who drew the pixels in the shadow
> layers.)
> 

Right, but the front-end side can manage that, having such an event will avoid the latency between the the real repaint  and the "hack" of waiting the MozAfterPaint event on the content side before dispatching a message to the chrome side...

> > Do you guys have any thoughts on that?
> 
> I think listening for AfterPaint in content sounds like a good idea.  Let's
> worry about harder solutions only if that approach doesn't work well enough.

I will use the proposed approach for this bug and filed a new one for having the event.

Comment 7

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

Attachment: Patch

The patch use MozAfterPaint but it will be easy to override that by something else.

Comment 8

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

Comment on attachment 512465 [details] [diff] [review]
Patch

>diff --git a/chrome/content/bindings/browser.js b/chrome/content/bindings/browser.js
>--- a/chrome/content/bindings/browser.js
>+++ b/chrome/content/bindings/browser.js
>@@ -45,16 +45,23 @@ let WebProgressListener = {
>       canGoBack: docShell.canGoBack,
>       canGoForward: docShell.canGoForward
>     };
> 
>     sendAsyncMessage("Content:LocationChange", json);
> 
>     // Keep track of hash changes
>     this.hashChanged = (location == this._lastLocation);
>+
>+    // When a new page is loaded fire a message for the first paint
>+    addEventListener("MozAfterPaint", function(aEvent) {
>+      removeEventListener("MozAfterPaint", arguments.callee, true);
>+      sendAsyncMessage("MozAfterPaint", {});
>+    }, true);
>+

Did you mean to drop this code in the middle of the hashChanged / _lastLocation code? If not, move it to the end of the method.

>     this._lastLocation = location;
>   },

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

>-                // XXX Actually this fire a SetCacheViewport event as well as the
>-                // scrolledAreaChanged handler in content/chrome/browser.js. So
>-                // we ended up doing twice the work for the same area
>-                view._updateCacheViewport();

Sounds good to me, but why can we remove this?

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>     browser.messageManager.addMessageListener("MozScrolledAreaChanged", (function() {
>+      if (this.waitForAfterPaint)
>+        return;
>+
>       // ensure that the browser's contentDocumentWidth property adjusts first
>       setTimeout(this.scrolledAreaChanged.bind(this), 0);
>     }).bind(this));
> 
>+    browser.messageManager.addMessageListener("MozAfterPaint", (function() {
>+      if (!this.waitForAfterPaint)
>+        return;
>+
>+      // We're about to have new page content, so scroll the content area
>+      // to the top so the new paints will draw correctly.
>+      // (background tabs are delayed scrolled to top in _documentStop)
>+      if (getBrowser() == browser) {
>+        let rootView = browser.getRootView();
>+        if ("_contentView" in rootView)
>+          rootView._contentView.scrollTo(0, 0);
>+        Browser.pageScrollboxScroller.scrollTo(0, 0);
>+      }
>+
>+      this.scrolledAreaChanged();
>+      delete this.waitForAfterPaint;
>+    }).bind(this));


Stop using tab.waitForAfterPaint flag. Instead, add and remove the "MozAfterPaint" message listener as you need to. That removes the need to a new flag and reduces potential message traffic.

So, add a new method, tab.waitForLoad and in that method tab.browser.messageManager.addMessageListener("MozAfterPaint", ...) and then remove the listener in the message handler.

You can also dynamically add and remove the listener for "MozscrolledAreaChanged" too. Remove it inside the tab.waitForLoad method and add it in the "MozAfterPaint" listener. Honestly, I'd love to remove the "MozScrolledAreaChanged" handler from Tab altogether. We already listen for it in the browser binding.

>     let isDefault = this.isDefaultZoomLevel();
>     this._defaultZoomLevel = this.getDefaultZoomLevel();
>-    if (isDefault)
>+    if (isDefault && browser.scale != this._defaultZoomLevel) {
>       browser.scale = this._defaultZoomLevel;
>+    } else if (isDefault) {
>+      let rootView = browser.getRootView();
>+      rootView._updateCacheViewport();
>+    }

I dislike this style of if/else block. Use:

if (isDefault) {
  if (browser.scale != this._defaultZoomLevel)
    browser.scale = this._defaultZoomLevel;
  else
    browser.getRootView()._updateCacheViewport();
}

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

>           switch (aEvent.direction) {
>-            case gesture.DIRECTION_UP:
>-              Browser.scrollContentToBottom();
>+            case gesture.DIRECTION_UP: {
>+              let x, y;
>+              Browser.contentScrollboxScroller.getPosition(x, y);
>+              Browser.scrollContentToBottom({x: x.value });

Does this work? Or do you need to use | let x = {}, y = {}; |
 
r- to rework the flow

Comment 9

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

Attachment: Patch

(In reply to comment #8)

> >     this._lastLocation = location;
> >   },
> 
> >diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml
> 
> >-                // XXX Actually this fire a SetCacheViewport event as well as the
> >-                // scrolledAreaChanged handler in content/chrome/browser.js. So
> >-                // we ended up doing twice the work for the same area
> >-                view._updateCacheViewport();
> 
> Sounds good to me, but why can we remove this?

The change in Tab.updateDefaultZoom guarantee that updateCacheViewport will be called. So we don't need to call it again here. If for sanity reason we want to do this call in the bindings.xml handler we probably need to make sure to not call this twice and this probably require more changes.


I've addressed others comments.

Comment 10

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

Attachment: Patch v0.2

Fix a small bug, sorry for the spam.

Comment 11

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

Comment on attachment 513469 [details] [diff] [review]
Patch v0.2

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

>+    // When a new page is loaded fire a message for the first paint
>+    addEventListener("MozAfterPaint", function(aEvent) {
>+      removeEventListener("MozAfterPaint", arguments.callee, true);
>+      sendAsyncMessage("MozAfterPaint", {});
>+    }, true);

Instead of sending a "MozAfterPaint" message, let's rename it to be more aligned with the intent here. We only ever send this on the first paint and it's not a general MozAfterPaint message. So let's call the message "Browser:PageVisible"

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

>+    this.scrolledAreaChanged = this.scrolledAreaChanged.bind(this);

Do this in the Tab constructor. We don't need to do this every time we create a browser.

>+  waitForLoad: function waitForLoad() {
>+    let browser = this._browser;
>+    browser.messageManager.removeMessageListener("MozScrolledAreaChanged", this.scrolledAreaChanged);
>+
>+    let self = this;
>+    browser.messageManager.addMessageListener("MozAfterPaint", function(aMessage) {
>+      browser.messageManager.removeMessageListener(aMessage.name, arguments.callee);
>+
>+      // We're about to have new page content, so scroll the content area
>+      // to the top so the new paints will draw correctly.
>+      // (background tabs are delayed scrolled to top in nsIWebProgressListener._documentStop)

nsIWebProgressListener -> Browser.WebProgress

>+      if (getBrowser() == browser) {
>+        let rootView = browser.getRootView();
>+        if ("_contentView" in rootView)
>+          rootView._contentView.scrollTo(0, 0);
>+        Browser.pageScrollboxScroller.scrollTo(0, 0);
>+      }
>+
>+      self.scrolledAreaChanged();
>+      browser.messageManager.addMessageListener("MozScrolledAreaChanged", self.scrolledAreaChanged);
>+    });
>   },

This function (waitForLoad) should not be in Tab. Since it depends on Browser knowledge it should really be in Browser somewhere. Probably added to Browser.WebProgress

See if you can move it (all or most of it) without much trouble. If not, we can move it later in a followup.

Comment 12

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

Ben - Can you take a look at this patch?

Comment 13

[Benjamin Stover (:stechz)]

Comment on attachment 513469 [details] [diff] [review]
Patch v0.2

Good job! I still see some flashes of checkerboard when loading a new page, but it's much better.

>+    // When a new page is loaded fire a message for the first paint
>+    addEventListener("MozAfterPaint", function(aEvent) {
>+      removeEventListener("MozAfterPaint", arguments.callee, true);
>+      sendAsyncMessage("MozAfterPaint", {});
>+    }, true);
>   },

We don't fire this for everything? Could we call this MozFirstPaint then?

>   scrollContentToTop: function scrollContentToTop(aOptions) {
>-    let x = {}, y = {};
>-    this.contentScrollboxScroller.getPosition(x, y);
>+    let x = 0;
>     if (aOptions)
>-      x.value = ("x" in aOptions ? aOptions.x : x.value);
>+      x = ("x" in aOptions ? aOptions.x : x);
> 
>-    this.contentScrollboxScroller.scrollTo(x.value, 0);
>+    this.contentScrollboxScroller.scrollTo(x, 0);
>     this.pageScrollboxScroller.scrollTo(0, 0);
>   },

Bleck. I think the default and only option should be to preserve the x value. For cases where we want to go to (0, 0) I'd recommend a separate scrollContentToTopLeft or resetScrollPosition function for clairty. Also, if you remove the options, couldn't you just use scrollBy here?

> 
>   // cmd_scrollBottom does not work in Fennec (Bug 590535).
>   scrollContentToBottom: function scrollContentToBottom(aOptions) {
>-    let x = {}, y = {};
>-    this.contentScrollboxScroller.getPosition(x, y);
>+    let x = 0;
>     if (aOptions)
>       x.value = ("x" in aOptions ? aOptions.x : x.value);
> 
>-    this.contentScrollboxScroller.scrollTo(x.value, Number.MAX_VALUE);
>+    this.contentScrollboxScroller.scrollTo(x, Number.MAX_VALUE);
>     this.pageScrollboxScroller.scrollTo(0, Number.MAX_VALUE);
>   },

Make preserving x value default and remove options. I doubt we need an equivalent function.

>+
>+      // We're about to have new page content, so scroll the content area
>+      // to the top so the new paints will draw correctly.
>+      // (background tabs are delayed scrolled to top in nsIWebProgressListener._documentStop)
>+      if (getBrowser() == browser) {
>+        let rootView = browser.getRootView();
>+        if ("_contentView" in rootView)
>+          rootView._contentView.scrollTo(0, 0);
>+        Browser.pageScrollboxScroller.scrollTo(0, 0);
>+      }

We should not be detecting for _contentView, though I know other places do this. Let's stamp out this pattern now. :) Why not just call rootView.scrollTo(0, 0)?

>-    if (isDefault)
>-      browser.scale = this._defaultZoomLevel;
>+    if (isDefault) {
>+      if (browser.scale != this._defaultZoomLevel)
>+        browser.scale = this._defaultZoomLevel;
>+      else
>+        browser.getRootView()._updateCacheViewport();
>+    }

Why are we forcing an update here?

Comment 14

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

Comment on attachment 513469 [details] [diff] [review]
Patch v0.2

stechz r-

Comment 15

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

Attachment: Patch v0.3

The patch:
 * address comments
 * answer Ben's questions with comments next to the relevant code
 * Remove the changes to scrollContentToTop/scrollContentToBottom since those are not needed

Comment 16

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

Comment on attachment 515906 [details] [diff] [review]
Patch v0.3

Can we change:

>+        let rootView = browser.getRootView();
>+        if ("_contentView" in rootView)
>+          rootView._contentView.scrollTo(0, 0);

To:

>+        browser.getRootView.scrollTo(0, 0);

As Ben suggested?

r+, but check into the question

Comment 17

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

http://hg.mozilla.org/mobile-browser/rev/22f00a9cf407

Comment 18

[Ioana Chiorean]

verified fixed on build id: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre)
Gecko/20110302
Firefox/4.0b13pre Fennec /4.0b6pre

Comment 19

[Martijn Wargers (dead)]

I'm still seeing this bug (or perhaps it regressed between when this was fixed and now?), so I filed bug 641794 for it.