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

Pinch to zoom [WIP] #3708

Closed
wants to merge 1 commit into from
Closed

Conversation

timvandermeij
Copy link
Contributor

This PR implements pinch to zoom functionality as described in #2582.

Things left to do:

  • Tweak pinch to zoom to make it more fluent/stable.
  • Fix the issue that the page always jumps to the top left side and not to the part that was pinched.
  • Make pinch to zoom work on Chrome. (Turns out that Chrome for Android crashes above 170%, probably also causing most of the problems with my implementation. Tracking at PDF.js crashes Chrome on Android when reaching 170% zoom #3799. The rest of the implementation appears to be working on Chrome for Android!)
  • Make sure Hammer.js license (see https://github.com/EightMedia/hammer.js and the commit file) is not a problem. Also make sure that the file in the external folder will be used in the add-on version as well (not sure if an include from the external folder will be used everywhere).
  • Test on different platforms (tablet, mobile, desktop with trackpad) and on different browsers/OS's.

@Snuffleupagus
Copy link
Collaborator

@timvandermeij Nice that you're making progress!
If we import external libraries, shouldn't they be placed in the /external folder (https://github.com/mozilla/pdf.js/tree/master/external)? Either way, I don't think that you need to worry about the coding style of external libraries.

One other idea: Instead of adding a lot of code in viewer.js, what about creating a new file instead (perhaps called touch_events.js and structured like the other files that contains code moved from viewer.js)?
In that file you could place all the necessary logic to deal with the imported library (that file would then act as a wrapper for the library). This way you would just have to call an initialization function in viewer.js, something like TouchEvents.initialize(...); and you could place the code that currently is added to viewer.js there instead. This might make the implementation a bit cleaner!?

@timvandermeij
Copy link
Contributor Author

@Snuffleupagus I have made the changes you proposed in this new commit, thanks! That also resolves Travis' issues.

@@ -77,6 +78,7 @@ var mozL10n = document.mozL10n || document.webL10n;
var cache = new Cache(CACHE_SIZE);
var currentPageNumber = 1;

//#include ../external/hammer.js/hammer.js
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest moving this line to touch_events.js instead.

@Snuffleupagus
Copy link
Collaborator

Fix the issue that the page always jumps to the top left side and not to the part that was pinched.

I assume that this would require something like a re-implementation of #3513. I've actually taken a stab at this again, but it still requires more testing before I'll consider submitting it again.
In the meantime, you could try using PDFView.parseScale(scale, true, true). This will not scroll the document at all when you zoom, which might look a bit better.

@timvandermeij
Copy link
Contributor Author

Rebased. No code changes were made.

@timvandermeij
Copy link
Contributor Author

I'm closing this. The implementation is far from stable and unfortunately I cannot put a lot of time in this feature right now. The original issue is still open so we won't forget about this feature.

@timvandermeij timvandermeij deleted the pinch-to-zoom branch January 13, 2014 22:50
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.

None yet

2 participants