-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Upgrade pdfjs-dist to 6.0.227 for latest stable PDF.js support. #2113
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,7 +15,6 @@ | |||||||||||||
| import invariant from 'tiny-invariant'; | ||||||||||||||
|
|
||||||||||||||
| import type { PDFDocumentProxy } from 'pdfjs-dist'; | ||||||||||||||
| import type { IPDFLinkService } from 'pdfjs-dist/types/web/interfaces.js'; | ||||||||||||||
| import type { | ||||||||||||||
| Dest, | ||||||||||||||
| ExternalLinkRel, | ||||||||||||||
|
|
@@ -31,7 +30,7 @@ type PDFViewer = { | |||||||||||||
| scrollPageIntoView: (args: ScrollPageIntoViewArgs) => void; | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| export default class LinkService implements IPDFLinkService { | ||||||||||||||
| export default class LinkService { | ||||||||||||||
| externalLinkEnabled: boolean; | ||||||||||||||
| externalLinkRel?: ExternalLinkRel; | ||||||||||||||
| externalLinkTarget?: ExternalLinkTarget; | ||||||||||||||
|
|
@@ -192,8 +191,8 @@ export default class LinkService implements IPDFLinkService { | |||||||||||||
| // Intentionally empty | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| executeSetOCGState(): void { | ||||||||||||||
| // Intentionally empty | ||||||||||||||
| executeSetOCGState(): Promise<void> { | ||||||||||||||
| return Promise.resolve(); | ||||||||||||||
| } | ||||||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or:
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| isPageVisible(): boolean { | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ import useResolver from '../shared/hooks/useResolver.js'; | |
| import { cancelRunningTask } from '../shared/utils.js'; | ||
|
|
||
| import type { AnnotationLayerParameters } from 'pdfjs-dist/types/src/display/annotation_layer.js'; | ||
| import type { PDFLinkService } from 'pdfjs-dist/types/web/pdf_link_service.js'; | ||
| import type { Annotations } from '../shared/types.js'; | ||
|
|
||
| export default function AnnotationLayer(): React.ReactElement { | ||
|
|
@@ -180,25 +181,39 @@ export default function AnnotationLayer(): React.ReactElement { | |
| annotationStorage: pdf.annotationStorage, | ||
| div: layer, | ||
| imageResourcesPath, | ||
| linkService, | ||
| linkService: linkService as unknown as PDFLinkService, | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a fan of type assertions, let's fix that in a way that does not involve lying to TypeScript |
||
| page, | ||
| renderForms, | ||
| viewport: clonedViewport, | ||
| }; | ||
|
|
||
| layer.innerHTML = ''; | ||
|
|
||
| try { | ||
| new pdfjs.AnnotationLayer(annotationLayerParameters).render(renderParameters); | ||
| let cancelled = false; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makeCancellable should handle that by default without the need of manually checking if promise has been cancelled. |
||
|
|
||
| // Intentional immediate callback | ||
| onRenderSuccess(); | ||
| } catch (error) { | ||
| onRenderError(error); | ||
| } | ||
| const cancellable = makeCancellable( | ||
| new pdfjs.AnnotationLayer({ | ||
| ...annotationLayerParameters, | ||
| linkService: linkService as unknown as PDFLinkService, | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, I would rather not lie to TypeScript here. |
||
| }).render(renderParameters), | ||
| ); | ||
| const runningTask = cancellable; | ||
|
|
||
| cancellable.promise | ||
| .then(() => { | ||
| if (!cancelled) { | ||
| onRenderSuccess(); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| if (!cancelled) { | ||
| onRenderError(error); | ||
| } | ||
| }); | ||
|
|
||
| return () => { | ||
| // TODO: Cancel running task? | ||
| cancelled = true; | ||
| cancelRunningTask(runningTask); | ||
| }; | ||
| }, | ||
| [ | ||
|
|
||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand these changes yet...
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PDF.js 6 can render more [role="presentation"] nodes than textContent.items.length when marked content is included. Added a getRenderedTextItemCount() helper with a comment explaining this, instead of comparing against desiredTextItems.length. |
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand these changes yet...
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switched from asserting PDF.js internal _page* fields to public callback fields (pageNumber), which is what onLoadSuccess actually exposes. Updated the PR with a comment in the spec explaining this. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,11 @@ | ||
| import * as pdfjs from 'pdfjs-dist'; | ||
|
|
||
| import './src/pdf.worker.entry.js'; | ||
|
|
||
| pdfjs.GlobalWorkerOptions.workerSrc = new URL( | ||
| 'pdfjs-dist/build/pdf.worker.min.mjs', | ||
| import.meta.url, | ||
| ).toString(); | ||
|
|
||
| document.body.style.setProperty('--react-pdf-annotation-layer', '1'); | ||
| document.body.style.setProperty('--react-pdf-text-layer', '1'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant? Why was this needed now?