stephanrauh / ngx-extended-pdf-viewer

A full-blown PDF viewer for Angular 16, 17, and beyond
https://pdfviewer.net
Apache License 2.0
484 stars 182 forks source link

enablePinchOnMobile set to true causes undesirable zoom issues #918

Closed adambeer closed 1 year ago

adambeer commented 3 years ago

When enabling the enablePinchOnMobile and zooming in and out, there are a couple issues that are happening:

I have tested this using an iphone. Every document I tested had this issue but just in case, here is a document that can be used to replicate this: Transport Canada AIM

I am using version 9 of the library with pdf.js 2.9. Thanks for all of your help with this library, its awesome!

stephanrauh commented 3 years ago

I know I'm demanding a lot, but I'm afraid I need your help. During the last couple of weeks, more issues come in than I can answer. Would you mind to help me hunting down the bug?

enablePinchOnMobile is implemented in a class called PinchOnMobileSupport. It's loosely coupled to ngx-extended-pdf-viewer, so you can easily override it with your own copy. I can help you if necessary, but the basic idea is not to set enablePinchOnMobile="true" but to instantiate your own copy of PinchOnMobileSupport. That's exactly what ngx-extended-pdf-viewer does (https://github.com/stephanrauh/ngx-extended-pdf-viewer/blob/03c9bfce672a60f0ebe5edacc160a23c5cbba753/projects/ngx-extended-pdf-viewer/src/lib/ngx-extended-pdf-viewer.component.ts#L881).

Now you can debug the pinch gesture support. I'm afraid that's not enough to solve your issue. It's probably a complex interaction with pdf.js itself. The only hint I can give you is to set [minifiedJSLibraries]=false.

Best regards, Stephan

stephanrauh commented 3 years ago

@adambeer Is your bug related to #672?

adambeer commented 3 years ago

@stephanrauh Ill take a deeper look at this and yes, from the video in #672 it looks like the identical issue Im having. Its just not limited to the last page like that bug report is suggesting.

adambeer commented 3 years ago

@stephanrauh I needed zooming capabilities in my app so I added a + and - button and created some methods to zoom in and out in fixed amounts. Even with manually setting the zoom parameter this way, there is still the jumping to different pages issue so that tells me this isnt necessarily related to enablePinchOnMobile.

stephanrauh commented 3 years ago

OK, that's probably good news. For some reason unknown, my Macbook doesn't connect to my iPad, so I can't use the debugger. If we can reproduce the bug on the desktop, my life is a lot easier. Or your life, if you want to give it a try yourself.

Unfortunately, I'm afraid the bug lurks in the depth of the pdf.js code. Probably it's related to the method scrollIntoView(), but that' just a guess.

adambeer commented 3 years ago

@stephanrauh I am trying to test the current version of the library to see is this issue is somehow solved before I jump into code and am having trouble getting it to start. I ran npm install and ng start but Im getting a bunch of errors. I needed to install @angular/material to get it to work as well. Am I missing any steps to just get this running?

stephanrauh commented 3 years ago

@adambeer What are you doing precisely? A simple "npm install ngx-extended-pdf-viewer" doesn't require you to add Angular Material. If it does, something's run havoc yesterday when I published the latest version. But maybe you've tried to do something different?

adambeer commented 3 years ago

@stephanrauh I copied the current code off the main branch, ran "npm install" and then "ng start --host 0.0.0.0" to try to test the current version of main to see if this zoom issue somehow got resolved since 2.9.

stephanrauh commented 3 years ago

In other words, you've tried to compile the library from source. Did I guess correctly?

I've published my latest changes yesterday as version 10.0.1, so currently, you don't need the source code version. But if you want to use it, use and modify the npm run showcase script of the package.json. When I'm developing the library, I simply copy the dist folder of the library to the node_modules folder of the target application.

If you want to modify the base library, pdf.js, too, things become even more convoluted. I've tried to document my workflow here: https://github.com/stephanrauh/ngx-extended-pdf-viewer/blob/main/projects/ngx-extended-pdf-viewer/how-to-build.md

BTW, I hope to find a more efficient workflow soon. Recently, progress has slowed down a lot because every change of pdf.js takes five minutes to compile until I can test it.

adambeer commented 3 years ago

@stephanrauh I guess I am. I dont really need to compile from source right now but Im also not familiar how to use the library without it being a "release".

All im trying to do is modify "src/app/app.component.html" and .ts and build/start it to test pdf.js 2.10 and the latest changes reflected on github. Im downloading this code as a zip, extract it, run npm install, and then try to run the start script.

Im getting errors like:

Module not found: Error: Can't resolve '@angular/material/dialog' Cannot find module '@angular/material' or its corresponding type declarations. Can't bind to 'src' since it isn't a known property of 'ngx-extended-pdf-viewer'. Can't bind to 'showZoomButtons' since it isn't a known property of 'ngx-extended-pdf-viewer'.

stephanrauh commented 3 years ago

I see. I've neglected the default project of ngx-extended-pdf-viewer. When I began the project, there was a library in the folder /projects/ngx-extended-pdf-viewer and a default project in the root folder. I used the default project to test the library. At some point in time, I created the showcase. Before long, I neglected the default project, using the showcase exclusively to test and develop the features.

So forget about src/app/app.component.html. Clone the showcase instead. It's the current reference project.

It's high time I update or delete the default project!

adambeer commented 3 years ago

@stephanrauh I dont know whats going on but I cloned the showcase and ran npm install and Im still getting errors.

./src/app/extended-pdf-viewer/export/export.component.ts:2:0-144 - Error: Module not found: Error: Can't resolve '../../../../../ngx-extended-pdf-viewer/projects/ngx-extended-pdf-viewer/src/lib/options/pdf-default-options' in 'D:\Test\Repos\default-pdf-viewer2\src\app\extended-pdf-viewer\export'

Error: src/app/extended-pdf-viewer/export/export.component.ts:3:35 - error TS2307: Cannot find module '../../../../../ngx-extended-pdf-viewer/projects/ngx-extended-pdf-viewer/src/lib/options/pdf-default-options' or its corresponding type declarations.

3 import { pdfDefaultOptions } from '../../../../../ngx-extended-pdf-viewer/projects/ngx-extended-pdf-viewer/src/lib/options/pdf-default-options';

I ran the prestart script, and start2. Any ideas?

stephanrauh commented 3 years ago

Oops. Sorry. My fault. One of my VS Code plugins always adds the wrong imports, and I never notice because I've checked out both the showcase and the library side-by-side.

I've corrected the import statements and pushed them to GitHub. Please try again!

adambeer commented 3 years ago

@stephanrauh I finally got it running! Is the showcase safe to build something for production exactly the way release 2.9 was setup? I want to test this in place of the 2.9 release version to see if the zooming issues are magically fixed.

adam-abdulaev commented 3 years ago

Hi. I struggled with this bug for a while too, and now I had to go back to it again. I suspect that the problem is with these lines. If you comment them out, there is no scrolling to a completely different page. image

adam-abdulaev commented 3 years ago

I have an opinion that this is a matter of calculations. Once zoomed in, the scroll height changes in pixels and container.scrollTop keeps the wrong value for us. I have verified this with alert (container.scrollTop) in the code.

adam-abdulaev commented 3 years ago

here are some of my attempts to fix the bug. I don't understand why the first alert gives the correct value, and the second one dumps it. Any ideas? image photo_2021-10-06_18-31-16 photo_2021-10-06_18-31-32

adambeer commented 3 years ago

@adamabdulaev When I was playing with just manually adjusting the zoom parameter, I was also getting the same jumping effect you posted on your video in the other ticket. I dont think this bug is limited to pinch to zoom, rather its related to zooming as a whole.

adam-abdulaev commented 3 years ago

@adamabdulaev When I was playing with just manually adjusting the zoom parameter, I was also getting the same jumping effect you posted on your video in the other ticket. I dont think this bug is limited to pinch to zoom, rather its related to zooming as a whole.

in my pdf.js implementation in angular i am not using ngx-extended-pdf-viewer. And with enthusiasm for the + and - buttons, this problem is not. But I am using pinchzoom from ngx-extended-pdf-viewer and after scaling the viewerContainer scrolls a few pages up. It seems to me that these are different problems.

stephanrauh commented 3 years ago

This might be a hot scent. It's plausible that scrollTop += something jumps to a different page, and it's also plausible to assume the original author of my pinch implementation only tested PDF files consisting of a single page.

adam-abdulaev commented 3 years ago

can you tell me how I can debug touchstart, touchend events on my laptop? what do I do to emulate a pinch and get the information I need in the console?

stephanrauh commented 3 years ago

I never thought debugging touch gestures on a laptop is possible - but it seems you're right, and it is possible. Look here: https://www.frontify.com/en/blog/how-to-emulate-touch-events-in-chrome/

I didn't test yet, but it looks promising.

adambeer commented 3 years ago

@adamabdulaev @stephanrauh Any updates on this?

stephanrauh commented 3 years ago

Yes, maybe. @barnewsapp sort of promised to send me a pull request. At the moment, I'm waiting for the things to come. :)

Richugan commented 3 years ago

Hey Guys! I have same problem with pinch zoom. So can i get information about fix?

stephanrauh commented 3 years ago

Now I know why the pdf.js team refused to implement the feature. :) They've experimented with it for some time, but no matter what they did, they were always unhappy with the result. I suppose that's what you're experiencing right now. :)

