From 6dc39cb873160a3bfffece6e98e5cc3decb9dd8b Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 17 Dec 2020 12:55:58 +0100 Subject: [PATCH 1/6] Tweak the new `mouseState` parameter, and its usage, in the viewer components and the `AnnotationLayer` - Actually remove the `isDown` property when destroying the scripting-instance. - Mark all `mouseState` usage as "private" in the various classes. - Ensure that the `AnnotationLayer` actually treats the parameter as properly *optional*, the same way that the viewer components do. - For now remove the `mouseState` parameter from the `PDFPageView` class, and keep it only on the `BaseViewer`, since it's questionable if all of the scripting-functionality will work all that well without e.g. a full `BaseViewer`. - Append the `mouseState` to the JSDoc for the `AnnotationElement` class, and just move its definition into the base-`AnnotationElement` class. --- src/display/annotation_layer.js | 7 ++++--- web/annotation_layer_builder.js | 1 + web/app.js | 1 + web/base_viewer.js | 8 ++++---- web/pdf_page_view.js | 4 +--- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js index 8a6f90072..80ad7554d 100644 --- a/src/display/annotation_layer.js +++ b/src/display/annotation_layer.js @@ -47,6 +47,7 @@ import { ColorConverters } from "../shared/scripting_utils.js"; * @property {Object} svgFactory * @property {boolean} [enableScripting] * @property {boolean} [hasJSActions] + * @property {Object} [mouseState] */ class AnnotationElementFactory { @@ -155,6 +156,7 @@ class AnnotationElement { this.annotationStorage = parameters.annotationStorage; this.enableScripting = parameters.enableScripting; this.hasJSActions = parameters.hasJSActions; + this._mouseState = parameters.mouseState; if (isRenderable) { this.container = this._createContainer(ignoreBorder); @@ -590,7 +592,6 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement { parameters.renderInteractiveForms || (!parameters.data.hasAppearance && !!parameters.data.fieldValue); super(parameters, { isRenderable }); - this.mouseState = parameters.mouseState; } render() { @@ -734,7 +735,7 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement { const _blurListener = blurListener; blurListener = null; element.addEventListener("blur", event => { - if (this.mouseState.isDown) { + if (this._mouseState.isDown) { // Focus out using the mouse: data are committed elementData.userValue = event.target.value; window.dispatchEvent( @@ -1951,7 +1952,7 @@ class AnnotationLayer { parameters.annotationStorage || new AnnotationStorage(), enableScripting: parameters.enableScripting, hasJSActions: parameters.hasJSActions, - mouseState: parameters.mouseState, + mouseState: parameters.mouseState || { isDown: false }, }); if (element.isRenderable) { const rendered = element.render(); diff --git a/web/annotation_layer_builder.js b/web/annotation_layer_builder.js index e81b7a554..761ad5086 100644 --- a/web/annotation_layer_builder.js +++ b/web/annotation_layer_builder.js @@ -30,6 +30,7 @@ import { SimpleLinkService } from "./pdf_link_service.js"; * @property {IL10n} l10n - Localization service. * @property {boolean} [enableScripting] * @property {Promise} [hasJSActionsPromise] + * @property {Object} [mouseState] */ class AnnotationLayerBuilder { diff --git a/web/app.js b/web/app.js index df37f0c66..0b8de60c1 100644 --- a/web/app.js +++ b/web/app.js @@ -794,6 +794,7 @@ const PDFViewerApplication = { } events.clear(); + delete this._mouseState.isDown; this._scriptingInstance = null; }, diff --git a/web/base_viewer.js b/web/base_viewer.js index df50b3761..0103797ba 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -79,7 +79,8 @@ const DEFAULT_CACHE_SIZE = 10; * @property {IL10n} l10n - Localization service. * @property {boolean} [enableScripting] - Enable embedded script execution. * The default value is `false`. - * @property {Object} [mouseState] - The mouse button state. + * @property {Object} [mouseState] - The mouse button state. The default value + * is `null`. */ function PDFPageViewBuffer(size) { @@ -195,7 +196,7 @@ class BaseViewer { this.maxCanvasPixels = options.maxCanvasPixels; this.l10n = options.l10n || NullL10n; this.enableScripting = options.enableScripting || false; - this.mouseState = options.mouseState || null; + this._mouseState = options.mouseState || null; this.defaultRenderingQueue = !options.renderingQueue; if (this.defaultRenderingQueue) { @@ -540,7 +541,6 @@ class BaseViewer { maxCanvasPixels: this.maxCanvasPixels, l10n: this.l10n, enableScripting: this.enableScripting, - mouseState: this.mouseState, }); this._pages.push(pageView); } @@ -1301,7 +1301,7 @@ class BaseViewer { enableScripting, hasJSActionsPromise: hasJSActionsPromise || this.pdfDocument?.hasJSActions(), - mouseState, + mouseState: mouseState || this._mouseState, }); } diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index 0aa427178..da8d86a6d 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -63,7 +63,6 @@ import { viewerCompatibilityParams } from "./viewer_compatibility.js"; * @property {IL10n} l10n - Localization service. * @property {boolean} [enableScripting] - Enable embedded script execution. * The default value is `false`. - * @property {Object} [mouseState] - The mouse button state. */ const MAX_CANVAS_PIXELS = viewerCompatibilityParams.maxCanvasPixels || 16777216; @@ -110,7 +109,6 @@ class PDFPageView { this.enableWebGL = options.enableWebGL || false; this.l10n = options.l10n || NullL10n; this.enableScripting = options.enableScripting || false; - this.mouseState = options.mouseState || null; this.paintTask = null; this.paintedViewportMap = new WeakMap(); @@ -554,7 +552,7 @@ class PDFPageView { this.l10n, this.enableScripting, /* hasJSActionsPromise = */ null, - this.mouseState + /* mouseState = */ null ); } this._renderAnnotationLayer(); From e2b6d79dee9ffd2aba715d6b3cb8fa143d86153c Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 17 Dec 2020 13:08:13 +0100 Subject: [PATCH 2/6] Tweak the `LinkAnnotationElement._bindJSAction` and `WidgetAnnotationElement.{_setEventListener, _setEventListeners}` methods - Update the `LinkAnnotationElement._bindJSAction` call-site to actually agree with the JSDocs, by passing in the `data`. - Prevent the links created by `LinkAnnotationElement._bindJSAction` from being displayed with empty hashes; compare with e.g. `LinkAnnotationElement. _bindNamedAction`. - The overall indentation-level in `WidgetAnnotationElement._setEventListener` can be reduced slightly by using early returns, which improves the overall readability of this method a bit. (We're also able to avoid unnecessary `in` usage here.) - The code can also be made *slightly* more efficient overall, by moving the `this.data.actions` check into `WidgetAnnotationElement._setEventListeners` instead. This way we can avoid useless `this._setEventListener`-calls when there are no actions present. --- src/display/annotation_layer.js | 75 +++++++++++++++++---------------- 1 file changed, 39 insertions(+), 36 deletions(-) diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js index 80ad7554d..025043ae1 100644 --- a/src/display/annotation_layer.js +++ b/src/display/annotation_layer.js @@ -399,7 +399,7 @@ class LinkAnnotationElement extends AnnotationElement { this.enableScripting && this.hasJSActions ) { - this._bindJSAction(link); + this._bindJSAction(link, data); } else { this._bindLink(link, ""); } @@ -465,9 +465,8 @@ class LinkAnnotationElement extends AnnotationElement { * @param {Object} data * @memberof LinkAnnotationElement */ - _bindJSAction(link) { - link.href = this.linkService.getAnchorUrl("#"); - const { data } = this; + _bindJSAction(link, data) { + link.href = this.linkService.getAnchorUrl(""); const map = new Map([ ["Action", "onclick"], ["MouseUp", "onmouseup"], @@ -546,40 +545,44 @@ class WidgetAnnotationElement extends AnnotationElement { } _setEventListener(element, baseName, eventName, valueGetter) { - if (this.data.actions && eventName.replace(" ", "") in this.data.actions) { - if (baseName.includes("mouse")) { - // Mouse events - element.addEventListener(baseName, event => { - window.dispatchEvent( - new CustomEvent("dispatchEventInSandbox", { - detail: { - id: this.data.id, - name: eventName, - value: valueGetter(event), - shift: event.shiftKey, - modifier: this._getKeyModifier(event), - }, - }) - ); - }); - } else { - // Non mouse event - element.addEventListener(baseName, event => { - window.dispatchEvent( - new CustomEvent("dispatchEventInSandbox", { - detail: { - id: this.data.id, - name: eventName, - value: event.target.checked, - }, - }) - ); - }); - } + if (this.data.actions[eventName.replace(" ", "")] === undefined) { + return; + } + if (baseName.includes("mouse")) { + // Mouse events + element.addEventListener(baseName, event => { + window.dispatchEvent( + new CustomEvent("dispatchEventInSandbox", { + detail: { + id: this.data.id, + name: eventName, + value: valueGetter(event), + shift: event.shiftKey, + modifier: this._getKeyModifier(event), + }, + }) + ); + }); + } else { + // Non mouse event + element.addEventListener(baseName, event => { + window.dispatchEvent( + new CustomEvent("dispatchEventInSandbox", { + detail: { + id: this.data.id, + name: eventName, + value: event.target.checked, + }, + }) + ); + }); } } _setEventListeners(element, names, getter) { + if (!this.data.actions) { + return; + } for (const [baseName, eventName] of names) { this._setEventListener(element, baseName, eventName, getter); } @@ -997,8 +1000,8 @@ class RadioButtonWidgetAnnotationElement extends WidgetAnnotationElement { element.setAttribute("id", id); element.addEventListener("change", function (event) { - const target = event.target; - for (const radio of document.getElementsByName(event.target.name)) { + const { target } = event; + for (const radio of document.getElementsByName(target.name)) { if (radio !== target) { storage.setValue(radio.getAttribute("id"), { value: false }); } From eff4d8182db0c8e7dd452012baa5124d5ef58fb6 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 17 Dec 2020 14:10:56 +0100 Subject: [PATCH 3/6] Update the events, used with scripting, to use lower-case names and avoid using DOM events internally in the viewer For DOM events all event names are lower-case, and the newly added PDF.js scripting-events thus "stick out" quite a bit. Even more so, considering that our internal `eventBus`-events follow the same naming convention. Hence this patch, which changes the "updateFromSandbox"/"dispatchEventInSandbox" events to be lower-case instead. Furthermore, using DOM events for communication *within* the PDF.js code itself (i.e. between code in `web/app.js` and `src/display/annotation_layer.js/`) feels *really* out of place. That's exactly the reason that we have the `EventBus` abstraction, since it allowed us to remove prior use of DOM events, and this patch thus re-factors the code to make use of the `EventBus` instead for scripting-related events. Obviously for events targeting a *specific element* using DOM events is still fine, but the "updatefromsandbox"/"dispatcheventinsandbox" ones should be using the `EventBus` internally. *Drive-by change:* Use the `BaseViewer.currentScaleValue` setter unconditionally in `PDFViewerApplication._initializeJavaScript`, since it accepts either a string or a number. --- src/display/annotation_layer.js | 175 +++++++++++++++----------------- src/pdf.sandbox.external.js | 2 +- web/app.js | 85 +++++++++------- web/firefoxcom.js | 4 +- 4 files changed, 136 insertions(+), 130 deletions(-) diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js index 025043ae1..b51d14d8d 100644 --- a/src/display/annotation_layer.js +++ b/src/display/annotation_layer.js @@ -478,14 +478,13 @@ class LinkAnnotationElement extends AnnotationElement { continue; } link[jsName] = () => { - window.dispatchEvent( - new CustomEvent("dispatchEventInSandbox", { - detail: { - id: data.id, - name, - }, - }) - ); + this.linkService.eventBus?.dispatch("dispatcheventinsandbox", { + source: this, + detail: { + id: data.id, + name, + }, + }); return false; }; } @@ -551,30 +550,28 @@ class WidgetAnnotationElement extends AnnotationElement { if (baseName.includes("mouse")) { // Mouse events element.addEventListener(baseName, event => { - window.dispatchEvent( - new CustomEvent("dispatchEventInSandbox", { - detail: { - id: this.data.id, - name: eventName, - value: valueGetter(event), - shift: event.shiftKey, - modifier: this._getKeyModifier(event), - }, - }) - ); + this.linkService.eventBus?.dispatch("dispatcheventinsandbox", { + source: this, + detail: { + id: this.data.id, + name: eventName, + value: valueGetter(event), + shift: event.shiftKey, + modifier: this._getKeyModifier(event), + }, + }); }); } else { // Non mouse event element.addEventListener(baseName, event => { - window.dispatchEvent( - new CustomEvent("dispatchEventInSandbox", { - detail: { - id: this.data.id, - name: eventName, - value: event.target.checked, - }, - }) - ); + this.linkService.eventBus?.dispatch("dispatcheventinsandbox", { + source: this, + detail: { + id: this.data.id, + name: eventName, + value: event.target.checked, + }, + }); }); } } @@ -650,7 +647,7 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement { } }); - element.addEventListener("updateFromSandbox", function (event) { + element.addEventListener("updatefromsandbox", function (event) { const { detail } = event; const actions = { value() { @@ -721,19 +718,18 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement { } // Save the entered value elementData.userValue = event.target.value; - window.dispatchEvent( - new CustomEvent("dispatchEventInSandbox", { - detail: { - id, - name: "Keystroke", - value: event.target.value, - willCommit: true, - commitKey, - selStart: event.target.selectionStart, - selEnd: event.target.selectionEnd, - }, - }) - ); + this.linkService.eventBus?.dispatch("dispatcheventinsandbox", { + source: this, + detail: { + id, + name: "Keystroke", + value: event.target.value, + willCommit: true, + commitKey, + selStart: event.target.selectionStart, + selEnd: event.target.selectionEnd, + }, + }); }); const _blurListener = blurListener; blurListener = null; @@ -741,19 +737,18 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement { if (this._mouseState.isDown) { // Focus out using the mouse: data are committed elementData.userValue = event.target.value; - window.dispatchEvent( - new CustomEvent("dispatchEventInSandbox", { - detail: { - id, - name: "Keystroke", - value: event.target.value, - willCommit: true, - commitKey: 1, - selStart: event.target.selectionStart, - selEnd: event.target.selectionEnd, - }, - }) - ); + this.linkService.eventBus?.dispatch("dispatcheventinsandbox", { + source: this, + detail: { + id, + name: "Keystroke", + value: event.target.value, + willCommit: true, + commitKey: 1, + selStart: event.target.selectionStart, + selEnd: event.target.selectionEnd, + }, + }); } _blurListener(event); }); @@ -783,19 +778,18 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement { if (elementData.beforeInputSelectionRange) { [selStart, selEnd] = elementData.beforeInputSelectionRange; } - window.dispatchEvent( - new CustomEvent("dispatchEventInSandbox", { - detail: { - id, - name: "Keystroke", - value: elementData.beforeInputValue, - change: event.data, - willCommit: false, - selStart, - selEnd, - }, - }) - ); + this.linkService.eventBus?.dispatch("dispatcheventinsandbox", { + source: this, + detail: { + id, + name: "Keystroke", + value: elementData.beforeInputValue, + change: event.data, + willCommit: false, + selStart, + selEnd, + }, + }); }); } @@ -929,7 +923,7 @@ class CheckboxWidgetAnnotationElement extends WidgetAnnotationElement { }); if (this.enableScripting && this.hasJSActions) { - element.addEventListener("updateFromSandbox", event => { + element.addEventListener("updatefromsandbox", event => { const { detail } = event; const actions = { value() { @@ -1010,7 +1004,7 @@ class RadioButtonWidgetAnnotationElement extends WidgetAnnotationElement { }); if (this.enableScripting && this.hasJSActions) { - element.addEventListener("updateFromSandbox", event => { + element.addEventListener("updatefromsandbox", event => { const { detail } = event; const actions = { value() { @@ -1132,7 +1126,7 @@ class ChoiceWidgetAnnotationElement extends WidgetAnnotationElement { } if (this.enableScripting && this.hasJSActions) { - selectElement.addEventListener("updateFromSandbox", event => { + selectElement.addEventListener("updatefromsandbox", event => { const { detail } = event; const actions = { value() { @@ -1162,22 +1156,21 @@ class ChoiceWidgetAnnotationElement extends WidgetAnnotationElement { .forEach(name => actions[name]()); }); - selectElement.addEventListener("input", function (event) { + selectElement.addEventListener("input", event => { const value = getValue(event); storage.setValue(id, { value }); - window.dispatchEvent( - new CustomEvent("dispatchEventInSandbox", { - detail: { - id, - name: "Keystroke", - changeEx: value, - willCommit: true, - commitKey: 1, - keyDown: false, - }, - }) - ); + this.linkService.eventBus?.dispatch("dispatcheventinsandbox", { + source: this, + detail: { + id, + name: "Keystroke", + changeEx: value, + willCommit: true, + commitKey: 1, + keyDown: false, + }, + }); }); this._setEventListeners( @@ -1852,14 +1845,12 @@ class FileAttachmentAnnotationElement extends AnnotationElement { this.filename = getFilenameFromUrl(filename); this.content = content; - if (this.linkService.eventBus) { - this.linkService.eventBus.dispatch("fileattachmentannotation", { - source: this, - id: stringToPDFString(filename), - filename, - content, - }); - } + this.linkService.eventBus?.dispatch("fileattachmentannotation", { + source: this, + id: stringToPDFString(filename), + filename, + content, + }); } render() { diff --git a/src/pdf.sandbox.external.js b/src/pdf.sandbox.external.js index 89b2fecf0..38b9faaeb 100644 --- a/src/pdf.sandbox.external.js +++ b/src/pdf.sandbox.external.js @@ -148,7 +148,7 @@ class SandboxSupportBase { if (!data) { return; } - const event = new this.win.CustomEvent("updateFromSandbox", { + const event = new this.win.CustomEvent("updatefromsandbox", { detail: this.importValueFromSandbox(data), }); this.win.dispatchEvent(event); diff --git a/web/app.js b/web/app.js index 0b8de60c1..672d03edb 100644 --- a/web/app.js +++ b/web/app.js @@ -784,15 +784,20 @@ const PDFViewerApplication = { if (!this._scriptingInstance) { return; } - const { scripting, events } = this._scriptingInstance; + const { scripting, internalEvents, domEvents } = this._scriptingInstance; try { await scripting.destroySandbox(); } catch (ex) {} - for (const [name, listener] of events) { + for (const [name, listener] of internalEvents) { + this.eventBus._off(name, listener); + } + internalEvents.clear(); + + for (const [name, listener] of domEvents) { window.removeEventListener(name, listener); } - events.clear(); + domEvents.clear(); delete this._mouseState.isDown; this._scriptingInstance = null; @@ -1468,16 +1473,19 @@ const PDFViewerApplication = { pdfDocument.getJSActions(), ]); - if ((!objects && !docActions) || pdfDocument !== this.pdfDocument) { - // No FieldObjects were found in the document, no JS Actions at doc level - // or the document was closed while the data resolved. + if (!objects && !docActions) { + // No FieldObjects or JavaScript actions were found in the document. return; } - + if (pdfDocument !== this.pdfDocument) { + return; // The document was closed while the data resolved. + } const scripting = this.externalServices.createScripting(); // Store a reference to the current scripting-instance, to allow destruction // of the sandbox and removal of the event listeners at document closing. - this._scriptingInstance = { scripting, events: new Map() }; + const internalEvents = new Map(), + domEvents = new Map(); + this._scriptingInstance = { scripting, internalEvents, domEvents }; if (!this.documentInfo) { // It should be *extremely* rare for metadata to not have been resolved @@ -1494,8 +1502,7 @@ const PDFViewerApplication = { } } - const updateFromSandbox = event => { - const { detail } = event; + const updateFromSandbox = ({ detail }) => { const { id, command, value } = detail; if (!id) { switch (command) { @@ -1520,11 +1527,7 @@ const PDFViewerApplication = { console.log(value); break; case "zoom": - if (typeof value === "string") { - this.pdfViewer.currentScaleValue = value; - } else { - this.pdfViewer.currentScale = value; - } + this.pdfViewer.currentScaleValue = value; break; } return; @@ -1532,7 +1535,7 @@ const PDFViewerApplication = { const element = document.getElementById(id); if (element) { - element.dispatchEvent(new CustomEvent("updateFromSandbox", { detail })); + element.dispatchEvent(new CustomEvent("updatefromsandbox", { detail })); } else { if (value !== undefined && value !== null) { // The element hasn't been rendered yet, use the AnnotationStorage. @@ -1540,30 +1543,29 @@ const PDFViewerApplication = { } } }; - window.addEventListener("updateFromSandbox", updateFromSandbox); - // Ensure that the event listener can be removed at document closing. - this._scriptingInstance.events.set("updateFromSandbox", updateFromSandbox); + internalEvents.set("updatefromsandbox", updateFromSandbox); - const dispatchEventInSandbox = event => { - scripting.dispatchEventInSandbox(event.detail); + const dispatchEventInSandbox = ({ detail }) => { + scripting.dispatchEventInSandbox(detail); }; - window.addEventListener("dispatchEventInSandbox", dispatchEventInSandbox); - // Ensure that the event listener can be removed at document closing. - this._scriptingInstance.events.set( - "dispatchEventInSandbox", - dispatchEventInSandbox - ); + internalEvents.set("dispatcheventinsandbox", dispatchEventInSandbox); const mouseDown = event => { this._mouseState.isDown = true; }; + domEvents.set("mousedown", mouseDown); + const mouseUp = event => { this._mouseState.isDown = false; }; - window.addEventListener("mousedown", mouseDown); - this._scriptingInstance.events.set("mousedown", mouseDown); - window.addEventListener("mouseup", mouseUp); - this._scriptingInstance.events.set("mouseup", mouseUp); + domEvents.set("mouseup", mouseUp); + + for (const [name, listener] of internalEvents) { + this.eventBus._on(name, listener); + } + for (const [name, listener] of domEvents) { + window.addEventListener(name, listener); + } if (!this._contentLength) { // Always waiting for the entire PDF document to be loaded will, most @@ -1602,12 +1604,10 @@ const PDFViewerApplication = { }); if (this.externalServices.isInAutomation) { - this.eventBus.dispatch("sandboxcreated", { - source: this, - }); + this.eventBus.dispatch("sandboxcreated", { source: this }); } } catch (error) { - console.error(error); + console.error(`_initializeJavaScript: "${error?.message}".`); this._destroyScriptingInstance(); } @@ -2125,6 +2125,12 @@ const PDFViewerApplication = { _boundEvents.windowAfterPrint = () => { eventBus.dispatch("afterprint", { source: window }); }; + _boundEvents.windowUpdateFromSandbox = event => { + eventBus.dispatch("updatefromsandbox", { + source: window, + detail: event.detail, + }); + }; window.addEventListener("visibilitychange", webViewerVisibilityChange); window.addEventListener("wheel", webViewerWheel, { passive: false }); @@ -2138,6 +2144,10 @@ const PDFViewerApplication = { window.addEventListener("hashchange", _boundEvents.windowHashChange); window.addEventListener("beforeprint", _boundEvents.windowBeforePrint); window.addEventListener("afterprint", _boundEvents.windowAfterPrint); + window.addEventListener( + "updatefromsandbox", + _boundEvents.windowUpdateFromSandbox + ); }, unbindEvents() { @@ -2212,11 +2222,16 @@ const PDFViewerApplication = { window.removeEventListener("hashchange", _boundEvents.windowHashChange); window.removeEventListener("beforeprint", _boundEvents.windowBeforePrint); window.removeEventListener("afterprint", _boundEvents.windowAfterPrint); + window.removeEventListener( + "updatefromsandbox", + _boundEvents.windowUpdateFromSandbox + ); _boundEvents.windowResize = null; _boundEvents.windowHashChange = null; _boundEvents.windowBeforePrint = null; _boundEvents.windowAfterPrint = null; + _boundEvents.windowUpdateFromSandbox = null; }, accumulateWheelTicks(ticks) { diff --git a/web/firefoxcom.js b/web/firefoxcom.js index f126d40a6..9af65e863 100644 --- a/web/firefoxcom.js +++ b/web/firefoxcom.js @@ -255,12 +255,12 @@ class FirefoxComDataRangeTransport extends PDFDataRangeTransport { } class FirefoxScripting { - static createSandbox(data) { + static async createSandbox(data) { return new Promise(resolve => { FirefoxCom.request("createSandbox", data, resolve); }).then(success => { if (!success) { - throw new Error("Cannot start sandbox"); + throw new Error("Cannot create sandbox."); } }); } From c78f153bda9178e24b1c46f93fe03d86ce66551a Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 17 Dec 2020 14:32:05 +0100 Subject: [PATCH 4/6] Remove the `ENABLE_SCRIPTING` build-target, since it's not necessary There's no really compelling reason, as far as I can tell, to introduce the `ENABLE_SCRIPTING` build-target, instead of simply re-using the existing `TESTING` build-target for the new `gulp integrationtest` task. In general there should be no problem with just always enable scripting in TESTING-builds, and if I were to *guess* the reason that this didn't seem to work was most likely because the Preferences ended up over-writing the `AppOptions`. As it turns out the GENERIC-viewer has already has built-in support for disabling of Preferences, via the `AppOptions`, and this can be utilized in TESTING-builds as well to ensure that whatever `AppOptions` are set they're always respected. --- gulpfile.js | 55 +++++++++++++++------------------------------- src/pdf.sandbox.js | 2 -- web/app_options.js | 7 +++--- 3 files changed, 21 insertions(+), 43 deletions(-) diff --git a/gulpfile.js b/gulpfile.js index 142b4e72b..fcc189e9c 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -97,7 +97,6 @@ const DEFINES = Object.freeze({ PRODUCTION: true, SKIP_BABEL: true, TESTING: false, - ENABLE_SCRIPTING: false, // The main build targets: GENERIC: false, MOZCENTRAL: false, @@ -682,7 +681,6 @@ gulp.task("default_preferences-pre", function () { LIB: true, BUNDLE_VERSION: 0, // Dummy version BUNDLE_BUILD: 0, // Dummy build - ENABLE_SCRIPTING: process.env.ENABLE_SCRIPTING === "true", }), map: { "pdfjs-lib": "../pdf", @@ -1551,46 +1549,29 @@ gulp.task("testing-pre", function (done) { done(); }); -gulp.task("enable-scripting", function (done) { - process.env.ENABLE_SCRIPTING = "true"; - done(); -}); - gulp.task( "test", - gulp.series( - "enable-scripting", - "testing-pre", - "generic", - "components", - function () { - return streamqueue( - { objectMode: true }, - createTestSource("unit"), - createTestSource("browser"), - createTestSource("integration") - ); - } - ) + gulp.series("testing-pre", "generic", "components", function () { + return streamqueue( + { objectMode: true }, + createTestSource("unit"), + createTestSource("browser"), + createTestSource("integration") + ); + }) ); gulp.task( "bottest", - gulp.series( - "enable-scripting", - "testing-pre", - "generic", - "components", - function () { - return streamqueue( - { objectMode: true }, - createTestSource("unit", true), - createTestSource("font", true), - createTestSource("browser (no reftest)", true), - createTestSource("integration") - ); - } - ) + gulp.series("testing-pre", "generic", "components", function () { + return streamqueue( + { objectMode: true }, + createTestSource("unit", true), + createTestSource("font", true), + createTestSource("browser (no reftest)", true), + createTestSource("integration") + ); + }) ); gulp.task( @@ -1609,7 +1590,7 @@ gulp.task( gulp.task( "integrationtest", - gulp.series("enable-scripting", "testing-pre", "generic", function () { + gulp.series("testing-pre", "generic", function () { return createTestSource("integration"); }) ); diff --git a/src/pdf.sandbox.js b/src/pdf.sandbox.js index cda7e4380..df4ce1632 100644 --- a/src/pdf.sandbox.js +++ b/src/pdf.sandbox.js @@ -63,8 +63,6 @@ class Sandbox { } const sandboxData = JSON.stringify(data); const code = [ - // Next line is replaced by code from initialization.js - // when we create the bundle for the sandbox. PDFJSDev.eval("PDF_SCRIPTING_JS_SOURCE"), `pdfjsScripting.initSandbox({ data: ${sandboxData} })`, ]; diff --git a/web/app_options.js b/web/app_options.js index 19c123448..2b67f4e0a 100644 --- a/web/app_options.js +++ b/web/app_options.js @@ -67,7 +67,7 @@ const defaultOptions = { }, enableScripting: { /** @type {boolean} */ - value: typeof PDFJSDev !== "undefined" && PDFJSDev.test("ENABLE_SCRIPTING"), + value: typeof PDFJSDev !== "undefined" && PDFJSDev.test("TESTING"), kind: OptionKind.VIEWER + OptionKind.PREFERENCE, }, enableWebGL: { @@ -249,7 +249,7 @@ if ( ) { defaultOptions.disablePreferences = { /** @type {boolean} */ - value: false, + value: typeof PDFJSDev !== "undefined" && PDFJSDev.test("TESTING"), kind: OptionKind.VIEWER, }; defaultOptions.locale = { @@ -260,8 +260,7 @@ if ( defaultOptions.sandboxBundleSrc = { /** @type {string} */ value: - typeof PDFJSDev === "undefined" || - PDFJSDev.test("!PRODUCTION && !ENABLE_SCRIPTING") + typeof PDFJSDev === "undefined" || !PDFJSDev.test("PRODUCTION") ? "../build/dev-sandbox/pdf.sandbox.js" : "../build/pdf.sandbox.js", kind: OptionKind.VIEWER, From 54f45dc935ba34f63f91291f35a5eeb3ee0e80ff Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 18 Dec 2020 22:16:24 +0100 Subject: [PATCH 5/6] Don't dispatch a "doc/Open" event in the sandbox when creating it failed There's really no point, as far as I can tell, to attempt to dispatch an event in a non-existent sandbox. Generally speaking, even trying to do this *could* possibly even lead to errors in some cases. Furthermore, utilize optional chaining to simplify some `dispatchEventInSandbox` calls throughout the viewer. Finally, replace superfluous `return` statements with `break` in the switch-statement in the `updateFromSandbox` event-handler. --- web/app.js | 49 +++++++++++++++++++++---------------------------- 1 file changed, 21 insertions(+), 28 deletions(-) diff --git a/web/app.js b/web/app.js index 672d03edb..4623b3632 100644 --- a/web/app.js +++ b/web/app.js @@ -1036,13 +1036,10 @@ const PDFViewerApplication = { this.download({ sourceEventType }); return; } - - if (this._scriptingInstance) { - this._scriptingInstance.scripting.dispatchEventInSandbox({ - id: "doc", - name: "WillSave", - }); - } + this._scriptingInstance?.scripting.dispatchEventInSandbox({ + id: "doc", + name: "WillSave", + }); this._saveInProgress = true; this.pdfDocument @@ -1051,12 +1048,10 @@ const PDFViewerApplication = { const blob = new Blob([data], { type: "application/pdf" }); downloadManager.download(blob, url, filename, sourceEventType); - if (this._scriptingInstance) { - this._scriptingInstance.scripting.dispatchEventInSandbox({ - id: "doc", - name: "DidSave", - }); - } + this._scriptingInstance?.scripting.dispatchEventInSandbox({ + id: "doc", + name: "DidSave", + }); }) .catch(() => { this.download({ sourceEventType }); @@ -1514,15 +1509,15 @@ const PDFViewerApplication = { break; case "layout": this.pdfViewer.spreadMode = apiPageLayoutToSpreadMode(value); - return; + break; case "page-num": this.pdfViewer.currentPageNumber = value + 1; - return; + break; case "print": this.pdfViewer.pagesPromise.then(() => { this.triggerPrinting(); }); - return; + break; case "println": console.log(value); break; @@ -1608,7 +1603,9 @@ const PDFViewerApplication = { } } catch (error) { console.error(`_initializeJavaScript: "${error?.message}".`); + this._destroyScriptingInstance(); + return; } scripting.dispatchEventInSandbox({ @@ -2033,21 +2030,17 @@ const PDFViewerApplication = { if (!this.supportsPrinting) { return; } - if (this._scriptingInstance) { - this._scriptingInstance.scripting.dispatchEventInSandbox({ - id: "doc", - name: "WillPrint", - }); - } + this._scriptingInstance?.scripting.dispatchEventInSandbox({ + id: "doc", + name: "WillPrint", + }); window.print(); - if (this._scriptingInstance) { - this._scriptingInstance.scripting.dispatchEventInSandbox({ - id: "doc", - name: "DidPrint", - }); - } + this._scriptingInstance?.scripting.dispatchEventInSandbox({ + id: "doc", + name: "DidPrint", + }); }, bindEvents() { From 6f40f4e7c2c46fc51b320d014294e9eebcbb18f8 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 18 Dec 2020 23:12:52 +0100 Subject: [PATCH 6/6] Remove the arbitrary timeout in the "must check that first text field has focus" integration-test (PR 12702 follow-up) It seems that the timeout is way too short in practice, since this new integration-test failed *intermittently* already in PR 12702 (which is where the test was added). The ideal solution here would be to simply await an event, dispatched by the viewer, however that unfortunately doesn't appear to be supported by Puppeteer. Instead, the solution implemented here is to add a new method in `PDFViewerApplication` which Puppeteer can query to check if the scripting/sandbox has been fully initialized. --- test/integration/scripting_spec.js | 5 ++++- web/app.js | 24 +++++++++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/test/integration/scripting_spec.js b/test/integration/scripting_spec.js index bde715249..43fc949d1 100644 --- a/test/integration/scripting_spec.js +++ b/test/integration/scripting_spec.js @@ -30,9 +30,12 @@ describe("Interaction", () => { it("must check that first text field has focus", async () => { await Promise.all( pages.map(async ([browserName, page]) => { + await page.waitForFunction( + "window.PDFViewerApplication.scriptingReady === true" + ); + // The document has an open action in order to give // the focus to 401R. - await page.waitForTimeout(1000); const id = await page.evaluate( () => window.document.activeElement.id ); diff --git a/web/app.js b/web/app.js index 4623b3632..67f558778 100644 --- a/web/app.js +++ b/web/app.js @@ -1480,7 +1480,12 @@ const PDFViewerApplication = { // of the sandbox and removal of the event listeners at document closing. const internalEvents = new Map(), domEvents = new Map(); - this._scriptingInstance = { scripting, internalEvents, domEvents }; + this._scriptingInstance = { + scripting, + ready: false, + internalEvents, + domEvents, + }; if (!this.documentInfo) { // It should be *extremely* rare for metadata to not have been resolved @@ -1612,6 +1617,15 @@ const PDFViewerApplication = { id: "doc", name: "Open", }); + + // Used together with the integration-tests, see the `scriptingReady` + // getter, to enable awaiting full initialization of the scripting/sandbox. + // (Defer this slightly, to make absolutely sure that everything is done.) + Promise.resolve().then(() => { + if (this._scriptingInstance) { + this._scriptingInstance.ready = true; + } + }); }, /** @@ -2242,6 +2256,14 @@ const PDFViewerApplication = { this._wheelUnusedTicks -= wholeTicks; return wholeTicks; }, + + /** + * Used together with the integration-tests, to enable awaiting full + * initialization of the scripting/sandbox. + */ + get scriptingReady() { + return this._scriptingInstance?.ready || false; + }, }; let validateFileURL;