Reader mode pages does not remember scroll positions in the navigation session

VERIFIED FIXED in Firefox 49

Status

()

VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

unspecified
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox49 verified)

Details

Attachments

(1 attachment)

STR:

1. Go to a reader mode-enabled page, e.g. 
http://www.paulgraham.com/makersschedule.html
2. Enter Reader mode
3. Scroll down a bit
4. Navigate away by typing https://mozilla.org/ on the address bar
5. Hit the back button to go back to the reader mode page

Expected result:

1. Pages scrolls at the right position

Actually:

1. Page reloads so it does not remember the scroll position.

Note:

Originally reported by :dolske at bug 1153393 comment 30.
So my comment in bug 1153393 comment 31 is not entirely correct. The page was not preserved by the bfcache at all because there is an unload event listener in AboutReader.jsm.

I can make this bug go away simply by comment out that event listener. However, it's definitely not the right fix. Let me dig deeper on what we do no the unload event listener and see if I can replace it with pagehide and/or inner-window-destoryed callback.
Created attachment 8748516 [details]
MozReview Request: Bug 1269996 - Remove unload event listener, enabling bfcache for about:reader pages, r=gijs, r=margaret

Review commit: https://reviewboard.mozilla.org/r/50337/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50337/
Attachment #8748516 - Flags: review?(gijskruitbosch+bugs)
The patch here is a rather classic fix: moving what we need to do upon unload to pagehide event listener and inner-window-destroyed observer.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12b526fdbc38

With this patch and bug 1153393, we should be covering all cases of scroll position restore, leveraging bfcache (if window is not destroyed) and session restore (if it is).

Comment 4

2 years ago
Comment on attachment 8748516 [details]
MozReview Request: Bug 1269996 - Remove unload event listener, enabling bfcache for about:reader pages, r=gijs, r=margaret

https://reviewboard.mozilla.org/r/50337/#review47179

Looks OK to me, assuming tests are fine with this, but please get review from margaret as well, because I'm not 100% sure how this plays out on Android.
Attachment #8748516 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8748516 [details]
MozReview Request: Bug 1269996 - Remove unload event listener, enabling bfcache for about:reader pages, r=gijs, r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50337/diff/1-2/
Attachment #8748516 - Attachment description: MozReview Request: Bug 1269996 - Remove unload event listener, enabling bfcache for about:reader pages, r=gijs → MozReview Request: Bug 1269996 - Remove unload event listener, enabling bfcache for about:reader pages, r=gijs, r=margaret
Attachment #8748516 - Flags: review?(margaret.leibovic)

Updated

2 years ago
Attachment #8748516 - Flags: review?(margaret.leibovic) → review+

Comment 7

2 years ago
Comment on attachment 8748516 [details]
MozReview Request: Bug 1269996 - Remove unload event listener, enabling bfcache for about:reader pages, r=gijs, r=margaret

https://reviewboard.mozilla.org/r/50337/#review47355

This shouldn't affect Android any differently than it affects desktop, so it should be fine.
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Comment on attachment 8748516 [details]
MozReview Request: Bug 1269996 - Remove unload event listener, enabling bfcache for about:reader pages, r=gijs, r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50337/diff/2-3/
Keywords: checkin-needed

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/de43607b1578
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Verified as fixed in build 49.0a1 2016-05-30;
Device: Nexus 5 (Android 6.0.1).
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
You need to log in before you can comment on or make changes to this bug.