Closed
Bug 1269996
Opened 8 years ago
Closed 8 years ago
Reader mode pages does not remember scroll positions in the navigation session
Categories
(Firefox for Android Graveyard :: Reader View, defect)
Firefox for Android Graveyard
Reader View
Tracking
(firefox49 verified)
VERIFIED
FIXED
Firefox 49
Tracking | Status | |
---|---|---|
firefox49 | --- | verified |
People
(Reporter: timdream, Assigned: timdream)
References
Details
Attachments
(1 file)
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.
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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•8 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+
Assignee | ||
Comment 5•8 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 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)
Assignee | ||
Comment 6•8 years ago
|
||
Try run for Android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c462fc38da56
Updated•8 years ago
|
Attachment #8748516 -
Flags: review?(margaret.leibovic) → review+
Comment 7•8 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.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 9•8 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 Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50337/diff/2-3/
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ea2f18f3ca7
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/de43607b1578
Keywords: checkin-needed
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/de43607b1578
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment 13•7 years ago
|
||
Verified as fixed in build 49.0a1 2016-05-30; Device: Nexus 5 (Android 6.0.1).
Status: RESOLVED → VERIFIED
Updated•3 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
•