search

dealfonso / pdfjs-viewer

A viewer based on PDFjs, which can be embedded in any web page (not using iframes)
Apache License 2.0
59 stars 3 forks source link

Rendering problem #14

Closed dfaber2 closed 2 weeks ago

dfaber2 commented 4 months ago

Carlos: really appreciated pdfs-viewer.js. I have incorporated it into a large project. Had a problem rendering many PDFs that had multiple pages. I changed one line of code and now it renders successfully:

    _setPageContent($page, $content) {
        //$page.find(`.${this.settings.contentClass}`).empty().append($content);
        $page.find(`.${this.settings.contentClass}`).append($content);
    }

I think that there was a race condition where empty() was completing after append() finished, so the image briefly appeared and then was removed. Even when append() runs more than once, it doesn't seem to duplicate the content of the page?

Thanks again for good work. --dfaber@mediyeti.com !Que le vaya bien!

dealfonso commented 4 months ago

Hi,

Thank you for reporting this issue.

I'd like to debug it before adding this patch to the main code because it makes no sense to me that empty ends before append. Under which kind of documents are you getting that problem? Please tell me about the number and type of pages (small images, lots of pictures, big images, text, etc.), and I'll prepare a document to reproduce your problem.

I appreciate your help.

dfaber2 commented 4 months ago

We incorporated your js into our clients instances and immediately got reports of partial display. These initially were incoming faxes or other image documents that were converted to PDF (this did not show up during initial testing with code generated PDF). When I tested on their first fax image it was about 4 pages long, and the last page would display but the others would not. Often only some of the thumbnails would display, and the main images were blank. What interested me was that on some documents during the initial rendering, I would see the real image display very briefly (less than a second) and then the page went blank. Removing the .empty() call completely fixed the problem.

Dan Faber

On Jul 21, 2024, at 1:53 AM, Carlos de Alfonso Laguna @.***> wrote:

Hi,

Thank you for reporting this issue.

I'd like to debug it before adding this patch to the main code because it makes no sense to me that empty ends before append. Under which kind of documents are you getting that problem? Please tell me about the number and type of pages (small images, lots of pictures, big images, text, etc.), and I'll prepare a document to reproduce your problem.

I appreciate your help.

— Reply to this email directly, view it on GitHub https://github.com/dealfonso/pdfjs-viewer/issues/14#issuecomment-2241515698, or unsubscribe https://github.com/notifications/unsubscribe-auth/BKABWW777GFMBPKFXO7VJBDZNNSHVAVCNFSM6AAAAABLGDKBRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBRGUYTKNRZHA. You are receiving this because you authored the thread.

musecreation commented 1 month ago

We are having the same issue, and the removal of the .empty() cleared the problem for us. It wasn't on all PDFs, but anything that was larger than about 500KB and was still loading had this issue. Seems like it needs a bit of a delay?

dealfonso commented 1 month ago

Hi,

I am trying to reproduce this problem... I downloaded docs https://research.nhm.org/pdfs/10840/10840.pdf, https://files.testfile.org/PDF/30MB-TESTFILE.ORG.pdf and https://cartographicperspectives.org/index.php/journal/article/view/cp43-complete-issue/pdf that I did not generate and I can see them without issues.

Could you please provide an example to try to reproduce it?

** it seems that the patch does not break the code... so I can use that patch anyway, but I'd like to understand the issue.

musecreation commented 1 month ago

Here are a few PDFs that my client gave me that were experiencing this issue. We are running these locally on a WAMP server but these are as my client gave them to me.

dealfonso commented 1 month ago

Sorry but I tried the docs and faced no issue... The docs are properly rendered. Could you please try the following update to the code?

(*) I intend to check whether it is a jQuery problem or mine.

_cleanPage($page) {
    let $emptyContent = this.settings.emptyContent();
    this._setPageContent($page, $emptyContent);
}
_setPageContent($page, $content) {
    for (let element of $page.find(`.${this.settings.contentClass}`)) {
        element.innerHTML = "";
        $(element).append($content);
    }
}
musecreation commented 1 month ago

If it helps, I'm using jQuery version: jquery-1.12.4.min.js

I can test this snippet tomorrow when I'm back in the office.

dealfonso commented 1 month ago

If it helps, I'm using jQuery version: jquery-1.12.4.min.js

I can test this snippet tomorrow when I'm back in the office.

That is a pretty old release! I have tried during my debug, but I found no issue. I tried Edge, Chrome and Safari... Which OS are you using?

Anyway, the patch did work? I have created an alternate version that does not need depend on (I implemented a "fallback" version).

dealfonso commented 3 weeks ago

Hi,

