From 0788c4d9188d1df79d817e33f75a4bbc60e62ea6 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 18 Apr 2024 11:59:49 +0200 Subject: [PATCH 1/3] Remove event listeners with `signal` in web/pdf_presentation_mode.js By using the `signal` option when invoking `addEventListener`, see [MDN](https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#signal), we're able to remove an arbitrary number of event listeners with (effectively) a single line of code. Besides getting rid of a bunch of `removeEventListener`-calls, which means shorter code, we no longer need to manually keep track of event-handling functions. --- web/pdf_presentation_mode.js | 93 +++++++++++++++++++----------------- 1 file changed, 50 insertions(+), 43 deletions(-) diff --git a/web/pdf_presentation_mode.js b/web/pdf_presentation_mode.js index cc2be0c2f..004af1af1 100644 --- a/web/pdf_presentation_mode.js +++ b/web/pdf_presentation_mode.js @@ -49,6 +49,10 @@ class PDFPresentationMode { #args = null; + #fullscreenChangeAbortController = null; + + #windowAbortController = null; + /** * @param {PDFPresentationModeOptions} options */ @@ -346,59 +350,62 @@ class PDFPresentationMode { } #addWindowListeners() { - this.showControlsBind = this.#showControls.bind(this); - this.mouseDownBind = this.#mouseDown.bind(this); - this.mouseWheelBind = this.#mouseWheel.bind(this); - this.resetMouseScrollStateBind = this.#resetMouseScrollState.bind(this); - this.contextMenuBind = this.#contextMenu.bind(this); - this.touchSwipeBind = this.#touchSwipe.bind(this); + if (this.#windowAbortController) { + return; + } + this.#windowAbortController = new AbortController(); + const { signal } = this.#windowAbortController; - window.addEventListener("mousemove", this.showControlsBind); - window.addEventListener("mousedown", this.mouseDownBind); - window.addEventListener("wheel", this.mouseWheelBind, { passive: false }); - window.addEventListener("keydown", this.resetMouseScrollStateBind); - window.addEventListener("contextmenu", this.contextMenuBind); - window.addEventListener("touchstart", this.touchSwipeBind); - window.addEventListener("touchmove", this.touchSwipeBind); - window.addEventListener("touchend", this.touchSwipeBind); + const touchSwipeBind = this.#touchSwipe.bind(this); + + window.addEventListener("mousemove", this.#showControls.bind(this), { + signal, + }); + window.addEventListener("mousedown", this.#mouseDown.bind(this), { + signal, + }); + window.addEventListener("wheel", this.#mouseWheel.bind(this), { + passive: false, + signal, + }); + window.addEventListener("keydown", this.#resetMouseScrollState.bind(this), { + signal, + }); + window.addEventListener("contextmenu", this.#contextMenu.bind(this), { + signal, + }); + window.addEventListener("touchstart", touchSwipeBind, { signal }); + window.addEventListener("touchmove", touchSwipeBind, { signal }); + window.addEventListener("touchend", touchSwipeBind, { signal }); } #removeWindowListeners() { - window.removeEventListener("mousemove", this.showControlsBind); - window.removeEventListener("mousedown", this.mouseDownBind); - window.removeEventListener("wheel", this.mouseWheelBind, { - passive: false, - }); - window.removeEventListener("keydown", this.resetMouseScrollStateBind); - window.removeEventListener("contextmenu", this.contextMenuBind); - window.removeEventListener("touchstart", this.touchSwipeBind); - window.removeEventListener("touchmove", this.touchSwipeBind); - window.removeEventListener("touchend", this.touchSwipeBind); - - delete this.showControlsBind; - delete this.mouseDownBind; - delete this.mouseWheelBind; - delete this.resetMouseScrollStateBind; - delete this.contextMenuBind; - delete this.touchSwipeBind; - } - - #fullscreenChange() { - if (/* isFullscreen = */ document.fullscreenElement) { - this.#enter(); - } else { - this.#exit(); - } + this.#windowAbortController?.abort(); + this.#windowAbortController = null; } #addFullscreenChangeListeners() { - this.fullscreenChangeBind = this.#fullscreenChange.bind(this); - window.addEventListener("fullscreenchange", this.fullscreenChangeBind); + if (this.#fullscreenChangeAbortController) { + return; + } + this.#fullscreenChangeAbortController = new AbortController(); + + window.addEventListener( + "fullscreenchange", + () => { + if (/* isFullscreen = */ document.fullscreenElement) { + this.#enter(); + } else { + this.#exit(); + } + }, + { signal: this.#fullscreenChangeAbortController.signal } + ); } #removeFullscreenChangeListeners() { - window.removeEventListener("fullscreenchange", this.fullscreenChangeBind); - delete this.fullscreenChangeBind; + this.#fullscreenChangeAbortController?.abort(); + this.#fullscreenChangeAbortController = null; } } From 46a29ff41b66390cd00e388d7e74eecf82c313cf Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 18 Apr 2024 12:25:55 +0200 Subject: [PATCH 2/3] Remove event listeners with `signal` in web/app.js By using the `signal` option when invoking `addEventListener`, see [MDN](https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#signal), we're able to remove an arbitrary number of event listeners with (effectively) a single line of code. Besides getting rid of a bunch of `removeEventListener`-calls, which means shorter code, we no longer need to manually keep track of event-handling functions. --- web/app.js | 166 +++++++++++++++++++++++------------------------------ 1 file changed, 72 insertions(+), 94 deletions(-) diff --git a/web/app.js b/web/app.js index 048707c11..cda101c7c 100644 --- a/web/app.js +++ b/web/app.js @@ -158,6 +158,7 @@ const PDFViewerApplication = { baseUrl: "", _downloadUrl: "", _boundEvents: Object.create(null), + _windowAbortController: null, documentInfo: null, metadata: null, _contentDispositionFilename: null, @@ -1906,10 +1907,15 @@ const PDFViewerApplication = { }, bindWindowEvents() { + if (this._windowAbortController) { + return; + } + this._windowAbortController = new AbortController(); + const { eventBus, - _boundEvents, appConfig: { mainContainer }, + _windowAbortController: { signal }, } = this; function addWindowResolutionChange(evt = null) { @@ -1921,58 +1927,73 @@ const PDFViewerApplication = { ); mediaQueryList.addEventListener("change", addWindowResolutionChange, { once: true, + signal, }); - - _boundEvents.removeWindowResolutionChange ||= function () { - mediaQueryList.removeEventListener("change", addWindowResolutionChange); - _boundEvents.removeWindowResolutionChange = null; - }; } addWindowResolutionChange(); - _boundEvents.windowResize = () => { - eventBus.dispatch("resize", { source: window }); - }; - _boundEvents.windowHashChange = () => { - eventBus.dispatch("hashchange", { - source: window, - hash: document.location.hash.substring(1), - }); - }; - _boundEvents.windowBeforePrint = () => { - eventBus.dispatch("beforeprint", { source: window }); - }; - _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 }); + window.addEventListener("visibilitychange", webViewerVisibilityChange, { + signal, + }); + window.addEventListener("wheel", webViewerWheel, { + passive: false, + signal, + }); window.addEventListener("touchstart", webViewerTouchStart, { passive: false, + signal, }); window.addEventListener("touchmove", webViewerTouchMove, { passive: false, + signal, }); window.addEventListener("touchend", webViewerTouchEnd, { passive: false, + signal, }); - window.addEventListener("click", webViewerClick); - window.addEventListener("keydown", webViewerKeyDown); - window.addEventListener("keyup", webViewerKeyUp); - window.addEventListener("resize", _boundEvents.windowResize); - window.addEventListener("hashchange", _boundEvents.windowHashChange); - window.addEventListener("beforeprint", _boundEvents.windowBeforePrint); - window.addEventListener("afterprint", _boundEvents.windowAfterPrint); + window.addEventListener("click", webViewerClick, { signal }); + window.addEventListener("keydown", webViewerKeyDown, { signal }); + window.addEventListener("keyup", webViewerKeyUp, { signal }); + window.addEventListener( + "resize", + () => { + eventBus.dispatch("resize", { source: window }); + }, + { signal } + ); + window.addEventListener( + "hashchange", + () => { + eventBus.dispatch("hashchange", { + source: window, + hash: document.location.hash.substring(1), + }); + }, + { signal } + ); + window.addEventListener( + "beforeprint", + () => { + eventBus.dispatch("beforeprint", { source: window }); + }, + { signal } + ); + window.addEventListener( + "afterprint", + () => { + eventBus.dispatch("afterprint", { source: window }); + }, + { signal } + ); window.addEventListener( "updatefromsandbox", - _boundEvents.windowUpdateFromSandbox + event => { + eventBus.dispatch("updatefromsandbox", { + source: window, + detail: event.detail, + }); + }, + { signal } ); if ( @@ -1987,17 +2008,18 @@ const PDFViewerApplication = { // TODO: remove them once the bug is fixed. ({ scrollTop: this._lastScrollTop, scrollLeft: this._lastScrollLeft } = mainContainer); - const scrollend = (_boundEvents.mainContainerScrollend = () => { + const scrollend = () => { ({ scrollTop: this._lastScrollTop, scrollLeft: this._lastScrollLeft } = mainContainer); this._isScrolling = false; mainContainer.addEventListener("scroll", scroll, { passive: true, + signal, }); - mainContainer.removeEventListener("scrollend", scrollend); - mainContainer.removeEventListener("blur", scrollend); - }); - const scroll = (_boundEvents.mainContainerScroll = () => { + mainContainer.removeEventListener("scrollend", scrollend, { signal }); + mainContainer.removeEventListener("blur", scrollend, { signal }); + }; + const scroll = () => { if ( this._isCtrlKeyDown || (this._lastScrollTop === mainContainer.scrollTop && @@ -2007,13 +2029,15 @@ const PDFViewerApplication = { } mainContainer.removeEventListener("scroll", scroll, { passive: true, + signal, }); this._isScrolling = true; - mainContainer.addEventListener("scrollend", scrollend); - mainContainer.addEventListener("blur", scrollend); - }); + mainContainer.addEventListener("scrollend", scrollend, { signal }); + mainContainer.addEventListener("blur", scrollend, { signal }); + }; mainContainer.addEventListener("scroll", scroll, { passive: true, + signal, }); }, @@ -2077,54 +2101,8 @@ const PDFViewerApplication = { }, unbindWindowEvents() { - const { - _boundEvents, - appConfig: { mainContainer }, - } = this; - - window.removeEventListener("visibilitychange", webViewerVisibilityChange); - window.removeEventListener("wheel", webViewerWheel, { passive: false }); - window.removeEventListener("touchstart", webViewerTouchStart, { - passive: false, - }); - window.removeEventListener("touchmove", webViewerTouchMove, { - passive: false, - }); - window.removeEventListener("touchend", webViewerTouchEnd, { - passive: false, - }); - window.removeEventListener("click", webViewerClick); - window.removeEventListener("keydown", webViewerKeyDown); - window.removeEventListener("keyup", webViewerKeyUp); - window.removeEventListener("resize", _boundEvents.windowResize); - window.removeEventListener("hashchange", _boundEvents.windowHashChange); - window.removeEventListener("beforeprint", _boundEvents.windowBeforePrint); - window.removeEventListener("afterprint", _boundEvents.windowAfterPrint); - window.removeEventListener( - "updatefromsandbox", - _boundEvents.windowUpdateFromSandbox - ); - mainContainer.removeEventListener( - "scroll", - _boundEvents.mainContainerScroll - ); - mainContainer.removeEventListener( - "scrollend", - _boundEvents.mainContainerScrollend - ); - mainContainer.removeEventListener( - "blur", - _boundEvents.mainContainerScrollend - ); - - _boundEvents.removeWindowResolutionChange?.(); - _boundEvents.windowResize = null; - _boundEvents.windowHashChange = null; - _boundEvents.windowBeforePrint = null; - _boundEvents.windowAfterPrint = null; - _boundEvents.windowUpdateFromSandbox = null; - _boundEvents.mainContainerScroll = null; - _boundEvents.mainContainerScrollend = null; + this._windowAbortController?.abort(); + this._windowAbortController = null; }, _accumulateTicks(ticks, prop) { From ff2e0c8afd5e9b3b43267409b5bbce63bcddccea Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 18 Apr 2024 12:39:57 +0200 Subject: [PATCH 3/3] Skip the scroll/scrollend workaround in the Firefox PDF Viewer (PR 17724 follow-up) Given that [bug 1881974](https://bugzilla.mozilla.org/show_bug.cgi?id=1881974) has been fixed in Firefox 126, the workaround should no longer be necessary in the *built-in* Firefox PDF Viewer. --- web/app.js | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/web/app.js b/web/app.js index cda101c7c..f288c2adf 100644 --- a/web/app.js +++ b/web/app.js @@ -177,8 +177,6 @@ const PDFViewerApplication = { _nimbusDataPromise: null, _caretBrowsing: null, _isScrolling: false, - _lastScrollTop: 0, - _lastScrollLeft: 0, // Called once when the document is loaded. async initialize(appConfig) { @@ -2002,15 +2000,20 @@ const PDFViewerApplication = { ) { return; } - - // Using the values lastScrollTop and lastScrollLeft is a workaround to - // https://bugzilla.mozilla.org/show_bug.cgi?id=1881974. - // TODO: remove them once the bug is fixed. - ({ scrollTop: this._lastScrollTop, scrollLeft: this._lastScrollLeft } = - mainContainer); - const scrollend = () => { + if (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")) { + // Using the values lastScrollTop and lastScrollLeft is a workaround to + // https://bugzilla.mozilla.org/show_bug.cgi?id=1881974. + // TODO: remove them once the bug is fixed. ({ scrollTop: this._lastScrollTop, scrollLeft: this._lastScrollLeft } = mainContainer); + } + + const scrollend = () => { + if (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")) { + ({ scrollTop: this._lastScrollTop, scrollLeft: this._lastScrollLeft } = + mainContainer); + } + this._isScrolling = false; mainContainer.addEventListener("scroll", scroll, { passive: true, @@ -2020,13 +2023,17 @@ const PDFViewerApplication = { mainContainer.removeEventListener("blur", scrollend, { signal }); }; const scroll = () => { + if (this._isCtrlKeyDown) { + return; + } if ( - this._isCtrlKeyDown || - (this._lastScrollTop === mainContainer.scrollTop && - this._lastScrollLeft === mainContainer.scrollLeft) + (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")) && + this._lastScrollTop === mainContainer.scrollTop && + this._lastScrollLeft === mainContainer.scrollLeft ) { return; } + mainContainer.removeEventListener("scroll", scroll, { passive: true, signal,