Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PDF scroll position on Firefox. #6264

Merged
merged 3 commits into from Apr 29, 2019

Conversation

@ian-r-rose
Copy link
Member

commented Apr 27, 2019

@jasongrout and I had thought that the DOM structure changes from #6255 were responsible for a regression in the PDF.js viewer on Firefox where it lost its scroll position upon unhiding. I did some more testing on this, however, and as far as I can tell it was not the DOM changes, but instead was the re-setting of the data URL.

So before we try a ton of user-agent testing and building out a DOM-structure-matrix, I'd like to try this more limited fix. For me, on Ubuntu+Firefox/Chrome, it seems to behave reasonably. I have no idea why this fixes the PDF.js issue, but overall browser PDF behavior is not something that we can control too much.

I'd appreciate if folks with other OS/browser combinations could test this:

  1. Does a PDF render correctly?
  2. Does the PDF retain its position upon unhiding or moving?
  3. Do markdown links to the document with page fragments work? (e.g. #page=10).
For some reason this allows Firefox to re-find its scroll position upon
unhiding the PDF widget. I have no idea why, but this check is
relatively harmless and fixes a regression.
@jupyterlab-dev-mode

This comment has been minimized.

Copy link

commented Apr 27, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@blink1073

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

On Windows 10:

Firefox 66:
Show/hide with page fragment -> scrolls to that fragment
Show/hide no fragment -> scrolls to last position

Chrome 74:
Show/hide with fragment -> scrolls to that fragment
Show/hide no fragment -> scrolls to top

@blink1073

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

Forgot: both keep your scroll position when moving the tab.

@blink1073

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

Without this change, neither one respects the page fragment.

@blink1073

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

(well, this change and the previous one).

@ian-r-rose

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2019

Some testing on this:

On Ubuntu 18.04

Firefox 66:
Show/hide with page fragment -> scrolls to that fragment
Show/hide with no fragment -> scrolls to last position
Move tab -> maintains position

Chrome 64:
Show/hide with page fragment -> scrolls to last position
Show/hide with no fragment -> scrolls to last position
Move tab -> maintains position

Edit: updates with more description

@blink1073

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

Is it possible to throw out the fragment part of the object onBeforeHide? Then we would avoid it forever trying to return to that fragment.

@ian-r-rose

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2019

Thanks for the info @blink1073. I can reproduce the same behavior on Windows 10.

@ian-r-rose

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2019

Is it possible to throw out the fragment part of the object onBeforeHide? Then we would avoid it forever trying to return to that fragment.

Ooh, this is a good idea. I was just trying to do that after a timeout, which was not working very well :)

Get rid of fragment before hiding PDF on firefox in order to retain
scroll position in the case where there was a fragment.
@ian-r-rose

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2019

Edits with before-hide commit:

On Ubuntu 18.04

Firefox 66:
Show/hide with page fragment -> scrolls to last position
Show/hide with no fragment -> scrolls to last position
Move tab -> maintains position

Chrome 64:
Show/hide with page fragment -> scrolls to last position
Show/hide with no fragment -> scrolls to last position
Move tab -> maintains position

@blink1073
Copy link
Member

left a comment

Looks great, thanks!

*/
protected onBeforeHide(): void {
// Dispose of any URL fragment before hiding the widget
// so that it is not rememberd upon show. Only Firefox

This comment has been minimized.

Copy link
@blink1073

blink1073 Apr 27, 2019

Member

remembered*

@blink1073 blink1073 merged commit 506e815 into jupyterlab:master Apr 29, 2019

9 checks passed

jupyterlab.jupyterlab Build #20190429.5 succeeded
Details
jupyterlab.jupyterlab (Linux CLI) Linux CLI succeeded
Details
jupyterlab.jupyterlab (Linux Docs) Linux Docs succeeded
Details
jupyterlab.jupyterlab (Linux Integrity) Linux Integrity succeeded
Details
jupyterlab.jupyterlab (Linux JS) Linux JS succeeded
Details
jupyterlab.jupyterlab (Linux Python) Linux Python succeeded
Details
jupyterlab.jupyterlab (Windows Integrity) Windows Integrity succeeded
Details
jupyterlab.jupyterlab (Windows JS) Windows JS succeeded
Details
jupyterlab.jupyterlab (Windows Python) Windows Python succeeded
Details

@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants
You can’t perform that action at this time.