704950 - Loading a new page doesn't reset scroll position

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Go to any page, change the pan/zoom position, and then click on a link. The new page should be rendered at a 1.0 zoom level in the top left corner. Instead, it gets rendered at the old pan/zoom position.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Reset viewport

I'm not sure if this is the best approach to doing this. It might also be doable from browser.js but there would be different events to hook into.

Also, this doesn't save/restore the viewport when going back/forward - does gecko store that state somewhere or should I just reset the viewport there as well?

Comment 2

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

Doing this from browser.js would also provide a way to update scroll position based on web content changing it.

As for session history (back/forward), Gecko does save this data in the history entry and we can pull it out. Another reason to consider doing this from browser.js ?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Allow forcing viewport from js

Comment 4

[Patrick Walton (:pcwalton)]

I like this approach, for what it's worth.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Allow forcing viewport from js (v2)

Same patch, but renamed resetViewport() to forceViewport() so that the purpose of the function is more clear.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Allow forcing viewport from js (v3)

Realized that the previous patches didn't always restore the viewport when switching tabs, because the zoom got wiped out. Update to fix.

Based on my mental mode of the code, I also thought this patch would fix bug 704690 but for some reason it doesn't - need to debug that further.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 576999 [details] [diff] [review]
Allow forcing viewport from js (v3)

Argh, there are still problems with this patch. I'm not sure if it's race conditions or what, but sometimes things just don't paint.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/3) Remove old viewport update code

I ended up redoing this with a new message to eliminate some possible race conditions (what was happening before was viewport updates were going from Java -> JS and JS -> Java concurrently and clobbering each other). I also split the patch into three parts just because it was easier to work with locally, but I can squash them when landing if necessary.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/3) Add new Viewport:Update message handling

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: (3/3) Send the Viewport:Update message from browser.js

Comment 11

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

Nice cleanup!

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 577464 [details] [diff] [review]
(1/3) Remove old viewport update code

Review of attachment 577464 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, not much to comment on.

Comment 13

[Chris Lord [:cwiiis]]

Comment on attachment 577465 [details] [diff] [review]
(2/3) Add new Viewport:Update message handling

Review of attachment 577465 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java
@@ +118,5 @@
>          layerController.setRoot(mTileLayer);
>          if (mGeckoViewport != null)
>              layerController.setViewportMetrics(mGeckoViewport);
>          geometryChanged();
> +        GeckoAppShell.registerGeckoEventListener("Viewport:Update", this);

Previously, we've handled all Gecko event-listener code in GeckoAppShell, though I like the idea of moving the code to where it's actually used. We should try and do this more in later patches.

@@ +263,5 @@
> +                } finally {
> +                    mTileLayer.endTransaction();
> +                }
> +            } catch (JSONException e) {
> +                Log.e(LOGTAG, "Unable to update viewport", e);

This try/catch block is a bit odd, should it not be around the updateViewport call? (for clarity)

Comment 14

[Chris Lord [:cwiiis]]

Comment on attachment 577466 [details] [diff] [review]
(3/3) Send the Viewport:Update message from browser.js

Review of attachment 577466 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good - r+, assuming there's a good reason for the code below and the viewportExcess thing isn't a problem.

::: mobile/android/chrome/content/browser.js
@@ +1074,5 @@
> +    let win = this.browser.contentWindow;
> +    let zoom = (aReset ? 1.0 : this._viewport.zoom);
> +    let xpos = (aReset ? win.scrollX * zoom : this._viewport.x);
> +    let ypos = (aReset ? win.scrollY * zoom : this._viewport.y);
> +    this.viewportExcess = { x: 0, y: 0 };

Should this call only happen if aReset is true? I'd have thought it'd end up forcing the transformation to change unnecessarily when this is called and it isn't already 0,0. the xpos/ypos above should probably add the viewportExcess on (there probably needs to be a division by the zoom too, if I remember correctly).

In fact, I'm not sure I understand why we set the viewport at all if aReset is false?

Comment 15

[Kartikaya Gupta (email:[e-mail])]

> This try/catch block is a bit odd, should it not be around the
> updateViewport call? (for clarity)

Fixed.

> Should this call only happen if aReset is true? I'd have thought it'd end up
> forcing the transformation to change unnecessarily when this is called and
> it isn't already 0,0. the xpos/ypos above should probably add the
> viewportExcess on (there probably needs to be a division by the zoom too, if
> I remember correctly).
> 
> In fact, I'm not sure I understand why we set the viewport at all if aReset
> is false?

When switching tabs, the viewport from the new tab needs to be sent (without resetting zoom/scroll) up to the Java layer, otherwise the Viewport:Change event from Java causes the new tab's viewport to get overridden with the old tab's viewport, which was cached in Java. This might also be solvable by making Java bind viewport information to a specific tab id, and ensure that viewport updates sent between Java/JS always update the correct tab's viewport to eliminate crosstalk across tab viewports. However that solution seemed more complicated so I figured this would be simpler.

And you're right in that setting xpos and ypos here is almost a no-op in the case where aReset is false. It just runs through the viewport setter code without making any changes to the viewport, but that assumes things like page size and whatnot haven't changed. If they have, then it might need to do some adjustments and update the transform/scroll position, so I thought it best to do it this way.

And this._viewport.x and this._viewport.y are already zoom-multiplied, and the viewport setter divides by the zoom, so that's why I'm multiplying win.scrollX by zoom but not this._viewport.x or xpos. I'm pretty sure that while debugging this I went through every other permutation of code possible here because it somehow made sense at some point.. but this is the one that works :p

Comment 16

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/projects/birch/rev/3373a4c28536
https://hg.mozilla.org/projects/birch/rev/a9211fd8e7ea
https://hg.mozilla.org/projects/birch/rev/16b2190c02e1

Comment 17

[Aaron Train [:aaronmt]]

Samsung Nexus S (Android 4.0.1)
20111130040240
http://hg.mozilla.org/projects/birch/rev/4e745f151abd