From a882a854461a60a3a38654a6d925c33ecf3cd979 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 12 Jan 2021 13:38:49 +0100 Subject: [PATCH 1/3] Fix the initialization/resetting of scripting-related events in the `BaseViewer` The "pageopen"/"pageclose"-events are only necessary if, and only if, there's actually a sandbox to dispatch the events in. Hence we shouldn't dispatch those events unconditionally, as soon as `enableScripting` is set, but rather initialize that functionality only when needed. Furthermore, in `web/app.js`, there's currently a bug since we're attempting to *manually* simulate a "pageopen"-event for a page that may not actually have been rendered at the time. With the modified `BaseViewer.initializeScriptingEvents` method, we'll now dispatch a correct "pageopen"-event here. --- web/app.js | 7 +++-- web/base_viewer.js | 65 +++++++++++++++++++++++++++++++--------------- 2 files changed, 47 insertions(+), 25 deletions(-) diff --git a/web/app.js b/web/app.js index 825b3eda9c1bf..e36e5f1a87f17 100644 --- a/web/app.js +++ b/web/app.js @@ -1588,7 +1588,7 @@ const PDFViewerApplication = { } } - this._scriptingInstance?.scripting.dispatchEventInSandbox({ + await this._scriptingInstance?.scripting.dispatchEventInSandbox({ id: "page", name: "PageOpen", pageNumber, @@ -1612,7 +1612,7 @@ const PDFViewerApplication = { return; // The document was closed while the actions resolved. } - this._scriptingInstance?.scripting.dispatchEventInSandbox({ + await this._scriptingInstance?.scripting.dispatchEventInSandbox({ id: "page", name: "PageClose", pageNumber, @@ -1678,8 +1678,7 @@ const PDFViewerApplication = { id: "doc", name: "Open", }); - - await pageOpen({ pageNumber: this.pdfViewer.currentPageNumber }); + await this.pdfViewer.initializeScriptingEvents(); // Used together with the integration-tests, see the `scriptingReady` // getter, to enable awaiting full initialization of the scripting/sandbox. diff --git a/web/base_viewer.js b/web/base_viewer.js index 60cfbcb2e408b..c9935ec1bf84a 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -215,7 +215,6 @@ class BaseViewer { if (this.removePageBorders) { this.viewer.classList.add("removePageBorders"); } - this._initializeScriptingEvents(); // Defer the dispatching of this event, to give other viewer components // time to initialize *and* register 'baseviewerinit' event listeners. Promise.resolve().then(() => { @@ -654,7 +653,6 @@ class BaseViewer { this._pagesCapability = createPromiseCapability(); this._scrollMode = ScrollMode.VERTICAL; this._spreadMode = SpreadMode.NONE; - this._pageOpenPendingSet?.clear(); if (this._onBeforeDraw) { this.eventBus._off("pagerender", this._onBeforeDraw); @@ -664,6 +662,8 @@ class BaseViewer { this.eventBus._off("pagerendered", this._onAfterDraw); this._onAfterDraw = null; } + this._resetScriptingEvents(); + // Remove the pages from the DOM... this.viewer.textContent = ""; // ... and reset the Scroll mode CSS class(es) afterwards. @@ -1507,15 +1507,13 @@ class BaseViewer { this.update(); } - /** - * @private - */ - _initializeScriptingEvents() { - if (!this.enableScripting) { + initializeScriptingEvents() { + if (!this.enableScripting || this._pageOpenPendingSet) { return; } const eventBus = this.eventBus, - pageOpenPendingSet = (this._pageOpenPendingSet ||= new Set()); + pageOpenPendingSet = (this._pageOpenPendingSet = new Set()), + scriptingEvents = (this._scriptingEvents ||= Object.create(null)); const dispatchPageClose = pageNumber => { if (pageOpenPendingSet.has(pageNumber)) { @@ -1523,9 +1521,9 @@ class BaseViewer { } eventBus.dispatch("pageclose", { source: this, pageNumber }); }; - const dispatchPageOpen = (pageNumber, force = false) => { + const dispatchPageOpen = pageNumber => { const pageView = this._pages[pageNumber - 1]; - if (force || pageView?.renderingState === RenderingStates.FINISHED) { + if (pageView?.renderingState === RenderingStates.FINISHED) { pageOpenPendingSet.delete(pageNumber); eventBus.dispatch("pageopen", { source: this, pageNumber }); @@ -1534,31 +1532,56 @@ class BaseViewer { } }; - eventBus._on("pagechanging", ({ pageNumber, previous }) => { + scriptingEvents.onPageChanging = ({ pageNumber, previous }) => { if (pageNumber === previous) { return; // The active page didn't change. } dispatchPageClose(previous); dispatchPageOpen(pageNumber); - }); + }; + eventBus._on("pagechanging", scriptingEvents.onPageChanging); - eventBus._on("pagerendered", ({ pageNumber }) => { + scriptingEvents.onPageRendered = ({ pageNumber }) => { if (!pageOpenPendingSet.has(pageNumber)) { return; // No pending "pageopen" event for the newly rendered page. } if (pageNumber !== this._currentPageNumber) { return; // The newly rendered page is no longer the current one. } - dispatchPageOpen(pageNumber, /* force = */ true); - }); - - eventBus._on("pagesinit", () => { - dispatchPageOpen(this._currentPageNumber); - }); + dispatchPageOpen(pageNumber); + }; + eventBus._on("pagerendered", scriptingEvents.onPageRendered); - eventBus._on("pagesdestroy", () => { + scriptingEvents.onPagesDestroy = () => { dispatchPageClose(this._currentPageNumber); - }); + }; + eventBus._on("pagesdestroy", scriptingEvents.onPagesDestroy); + + // Ensure that a "pageopen" event is dispatched for the initial page. + dispatchPageOpen(this._currentPageNumber); + } + + /** + * @private + */ + _resetScriptingEvents() { + if (!this.enableScripting || !this._pageOpenPendingSet) { + return; + } + const eventBus = this.eventBus, + scriptingEvents = this._scriptingEvents; + + // Remove the event listeners. + eventBus._off("pagechanging", scriptingEvents.onPageChanging); + scriptingEvents.onPageChanging = null; + + eventBus._off("pagerendered", scriptingEvents.onPageRendered); + scriptingEvents.onPageRendered = null; + + eventBus._off("pagesdestroy", scriptingEvents.onPagesDestroy); + scriptingEvents.onPagesDestroy = null; + + this._pageOpenPendingSet = null; } } From 13742eb82d7f3aedf6b9b93d81fbdadcb5f232f7 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 12 Jan 2021 13:59:53 +0100 Subject: [PATCH 2/3] Inlude the JS `actions` for the page when dispatching the "pageopen"-event in the `BaseViewer` Note first of all how the `PDFDocumentProxy.getJSActions` method in the API caches the result, which makes repeated lookups cheap enough to not really be an issue. Secondly, with the previous patch, we're now only dispatching "pageopen"/"pageclose"-events when there's actually a sandbox that listens for them. All-in-all, with these changes we can thus simplify the default-viewer "pageopen"-event handler a fair bit. --- src/display/api.js | 9 +++------ web/app.js | 24 ++++++++---------------- web/base_viewer.js | 6 +++++- 3 files changed, 16 insertions(+), 23 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index 1a291ae8e2f20..055a9e9851a53 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -1140,12 +1140,9 @@ class PDFPageProxy { * {Object} with JS actions. */ getJSActions() { - if (!this._jsActionsPromise) { - this._jsActionsPromise = this._transport.getPageJSActions( - this._pageIndex - ); - } - return this._jsActionsPromise; + return (this._jsActionsPromise ||= this._transport.getPageJSActions( + this._pageIndex + )); } /** diff --git a/web/app.js b/web/app.js index e36e5f1a87f17..45e1567348378 100644 --- a/web/app.js +++ b/web/app.js @@ -1564,24 +1564,15 @@ const PDFViewerApplication = { internalEvents.set("updatefromsandbox", updateFromSandbox); const visitedPages = new Map(); - const pageOpen = ({ pageNumber }) => { + const pageOpen = ({ pageNumber, actionsPromise }) => { visitedPages.set( pageNumber, (async () => { // Avoid sending, and thus serializing, the `actions` data - // when the same page is open several times. + // when the same page is opened several times. let actions = null; if (!visitedPages.has(pageNumber)) { - // visitedPages doesn't contain pageNumber: first visit. - - const pageView = this.pdfViewer.getPageView( - /* index = */ pageNumber - 1 - ); - if (pageView?.pdfPage) { - actions = await pageView.pdfPage.getJSActions(); - } else { - actions = await pdfDocument.getPage(pageNumber).getJSActions(); - } + actions = await actionsPromise; if (pdfDocument !== this.pdfDocument) { return; // The document was closed while the actions resolved. @@ -1599,14 +1590,15 @@ const PDFViewerApplication = { }; const pageClose = async ({ pageNumber }) => { - const promise = visitedPages.get(pageNumber); - if (!promise) { + const actionsPromise = visitedPages.get(pageNumber); + if (!actionsPromise) { + // Ensure that the "pageclose" event was preceded by a "pageopen" event. return; } visitedPages.set(pageNumber, null); - // Wait for PageOpen has been sent. - await promise; + // Ensure that the "pageopen" event is handled first. + await actionsPromise; if (pdfDocument !== this.pdfDocument) { return; // The document was closed while the actions resolved. diff --git a/web/base_viewer.js b/web/base_viewer.js index c9935ec1bf84a..4060145c94db3 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -1526,7 +1526,11 @@ class BaseViewer { if (pageView?.renderingState === RenderingStates.FINISHED) { pageOpenPendingSet.delete(pageNumber); - eventBus.dispatch("pageopen", { source: this, pageNumber }); + eventBus.dispatch("pageopen", { + source: this, + pageNumber, + actionsPromise: pageView.pdfPage?.getJSActions(), + }); } else { pageOpenPendingSet.add(pageNumber); } From f1749f03e3f3c691653b6ac7108195bfea67e6a2 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 12 Jan 2021 14:29:15 +0100 Subject: [PATCH 3/3] Ensure that `PDFViewerApplication.close` has a *consistent* return value --- web/app.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/web/app.js b/web/app.js index 45e1567348378..0512310b301c2 100644 --- a/web/app.js +++ b/web/app.js @@ -867,7 +867,9 @@ const PDFViewerApplication = { if (typeof PDFBug !== "undefined") { PDFBug.cleanup(); } - return Promise.all(promises); + await Promise.all(promises); + + return undefined; }, /**