From e1ad5357ecaa78cb2a9579d352ee7c9d8c85d875 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Wed, 16 Sep 2026 17:16:06 -0400 Subject: [PATCH] fix: keep the settings dialog inside the window as sections are opened MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The settings modal grows with its content and is centred in a fixed, non-scrolling overlay, so expanding a few accordion sections pushed it past both edges of the window — and the half above the top edge, title and close button included, could not be scrolled to. Opening a section now measures the dialog and collapses the sections opened before it, oldest first, until it fits again. A module-level queue tracks expanded sections in the order they were opened, so the section just opened is last in line and the one opened longest ago gives up its space first. Details worth knowing: - The budget comes from panel-anchor's visibleViewportBottom() rather than innerHeight, and #settingsOverlay is now centred within that same visible height. Limiting only the dialog's size is not enough: centred in a layout viewport taller than the visible one, a dialog of exactly the allowed height still hangs off the bottom of the screen by half the difference. - A raised software keyboard shrinks the visible viewport exactly as a stranded layout viewport does, and tapping a header blurs the field first, so the collapse would have run on the keyboard's reading — and persisted it. panel-anchor now exports visibleViewportSettled(), and the fit does nothing until the measurement is trustworthy again. - openSettingsModal() awaits populateModalFields() before measuring. A reachable InvokeAI backend reveals the username, password and board rows a round trip later, and the old fire-and-forget call measured a modal ~170px shorter than the one the user ended up with. - .settings-modal gains a max-height and scrolls as a last resort, for the one case the collapsing cannot help: a single section taller than the window, which is never collapsed because shutting it would make it unopenable. Its budget subtracts the margin in px and hands back .modal-content's padding in em, so it agrees with the JS at any root font size. - The accordion click handler is a header.onclick property assignment rather than addEventListener, so running setupAccordions twice over the same dialog cannot stack handlers that toggle a section twice per click. Co-Authored-By: Claude Opus 5 (1M context) --- photomap/frontend/static/css/settings.css | 21 ++ .../static/javascript/panel-anchor.js | 17 ++ .../frontend/static/javascript/settings.js | 131 ++++++++-- tests/frontend/settings-accordion-fit.test.js | 247 ++++++++++++++++++ 4 files changed, 400 insertions(+), 16 deletions(-) create mode 100644 tests/frontend/settings-accordion-fit.test.js diff --git a/photomap/frontend/static/css/settings.css b/photomap/frontend/static/css/settings.css index 570efb0c..6edebc1e 100644 --- a/photomap/frontend/static/css/settings.css +++ b/photomap/frontend/static/css/settings.css @@ -4,6 +4,27 @@ position: fixed; min-width: 320px; z-index: 11000; + /* Last resort for a single expanded section taller than the window: + settings.js collapses the other sections to make room, but never the only + one left open. 32px is that module's MODAL_VIEWPORT_MARGIN; the 4em is the + padding of .modal-content, which max-height does not cover (the dialog is + content-box — there is no global border-box reset) and which therefore has + to be handed back so the outer height matches the budget settings.js + measures against. Keeping the margin in px and the padding in em is what + makes the two agree at any root font size. */ + max-height: calc(var(--visible-viewport-height, 100vh) - 32px - 4em); + overflow-y: auto; +} + +/* The dialog is centred in this overlay, so a budget that only limits its + height is not enough: centred in a layout viewport taller than the visible + one, a dialog of exactly the allowed height still hangs off the bottom of + the screen by half the difference, and a fixed overlay does not scroll. + Centring within the visible height instead is what makes the height budget + in settings.js mean what it says. Unset (every desktop browser, and iPad + when it behaves) this is just 100vh, the .modal-overlay default. */ +#settingsOverlay { + height: var(--visible-viewport-height, 100vh); } .settings-header { diff --git a/photomap/frontend/static/javascript/panel-anchor.js b/photomap/frontend/static/javascript/panel-anchor.js index 1b96c563..a52b7ba3 100644 --- a/photomap/frontend/static/javascript/panel-anchor.js +++ b/photomap/frontend/static/javascript/panel-anchor.js @@ -101,6 +101,23 @@ export function visibleViewportBottom() { return document.documentElement.clientHeight - liveOvershoot(); } +/** + * Is what visibleViewportBottom() reports right now trustworthy? + * + * The software keyboard shrinks the visible viewport exactly as a stranded + * layout viewport does, and nothing in the geometry tells the two apart — see + * syncPanelAnchor(), which holds its correction rather than guess. Re-seating + * a panel on a bad sample is self-correcting; anything that acts on the + * measurement irreversibly (collapsing a section and persisting that, say) + * wants to know first, and to do nothing until this is true again. + * + * @returns {boolean} false while a text field is focused and until the + * keyboard has finished collapsing after it blurs + */ +export function visibleViewportSettled() { + return !isTextEntryFocused() && !keyboardSettling; +} + /** Re-seat the registered panels against the current viewport. */ export function syncPanelAnchor() { // The software keyboard shrinks the visual viewport exactly as a stranded diff --git a/photomap/frontend/static/javascript/settings.js b/photomap/frontend/static/javascript/settings.js index 3bcd1589..3449622c 100644 --- a/photomap/frontend/static/javascript/settings.js +++ b/photomap/frontend/static/javascript/settings.js @@ -14,6 +14,7 @@ import { } from "./state.js"; import { clearImageLabelCache, setClusterLabels } from "./cluster-utils.js"; import { refreshInvokeCapabilities } from "./invoke-capabilities.js"; +import { visibleViewportBottom, visibleViewportSettled } from "./panel-anchor.js"; import { fetchJson, hideSpinner, showSpinner } from "./utils.js"; // Constants @@ -174,9 +175,22 @@ function adjustDelay(direction) { } // Model window management -export function openSettingsModal() { - populateModalFields(); +export async function openSettingsModal() { elements.settingsOverlay.classList.add("visible"); + try { + // Awaited, unlike the fire-and-forget call this replaced: populating the + // fields reaches the network, and a reachable InvokeAI backend reveals the + // username, password and board rows a round trip later. Measuring before + // they land reads a modal ~170px shorter than the one the user ends up + // with, and nothing would re-measure afterwards. + await populateModalFields(); + } finally { + // The restored set of open sections need not fit this window: it was saved + // on a device or at a size that is no longer the current one. Nothing can + // be measured while the modal is display:none, so the check belongs here + // rather than where the state is restored. + fitAccordionsToViewport(); + } } export function closeSettingsModal() { @@ -721,26 +735,111 @@ function setupResetAllPreferencesButton() { }); } -// Accordion section toggle -function setupAccordions() { - document.querySelectorAll(".settings-accordion .accordion-header").forEach((header) => { - const section = header.closest(".settings-accordion").dataset.section; +// ===== Accordion sections ===== +// +// The modal is centred in a full-height overlay and grows with its content, so +// with enough sections expanded it becomes taller than the window and spills +// off both ends — and the half above the top edge, title and close button +// included, cannot be scrolled to. Opening a section therefore collapses the +// sections opened before it, oldest first, until the dialog fits again. + +// Expanded sections, oldest first — the order they give up their space in. +let accordionOpenOrder = []; + +// Breathing room kept around the dialog. The max-height in settings.css +// subtracts this same figure in px (plus the padding max-height does not +// cover), so the budget here and the CSS fallback that catches a single +// over-tall section agree on what "fits" means at any root font size. +const MODAL_VIEWPORT_MARGIN = 32; + +/** Expand or collapse one section, persisting the new state. */ +function setAccordionOpen(accordion, open) { + const header = accordion.querySelector(".accordion-header"); + const body = header.nextElementSibling; + + header.setAttribute("aria-expanded", String(open)); + body.classList.toggle("open", open); + localStorage.setItem(`settings-accordion-${accordion.dataset.section}`, String(open)); + + accordionOpenOrder = accordionOpenOrder.filter((other) => other !== accordion); + if (open) { + accordionOpenOrder.push(accordion); + } +} + +/** + * Collapse the oldest expanded sections until the modal fits on screen. + * + * The last section standing is never collapsed: a section too tall to fit on + * its own would otherwise be shut the instant the user opened it. That case + * falls through to the modal's own max-height and scrolls instead. `keep` + * belongs to the same rule rather than adding one — a section is pushed onto + * the queue as it opens, so the one just opened is last in line and is only + * reached once every other has gone, where the rule above already spares it. + * It is named anyway so re-ordering the queue cannot quietly shut the section + * the user is looking at. + * + * @param {HTMLElement} [keep] section to spare — the one just opened + */ +function fitAccordionsToViewport(keep) { + const modal = document.querySelector(".settings-modal"); + if (!modal) { + return; + } + // A raised software keyboard shrinks the visible viewport exactly as a + // stranded layout viewport does. Collapsing on that reading would shut + // sections that fit perfectly well — and write it to localStorage, where it + // outlives the keyboard. Tapping a header blurs the field first, so this is + // the live path, not a corner: measure nothing until the keyboard has gone. + if (!visibleViewportSettled()) { + return; + } + // visibleViewportBottom(), not innerHeight: on a stranded iPadOS layout + // viewport the bottom of the window is well below the bottom of the screen. + const available = visibleViewportBottom() - MODAL_VIEWPORT_MARGIN; + // Re-measured each pass — collapsing one section is usually enough. + for (const accordion of [...accordionOpenOrder]) { + if (accordionOpenOrder.length <= 1 || modal.scrollHeight <= available) { + break; + } + if (accordion !== keep) { + setAccordionOpen(accordion, false); + } + } +} + +export function setupAccordions() { + // Rebuilt from the DOM rather than carried over, so a second pass over the + // same dialog starts from what is actually on screen. + accordionOpenOrder = []; + + document.querySelectorAll(".settings-accordion").forEach((accordion) => { + const section = accordion.dataset.section; + const header = accordion.querySelector(".accordion-header"); const body = header.nextElementSibling; - const storageKey = `settings-accordion-${section}`; // Restore persisted open/closed state - const wasOpen = localStorage.getItem(storageKey) === "true"; + const wasOpen = localStorage.getItem(`settings-accordion-${section}`) === "true"; + header.setAttribute("aria-expanded", String(wasOpen)); + body.classList.toggle("open", wasOpen); if (wasOpen) { - header.setAttribute("aria-expanded", "true"); - body.classList.add("open"); + accordionOpenOrder.push(accordion); } - header.addEventListener("click", () => { - const expanded = header.getAttribute("aria-expanded") === "true"; - header.setAttribute("aria-expanded", String(!expanded)); - body.classList.toggle("open"); - localStorage.setItem(storageKey, String(!expanded)); - }); + // Property assignment rather than addEventListener, so running this twice + // over the same dialog is a no-op: stacked listeners would toggle the + // section once per pass, leaving a click with nothing to show. (The + // settingsUpdated re-init the rest of this file guards against cannot + // currently fire — state.js dispatches on window, the listener at the + // bottom of this file is on document — but setupAccordions is written to + // survive it either way.) + header.onclick = () => { + const open = header.getAttribute("aria-expanded") !== "true"; + setAccordionOpen(accordion, open); + if (open) { + fitAccordionsToViewport(accordion); + } + }; }); } diff --git a/tests/frontend/settings-accordion-fit.test.js b/tests/frontend/settings-accordion-fit.test.js new file mode 100644 index 00000000..7d138243 --- /dev/null +++ b/tests/frontend/settings-accordion-fit.test.js @@ -0,0 +1,247 @@ +// Unit tests for settings.js — collapsing accordion sections so the settings +// modal never grows taller than the window. +import { jest, describe, it, expect, beforeEach } from "@jest/globals"; + +// settings.js pulls in several other modules; stub them so the module loads +// cleanly under jsdom. +jest.unstable_mockModule("../../photomap/frontend/static/javascript/album-manager.js", () => ({ + albumManager: { fetchAvailableAlbums: jest.fn(() => Promise.resolve([])) }, + checkAlbumIndex: jest.fn(), +})); +jest.unstable_mockModule("../../photomap/frontend/static/javascript/search-ui.js", () => ({ + exitSearchMode: jest.fn(), +})); +jest.unstable_mockModule("../../photomap/frontend/static/javascript/preferences-client.js", () => ({ + cancelPendingPatches: jest.fn(), +})); +jest.unstable_mockModule("../../photomap/frontend/static/javascript/slideshow.js", () => ({ + setSlideshowMode: jest.fn(), +})); +const mockState = { + mode: "chronological", + currentDelay: 10, + album: "", + showControlPanelText: true, +}; +jest.unstable_mockModule("../../photomap/frontend/static/javascript/state.js", () => ({ + clearPersistedSettingsCache: jest.fn(), + saveSettingsToLocalStorage: jest.fn(), + setAlbum: jest.fn(), + setAutotaggingEnabled: jest.fn(), + setWrapNavigation: jest.fn(), + state: mockState, +})); + +// jsdom has no layout, so the real measurement is stubbed on both sides: the +// visible bottom edge here, and the modal's scrollHeight below. +let viewportBottom = 600; +let viewportSettled = true; +jest.unstable_mockModule("../../photomap/frontend/static/javascript/panel-anchor.js", () => ({ + visibleViewportBottom: () => viewportBottom, + visibleViewportSettled: () => viewportSettled, +})); + +// Height the modal gains once its fields have finished loading — the InvokeAI +// auth rows, in production. Applied when the field loaders' fetch resolves, +// which is the point the un-awaited version of openSettingsModal measured +// before. utils.js itself is left real; only the network underneath it moves. +let lateGrowth = 0; +let pendingGrowth = 0; +global.fetch = jest.fn(async () => { + // Off the synchronous turn, as a real request is: applying the growth + // eagerly would let a fire-and-forget populateModalFields() still see it. + await new Promise((resolve) => setTimeout(resolve, 0)); + lateGrowth = pendingGrowth; + return { ok: true, status: 200, json: async () => ({ has_key: false }) }; +}); + +const { cacheElements, openSettingsModal, setupAccordions } = + await import("../../photomap/frontend/static/javascript/settings.js"); + +const SECTIONS = ["slideshow", "appearance", "autotagging", "api-integration"]; + +// Modal chrome that is there whether or not anything is expanded, plus what +// one expanded section costs. At viewportBottom 600 the fit budget is 568, so +// two sections fit (100 + 400) and three do not (100 + 600). +const CHROME_PX = 100; +const SECTION_PX = 200; + +const header = (section) => document.querySelector(`.settings-accordion[data-section="${section}"] .accordion-header`); +const isOpen = (section) => header(section).getAttribute("aria-expanded") === "true"; +const openSections = () => SECTIONS.filter(isOpen); + +function buildModal() { + document.body.innerHTML = ` + + `; + + const modal = document.querySelector(".settings-modal"); + Object.defineProperty(modal, "scrollHeight", { + configurable: true, + get: () => CHROME_PX + document.querySelectorAll(".accordion-body.open").length * SECTION_PX + lateGrowth, + }); +} + +describe("settings modal accordion fitting", () => { + beforeEach(() => { + localStorage.clear(); + viewportBottom = 600; + viewportSettled = true; + lateGrowth = 0; + pendingGrowth = 0; + buildModal(); + cacheElements(); + setupAccordions(); + }); + + it("leaves the other sections alone while there is room", () => { + header("slideshow").click(); + header("appearance").click(); + + expect(openSections()).toEqual(["slideshow", "appearance"]); + }); + + it("collapses the oldest open section when a new one would overflow", () => { + header("slideshow").click(); + header("appearance").click(); + header("autotagging").click(); + + expect(openSections()).toEqual(["appearance", "autotagging"]); + expect(localStorage.getItem("settings-accordion-slideshow")).toBe("false"); + }); + + it("keeps collapsing until the dialog fits", () => { + viewportBottom = 332; // budget 300 — room for one section only + + header("slideshow").click(); + header("appearance").click(); + header("autotagging").click(); + + expect(openSections()).toEqual(["autotagging"]); + }); + + it("never collapses the section the user just opened, even alone", () => { + // A section taller than the whole window: shutting it on the way in would + // make it impossible to open. The modal's max-height scrolls instead. + viewportBottom = 150; + + header("slideshow").click(); + + expect(openSections()).toEqual(["slideshow"]); + }); + + it("does not collapse anything when a section is closed", () => { + header("slideshow").click(); + header("appearance").click(); + header("appearance").click(); + + expect(openSections()).toEqual(["slideshow"]); + }); + + it("collapses by age of opening, not document order", () => { + header("appearance").click(); + header("slideshow").click(); + header("api-integration").click(); + + // "appearance" was opened first, so it goes even though "slideshow" sits + // above it in the dialog. + expect(openSections()).toEqual(["slideshow", "api-integration"]); + }); + + it("re-opening a section refreshes its place in the queue", () => { + header("slideshow").click(); + header("appearance").click(); + header("slideshow").click(); // close + header("slideshow").click(); // and open again — now the newest + header("autotagging").click(); + + expect(openSections()).toEqual(["slideshow", "autotagging"]); + }); + + it("trims sections restored from a larger window when the modal opens", async () => { + SECTIONS.forEach((section) => localStorage.setItem(`settings-accordion-${section}`, "true")); + buildModal(); + cacheElements(); + setupAccordions(); + expect(openSections()).toEqual(SECTIONS); + + await openSettingsModal(); + + // Restore order is document order, so the last two survive. + expect(openSections()).toEqual(["autotagging", "api-integration"]); + }); + + it("measures only after the fields the modal grows with have loaded", async () => { + // populateModalFields() reaches the network, and a reachable InvokeAI + // backend reveals three more rows a round trip later. Measuring before + // they land leaves the dialog over budget with nothing to re-check it. + pendingGrowth = 200; + ["slideshow", "appearance"].forEach((s) => localStorage.setItem(`settings-accordion-${s}`, "true")); + buildModal(); + cacheElements(); + setupAccordions(); + + await openSettingsModal(); + + expect(openSections()).toEqual(["appearance"]); + }); + + it("collapses nothing while the viewport measurement is unsettled", () => { + // A raised software keyboard shrinks the visible viewport exactly as a + // stranded layout viewport does. Tapping a header blurs the field first, + // so a section is opened while the keyboard is still up — collapsing on + // that reading would persist a decision the keyboard caused. + header("slideshow").click(); + viewportSettled = false; + viewportBottom = 150; // what the keyboard makes it look like + + header("appearance").click(); + + expect(openSections()).toEqual(["slideshow", "appearance"]); + expect(localStorage.getItem("settings-accordion-slideshow")).toBe("true"); + }); + + it("spends the whole budget, and not a pixel more", () => { + // Pins MODAL_VIEWPORT_MARGIN: two sections measure exactly 500. + viewportBottom = 532; // budget 500 + header("slideshow").click(); + header("appearance").click(); + expect(openSections()).toEqual(["slideshow", "appearance"]); + + localStorage.clear(); + buildModal(); + cacheElements(); + setupAccordions(); + viewportBottom = 531; // budget 499 — one pixel short + header("slideshow").click(); + header("appearance").click(); + expect(openSections()).toEqual(["appearance"]); + }); + + it("toggles once per click when set up twice over the same dialog", () => { + // A stacked click listener would open and immediately re-close the + // section. setupAccordions is written to be re-runnable over a dialog that + // is already wired up. + setupAccordions(); + setupAccordions(); + + header("slideshow").click(); + + expect(isOpen("slideshow")).toBe(true); + }); +});