Closed Bug 1153393 Opened 9 years ago Closed 8 years ago

Pages in reader mode forget their scroll position across restarts

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Version:
39 Branch
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 49
Tracking Flags:
Tracking Status
firefox49 --- verified

People

(Reporter: aleth, Assigned: timdream)

References

Details

Attachments

(2 files)

MozReview Request: Bug 1153393 - Make session restore restores the scroll position of an about:reader page, r=gijs
58 bytes, text/x-review-board-request
Gijs
: review+
Details
beforeUnload.html
731 bytes, text/html
Details 
STR
View page in reader mode, scroll down a bit, switch to a different tab, restart.
Switch to the restored tab: the page is correctly displayed in reader mode -- but the scroll position is lost.
Component: Reading List → Reader Mode
Product: Firefox → Toolkit
On second thoughts, should this bug actually be in the session restore component?
Flags: needinfo?(gijskruitbosch+bugs)
Component: Reader Mode → Session Restore
Flags: needinfo?(gijskruitbosch+bugs)
Product: Toolkit → Firefox
The problem here is that sessionstore sets the scroll position right after about:reader has loaded. As it loads without any real content the scroll position is basically discarded. Asynchronously, the article is then loaded and displayed with the frame showing the top of the document.
(In reply to Tim Taubert [:ttaubert] from comment #2)
> The problem here is that sessionstore sets the scroll position right after
> about:reader has loaded. As it loads without any real content the scroll
> position is basically discarded. Asynchronously, the article is then loaded
> and displayed with the frame showing the top of the document.

Can we do something in reader mode to mitigate this?
Flags: needinfo?(ttaubert)
Override window.scrollTo() and save the coordinates to apply them when the article has loaded? :/ This mechanism wasn't really written for dynamic pages...

Another thing might be to use .pushState() or window.location.hash to save the current scroll position whenever scrolling occurred?

Another solution might be to defer the page load event until the article has loaded. That would re-use sessionstore's capabilities and would probably be the best option?
Flags: needinfo?(ttaubert)
Just a reminder that bug 857990 is about saving/restoring Reader Mode scroll position, but it seems it is focused on saving it locally to the reading list (and ultimately syncing).
Perhaps the method should be shared? Or at least ensure that you don't end up stepping on each other's toes.
(In reply to Tim Taubert [:ttaubert] from comment #5)
> Another solution might be to defer the page load event until the article has
> loaded. That would re-use sessionstore's capabilities and would probably be
> the best option?

How would we actually do that, though? What event does sessionstore use, just "load" on the page? I expect that we're XHR'ing the article, and if that isn't already delaying the load event then I'm not sure how we *would* do that...
Flags: needinfo?(ttaubert)
Yeah, sessionstore is waiting for the window.top's load event. I don't have any great ideas about how to defer that load event but maybe we can figure something out.
Flags: needinfo?(ttaubert)
Just thought I'd mention that a similar problem can occur for pdf.js tabs.
There's a bunch of asyncness inbetween DOMContentLoaded firing and about:reader sending off its XHR.

However, a quick testpage that waits on a 10s php script over XHR from a script directly in its <body>, where <body> has an onload event, still has the onload event beating the XHR (ie the XHR does not seem to delay the onload event).

Seems to me like the easiest (but hacky) solution would be to specialcase pdfjs and about:reader in session restore...
(that is, fire a custom load event once the page is fully displayed and make sessionstore listen for that.)
This bug annoys me a lot. Probably should be in a QX cluster? Will have test ready before asking for review...
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Would it be possible to fix this in a way that can also be used for restoring the scroll position of pdf.js tabs?
(In reply to aleth [:aleth] from comment #15)
> Would it be possible to fix this in a way that can also be used for
> restoring the scroll position of pdf.js tabs?

I will look into it as well.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #16)
> I will look into it as well.

Spent some time look into the code; pdf.js viewer.html is loaded differently than readerMode.html; it hook itself into mine type converter and "convert" application/pdf to text/html. The way to detect it is porbably |content.document.nodePrincipal.origin === "resource://pdf.js"|. I would also need to figure out the event the viewer.html dispatches when the PDF is rendered.
Bug 1235368 is also about session restore failing to restore the scroll offsets, in general... I guess earlier comments here indicate we'd still need a Reader-specific fix anyway?
(In reply to Justin Dolske [:Dolske] from comment #18)
> Bug 1235368 is also about session restore failing to restore the scroll
> offsets, in general... I guess earlier comments here indicate we'd still
> need a Reader-specific fix anyway?

Yes, I am working on a Reader-specific fix.
Comment on attachment 8744217 [details]
MozReview Request: Bug 1153393 - Make session restore restores the scroll position of an about:reader page, r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48411/diff/1-2/
With suggestions from :gijs the patch now listens to a AboutReaderContntReady event, and is much simpler.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #17)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #16)
> > I will look into it as well.
> 
> Spent some time look into the code; pdf.js viewer.html is loaded differently
> than readerMode.html; it hook itself into mine type converter and "convert"
> application/pdf to text/html. The way to detect it is porbably
> |content.document.nodePrincipal.origin === "resource://pdf.js"|. I would
> also need to figure out the event the viewer.html dispatches when the PDF is
> rendered.

I can't find the right event to listen to and/or the right place to add a event. This would require a pdf.js expert here...

I am still struggling to write a test for this, for now.
Hi Yury, could you help us identify the right event to wait on the pdf viewer page when the scroll bar is ready to be scrolled?

The session store would need to wait for it before scrolling (You can see my WIP patch on reader mode page as a reference).

Thanks!
Flags: needinfo?(ydelendik)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #22)
> Hi Yury, could you help us identify the right event to wait on the pdf
> viewer page when the scroll bar is ready to be scrolled?
>
> The session store would need to wait for it before scrolling

PDF Viewer page has "documentload" event (see example at http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/test/head.js). However this might not be enough, we are loading, showing and scrolling to the pages asynchronously after that based on the URL hash parameters.

Normally we save history with last scroll position and we have logic to maintain it in the history (https://github.com/mozilla/pdf.js/blob/master/web/pdf_history.js). I'll CC Jonas who helped us a lot with it -- probably he has ideas how to coordinate it with restore.
Flags: needinfo?(ydelendik)
(In reply to Yury Delendik (:yury) from comment #23)
> PDF Viewer page has "documentload" event (see example at
> http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/test/
> head.js). However this might not be enough, we are loading, showing and
> scrolling to the pages asynchronously after that based on the URL hash
> parameters.
> 
> Normally we save history with last scroll position and we have logic to
> maintain it in the history
> (https://github.com/mozilla/pdf.js/blob/master/web/pdf_history.js). I'll CC
> Jonas who helped us a lot with it -- probably he has ideas how to coordinate
> it with restore.

Interesting. Anything prevent PDF viewer itself from restoring the scroll position? There is no need for session restore to do anything for PDF viewer right?

Judging by the complexity, should we just clone another bug for the PDF viewer case?
(In reply to Yury Delendik (:yury) from comment #23)
> Normally we save history with last scroll position and we have logic to
> maintain it in the history
> (https://github.com/mozilla/pdf.js/blob/master/web/pdf_history.js). I'll CC
> Jonas who helped us a lot with it -- probably he has ideas how to coordinate
> it with restore.

Yes, when navigating away from PDF Viewer, we try to update window.history with the current viewer position. This is done with a beforeUnload event listener, and it *should* thus be working when navigating away from the viewer, and when the browser is closed.

However, quick testing suggests that there might be some timing issues here, but I'm not entirely sure why that is. I tried setting a breakpoint in the relevant code, and then window.history is correctly updated, and thus the position is restored, *every* time when exiting and then restarting the browser. This suggests to me that the code, as it currently is, should actually be working.

I'm wondering if, for some reason, the beforeUnload event listener isn't given enough time to run when the browser is closed, meaning the window.history isn't set correctly?

I can only think of really hacky workarounds for this on the PDF.js side, so I'd really prefer not having to do that. (Also, the PDFHistory code is quite old, and *very* messy. I've actually rewritten the implementation from scratch, but that hasn't been landed yet unfortunately, since the size/scope of it has meant that it's not that easy to test/review.)
Attached file beforeUnload.html — 
This is a reduced test-case that does the same thing as the PDF Viewer when closing and then restarting the browser. Setting a breakpoint in the `beforeUnload` function makes things work, but when the test-case is run as is window.history isn't updated correctly.
Could this point to a browser bug, or is beforeunload not intended to be used in this way?
(In reply to Jonas Jenwald [:Snuffleupagus] from comment #26)
> Created attachment 8745978 [details]
> beforeUnload.html
> 
> This is a reduced test-case that does the same thing as the PDF Viewer when
> closing and then restarting the browser. Setting a breakpoint in the
> `beforeUnload` function makes things work, but when the test-case is run as
> is window.history isn't updated correctly.
> Could this point to a browser bug, or is beforeunload not intended to be
> used in this way?

Could be a bug IMHO.

From a pure web development point of view, I would avoid beforeunload and use visibilitychange or scroll event + timer. It's entirely possible the browser/content process simply crashed so you won't get beforeunload event at all.
(In reply to Jonas Jenwald [:Snuffleupagus] from comment #26)
> This is a reduced test-case that does the same thing as the PDF Viewer when
> closing and then restarting the browser. Setting a breakpoint in the
> `beforeUnload` function makes things work, but when the test-case is run as
> is window.history isn't updated correctly.

Maybe adding a shutdown blocker (via AsyncShutdown.jsm) would help?
(In reply to aleth [:aleth] from comment #28)
> (In reply to Jonas Jenwald [:Snuffleupagus] from comment #26)
> > This is a reduced test-case that does the same thing as the PDF Viewer when
> > closing and then restarting the browser. Setting a breakpoint in the
> > `beforeUnload` function makes things work, but when the test-case is run as
> > is window.history isn't updated correctly.
> 
> Maybe adding a shutdown blocker (via AsyncShutdown.jsm) would help?

No. For one, we should limit shutdown blockers as much as possible and just the scroll position of a pdfjs document shouldn't require a shutdown blocker. More directly related to the issue here: it wouldn't solve anything. The position of the document needs to be correct in session history by the time TabState/SessionStore tries to collect it. No point correcting it after that. And AsyncShutdown won't help with that.

Anyway, this bug is about reader mode, and the pdfjs thing probably already has its own bug - either way, please can we move further discussion of that problem to another bug, as it's orthogonal to the reader mode one.
(In reply to :Gijs Kruitbosch from comment #29)

> [...] The position of the document needs to be correct in session
> history by the time TabState/SessionStore tries to collect it. No point
> correcting it after that. And AsyncShutdown won't help with that.

Hmm, that implies this isn't session-restore related, and should be broken in forward/back navigation.

And indeed it seems to be... If I load a page in reader view, scroll halfway down, go to another site, and click back, the page is shown at the top -- scroll position not restored.

[Well, there is one weird quirk -- at least on OS X, I see the *scroll bar* restored to the halfway point, for a moment before it auto-hides as normal, but the page itself is not scrolled. O_o]
(In reply to Justin Dolske [:Dolske] from comment #30)
> (In reply to :Gijs Kruitbosch from comment #29)
> 
> > [...] The position of the document needs to be correct in session
> > history by the time TabState/SessionStore tries to collect it. No point
> > correcting it after that. And AsyncShutdown won't help with that.
> 
> Hmm, that implies this isn't session-restore related, and should be broken
> in forward/back navigation.
> 
> And indeed it seems to be... If I load a page in reader view, scroll halfway
> down, go to another site, and click back, the page is shown at the top --
> scroll position not restored.
> 
> [Well, there is one weird quirk -- at least on OS X, I see the *scroll bar*
> restored to the halfway point, for a moment before it auto-hides as normal,
> but the page itself is not scrolled. O_o]

I don't see any issue for session store *not* being able to collect scroll position, given the fact my patch works (and passes the test to be posted in the next comment).

I think what :dolske sees here is another but where AboutReader.jsm refreshes the page even though the content is already preserved by the bfcache. I will file another bug to investigate.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d777b382591f
Comment on attachment 8744217 [details]
MozReview Request: Bug 1153393 - Make session restore restores the scroll position of an about:reader page, r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48411/diff/2-3/
Attachment #8744217 - Attachment description: MozReview Request: Bug 1153393 - WIP Make session restore restores the scroll position of an about:reader page → MozReview Request: Bug 1153393 - Make session restore restores the scroll position of an about:reader page, r=gijs
Attachment #8744217 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8744217 [details]
MozReview Request: Bug 1153393 - Make session restore restores the scroll position of an about:reader page, r=gijs

https://reviewboard.mozilla.org/r/48411/#review47177

r=me with the below addressed

::: browser/components/sessionstore/content/content-sessionStore.js:90
(Diff revision 3)
>  
> +    if (content.document.documentURI.startsWith("about:reader")) {
> +      if (event.type == "load") {
> +        // Don't restore the scroll position of an about:reader page at this
> +        // point; listen for the custom event dispatched from AboutReader.jsm.
> +        content.addEventListener("AboutReaderContentReady", this, true);

Does this have to be a capturing listener?

::: browser/components/sessionstore/content/content-sessionStore.js:91
(Diff revision 3)
> +        return;
> +      } else {

No else after return, please.
Attachment #8744217 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #31)
> Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d777b382591f

My patch might be contribute more to more intermittent failures in e10s (bug 1259915). I am not entire sure how to address that. Should I still land this bug?
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8744217 [details]
MozReview Request: Bug 1153393 - Make session restore restores the scroll position of an about:reader page, r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48411/diff/3-4/
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #34)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #31)
> > Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d777b382591f
> 
> My patch might be contribute more to more intermittent failures in e10s (bug
> 1259915). I am not entire sure how to address that. Should I still land this
> bug?

Judging from https://brasstacks.mozilla.com/orangefactor/?display=Bug&tree=trunk&startday=2016-03-27&endday=2016-05-04&bugid=1259915 , we've been averaging 0 instances of that orange for a month now, and this patch clearly introduces a lot more of them. So yeah, it sounds like we should fix the test addition to work better in e10s (not entirely sure off-hand why it breaks right now, you'd have to investigate that in more detail).
Flags: needinfo?(gijskruitbosch+bugs)
Thanks for pointing out the orangefactor page. I can't reproduce that on my linux machine locally, so here is a try run with a lot of dump() in it:

ttps://treeherder.mozilla.org/#/jobs?repo=try&revision=e3fff043e701
Could not figure out what's wrong unfortunately; another try with more logs:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be7171d07ba2
I think I found the problem and it's a legit bug in the patch:

The async tasks involves in the about reader world goes like this:

-> DOMContentLoaded -sync-> AboutReaderContentLoaded -sync-> new AboutReader() -async-> AboutReader#_showContent() -sync-> AboutReaderContentReady

and in the content-sessionStore.js:

-> load -async-> AboutReaderContentReady -sync-> gContentRestore.restoreDocument().

There is no way to make sure `AboutReaderContentReady` event happens *after* the `load` event, since the whole chain triggered by the `DOMContentLoaded`, not `load`.

I would need to check if `loaded` classes is added on the <body>.
Comment on attachment 8744217 [details]
MozReview Request: Bug 1153393 - Make session restore restores the scroll position of an about:reader page, r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48411/diff/4-5/
Comment on attachment 8744217 [details]
MozReview Request: Bug 1153393 - Make session restore restores the scroll position of an about:reader page, r=gijs

Will ask for re-review after try passes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab1071bafcd0
Attachment #8744217 - Flags: review+
Comment on attachment 8744217 [details]
MozReview Request: Bug 1153393 - Make session restore restores the scroll position of an about:reader page, r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48411/diff/5-6/
Comment on attachment 8744217 [details]
MozReview Request: Bug 1153393 - Make session restore restores the scroll position of an about:reader page, r=gijs

Hi, I am asking for re-review for the following changes:

1. The script now acounts the fact AboutReaderContentReady event might happen before the load event.
2. The newly added test have moved to a new file, to avoid bug 1254842 from taking place even more often.
Attachment #8744217 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8744217 [details]
MozReview Request: Bug 1153393 - Make session restore restores the scroll position of an about:reader page, r=gijs

https://reviewboard.mozilla.org/r/48411/#review48039

Makes sense to me. Thanks!
Attachment #8744217 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/c94c82ce2365
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
I have reproduced this bug on Nightly 40.0a1 (2015-04-10) .
This Bug's fix is verified on Latest Aurora 49.0a2.

Build ID : 20160723004004
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

Tested OS Windows 8.1 64 bit!

[testday-20160722]
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
I have tested it on my phone with Firefox for Android 49.0b11.
I think it is a regression since I have seen it fixed with a previous beta version, maybe in 49.0b04.
(In reply to billiob from comment #52)
> I have tested it on my phone with Firefox for Android 49.0b11.
> I think it is a regression since I have seen it fixed with a previous beta
> version, maybe in 49.0b04.

That might be possible, but this bug is about Firefox Desktop, not for Android. You might want to consider filing a new bug in that product, referencing this bug.
Thanks for reporting in!
See Also: → 1301016
Blocks: 1301016
See Also: 1301016 →
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: