From 789e318cf745edea9ff11c42112905a86dc1e30e Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 28 Jun 2023 12:16:46 +0200 Subject: [PATCH 1/4] A couple of small tweaks of the `PDFCursorTools` class - Introduce a few private fields for internal state. - Inline a small method at its only call-site. --- web/pdf_cursor_tools.js | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/web/pdf_cursor_tools.js b/web/pdf_cursor_tools.js index 2d2eb8258..2bba8520a 100644 --- a/web/pdf_cursor_tools.js +++ b/web/pdf_cursor_tools.js @@ -27,6 +27,10 @@ import { GrabToPan } from "./grab_to_pan.js"; */ class PDFCursorTools { + #active = CursorTool.SELECT; + + #prevActive = null; + /** * @param {PDFCursorToolsOptions} options */ @@ -34,9 +38,6 @@ class PDFCursorTools { this.container = container; this.eventBus = eventBus; - this.active = CursorTool.SELECT; - this.previouslyActive = null; - this.handTool = new GrabToPan({ element: this.container, }); @@ -54,7 +55,7 @@ class PDFCursorTools { * @type {number} One of the values in {CursorTool}. */ get activeTool() { - return this.active; + return this.#active; } /** @@ -62,16 +63,16 @@ class PDFCursorTools { * must be one of the values in {CursorTool}. */ switchTool(tool) { - if (this.previouslyActive !== null) { + if (this.#prevActive !== null) { // Cursor tools cannot be used in PresentationMode/AnnotationEditor. return; } - if (tool === this.active) { + if (tool === this.#active) { return; // The requested tool is already active. } const disableActiveTool = () => { - switch (this.active) { + switch (this.#active) { case CursorTool.SELECT: break; case CursorTool.HAND: @@ -99,15 +100,11 @@ class PDFCursorTools { } // Update the active tool *after* it has been validated above, // in order to prevent setting it to an invalid state. - this.active = tool; + this.#active = tool; - this.#dispatchEvent(); - } - - #dispatchEvent() { this.eventBus.dispatch("cursortoolchanged", { source: this, - tool: this.active, + tool, }); } @@ -120,26 +117,26 @@ class PDFCursorTools { presentationModeState = PresentationModeState.NORMAL; const disableActive = () => { - const previouslyActive = this.active; + const prevActive = this.#active; this.switchTool(CursorTool.SELECT); - this.previouslyActive ??= previouslyActive; // Keep track of the first one. + this.#prevActive ??= prevActive; // Keep track of the first one. }; const enableActive = () => { - const previouslyActive = this.previouslyActive; + const prevActive = this.#prevActive; if ( - previouslyActive !== null && + prevActive !== null && annotationEditorMode === AnnotationEditorType.NONE && presentationModeState === PresentationModeState.NORMAL ) { - this.previouslyActive = null; - this.switchTool(previouslyActive); + this.#prevActive = null; + this.switchTool(prevActive); } }; this.eventBus._on("secondarytoolbarreset", evt => { - if (this.previouslyActive !== null) { + if (this.#prevActive !== null) { annotationEditorMode = AnnotationEditorType.NONE; presentationModeState = PresentationModeState.NORMAL; From 4f82dd39326968da7f313eeee592d3e4bb632cfe Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 28 Jun 2023 12:23:02 +0200 Subject: [PATCH 2/4] Create a `GrabToPan`-instance lazily in the `PDFCursorTools` class Unless the user enables the "HandTool" we don't actually need to create a `GrabToPan`-instance. --- web/pdf_cursor_tools.js | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/web/pdf_cursor_tools.js b/web/pdf_cursor_tools.js index 2bba8520a..416122b13 100644 --- a/web/pdf_cursor_tools.js +++ b/web/pdf_cursor_tools.js @@ -13,8 +13,8 @@ * limitations under the License. */ +import { AnnotationEditorType, shadow } from "pdfjs-lib"; import { CursorTool, PresentationModeState } from "./ui_utils.js"; -import { AnnotationEditorType } from "pdfjs-lib"; import { GrabToPan } from "./grab_to_pan.js"; /** @@ -38,10 +38,6 @@ class PDFCursorTools { this.container = container; this.eventBus = eventBus; - this.handTool = new GrabToPan({ - element: this.container, - }); - this.#addEventListeners(); // Defer the initial `switchTool` call, to give other viewer components @@ -76,7 +72,7 @@ class PDFCursorTools { case CursorTool.SELECT: break; case CursorTool.HAND: - this.handTool.deactivate(); + this._handTool.deactivate(); break; case CursorTool.ZOOM: /* falls through */ @@ -90,7 +86,7 @@ class PDFCursorTools { break; case CursorTool.HAND: disableActiveTool(); - this.handTool.activate(); + this._handTool.activate(); break; case CursorTool.ZOOM: /* falls through */ @@ -164,6 +160,19 @@ class PDFCursorTools { } }); } + + /** + * @private + */ + get _handTool() { + return shadow( + this, + "_handTool", + new GrabToPan({ + element: this.container, + }) + ); + } } export { PDFCursorTools }; From d32926792677bd1f11db2f49dfefdaf12bffba44 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 28 Jun 2023 12:34:03 +0200 Subject: [PATCH 3/4] Use `Element.scrollTo` unconditionally in the `GrabToPan` class According to the MDN compatibility data this is available in all browsers that we currently support; please see https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollTo#browser_compatibility --- web/grab_to_pan.js | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/web/grab_to_pan.js b/web/grab_to_pan.js index a985afc6b..b3f86d728 100644 --- a/web/grab_to_pan.js +++ b/web/grab_to_pan.js @@ -140,18 +140,12 @@ class GrabToPan { } const xDiff = event.clientX - this.clientXStart; const yDiff = event.clientY - this.clientYStart; - const scrollTop = this.scrollTopStart - yDiff; - const scrollLeft = this.scrollLeftStart - xDiff; - if (this.element.scrollTo) { - this.element.scrollTo({ - top: scrollTop, - left: scrollLeft, - behavior: "instant", - }); - } else { - this.element.scrollTop = scrollTop; - this.element.scrollLeft = scrollLeft; - } + this.element.scrollTo({ + top: this.scrollTopStart - yDiff, + left: this.scrollLeftStart - xDiff, + behavior: "instant", + }); + if (!this.overlay.parentNode) { document.body.append(this.overlay); } From f373fcb35689f0f6e4b69a3f6a3464e3f0934438 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 29 Jun 2023 13:16:10 +0200 Subject: [PATCH 4/4] Remove a couple of unused options from the `GrabToPan` constructor These options are completely unused in the PDF.js viewer, and given that the last update of the `GrabToPan`-code from upstream was in 2016 it shouldn't hurt to remove them. --- web/grab_to_pan.js | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/web/grab_to_pan.js b/web/grab_to_pan.js index b3f86d728..c564802b2 100644 --- a/web/grab_to_pan.js +++ b/web/grab_to_pan.js @@ -21,18 +21,10 @@ class GrabToPan { /** * Construct a GrabToPan instance for a given HTML element. * @param {Element} options.element - * @param {function} [options.ignoreTarget] - See `ignoreTarget(node)`. - * @param {function(boolean)} [options.onActiveChanged] - Called when - * grab-to-pan is (de)activated. The first argument is a boolean that - * shows whether grab-to-pan is activated. */ - constructor(options) { - this.element = options.element; - this.document = options.element.ownerDocument; - if (typeof options.ignoreTarget === "function") { - this.ignoreTarget = options.ignoreTarget; - } - this.onActiveChanged = options.onActiveChanged; + constructor({ element }) { + this.element = element; + this.document = element.ownerDocument; // Bind the contexts to ensure that `this` always points to // the GrabToPan instance. @@ -57,8 +49,6 @@ class GrabToPan { this.active = true; this.element.addEventListener("mousedown", this._onMouseDown, true); this.element.classList.add(CSS_CLASS_GRAB); - - this.onActiveChanged?.(true); } } @@ -71,8 +61,6 @@ class GrabToPan { this.element.removeEventListener("mousedown", this._onMouseDown, true); this._endPan(); this.element.classList.remove(CSS_CLASS_GRAB); - - this.onActiveChanged?.(false); } }