Closed Bug 704950 Opened 8 years ago Closed 8 years ago

Loading a new page doesn't reset scroll position

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(3 files, 4 obsolete files)

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.
Attached patch Reset viewport (obsolete) — Splinter Review
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?
Attachment #576616 - Flags: feedback?(chrislord.net)
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 ?
Attached patch Allow forcing viewport from js (obsolete) — Splinter Review
Attachment #576616 - Attachment is obsolete: true
Attachment #576616 - Flags: feedback?(chrislord.net)
Attachment #576968 - Flags: review?(chrislord.net)
I like this approach, for what it's worth.
Same patch, but renamed resetViewport() to forceViewport() so that the purpose of the function is more clear.
Attachment #576968 - Attachment is obsolete: true
Attachment #576968 - Flags: review?(chrislord.net)
Attachment #576979 - Flags: review?(chrislord.net)
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.
Attachment #576999 - Flags: review?(chrislord.net)
Attachment #576979 - Attachment is obsolete: true
Attachment #576979 - Flags: review?(chrislord.net)
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.
Attachment #576999 - Attachment is obsolete: true
Attachment #576999 - Flags: review?(chrislord.net)
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.
Attachment #577464 - Flags: review?(chrislord.net)
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.
Attachment #577464 - Flags: review?(chrislord.net) → review+
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)
Attachment #577465 - Flags: review?(chrislord.net) → review+
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?
Attachment #577466 - Flags: review?(chrislord.net) → review+
> 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
Depends on: 706215
No longer depends on: 706215
Samsung Nexus S (Android 4.0.1)
20111130040240
http://hg.mozilla.org/projects/birch/rev/4e745f151abd
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.