BTW, the best way to speed up the bugfix is to offer your help. I know that's demanding a lot, but on the other hand, that's the spirit of open source. I'd like to encourage you to ping @barnewsapp and to see how you can help them. If you think that's a difficult task: it's not. This particular feature is implemented in TypeScript. It's just Angular code.

barnewsapp commented 3 years ago

Hi guys. As @stephanrauh mentioned, I am working on a PR with improvements to the mobile zoom functionality. But last couple of weeks I had to work on other projects. I hope I will have to the time to work on this next week. I'll let you know when I have something to show.

cverons commented 3 years ago

Hi,

I came from : https://github.com/stephanrauh/ngx-extended-pdf-viewer/blob/b3cc11013499b76964924b1b911faee99890a099/projects/ngx-extended-pdf-viewer/src/lib/pinch-on-mobile-support.ts#L98-L103

to :

    const container = document.getElementById('viewerContainer') as HTMLDivElement;
    const dx = this.startX * (this.pinchScale - 1);
    const dy = this.startY * (this.pinchScale - 1);
    container.scrollLeft += dx;
    container.scrollTop += dy;

Now, the zoom stays where I left it at the end of the pinch. As I am managing the paging by my own, I didn't check why it jumps to another page.

@barnewsapp, how about your PR ? Do you think that you can provide us with a fix in the coming days ?

