Closed Bug 851445 Opened 12 years ago Closed 12 years ago

Back button does not restore scroll position on a specific site (Allegro.pl)

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Version:
19 Branch
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla23
Tracking Flags:
Tracking Status
firefox19 --- affected
firefox20 - ---
firefox21 - ---
firefox22 - ---

People

(Reporter: krzysztof.glebowicz, Assigned: roc)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

reduced testcase
(deleted), text/html
Details
fix v2
(deleted), patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20130314 Firefox/22.0 Build ID: 20130314030914 Steps to reproduce: After pressing back button on website allegro.pl, scroll position is not restored. Versions affected: 19 to 22. Version not affected: 18. Steps to reproduce: 1. Go to http://allegro.pl/dla-dzieci 2. Scroll to the middle of the page 3. Press any link (for instance "Akcesoria") 4. Press the Back button Actual results: Scroll position is not restored. I see the top of the page. Expected results: Scroll position should be restored. I should see the "Akcesoria" link.
Blocks: 251784
Regression range: m-c good=2012-11-15 bad=2012-11-16 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a761bfc192b5&tochange=58ebb638a7ea Suspected bug: Bug 811301
Status: UNCONFIRMED → NEW
status-firefox19: --- → affected
tracking-firefox20: --- → ?
tracking-firefox21: --- → ?
tracking-firefox22: --- → ?
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Blocks: 811301
We'd take a low risk uplift, but this is not critical enough to track for upcoming releases.
Assignee: nobody → roc
tracking-firefox20: ? → -
tracking-firefox21: ? → -
tracking-firefox22: ? → -
This is also an issue on engadget.com, amongst others -- for all cases: when going back, refreshing, or restoring session.
Attached file reduced testcase (deleted) —
It looks like reframing the root element while the page is loading makes us forget the scroll position that's being restored.
This behavior gets triggered when Modernizr is loaded in <head>, which is probably why it affects many sites.
I think basically the problem is that before the patch in bug 811301 we extracted the old scrolling state from the history to the root scroll frame after document.load fired. But after bug 811301 we restore it into mRestorePos as soon as the root scroll frame is constructed and remove it from the history data. Then the reframe of the root scroll frame causes mRestorePos to be wiped.
Attached patch fix (obsolete) (deleted) — — Splinter Review
Attachment #727512 - Flags: review?(matspal)
Comment on attachment 727512 [details] [diff] [review] fix r=mats
Attachment #727512 - Flags: review?(matspal) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7) > Created attachment 727512 [details] [diff] [review] > fix I think there is a useless blank line (#12) at the end of the file bug851445_helper.html.
Is it possible to get this landed or is it still waiting for something?
(In reply to Loic from comment #9) > I think there is a useless blank line (#12) at the end of the file > bug851445_helper.html. Thanks, fixed. https://hg.mozilla.org/integration/mozilla-inbound/rev/dd6b604d1706
Comment on attachment 727512 [details] [diff] [review] fix [Approval Request Comment] Bug caused by (feature/regressing bug #): 811301 User impact if declined: Fail to restore scroll position when going back to certain sites Testing completed (on m-c, etc.): just landed Risk to taking this patch (and alternatives if risky): pretty low-risk patch String or IDL/UUID changes made by this patch: none
Attachment #727512 - Flags: approval-mozilla-aurora?
Backed out due to test failure. https://hg.mozilla.org/integration/mozilla-inbound/rev/45dc77d9bb09 https://tbpl.mozilla.org/php/getParsedLog.php?id=21337983&tree=Mozilla-Inbound&full=1 22:45:16 INFO - 102207 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_wheel_default_action.html | doTestScroll(aSettings=all delta values are not customized), Scroll to only right by oblique pixel wheel event with overflow-y: hidden (body is scrollable): scrolled vertical - got 820, expected 1000 etc
This actually detected a real bug. What happens is: -- Scroll frame to 1000,820. -- Reframe. mRestorePos.y == 820 in the new frame. mLastPos.y == 820. -- Scroll frame to 1000,1000. mRestorePos.y is still 820, however. -- Reframe. We use mRestorePos.y as the scroll state, and scroll back to 820. The underlying issue is that we're only supposed to use mRestorePos if mLastPos == GetLogicalScrollPosition().
Attached patch fix v2 (deleted) — — Splinter Review
Attachment #727512 - Attachment is obsolete: true
Attachment #727512 - Flags: approval-mozilla-aurora?
Attachment #732648 - Flags: review?(matspal)
Comment on attachment 732648 [details] [diff] [review] fix v2 r=mats
Attachment #732648 - Flags: review?(matspal) → review+
https://hg.mozilla.org/mozilla-central/rev/51ea67404430
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: