diff --git a/src/js/components/lightbox-image-group.js b/src/js/components/lightbox-image-group.js index 59cd7feb..35294c0b 100644 --- a/src/js/components/lightbox-image-group.js +++ b/src/js/components/lightbox-image-group.js @@ -4,6 +4,7 @@ import { ImageLoader } from "/js/utils.js"; import { chevronLeftIconTemplate } from "/js/templates/icons/chevronLeft.template.js"; import { chevronRightIconTemplate } from "/js/templates/icons/chevronRight.template.js"; import { closeIconTemplate } from "/js/templates/icons/closeIcon.template.js"; +import { pushOverlayHistoryEntry } from "/js/router.js"; class LightboxDialog extends Component { connectedCallback() { @@ -120,12 +121,23 @@ class LightboxDialog extends Component { this.isOpen = true; this.handleKeyDown = this.handleKeyDown.bind(this); document.addEventListener("keydown", this.handleKeyDown); + this.handlePopState = this.handlePopState.bind(this); + window.addEventListener("popstate", this.handlePopState); + pushOverlayHistoryEntry(); this.render(); } - close() { + handlePopState() { + this.close({ viaPopState: true }); + } + + close({ viaPopState = false } = {}) { document.body.style.overflow = ""; document.removeEventListener("keydown", this.handleKeyDown); + window.removeEventListener("popstate", this.handlePopState); + if (!viaPopState) { + window.history.back(); + } this.isOpen = false; this._imageLoader.abort(); this.render(); diff --git a/src/js/router.js b/src/js/router.js index 08250ced..ade86422 100644 --- a/src/js/router.js +++ b/src/js/router.js @@ -4,6 +4,22 @@ import { BoundedMap } from "/js/utils.js"; const MAX_PAGES = 5; +// Lets an overlay (e.g. the image lightbox) claim the next back navigation +// to close itself instead of the router navigating the underlying page. +// Call once when the overlay opens; the pushed entry is consumed either by +// a real back button press or by the overlay's own history.back() when +// it's dismissed some other way (see the popstate listener below). +let activeOverlayHistoryEntries = 0; + +export function pushOverlayHistoryEntry() { + activeOverlayHistoryEntries++; + window.history.pushState( + { ...window.history.state }, + "", + window.location.href, + ); +} + // Browsers fire auxclick rather than click for the middle button, so // we need to manually dispatch a click event for it function bindMiddleClickRedispatch() { @@ -118,6 +134,10 @@ export class Router extends EventEmitter { }); // on back button, go back to the previous page window.addEventListener("popstate", async (e) => { + if (activeOverlayHistoryEntries > 0) { + activeOverlayHistoryEntries--; + return; + } this.emit("navigate"); await this.load(window.location.pathname + window.location.search, { isBack: true, diff --git a/tests/unit/specs/components/lightbox-image-group.test.js b/tests/unit/specs/components/lightbox-image-group.test.js index 23f484ea..70b6c387 100644 --- a/tests/unit/specs/components/lightbox-image-group.test.js +++ b/tests/unit/specs/components/lightbox-image-group.test.js @@ -334,6 +334,83 @@ describe("lightbox-image-group", () => { }); }); + describe("LightboxDialog - back button / history integration", () => { + it("should push a history entry when opened", () => { + const originalLength = window.history.length; + const element = document.createElement("lightbox-dialog"); + element.images = [createTestImage("test.jpg", "Test")]; + document.body.appendChild(element); + element.open(); + + assert.deepEqual(window.history.length, originalLength + 1); + + element.close(); + }); + + it("should close when a popstate event fires while open (e.g. Android back button)", () => { + const element = document.createElement("lightbox-dialog"); + element.images = [createTestImage("test.jpg", "Test")]; + document.body.appendChild(element); + element.open(); + + window.dispatchEvent(new Event("popstate")); + + assert.deepEqual(element.isOpen, false); + }); + + it("should not call history.back() again when closed via popstate", () => { + const element = document.createElement("lightbox-dialog"); + element.images = [createTestImage("test.jpg", "Test")]; + document.body.appendChild(element); + element.open(); + + const originalBack = window.history.back; + let backCalls = 0; + window.history.back = () => { + backCalls++; + }; + try { + window.dispatchEvent(new Event("popstate")); + } finally { + window.history.back = originalBack; + } + + assert.deepEqual(backCalls, 0); + }); + + it("should call history.back() to release its pushed entry when closed some other way", () => { + const element = document.createElement("lightbox-dialog"); + element.images = [createTestImage("test.jpg", "Test")]; + document.body.appendChild(element); + element.open(); + + const originalBack = window.history.back; + let backCalls = 0; + window.history.back = () => { + backCalls++; + }; + try { + element.close(); + } finally { + window.history.back = originalBack; + } + + assert.deepEqual(backCalls, 1); + }); + + it("should stop listening for popstate after closing", () => { + const element = document.createElement("lightbox-dialog"); + element.images = [createTestImage("test.jpg", "Test")]; + document.body.appendChild(element); + element.open(); + element.close(); + + // A stray popstate after close should not throw or reopen the dialog. + window.dispatchEvent(new Event("popstate")); + assert.deepEqual(element.isOpen, false); + }); + }); + describe("LightboxDialog - keyboard navigation", () => { it("should close on Escape key", () => { const element = document.createElement("lightbox-dialog"); diff --git a/tests/unit/specs/router.test.js b/tests/unit/specs/router.test.js index cb417aa6..794f3786 100644 --- a/tests/unit/specs/router.test.js +++ b/tests/unit/specs/router.test.js @@ -1,6 +1,6 @@ import { describe, it, beforeEach, afterEach, mock } from "node:test"; import assert from "node:assert/strict"; -import { Router, Layout } from "/js/router.js"; +import { Router, Layout, pushOverlayHistoryEntry } from "/js/router.js"; class TestLayout extends Layout { constructor() { @@ -322,6 +322,48 @@ describe("popstate", () => { window.history.replaceState(originalState, "", originalPath); } }); + + it("should not emit navigate when popstate fires while an overlay history entry is pending", async () => { + const originalState = window.history.state; + const originalPath = + window.location.pathname + window.location.search + window.location.hash; + const { router, popstateHandler } = createRouterWithPopstateHandler(); + mountRouter(router); + + const listener = mock.fn(); + router.on("navigate", listener); + + try { + // Mirrors what the image lightbox does when it opens. + pushOverlayHistoryEntry(); + await popstateHandler(new Event("popstate")); + + assert.deepEqual(listener.mock.callCount(), 0); + } finally { + window.history.replaceState(originalState, "", originalPath); + } + }); + + it("should resume normal navigation on the popstate after an overlay entry is consumed", async () => { + const originalState = window.history.state; + const originalPath = + window.location.pathname + window.location.search + window.location.hash; + const { router, popstateHandler } = createRouterWithPopstateHandler(); + mountRouter(router); + + const listener = mock.fn(); + router.on("navigate", listener); + + try { + pushOverlayHistoryEntry(); + await popstateHandler(new Event("popstate")); // consumes the overlay entry + await popstateHandler(new Event("popstate")); // a real back navigation + + assert.deepEqual(listener.mock.callCount(), 1); + } finally { + window.history.replaceState(originalState, "", originalPath); + } + }); }); describe("load", () => { -- 2.54.0 From b29d2563657b84d1595eb87e6bd160b189b404ec Mon Sep 17 00:00:00 2001 From: Nameless 7778777 <7778777@7778777.online> Date: Wed, 5 Aug 2026 16:59:28 +0200 Subject: [PATCH 2/3] Track the pending overlay history entry via a DOM marker instead of a counter The in-memory counter was module-level state shared by every Router instance, including the dozens the test suite creates. Since the parallel test runner shares one process (and module cache) across files, a lightbox test that opens without closing left the counter incremented forever, causing unrelated popstate tests elsewhere to spuriously skip navigation. A marker appended to document.body is scoped to the live DOM instead, so it can't outlive a test's own cleanup. --- src/js/components/lightbox-image-group.js | 24 +++++++++++++++++--- src/js/router.js | 27 +++++++++++++++-------- tests/unit/specs/router.test.js | 13 ++++++----- 3 files changed, 47 insertions(+), 17 deletions(-) diff --git a/src/js/components/lightbox-image-group.js b/src/js/components/lightbox-image-group.js index 35294c0b..6bd1800c 100644 --- a/src/js/components/lightbox-image-group.js +++ b/src/js/components/lightbox-image-group.js @@ -119,23 +119,41 @@ class LightboxDialog extends Component { open() { document.body.style.overflow = "hidden"; this.isOpen = true; + this._closed = false; this.handleKeyDown = this.handleKeyDown.bind(this); document.addEventListener("keydown", this.handleKeyDown); this.handlePopState = this.handlePopState.bind(this); window.addEventListener("popstate", this.handlePopState); - pushOverlayHistoryEntry(); + this._historyMarker = pushOverlayHistoryEntry(); this.render(); } handlePopState() { + if (this._closed) { + // Just confirming the history.back() we triggered ourselves when + // closing some other way; release the marker that told the router + // to ignore this popstate. + window.removeEventListener("popstate", this.handlePopState); + this._historyMarker?.remove(); + this._historyMarker = null; + return; + } this.close({ viaPopState: true }); } close({ viaPopState = false } = {}) { + if (this._closed) return; + this._closed = true; document.body.style.overflow = ""; document.removeEventListener("keydown", this.handleKeyDown); - window.removeEventListener("popstate", this.handlePopState); - if (!viaPopState) { + if (viaPopState) { + window.removeEventListener("popstate", this.handlePopState); + this._historyMarker?.remove(); + this._historyMarker = null; + } else { + // Leave the popstate listener attached: handlePopState() above + // releases the marker once this history.back() actually consumes + // the entry pushed in open(). window.history.back(); } this.isOpen = false; diff --git a/src/js/router.js b/src/js/router.js index ade86422..9fea1d2c 100644 --- a/src/js/router.js +++ b/src/js/router.js @@ -6,18 +6,30 @@ const MAX_PAGES = 5; // Lets an overlay (e.g. the image lightbox) claim the next back navigation // to close itself instead of the router navigating the underlying page. -// Call once when the overlay opens; the pushed entry is consumed either by -// a real back button press or by the overlay's own history.back() when -// it's dismissed some other way (see the popstate listener below). -let activeOverlayHistoryEntries = 0; +// Call once when the overlay opens; it pushes a history entry and tags the +// document with a marker element for as long as that entry hasn't been +// consumed yet. The caller is responsible for removing the returned marker +// once the entry is actually popped (either by a real back button press or +// by the overlay's own history.back() when it's dismissed some other way) +// — see the popstate listener below, which just checks for the marker's +// presence. Tracking this via a DOM node rather than in-memory state means +// a test that opens an overlay and never tears it down can't leave stale +// state behind for unrelated code: the marker disappears the moment +// something clears the document (e.g. a test's own cleanup), instead of +// silently lingering for the lifetime of the module. +const OVERLAY_HISTORY_PENDING_SELECTOR = "[data-overlay-history-pending]"; export function pushOverlayHistoryEntry() { - activeOverlayHistoryEntries++; + const marker = document.createElement("span"); + marker.hidden = true; + marker.setAttribute("data-overlay-history-pending", ""); + document.body.appendChild(marker); window.history.pushState( { ...window.history.state }, "", window.location.href, ); + return marker; } // Browsers fire auxclick rather than click for the middle button, so @@ -134,10 +146,7 @@ export class Router extends EventEmitter { }); // on back button, go back to the previous page window.addEventListener("popstate", async (e) => { - if (activeOverlayHistoryEntries > 0) { - activeOverlayHistoryEntries--; - return; - } + if (document.querySelector(OVERLAY_HISTORY_PENDING_SELECTOR)) return; this.emit("navigate"); await this.load(window.location.pathname + window.location.search, { isBack: true, diff --git a/tests/unit/specs/router.test.js b/tests/unit/specs/router.test.js index 794f3786..7fe1852a 100644 --- a/tests/unit/specs/router.test.js +++ b/tests/unit/specs/router.test.js @@ -333,18 +333,19 @@ describe("popstate", () => { const listener = mock.fn(); router.on("navigate", listener); + // Mirrors what the image lightbox does when it opens. + const marker = pushOverlayHistoryEntry(); try { - // Mirrors what the image lightbox does when it opens. - pushOverlayHistoryEntry(); await popstateHandler(new Event("popstate")); assert.deepEqual(listener.mock.callCount(), 0); } finally { + marker.remove(); window.history.replaceState(originalState, "", originalPath); } }); - it("should resume normal navigation on the popstate after an overlay entry is consumed", async () => { + it("should resume normal navigation once the overlay releases its marker", async () => { const originalState = window.history.state; const originalPath = window.location.pathname + window.location.search + window.location.hash; @@ -354,9 +355,11 @@ describe("popstate", () => { const listener = mock.fn(); router.on("navigate", listener); + const marker = pushOverlayHistoryEntry(); try { - pushOverlayHistoryEntry(); - await popstateHandler(new Event("popstate")); // consumes the overlay entry + await popstateHandler(new Event("popstate")); // overlay consumes this one + // Mirrors what the lightbox does once its own history.back() resolves. + marker.remove(); await popstateHandler(new Event("popstate")); // a real back navigation assert.deepEqual(listener.mock.callCount(), 1); -- 2.54.0 From 345c0e90d957108d49a9d43e7788b3bb79f31e62 Mon Sep 17 00:00:00 2001 From: Nameless 7778777 <7778777@7778777.online> Date: Thu, 6 Aug 2026 13:11:50 +0200 Subject: [PATCH 3/3] Convert lightbox to a native dialog instead of custom history tracking Per review feedback on #34: replace the router-level history-tracking mechanism (pushOverlayHistoryEntry, the popstate special-case) with the data-dialog-wrapper convention every other dialog in the app already uses. app.js already closes all open dialog[open] elements on every navigation, including back-button navigation, so converting the lightbox's plain
to a real + showModal() makes it close on back for free. router.js is now byte-for-byte identical to upstream. --- src/css/style.css | 4 + src/js/components/lightbox-image-group.js | 66 +++++--------- src/js/router.js | 29 ------ .../components/lightbox-image-group.test.js | 89 +++---------------- tests/unit/specs/router.test.js | 47 +--------- 5 files changed, 37 insertions(+), 198 deletions(-) diff --git a/src/css/style.css b/src/css/style.css index 73b156d9..244264c9 100644 --- a/src/css/style.css +++ b/src/css/style.css @@ -7684,6 +7684,10 @@ context-menu-item-group > context-menu-item > button { } .lightbox { + border: none; + padding: 0; + margin: 0; + color: inherit; position: fixed; top: 0; left: 0; diff --git a/src/js/components/lightbox-image-group.js b/src/js/components/lightbox-image-group.js index 6bd1800c..7cd4742c 100644 --- a/src/js/components/lightbox-image-group.js +++ b/src/js/components/lightbox-image-group.js @@ -1,16 +1,17 @@ import { Component, getChildrenFragment } from "/js/components/component.js"; import { html, render } from "/js/lib/lit-html.js"; import { ImageLoader } from "/js/utils.js"; +import { closeWithAnimation } from "/js/dialogHelpers.js"; import { chevronLeftIconTemplate } from "/js/templates/icons/chevronLeft.template.js"; import { chevronRightIconTemplate } from "/js/templates/icons/chevronRight.template.js"; import { closeIconTemplate } from "/js/templates/icons/closeIcon.template.js"; -import { pushOverlayHistoryEntry } from "/js/router.js"; class LightboxDialog extends Component { connectedCallback() { if (this._initialized) { return; } + this.setAttribute("data-dialog-wrapper", ""); this.innerHTML = ""; this.hideAltText = this.getAttribute("hide-alt-text") === "true"; this.imageShape = this.getAttribute("image-shape"); @@ -56,14 +57,26 @@ class LightboxDialog extends Component { render( html` - `, this, ); @@ -119,47 +132,14 @@ class LightboxDialog extends Component { open() { document.body.style.overflow = "hidden"; this.isOpen = true; - this._closed = false; this.handleKeyDown = this.handleKeyDown.bind(this); document.addEventListener("keydown", this.handleKeyDown); - this.handlePopState = this.handlePopState.bind(this); - window.addEventListener("popstate", this.handlePopState); - this._historyMarker = pushOverlayHistoryEntry(); this.render(); + this.querySelector(".lightbox").showModal(); } - handlePopState() { - if (this._closed) { - // Just confirming the history.back() we triggered ourselves when - // closing some other way; release the marker that told the router - // to ignore this popstate. - window.removeEventListener("popstate", this.handlePopState); - this._historyMarker?.remove(); - this._historyMarker = null; - return; - } - this.close({ viaPopState: true }); - } - - close({ viaPopState = false } = {}) { - if (this._closed) return; - this._closed = true; - document.body.style.overflow = ""; - document.removeEventListener("keydown", this.handleKeyDown); - if (viaPopState) { - window.removeEventListener("popstate", this.handlePopState); - this._historyMarker?.remove(); - this._historyMarker = null; - } else { - // Leave the popstate listener attached: handlePopState() above - // releases the marker once this history.back() actually consumes - // the entry pushed in open(). - window.history.back(); - } - this.isOpen = false; - this._imageLoader.abort(); - this.render(); - this.dispatchEvent(new Event("close")); + close() { + return closeWithAnimation(this.querySelector(".lightbox")); } navigate(steps) { @@ -171,9 +151,7 @@ class LightboxDialog extends Component { } handleKeyDown(e) { - if (e.key === "Escape") { - this.close(); - } else if (e.key === "ArrowLeft") { + if (e.key === "ArrowLeft") { this.navigate(-1); } else if (e.key === "ArrowRight") { this.navigate(1); diff --git a/src/js/router.js b/src/js/router.js index 9fea1d2c..08250ced 100644 --- a/src/js/router.js +++ b/src/js/router.js @@ -4,34 +4,6 @@ import { BoundedMap } from "/js/utils.js"; const MAX_PAGES = 5; -// Lets an overlay (e.g. the image lightbox) claim the next back navigation -// to close itself instead of the router navigating the underlying page. -// Call once when the overlay opens; it pushes a history entry and tags the -// document with a marker element for as long as that entry hasn't been -// consumed yet. The caller is responsible for removing the returned marker -// once the entry is actually popped (either by a real back button press or -// by the overlay's own history.back() when it's dismissed some other way) -// — see the popstate listener below, which just checks for the marker's -// presence. Tracking this via a DOM node rather than in-memory state means -// a test that opens an overlay and never tears it down can't leave stale -// state behind for unrelated code: the marker disappears the moment -// something clears the document (e.g. a test's own cleanup), instead of -// silently lingering for the lifetime of the module. -const OVERLAY_HISTORY_PENDING_SELECTOR = "[data-overlay-history-pending]"; - -export function pushOverlayHistoryEntry() { - const marker = document.createElement("span"); - marker.hidden = true; - marker.setAttribute("data-overlay-history-pending", ""); - document.body.appendChild(marker); - window.history.pushState( - { ...window.history.state }, - "", - window.location.href, - ); - return marker; -} - // Browsers fire auxclick rather than click for the middle button, so // we need to manually dispatch a click event for it function bindMiddleClickRedispatch() { @@ -146,7 +118,6 @@ export class Router extends EventEmitter { }); // on back button, go back to the previous page window.addEventListener("popstate", async (e) => { - if (document.querySelector(OVERLAY_HISTORY_PENDING_SELECTOR)) return; this.emit("navigate"); await this.load(window.location.pathname + window.location.search, { isBack: true, diff --git a/tests/unit/specs/components/lightbox-image-group.test.js b/tests/unit/specs/components/lightbox-image-group.test.js index 70b6c387..0a9989f2 100644 --- a/tests/unit/specs/components/lightbox-image-group.test.js +++ b/tests/unit/specs/components/lightbox-image-group.test.js @@ -50,6 +50,13 @@ describe("lightbox-image-group", () => { element.open(); const lightbox = element.querySelector(".lightbox"); assert(lightbox !== null); + assert.deepEqual(lightbox.tagName, "DIALOG"); + }); + + it("should set data-dialog-wrapper on the host element", () => { + const element = document.createElement("lightbox-dialog"); + document.body.appendChild(element); + assert(element.hasAttribute("data-dialog-wrapper")); }); it("should render close button", () => { @@ -334,91 +341,15 @@ describe("lightbox-image-group", () => { }); }); - describe("LightboxDialog - back button / history integration", () => { - it("should push a history entry when opened", () => { - const originalLength = window.history.length; - const element = document.createElement("lightbox-dialog"); - element.images = [createTestImage("test.jpg", "Test")]; - document.body.appendChild(element); - element.open(); - - assert.deepEqual(window.history.length, originalLength + 1); - - element.close(); - }); - - it("should close when a popstate event fires while open (e.g. Android back button)", () => { - const element = document.createElement("lightbox-dialog"); - element.images = [createTestImage("test.jpg", "Test")]; - document.body.appendChild(element); - element.open(); - - window.dispatchEvent(new Event("popstate")); - - assert.deepEqual(element.isOpen, false); - }); - - it("should not call history.back() again when closed via popstate", () => { - const element = document.createElement("lightbox-dialog"); - element.images = [createTestImage("test.jpg", "Test")]; - document.body.appendChild(element); - element.open(); - - const originalBack = window.history.back; - let backCalls = 0; - window.history.back = () => { - backCalls++; - }; - try { - window.dispatchEvent(new Event("popstate")); - } finally { - window.history.back = originalBack; - } - - assert.deepEqual(backCalls, 0); - }); - - it("should call history.back() to release its pushed entry when closed some other way", () => { - const element = document.createElement("lightbox-dialog"); - element.images = [createTestImage("test.jpg", "Test")]; - document.body.appendChild(element); - element.open(); - - const originalBack = window.history.back; - let backCalls = 0; - window.history.back = () => { - backCalls++; - }; - try { - element.close(); - } finally { - window.history.back = originalBack; - } - - assert.deepEqual(backCalls, 1); - }); - - it("should stop listening for popstate after closing", () => { - const element = document.createElement("lightbox-dialog"); - element.images = [createTestImage("test.jpg", "Test")]; - document.body.appendChild(element); - element.open(); - element.close(); - - // A stray popstate after close should not throw or reopen the dialog. - window.dispatchEvent(new Event("popstate")); - assert.deepEqual(element.isOpen, false); - }); - }); - describe("LightboxDialog - keyboard navigation", () => { - it("should close on Escape key", () => { + it("should close on Escape key (native dialog cancel event)", () => { const element = document.createElement("lightbox-dialog"); element.images = [createTestImage("test.jpg", "Test")]; document.body.appendChild(element); element.open(); - element.handleKeyDown({ key: "Escape" }); + const dialog = element.querySelector(".lightbox"); + dialog.dispatchEvent(new Event("cancel", { cancelable: true })); assert.deepEqual(element.isOpen, false); }); diff --git a/tests/unit/specs/router.test.js b/tests/unit/specs/router.test.js index 7fe1852a..cb417aa6 100644 --- a/tests/unit/specs/router.test.js +++ b/tests/unit/specs/router.test.js @@ -1,6 +1,6 @@ import { describe, it, beforeEach, afterEach, mock } from "node:test"; import assert from "node:assert/strict"; -import { Router, Layout, pushOverlayHistoryEntry } from "/js/router.js"; +import { Router, Layout } from "/js/router.js"; class TestLayout extends Layout { constructor() { @@ -322,51 +322,6 @@ describe("popstate", () => { window.history.replaceState(originalState, "", originalPath); } }); - - it("should not emit navigate when popstate fires while an overlay history entry is pending", async () => { - const originalState = window.history.state; - const originalPath = - window.location.pathname + window.location.search + window.location.hash; - const { router, popstateHandler } = createRouterWithPopstateHandler(); - mountRouter(router); - - const listener = mock.fn(); - router.on("navigate", listener); - - // Mirrors what the image lightbox does when it opens. - const marker = pushOverlayHistoryEntry(); - try { - await popstateHandler(new Event("popstate")); - - assert.deepEqual(listener.mock.callCount(), 0); - } finally { - marker.remove(); - window.history.replaceState(originalState, "", originalPath); - } - }); - - it("should resume normal navigation once the overlay releases its marker", async () => { - const originalState = window.history.state; - const originalPath = - window.location.pathname + window.location.search + window.location.hash; - const { router, popstateHandler } = createRouterWithPopstateHandler(); - mountRouter(router); - - const listener = mock.fn(); - router.on("navigate", listener); - - const marker = pushOverlayHistoryEntry(); - try { - await popstateHandler(new Event("popstate")); // overlay consumes this one - // Mirrors what the lightbox does once its own history.back() resolves. - marker.remove(); - await popstateHandler(new Event("popstate")); // a real back navigation - - assert.deepEqual(listener.mock.callCount(), 1); - } finally { - window.history.replaceState(originalState, "", originalPath); - } - }); }); describe("load", () => {