barnewsapp commented 3 years ago

@cverons Hi. I will be working on it this week but I do not expect to have a PR ready this week. Maybe next week, but no promises. It seems every time I start working on it, some other project gets in the way :-p

adambeer commented 2 years ago

@barnewsapp Any updates on this?

barnewsapp commented 2 years ago

Hi. I worked about 25 hours on this last week.

It is more complicated than I first expected. But I have made som progress and will do more work this week.

My version of pinch-on-mobile-support.ts-file is currency 563 lines with a lot of debugging code, original was 137 lines :-)

One thing that I improved is that you now can zoom and pan at the same time. No PR sent yet.

But the big issue is with the jumping when finishing a pinch-zoom. It has been very frustrating to debug.

I understand that you want this fixed ASAP, me too. I'll let you know when I have something to show.

stephanrauh commented 2 years ago

@barnewsapp The other day, I found something that might help you. The method _setScale(value, noScroll = false) in the file base_viewer.js has a second parameters that's always set to true. As far as I understand, you can disable scrolling by setting it to false. But I didn't examine it yet.

barnewsapp commented 2 years ago

@stephanrauh Thanks, I will look into it.

barnewsapp commented 2 years ago

Hi. Just a quick update on this. To make this work reliable, I think we have to do a bigger change than just do some changes to the pinch-on-mobile-support.ts-file.

I think the problem is that currently the positioning of the viewer is done by using viewerContainer.scrollLeft and viewerContainer.scrolLTop.

It would be much easier if the viewer was positioned relative with x and y. So my current strategy is to implement a new "module" called RelativeCoordsSupport. By doing it this way it is much easier to enable or disable it without breaking the current implementation.

