diff --git a/app/composer/elements/floating.js b/app/composer/elements/floating.js index c3b0bfff..dc45dcb3 100644 --- a/app/composer/elements/floating.js +++ b/app/composer/elements/floating.js @@ -36,7 +36,27 @@ define("floating", "window", ({ // Bound, because another window coming to the front shifts this one's z. const layer = claimFor("floatingWindow", node); + // Enough of the window to grab: a strip of titlebar wide enough to hit and + // tall enough to see. Everything that positions the window goes through here, + // because the one path that did not was the one that lost it. + const GRABBABLE = 80; + const TITLEBAR = 32; + + const clampIntoView = () => { + // Math.max(0, ...) matters on a viewport narrower than the reachable strip: + // without it the upper bound goes negative and the clamp pushes the window + // further off rather than back on. + box.x = Math.min(Math.max(box.x, -box.width + GRABBABLE), + Math.max(0, window.innerWidth - GRABBABLE)); + box.y = Math.min(Math.max(box.y, 0), Math.max(0, window.innerHeight - TITLEBAR)); + }; + const apply = () => { + // Clamped on EVERY apply, not only while dragging. A position is remembered + // per window id, so a window moved to the right of a wide screen and reopened + // on a narrow one opened entirely off it -- no titlebar to grab, nothing to + // drag back, and the only way out was clearing localStorage by hand. + clampIntoView(); node.style.left = `${box.x}px`; node.style.top = `${box.y}px`; node.style.width = `${box.width}px`; @@ -89,14 +109,19 @@ define("floating", "window", ({ const toFront = () => { layer.raise(); }; node.addEventListener("pointerdown", toFront); + // A window open while the viewport shrinks has the same problem as one + // reopened on a smaller screen. + const onViewportResize = () => apply(); + window.addEventListener("resize", onViewportResize); + let start = null; const stopDrag = drag(titlebar, { onStart: () => { start = { ...box }; toFront(); }, onMove: ({ dx, dy }) => { - // Clamped so a window can never be dragged somewhere it cannot be - // dragged back from. - box.x = Math.min(Math.max(start.x + dx, -box.width + 80), window.innerWidth - 80); - box.y = Math.min(Math.max(start.y + dy, 0), window.innerHeight - 32); + // apply() does the clamping, so a window can never be dragged somewhere + // it cannot be dragged back from -- and the rule lives in one place. + box.x = start.x + dx; + box.y = start.y + dy; apply(); }, onEnd: () => remember(id, box), @@ -127,6 +152,7 @@ define("floating", "window", ({ remember(id, box); dismissal.release(); stopDrag(); stopResize(); + window.removeEventListener("resize", onViewportResize); layer.release(); unmount(node); if (onClose) onClose(reason); diff --git a/app/tests-layout/window.spec.js b/app/tests-layout/window.spec.js new file mode 100644 index 00000000..eca45003 --- /dev/null +++ b/app/tests-layout/window.spec.js @@ -0,0 +1,87 @@ +// A floating window must always be reachable. +// +// Position is remembered per window id, so the dangerous case is not "where did +// it open" but "where did it open GIVEN a screen it no longer fits". jsdom +// cannot see this at all: getBoundingClientRect is zeros there, so a window +// entirely off the right edge measures the same as one in the middle. + +import { test, expect } from "@playwright/test"; + +test.beforeEach(async ({ page }) => { + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); +}); + +async function openWindow(page, id) { + return page.evaluate(async (id) => { + const { composer } = await import("/funpack/app/composer/composer.js"); + const handle = composer.floating.window({ + id, title: "Attack", body: composer.text.md({ text: "content" }), + }); + const box = handle.node.getBoundingClientRect(); + return { x: box.x, y: box.y, w: box.width, h: box.height, + vw: innerWidth, vh: innerHeight }; + }, id); +} + +test("a window remembered off a wider screen still opens reachable", async ({ page }) => { + // Remember a position far to the right, as dragging on a wide screen would. + await page.evaluate(() => localStorage.setItem( + "cx.win.attack", JSON.stringify({ x: 1400, y: 40, width: 420, height: 320 }))); + + await page.setViewportSize({ width: 800, height: 600 }); + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); + + const box = await openWindow(page, "attack"); + + // Something grabbable has to be on screen -- a titlebar with no pixels in the + // viewport cannot be dragged back, and the position is remembered, so the + // window would be lost until localStorage was cleared by hand. + expect(box.x, "the whole window is past the right edge").toBeLessThan(box.vw); + expect(box.x + box.w, "the whole window is past the left edge").toBeGreaterThan(0); + expect(box.y).toBeGreaterThanOrEqual(0); + expect(box.y, "the titlebar is below the bottom edge").toBeLessThan(box.vh); + + // And enough of it to actually hit with a pointer. + const visible = Math.min(box.x + box.w, box.vw) - Math.max(box.x, 0); + expect(visible, "less than a grabbable strip is on screen").toBeGreaterThanOrEqual(60); +}); + +test("a window open while the viewport shrinks stays reachable", async ({ page }) => { + await page.setViewportSize({ width: 1400, height: 900 }); + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); + await openWindow(page, "shrink"); + + await page.setViewportSize({ width: 600, height: 500 }); + const box = await page.evaluate(() => { + const node = document.querySelector(".cx-window"); + const r = node.getBoundingClientRect(); + return { x: r.x, y: r.y, w: r.width, vw: innerWidth }; + }); + + expect(box.x).toBeLessThan(box.vw); + expect(box.x + box.w).toBeGreaterThan(0); +}); + +test("a window cannot be dragged somewhere it cannot be dragged back from", async ({ page }) => { + await page.setViewportSize({ width: 900, height: 700 }); + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); + await openWindow(page, "dragged"); + + const bar = page.locator(".cx-window-bar").first(); + await bar.hover(); + await page.mouse.down(); + await page.mouse.move(5000, 5000, { steps: 5 }); // far past both edges + await page.mouse.up(); + + const box = await page.evaluate(() => { + const r = document.querySelector(".cx-window").getBoundingClientRect(); + return { x: r.x, y: r.y, w: r.width, vw: innerWidth, vh: innerHeight }; + }); + expect(box.x).toBeLessThan(box.vw); + expect(box.y).toBeLessThan(box.vh); + expect(box.x + box.w).toBeGreaterThan(0); +}); diff --git a/core/routes.py b/core/routes.py index 0d2a2819..bd0eb499 100644 --- a/core/routes.py +++ b/core/routes.py @@ -123,7 +123,12 @@ def register(routes, prefix=None): except Exception: # noqa: BLE001 return web.json_response({"problems": ["that is not JSON"]}, status=400) - slots = body.get("slots") or _pipeline() + # `or` would resurrect the default here: an explicitly empty pipeline is + # falsy, and a client that has removed every slot is entitled to be told + # it has none rather than handed the defaults back. + slots = body.get("slots") + if slots is None: + slots = _pipeline() action, slot_id = body.get("action"), body.get("slot") if action == "replace": diff --git a/core/tests/test_routes_pipeline.py b/core/tests/test_routes_pipeline.py index 8bc2e770..e55a982f 100644 --- a/core/tests/test_routes_pipeline.py +++ b/core/tests/test_routes_pipeline.py @@ -147,3 +147,47 @@ def test_an_unknown_action_is_refused(server): status, body = _request(server, "POST", "/funpack/api/pipeline", {"action": "explode", "slot": "model"}) assert status == 400 and body["problems"] # a malformed request, not an edit + + +def test_an_explicitly_empty_pipeline_is_not_replaced_by_the_default(server): + """`slots or default()` resurrects the default, because an empty list is + falsy. A client that removed every slot is entitled to be told it has none + rather than handed the built-ins back.""" + status, body = _request(server, "POST", "/funpack/api/pipeline", + {"action": "check", "slots": []}) + assert status == 200 + assert body["slots"] == [], "an empty pipeline came back full" + + +def test_omitting_slots_still_falls_back_to_the_default(server): + """The distinction that matters: absent means "use the default", empty means + "there is nothing". Only one of them is a fallback.""" + status, body = _request(server, "POST", "/funpack/api/pipeline", + {"action": "check"}) + assert status == 200 + assert body["slots"], "omitting slots should use the default pipeline" + + +def test_removing_slots_one_by_one_never_resurrects_the_default(server): + """The failure this guards: each request carries the client's current slots, + so a fallback that fires on an empty list would silently hand the built-ins + back mid-way through and the count would jump UP.""" + _status, body = _request(server, "GET", "/funpack/api/pipeline") + slots = body["slots"] + started_with = len(slots) + assert started_with > 1, "the default pipeline is too small to test this" + + counts = [started_with] + # Consumers before sources: removing a source nothing can replace is refused, + # which is correct and not what this test is about. + for slot_id in [s["id"] for s in reversed(slots)]: + _status, body = _request(server, "POST", "/funpack/api/pipeline", + {"action": "remove", "slot": slot_id, "slots": slots}) + if body["refused"]: + continue + slots = body["slots"] + counts.append(len(slots)) + + assert len(slots) < started_with, "nothing was actually removed" + assert counts == sorted(counts, reverse=True), ( + f"the pipeline grew back part way through: {counts}")