Closed Bug 942736 Opened 7 years ago Closed 6 years ago

Page-position when going back a page is not always remembered when conducting a scroll on prior page and then hitting back

Categories

(Firefox for Android :: Toolbar, defect, P5)

25 Branch
ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 36
Tracking Status
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 - fixed
firefox36 --- fixed
fennec + ---

People

(Reporter: julian.parker123, Assigned: esawin)

References

()

Details

(Keywords: reproducible)

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131112160018

Steps to reproduce:

(Note: This is not like bug #154969, I mean something else.)
When you follow a link to another page and go back to the previous page, you normally go to the position where you was before.


Actual results:

When you scroll the page and go back while it is still scrolling and not "standing still" before you go back, the previous page is on top and not on the position where the followed link was.



Expected results:

Going back should always go to the correct position of the previous page and not only if the page that you go back from isn't moving any more.
Summary: Page-psoition when going back is not always remembered → Page-position when going back is not always remembered
Is there a specific page where this happens?
On many pages, e.g. the German newssite www.spiegel.de
Doesn't this depend on a if a site is using cache-control: no-store or a max-age in their http header?
@kbrosnan:
Sorry, I meant the mobile site: m.spiegel.de
I don't know if this affects the classic site too.

@aaronmt:
But when the page isn't scrolling, it works (e.g. on the mentioned m.spiegel.de). Only if I give the page a swipe to scroll and then go back before the page stopped scrolling and stands totally still, this happens.
Ah I see, that's weird. I was able to reproduce on my second attempt after giving the page a large scroll and then hitting back; the browser brought me back to the top of m.spiegel.de. I was able to reproduce on Nightly (28.0) so I imagine that this has been a long-standing bug maybe. Kats might have an idea here on the issue.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Component: General → Graphics, Panning and Zooming
Ever confirmed: true
Flags: needinfo?(bugmail.mozilla)
OS: Windows 7 → Android
Hardware: x86_64 → ARM
Summary: Page-position when going back is not always remembered → Page-position when going back a page is not always remembered when conducting a scroll on prior page and then hitting back
Keywords: reproducible
And of course that not only affects m.spiegel.de but also many other sites. And sometimes it happens even when not scrolling, but in these cases I don't see any regularity that make this happen. While scrolling it always happens.
I'm not able to reproduce this. What I'm doing is:

1. Load m.spiegel.de
2. Scroll down to some article halfway down the page
3. Click on the link to the article
4. Once the article is loaded, do a large fling
5. While the page is still moving hit the back button

If I understand correctly the bug is that this renders m.spiegel.de back at the top. I'm not seeing this; I always see it go back to the scroll position I was at when I clicked on the link. Testing on a week-old nightly build on a Nexus 4.
Flags: needinfo?(bugmail.mozilla)
Assignee: nobody → bugmail.mozilla
tracking-fennec: ? → +
I can reproduce that on my Sony Xperia Ray. It happens not only on m.spiegel.de but also on m.heise.de (a German IT-newsletter) and many more sites. And sometimes Firefox even crashes when doing that.
(In reply to Pascal Fischer from comment #8)
> I can reproduce that on my Sony Xperia Ray. It happens not only on
> m.spiegel.de but also on m.heise.de (a German IT-newsletter) and many more
> sites. And sometimes Firefox even crashes when doing that.

PS:

But on m.heise.de something very strange happens:
the described problem never happens, when you do this with the first half of the page and not on the last half of the page. And it not renders from the top, but from the middle of the page.

To make it clearer:
on the upper parts of m.heise.de all happens as it should. But then there is a point like a "magic border". When you get below that point and click articles and then go back, the page goes up to that point in the middle of the page. And not only when scrolling on the second page.
I always have this problem too on most pages. That is the most annoying bug in mobile Firefox. Only because of this bug i switched to another browser for mobile browsing although I love Firefox on the desktop.
(In reply to Carlos F from comment #10)
> I always have this problem too on most pages. That is the most annoying bug
> in mobile Firefox. Only because of this bug i switched to another browser
> for mobile browsing although I love Firefox on the desktop.

Sorry to hear that. Did you have steps to reproduce? The reason this is open is that we don't have concrete steps to reproduce and it's apparently not reproducible on different devices. Which device were you using Firefox on? Do you recall which pages?
Unassigning since I'm not working on this, and was never able to reproduce it.
Assignee: bugmail.mozilla → nobody
Assignee: nobody → scleymans
Assignee: scleymans → esawin
i can reproduce it on a htc one s and current versions of firefox on android (but it was the same since as long as i can remember) like this:

- go to http://m.diepresse.com/home/index.do
- scroll down and tap on any article
- hit the back button, which will land you on the top of the prior page and not at the scroll position where you've left
filter on [mass-p5]
Priority: -- → P5
Attached patch fling-back-scroll-position.patch (obsolete) — Splinter Review
This patch consistently fixes the described issues for all the mentioned sites.

Obtaining the scroll range via PresShell::GetScrollPositionClampingScrollPortSize within ScrollFrameHelper::ScrollToImpl seems to invalidate the previously stored scroll position (possibly out of range).
I don't see how this could be caused by a race condition though, which the STR more or less imply. Any ideas?
Attachment #8509500 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8509500 [details] [diff] [review]
fling-back-scroll-position.patch

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

I'm not really comfortable with this code so I don't think I can meaningfully provide f+ or f- either way. However, some thoughts:

- Based on my admittedly poor understanding of this code, it looks like it will prevent scrolling into the bottom-right corner of pages when zoomed in. i.e. by ignoring the scroll-clamping-position-scroll-port-size here and using the regular scrollport, the scroll position will get clamped incorrectly, both for web content calling scrollTo and when we call scrollTo from browser.js

- The raciness described in the STR might be related to when exactly calls to setScrollPositionClampingScrollPortSize are made. I haven't looked in detail but it may be that after doing a fling and then hitting back, we end up calling setScrollPositionClampingScrollPortSize on the wrong document. The fling will trigger a series of viewport updates, so we'll have a stream of setSPCSPS calls going, and if the document changes while those are inflight it's possible we end up setting it on the other document. Then the restored scroll position might get clamped to the wrong thing.

Does that help at all?
Attachment #8509500 - Flags: feedback?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> I'm not really comfortable with this code so I don't think I can
> meaningfully provide f+ or f- either way. However, some thoughts:
> 
> - Based on my admittedly poor understanding of this code, it looks like it
> will prevent scrolling into the bottom-right corner of pages when zoomed in.
> i.e. by ignoring the scroll-clamping-position-scroll-port-size here and
> using the regular scrollport, the scroll position will get clamped
> incorrectly, both for web content calling scrollTo and when we call scrollTo
> from browser.js
You're right, there are a lot of ways to cure the symptoms here at different levels, this would be the lowest level and would break things.

> - The raciness described in the STR might be related to when exactly calls
> to setScrollPositionClampingScrollPortSize are made. I haven't looked in
> detail but it may be that after doing a fling and then hitting back, we end
> up calling setScrollPositionClampingScrollPortSize on the wrong document.
> The fling will trigger a series of viewport updates, so we'll have a stream
> of setSPCSPS calls going, and if the document changes while those are
> inflight it's possible we end up setting it on the other document. Then the
> restored scroll position might get clamped to the wrong thing.
Yes, that's what I've been chasing after, however it is not clear to me where the faulty viewport values come from, as they're only occurring in-between switching pages and are neither correct for the previous nor the next page loaded. I'm looking into the cause for this now.

> Does that help at all?
You're always helpful, thanks!
Attached patch fling-back-scroll-position.patch (obsolete) — Splinter Review
The issue is being caused by the margins animation during flinging. When we abort the animation to load the new page (triggered by the back button action) the margins are still zeroed, which results in invalid viewport metrics returned by JavaPanZoomController::getValidViewportMetrics (wrong scaleFactor for minZoomFactor due to missing marginTop and marginBottom values results in wrong zoom).

A way to fix this would be to abort the margins animation when necessary and immediately set the correct margins.
Attachment #8509500 - Attachment is obsolete: true
Attachment #8512677 - Flags: review?(bugmail.mozilla)
Comment on attachment 8512677 [details] [diff] [review]
fling-back-scroll-position.patch

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

This seems ok to me but it also looks like it will cause the toolbar to appear in some cases when it doesn't right now. For example I think if you're just static (not scrolling) on a page and the web content calls scrollTo then that will trigger a call to abortAnimations and that will now make the toolbar appear. Can you check to see if this is the case, and if so, if that makes sense or not?

If that is the case, it should be possible to avoid it by moving the showMargins call higher up in the call stack. For example you could add a bool argument to GeckoLayerClient.abortPanZoomAnimation that controls whether or not we show the margins, and do the call based on that argument at [1]. The call to abortPanZoomAnimation in setFirstPaintViewport could then pass true and the others could pass false, and that should handle the page-load case but leave the others as-is.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/GeckoLayerClient.java?rev=1f9b1f477588#376
Attachment #8512677 - Flags: review?(bugmail.mozilla) → review+
Attached patch fling-back-scroll-position.patch (obsolete) — Splinter Review
The additional step in calculating the actual margins might be redundant, but that way we make sure to account for situations where we don't have the toolbar margins set (i.e., no direct dependence on LayerMarginsAnimator always having the max margins set).
Attachment #8512677 - Attachment is obsolete: true
Attachment #8512951 - Flags: review?(bugmail.mozilla)
Comment on attachment 8512951 [details] [diff] [review]
fling-back-scroll-position.patch

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

This looks great to me, thanks!

::: mobile/android/base/gfx/LayerMarginsAnimator.java
@@ +92,5 @@
>                  "{ \"top\" : " + top + ", \"right\" : " + right
>                  + ", \"bottom\" : " + bottom + ", \"left\" : " + left + " }"));
>      }
>  
> +    public synchronized RectF getMaxMargins() {

drop the public, no need for it.
Attachment #8512951 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/8081294c87ee
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
i still see this issue in the current nightly with the site mentioned in comment #13.
(In reply to philipp from comment #24)
> i still see this issue in the current nightly with the site mentioned in
> comment #13.

That comment describes a different issue, which is not related to flinging.
It would be great if you could create a new bug with the specific STR for that!
DiePresse does not serve me the m.* version of their site and it's not producing on diepress.com.
Do we want to uplift to Aurora? The patch seems well constrained and this has baked for two weeks and improves a longstanding issue.
Flags: needinfo?(snorp)
(In reply to Kevin Brosnan [:kbrosnan] from comment #26)
> Do we want to uplift to Aurora? The patch seems well constrained and this
> has baked for two weeks and improves a longstanding issue.

Sure, why not. Eugen, please request approval, etc.
Flags: needinfo?(snorp)
[Tracking Requested - why for this release]: see comment 26
Comment on attachment 8513446 [details] [diff] [review]
fling-back-scroll-position.patch

Approval Request Comment
[Feature/regressing bug #]: proper scroll position restoring
[User impact if declined]: scroll position is not restored when going back in history while flinging
[Describe test coverage new/current, TBPL]: local testing only
[Risks and why]: low, patch has no effect on the regular (non-flinging) case, has been on Nightly for two weeks 
[String/UUID change made/needed]: none
Attachment #8513446 - Flags: approval-mozilla-aurora?
Comment on attachment 8513446 [details] [diff] [review]
fling-back-scroll-position.patch

It's been around for a while but given the low risk, we can take this uplift.
Attachment #8513446 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No need to track this, it's been around way too long.
You need to log in before you can comment on or make changes to this bug.