From 545ad313e355b2ac844aabe8d4325e5e056638fd Mon Sep 17 00:00:00 2001 From: Vansh Date: Sat, 23 May 2026 18:31:51 +0530 Subject: [PATCH] cleanup: drop unused disposer return from installSafariPinchZoomBlocker (#3992) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #3901 (cc @evanpelle). ## Description: In the review on #3901, evanpelle pointed out that the disposer returned by `installSafariPinchZoomBlocker` is never called at the call site in `Main.ts`, and asked whether there's any reason to return it. There isn't — the listeners live for the document's lifetime and the browser releases them on teardown — so this PR drops the disposer. ### Changes - `installSafariPinchZoomBlocker` now returns `void`. Removed the `return () => { ... }` block and the `@returns` JSDoc line. Added a sentence explaining why no disposer is needed. - Tests: dropped the disposer-removal test, switched the behavior tests to use fresh detached `
` elements (no document state leak across tests), and verified the default-target = `document` case with `vi.spyOn(document, 'addEventListener').mockImplementation(() => {})` so no real listener actually attaches to the shared jsdom document. Net diff: -23 lines (30 insertions, 53 deletions). ### What I tested - `npm test` — 1245 + 65 tests pass, including the 4 surviving tests for this helper - `npm run build-prod` — succeeds (tsc + vite) - `npx eslint` — clean - `npx prettier --check` on the touched files — clean ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: @vansszh --- .../utilities/DisableSafariPinchZoom.ts | 12 +-- tests/client/DisableSafariPinchZoom.test.ts | 79 +++++++------------ 2 files changed, 34 insertions(+), 57 deletions(-) diff --git a/src/client/utilities/DisableSafariPinchZoom.ts b/src/client/utilities/DisableSafariPinchZoom.ts index ca3c96139..87255a411 100644 --- a/src/client/utilities/DisableSafariPinchZoom.ts +++ b/src/client/utilities/DisableSafariPinchZoom.ts @@ -13,16 +13,18 @@ * (Chrome, Firefox, every Android browser) treat the listeners as a no-op, * so it is safe to install them unconditionally. * + * The listeners live for the document's lifetime; the browser releases them + * when the page is torn down, so no disposer is needed. + * * @param target - The EventTarget to attach the listeners to. Defaults to * `document`, which is the scope Safari uses to decide whether to zoom * the page. - * @returns A function that removes the installed listeners. * * @see https://github.com/openfrontio/OpenFrontIO/issues/2330 */ export function installSafariPinchZoomBlocker( target: EventTarget = document, -): () => void { +): void { const block = (e: Event) => { e.preventDefault(); }; @@ -31,10 +33,4 @@ export function installSafariPinchZoomBlocker( for (const type of events) { target.addEventListener(type, block); } - - return () => { - for (const type of events) { - target.removeEventListener(type, block); - } - }; } diff --git a/tests/client/DisableSafariPinchZoom.test.ts b/tests/client/DisableSafariPinchZoom.test.ts index 1f588703a..339f80b25 100644 --- a/tests/client/DisableSafariPinchZoom.test.ts +++ b/tests/client/DisableSafariPinchZoom.test.ts @@ -15,69 +15,50 @@ function dispatchCancelableGestureEvent( } describe("installSafariPinchZoomBlocker", () => { - it("prevents the default action of each Safari gesture event on document", () => { - const uninstall = installSafariPinchZoomBlocker(); - - try { - for (const type of GESTURE_EVENTS) { - const event = dispatchCancelableGestureEvent(document, type); - expect(event.defaultPrevented).toBe(true); - } - } finally { - uninstall(); - } - }); - - it("attaches listeners to the provided target", () => { + it("calls preventDefault on each Safari gesture event dispatched at the target", () => { const target = document.createElement("div"); - const uninstall = installSafariPinchZoomBlocker(target); - - try { - for (const type of GESTURE_EVENTS) { - const event = dispatchCancelableGestureEvent(target, type); - expect(event.defaultPrevented).toBe(true); - } - } finally { - uninstall(); - } - }); - - it("removes the listeners when the returned disposer is called", () => { - const target = document.createElement("div"); - const uninstall = installSafariPinchZoomBlocker(target); - uninstall(); + installSafariPinchZoomBlocker(target); for (const type of GESTURE_EVENTS) { const event = dispatchCancelableGestureEvent(target, type); - expect(event.defaultPrevented).toBe(false); + expect(event.defaultPrevented).toBe(true); } }); - it("does not affect events on unrelated targets", () => { - const scope = document.createElement("div"); - const other = document.createElement("div"); - const uninstall = installSafariPinchZoomBlocker(scope); + it("defaults to attaching the listeners to document", () => { + const addEventListenerSpy = vi + .spyOn(document, "addEventListener") + .mockImplementation(() => {}); try { - const event = dispatchCancelableGestureEvent(other, "gesturestart"); - expect(event.defaultPrevented).toBe(false); + installSafariPinchZoomBlocker(); + + for (const type of GESTURE_EVENTS) { + expect(addEventListenerSpy).toHaveBeenCalledWith( + type, + expect.any(Function), + ); + } } finally { - uninstall(); + addEventListenerSpy.mockRestore(); } }); + it("does not affect events dispatched at unrelated targets", () => { + const scope = document.createElement("div"); + const other = document.createElement("div"); + installSafariPinchZoomBlocker(scope); + + const event = dispatchCancelableGestureEvent(other, "gesturestart"); + expect(event.defaultPrevented).toBe(false); + }); + it("leaves unrelated event types alone", () => { - const uninstall = installSafariPinchZoomBlocker(); + const target = document.createElement("div"); + installSafariPinchZoomBlocker(target); - try { - const event = new Event("touchstart", { - bubbles: true, - cancelable: true, - }); - document.dispatchEvent(event); - expect(event.defaultPrevented).toBe(false); - } finally { - uninstall(); - } + const event = new Event("touchstart", { bubbles: true, cancelable: true }); + target.dispatchEvent(event); + expect(event.defaultPrevented).toBe(false); }); });