I have created a version that uses a different library from jQuery: it uses nojquery, which is a subset of jQuery and its code is far more simple.

Could you please try this version? You will need to include nojquery:

<script src="https://cdn.jsdelivr.net/gh/jsutilslib/nojquery@1/dist/nojquery.min.js"></script>
<script src="https://cdn.jsdelivr.net/gh/dealfonso/pdfjs-viewer@nojquery/js/pdfjs-viewer.js">
dfaber2 commented 3 weeks ago

My problem is that I include this element on a page that has other elements that need regular jQuery.

Dan Faber

On Nov 5, 2024, at 5:22 AM, Carlos de Alfonso Laguna @.***> wrote:

Hi,

I have created a version that uses a different library from jQuery: it uses nojquery https://github.com/jsutilslib/nojquery, which is a subset of jQuery and its code is far more simple.

Could you please try this version? You will need to include nojquery:

<script @./dist/nojquery.min.js"> <script @./js/pdfjs-viewer.js"> — Reply to this email directly, view it on GitHub https://github.com/dealfonso/pdfjs-viewer/issues/14#issuecomment-2457029387, or unsubscribe https://github.com/notifications/unsubscribe-auth/BKABWWY5GNMN6QI76LLL4QLZ7CZ7DAVCNFSM6AAAAABLGDKBRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJXGAZDSMZYG4. You are receiving this because you authored the thread.

dealfonso commented 3 weeks ago

in principle, there should be no problem to use common jQuery... including nojquery will make that pdfjs-viewer uses it, but you can still use jQuery (or nojquery, or other) in the rest of the project.

dealfonso commented 3 weeks ago

hi, I changed stopped used the $.empty function (which seems to be the problem) and used $.html function. In this way it is supposed to no to need the nojquery library. Could anyone test if this correct the problem? (I cannot reproduce it)

dfaber2 commented 3 weeks ago

I switched out line 148 on pdfs-viewer.js as follows:

Instead of: $page.find(.${this.settings.contentClass}).empty().append($content);

I put in: $page.find(.${this.settings.contentClass}).html().append($content);

This caused the display to hang.



If I then changed the line to: $page.find(.${this.settings.contentClass}).append($content);

Then it rendered correctly.

 Using the “empty” code caused a single page PDF to render correctly. The problem occurred only sometimes when there were many pages: some of them would be blank.

And TODAY I cannot reproduce the problem. My multipage PDFs all render correctly with the original code using “empty”. 🤔

Faber

On Nov 11, 2024, at 5:41 AM, Carlos de Alfonso Laguna @.***> wrote:

hi, I changed stopped used the $.empty function (which seems to be the problem) and used $.html function. In this way it is supposed to no to need the nojquery library. Could anyone test if this correct the problem? (I cannot reproduce it)

— Reply to this email directly, view it on GitHub https://github.com/dealfonso/pdfjs-viewer/issues/14#issuecomment-2468086910, or unsubscribe https://github.com/notifications/unsubscribe-auth/BKABWW2AKFEVWBKHQ3TE2QL2ACQWHAVCNFSM6AAAAABLGDKBRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRYGA4DMOJRGA. You are receiving this because you authored the thread.

dealfonso commented 2 weeks ago

hi, it is normal that your code don't work, because function html without arguments returns the inner HTML code. You need to set it to $page.find(`.${this.settings.contentClass}`).html("").append($content); to test the patch. Thank you for your help.

dealfonso commented 2 weeks ago

hi, I have just released a new version of the library (2.0.0). It includes the cleaning fix, but it is not needed to use nojquery. I'd consider this problem as solved, as I introduced a solution and it cannot be reproduced.

dfaber2 commented 1 week ago

Thanks you,. I expect this will completely resolve any issues.

On Nov 14, 2024, at 6:17 AM, Carlos de Alfonso Laguna @.***> wrote:

hi, I have just released a new version of the library (2.0.0). It includes the cleaning fix, but it is not needed to use nojquery. I'd consider this problem as solved, as I introduced a solution and it cannot be reproduced.

Version 2.0.0 introduces a major change for callbacks: the objects received are not jQuery objects (e.g. new pages). These objects are plain HTML objects, but you can easily covert them to jQuery objects by using $(page) — Reply to this email directly, view it on GitHub https://github.com/dealfonso/pdfjs-viewer/issues/14#issuecomment-2476333721, or unsubscribe https://github.com/notifications/unsubscribe-auth/BKABWW3JNWX4LDHLAZLCX2L2ASPFBAVCNFSM6AAAAABLGDKBRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINZWGMZTGNZSGE. You are receiving this because you authored the thread.