@stephanrauh Do you have any objections doing it this way?

We are focusing on the usecase with pageViewMode="single" scrollMode="page" spreadMode="off|even", because this is what we use. But I think it should work in the "normal" modes aswell.

I'll keep you updated.

adambeer commented 2 years ago

@barnewsapp Any updates on this?

barnewsapp commented 2 years ago

Hi, Still working on it and are making some progress. I think using relative corrds is the way to go, but the progress is slow. The last two weeks I expected to have i MVP done, but no. I will work on it to I get it working, i can promise you that.

barnewsapp commented 2 years ago

Hi again. Okey, just had a big breakthrough :-) Still some work to be done and code clean up, but now I have working version locally.

I will attach a video:

https://user-images.githubusercontent.com/67910734/146373604-5551736b-a9cc-41b0-81cd-25ad1418c74a.MP4

PS: I have also implemented a contain-function, if the viewer is smaller then the container, you may not pan the page outside the container.. If the viewer is bigger, you may not pan the outside the page edge.

stephanrauh commented 2 years ago

Sounds good! And the video looks promising, too. I'm looking forward to your solution!

barnewsapp commented 2 years ago

@stephanrauh @adambeer Hi. I will not have the time to complete the task and code clean up before Christmas. Should I send a PR tomorrow with my current code or wait until January when I have a fully functional version? Let me know what you think.

stephanrauh commented 2 years ago

Well, being to able to have a look at the progress in the workshop is always nice. Feel free to submit the PR, so I can have a look, and tell me when you feel it's finished.

barnewsapp commented 2 years ago

PR sent: #1099 Merry Christmas

TiBz0u commented 2 years ago

Hi, I don't know if it could help but we add some issues with this option too (using Ionic) => Application crash/slow zoom on Android since Android Webview 97+ (or close, don't remember exactly). We finally choose to overwrite the defaultOptions useOnlyCssZoom to "true" and it solve our issue. Maybe it can help :) Kr

adambeer commented 2 years ago

Has the PR from barnewsapp made it into the codebase yet?

stephanrauh commented 2 years ago

Not yet. But I started to wonder about the current state of the art, too. @barnewsapp Do you want me to merge the PR? After our last chat, I believed you still wanted to fix a bug, run a test or something like that. Did I get you wrong?

barnewsapp commented 2 years ago

Hi. I think we are close to a release, but where is couple of gitches (bugs) that I want to fix before it is production-ready.

But I think it is pretty safe to merge, because it is implemented as an optional "module" that need to be enabled, and does not change any of the existing code.

I hope I will have time next week to work on the last bugs.

stephanrauh commented 2 years ago

OK, I'll try to merge the PR this evening.

stephanrauh commented 2 years ago

The PR is now part of version 15.0.0-alpha.0. However, I didn't manage to publish a showcase demo yet.

barnewsapp commented 2 years ago

Great @stephanrauh Thank you.

adamwarnick commented 2 years ago

We had this issue and it seems to me that it hasn't been solved on the node package level. We made a temporary solution that didn't fix it 100% of the time, but was for the most part effective.

zoomInOut: any = this.platform.is("mobile") ? "page-fit" : 100;
zoomArray = [70, 80, 90, 100, 110, 120, 130, 140, 150, 160, 170, 180, 190, 200];

setZoom(zoom) {
    this.zoomInOut = Math.floor((zoom * 100) / 10) * 10;

    this.zoomArray.push(this.zoomInOut);
    this.zoomArray.splice(0, 1);

    const uniqueCount = new Set();
    for (const zoomLevel of this.zoomArray) {
      uniqueCount.add(zoomLevel);
    }
    if (uniqueCount.size < 8) {
      this.zoomInOut = 100;
    }
  }

How we got to this is we graphed the zoom data and foud that each of the 4 values was frequently repeating in a small range. So, if half or less of the possible zoom values are accounted for in a sequence, then there is a zoom issue and we reset it.

stephanrauh commented 2 years ago

@adamwarnick Give me some context, please. What did you do precisely? Which version of ngx-extended-pdf-viewer are you using? Did you already activate [enableRelativeCoords]="true"?

The cycle of four zoom values rings a bell. I've seen this, too, but I've forgotten where and when.