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 document loading with zoom set to 'page-fit', regression from #2719 #3224

Closed
wants to merge 1 commit into from
Closed

Fix document loading with zoom set to 'page-fit', regression from #2719 #3224

wants to merge 1 commit into from

Conversation

Snuffleupagus
Copy link
Collaborator

This PR fixes an issue when the PDF document is loaded with zoom set to 'page-fit'.
Currently, once the progressive loading bar is removed, the pages will be slightly shorter than the viewer height. To see this issue, try opening: http://mozilla.github.io/pdf.js/web/viewer.html#page=2&zoom=page-fit.

@pdfjsbot
Copy link

pdfjsbot commented May 7, 2013

From: Bot.io (Linux)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.21.233.14:8877/f735df845f2e94c/output.txt

@pdfjsbot
Copy link

pdfjsbot commented May 7, 2013

@yurydelendik
Copy link
Contributor

I think it will be much better if we will not change the view height because of the progress bar. Can we overlay the bar on top of the pages?

@Snuffleupagus
Copy link
Collaborator Author

I think it will be much better if we will not change the view height because of the progress bar. Can we overlay the bar on top of the pages?

I think that would re-introduce the visual bug described here: #2719 (comment).

I suppose that we could instead just ignore the height of the progressive loading bar, here: https://github.com/Snuffleupagus/pdf.js/blob/f6e1c9e23122c7c8054003355a11d8bdcda7f24c/web/viewer.js#L800.
The only issue with that solution is that it wouldn't look as good, since the pages wouldn't fit perfectly in the viewer until the loading bar was removed.

@yurydelendik Visually I prefer my solution, but from a performance perspective your solution is a lot better since it avoids having to rescale the document. Should I implement your idea instead?

@yurydelendik
Copy link
Contributor

I'm afraid we have too much specialized / branched logic for UI already without it's being tested. So it's good as simple solution as possible. (Even if it will introduce the visual bug above)

Let's try "window resize" logic here. See https://github.com/mozilla/pdf.js/blob/master/web/viewer.js#L3170, we can add it near https://github.com/mozilla/pdf.js/blob/master/web/viewer.js#L1377. That might be easier to follow, I think

@Snuffleupagus
Copy link
Collaborator Author

Closing in favour of #3332.

@Snuffleupagus Snuffleupagus deleted the fix-initial-page-fit branch June 4, 2013 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants