Closed
Bug 704950
Opened 13 years ago
Closed 13 years ago
Loading a new page doesn't reset scroll position
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(3 files, 4 obsolete files)
(deleted),
patch
|
cwiiis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cwiiis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cwiiis
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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)
Comment 2•13 years ago
|
||
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 ?
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #576616 -
Attachment is obsolete: true
Attachment #576616 -
Flags: feedback?(chrislord.net)
Attachment #576968 -
Flags: review?(chrislord.net)
Comment 4•13 years ago
|
||
I like this approach, for what it's worth.
Assignee | ||
Comment 5•13 years ago
|
||
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)
Assignee | ||
Comment 6•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Attachment #576979 -
Attachment is obsolete: true
Attachment #576979 -
Flags: review?(chrislord.net)
Assignee | ||
Comment 7•13 years ago
|
||
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)
Updated•13 years ago
|
Blocks: metaviewport
Assignee | ||
Comment 8•13 years ago
|
||
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)
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #577465 -
Flags: review?(chrislord.net)
Assignee | ||
Comment 10•13 years ago
|
||
Attachment #577466 -
Flags: review?(chrislord.net)
Comment 11•13 years ago
|
||
Nice cleanup!
Comment 12•13 years ago
|
||
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 13•13 years ago
|
||
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 14•13 years ago
|
||
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+
Assignee | ||
Comment 15•13 years ago
|
||
> 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
Assignee | ||
Comment 16•13 years ago
|
||
https://hg.mozilla.org/projects/birch/rev/3373a4c28536
https://hg.mozilla.org/projects/birch/rev/a9211fd8e7ea
https://hg.mozilla.org/projects/birch/rev/16b2190c02e1
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 17•13 years ago
|
||
Samsung Nexus S (Android 4.0.1)
20111130040240
http://hg.mozilla.org/projects/birch/rev/4e745f151abd
Status: RESOLVED → VERIFIED
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•