diff --git a/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.js b/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.js index c6e5e9c771..2f6c30d69c 100644 --- a/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.js +++ b/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.js @@ -33,15 +33,19 @@ function PdfJsViewer({ url }) { // create the viewer when the container is mounted const handleContainer = useCallback(parent => { - setPdfJsWrapper(parent ? new PDFJSWrapper(parent.firstChild) : undefined) + if (parent) { + const viewer = new PDFJSWrapper(parent.firstChild) + setPdfJsWrapper(viewer) + return () => viewer.destroy() + } }, []) // listen for initialize event useEffect(() => { if (pdfJsWrapper) { - pdfJsWrapper.eventBus.on('pagesinit', () => { - setInitialised(true) - }) + const handlePagesinit = () => setInitialised(true) + pdfJsWrapper.eventBus.on('pagesinit', handlePagesinit) + return () => pdfJsWrapper.eventBus.off('pagesinit', handlePagesinit) } }, [pdfJsWrapper]) @@ -53,6 +57,7 @@ function PdfJsViewer({ url }) { // TODO: anything else to be reset? pdfJsWrapper.loadDocument(url).catch(error => setError(error)) + return () => pdfJsWrapper.abortDocumentLoading() } }, [pdfJsWrapper, url]) @@ -73,21 +78,22 @@ function PdfJsViewer({ url }) { // listen for scroll events useEffect(() => { - if (pdfJsWrapper) { + if (initialised && pdfJsWrapper) { // store the scroll position in localStorage, for the synctex button const storePosition = debounce(pdfViewer => { // set position for "sync to code" button try { setPosition(pdfViewer.currentPosition) } catch (error) { - // console.error(error) // TODO + // TODO + console.error(error) } }, 500) // store the scroll position in localStorage, for use when reloading const storeScrollTop = debounce(pdfViewer => { // set position for "sync to code" button - setScrollTop(pdfJsWrapper.container.scrollTop) + setScrollTop(pdfViewer.container.scrollTop) }, 500) storePosition(pdfJsWrapper) @@ -100,15 +106,17 @@ function PdfJsViewer({ url }) { pdfJsWrapper.container.addEventListener('scroll', scrollListener) return () => { + storePosition.cancel() + storeScrollTop.cancel() pdfJsWrapper.container.removeEventListener('scroll', scrollListener) } } - }, [setPosition, setScrollTop, pdfJsWrapper]) + }, [setPosition, setScrollTop, pdfJsWrapper, initialised]) // listen for double-click events useEffect(() => { if (pdfJsWrapper) { - pdfJsWrapper.eventBus.on('textlayerrendered', textLayer => { + const handleTextlayerrendered = textLayer => { const pageElement = textLayer.source.textLayerDiv.closest('.page') const doubleClickListener = event => { @@ -120,7 +128,11 @@ function PdfJsViewer({ url }) { } pageElement.addEventListener('dblclick', doubleClickListener) - }) + } + + pdfJsWrapper.eventBus.on('textlayerrendered', handleTextlayerrendered) + return () => + pdfJsWrapper.eventBus.off('textlayerrendered', handleTextlayerrendered) } }, [pdfJsWrapper]) diff --git a/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.js b/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.js index ef3b073ad8..a8e8a9756f 100644 --- a/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.js +++ b/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.js @@ -32,7 +32,7 @@ export default class PDFJSWrapper { }) // create the localization - const l10n = new PDFJSViewer.GenericL10n('en-GB') // TODO: locale mapping? + // const l10n = new PDFJSViewer.GenericL10n('en-GB') // TODO: locale mapping? // create the viewer const viewer = new PDFJSViewer.PDFViewer({ @@ -40,7 +40,7 @@ export default class PDFJSWrapper { eventBus, imageResourcesPath, linkService, - l10n, + // l10n, // commented out since it currently breaks `aria-label` rendering in pdf pages enableScripting: false, // default is false, but set explicitly to be sure renderInteractiveForms: false, }) @@ -53,22 +53,48 @@ export default class PDFJSWrapper { } // load a document from a URL - async loadDocument(url) { - const doc = await PDFJS.getDocument({ - url, - cMapUrl, - cMapPacked: true, - disableFontFace, - rangeChunkSize, - disableAutoFetch: true, - disableStream: true, - textLayerMode: 2, // PDFJSViewer.TextLayerMode.ENABLE, - }).promise + loadDocument(url) { + // prevents any previous loading task from populating the viewer + this.loadDocumentTask = undefined - this.viewer.setDocument(doc) - this.linkService.setDocument(doc) + return new Promise((resolve, reject) => { + this.loadDocumentTask = PDFJS.getDocument({ + url, + cMapUrl, + cMapPacked: true, + disableFontFace, + rangeChunkSize, + disableAutoFetch: true, + disableStream: true, + textLayerMode: 2, // PDFJSViewer.TextLayerMode.ENABLE, + }) - return doc + this.loadDocumentTask.promise + .then(doc => { + if (!this.loadDocumentTask) { + return // ignoring the response since loading task has been aborted + } + + const previousDoc = this.viewer.pdfDocument + + this.viewer.setDocument(doc) + this.linkService.setDocument(doc) + resolve(doc) + + if (previousDoc) { + previousDoc.cleanup().catch(console.error) + previousDoc.destroy() + } + }) + .catch(error => { + if (this.loadDocumentTask) { + reject(error) + } + }) + .finally(() => { + this.loadDocumentTask = undefined + }) + }) } // update the current scale value if the container size changes @@ -123,4 +149,16 @@ export default class PDFJSWrapper { pageSize: { height, width }, } } + + abortDocumentLoading() { + this.loadDocumentTask = undefined + } + + destroy() { + if (this.loadDocumentTask) { + this.loadDocumentTask.destroy() + this.loadDocumentTask = undefined + } + this.viewer.destroy() + } } diff --git a/services/web/test/frontend/features/pdf-preview/components/pdf-js-viewer.test.js b/services/web/test/frontend/features/pdf-preview/components/pdf-js-viewer.test.js new file mode 100644 index 0000000000..ca298acebb --- /dev/null +++ b/services/web/test/frontend/features/pdf-preview/components/pdf-js-viewer.test.js @@ -0,0 +1,83 @@ +import { expect } from 'chai' +import { screen } from '@testing-library/react' +import path from 'path' +import { renderWithEditorContext } from '../../../helpers/render-with-context' +import { pathToFileURL } from 'url' +import PdfJsViewer from '../../../../../frontend/js/features/pdf-preview/components/pdf-js-viewer' + +const example = pathToFileURL( + path.join(__dirname, '../fixtures/test-example.pdf').toString() +) + +const exampleCorrupt = pathToFileURL( + path.join(__dirname, '../fixtures/test-example-corrupt.pdf') +).toString() + +const invalidURL = 'http://nonexisting.com/doc' + +describe('', function () { + it('loads all PDF pages', async function () { + renderWithEditorContext() + + await screen.findByLabelText('Page 1') + await screen.findByLabelText('Page 2') + await screen.findByLabelText('Page 3') + expect(screen.queryByLabelText('Page 4')).to.not.exist + }) + + it('renders pages in a "loading" state', async function () { + renderWithEditorContext() + await screen.findByLabelText('Loading…') + }) + + it('can be unmounted while loading a document', async function () { + const { unmount } = renderWithEditorContext() + unmount() + }) + + it('can be unmounted after loading a document', async function () { + const { unmount } = renderWithEditorContext() + await screen.findByLabelText('Page 1') + unmount() + }) + + describe('with an invalid URL', function () { + it('renders an error alert', async function () { + renderWithEditorContext() + await screen.findByRole('alert') + expect(screen.queryByLabelText('Page 1')).to.not.exist + }) + + it('can load another document after the error', async function () { + const { rerender } = renderWithEditorContext( + + ) + await screen.findByRole('alert') + + rerender() + + await screen.findByLabelText('Page 1') + expect(screen.queryByRole('alert')).to.not.exist + }) + }) + + describe('with an corrupted document', function () { + it('renders an error alert', async function () { + renderWithEditorContext() + await screen.findByRole('alert') + expect(screen.queryByLabelText('Page 1')).to.not.exist + }) + + it('can load another document after the error', async function () { + const { rerender } = renderWithEditorContext( + + ) + await screen.findByRole('alert') + + rerender() + + await screen.findByLabelText('Page 1') + expect(screen.queryByRole('alert')).to.not.exist + }) + }) +}) diff --git a/services/web/test/frontend/features/pdf-preview/fixtures/test-example-2.pdf b/services/web/test/frontend/features/pdf-preview/fixtures/test-example-2.pdf new file mode 100644 index 0000000000..d9f89df67d Binary files /dev/null and b/services/web/test/frontend/features/pdf-preview/fixtures/test-example-2.pdf differ diff --git a/services/web/test/frontend/features/pdf-preview/fixtures/test-example-corrupt.pdf b/services/web/test/frontend/features/pdf-preview/fixtures/test-example-corrupt.pdf new file mode 100644 index 0000000000..8d92665ae8 Binary files /dev/null and b/services/web/test/frontend/features/pdf-preview/fixtures/test-example-corrupt.pdf differ