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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: 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/

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

[:Gijs (he/him)]

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.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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/

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Try run for Android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c462fc38da56

Comment 7

[:Margaret Leibovic]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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/

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ea2f18f3ca7

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/de43607b1578

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de43607b1578

Comment 13

[Sorina Florean [:sflorean]]

Verified as fixed in build 49.0a1 2016-05-30;
Device: Nexus 5 (Android 6.0.1).