diff --git a/app/src/components/projects/home/FilesTab.test.tsx b/app/src/components/projects/home/FilesTab.test.tsx index 77f677c..e16f6f9 100644 --- a/app/src/components/projects/home/FilesTab.test.tsx +++ b/app/src/components/projects/home/FilesTab.test.tsx @@ -381,11 +381,13 @@ describe("FilesTab host drag-and-drop", () => { }); it("accepts a drop that lands on a toast floating over the pane", async () => { - // `ToastHost` is `fixed bottom-4 right-4 z-[60]` and 24rem wide, and its - // error cards stay until dismissed — so a z-order gate asking "is what is - // painted here part of my pane?" made the bottom-right corner of this pane - // refuse drops for as long as one error was on screen. jsdom has no - // `elementFromPoint`, so that branch only runs when a test supplies one. + // Round 1. `ToastHost` is `fixed bottom-4 right-4 z-[60]` and 24rem wide, + // and its error cards stay until dismissed — so a z-order gate asking "is + // what is painted here part of my pane?" made the bottom-right corner of + // this pane refuse drops for as long as one error was on screen. jsdom has + // no `elementFromPoint`, so that branch only ran when a test supplied one; + // the gate no longer asks, and this pins that nothing painted over a pane + // can refuse a drop on its own account. await renderTab(); const toastCard = document.createElement("div"); document.body.appendChild(toastCard); @@ -402,21 +404,37 @@ describe("FilesTab host drag-and-drop", () => { toastCard.remove(); }); - it("refuses a drop that lands on a dialog painted over the pane", async () => { + it("refuses a drop while a dialog is open, toast painted over it or not", async () => { + // Round 2, which is the reason this file exists in its current shape. The + // refusal pushes a toast; `ToastHost` is `z-[60]` and the `Modal` backdrop + // is `z-50` in the same stacking context, so the *toast* becomes the + // topmost element over a covered pane. A gate that asked `elementFromPoint` + // "is a blocker painted here?" then answered no and uploaded into the + // directory the dialog was covering — one refused drop was all it took to + // open the hole. Both stubs below therefore have to be refused. await renderTab(); const backdrop = document.createElement("div"); backdrop.setAttribute("data-blocks-drop", "true"); document.body.appendChild(backdrop); - Object.defineProperty(document, "elementFromPoint", { - configurable: true, - writable: true, - value: () => backdrop, - }); + const toastCard = document.createElement("div"); // z-[60], above the backdrop + document.body.appendChild(toastCard); + const stub = (top: Element) => + Object.defineProperty(document, "elementFromPoint", { + configurable: true, + writable: true, + value: () => top, + }); + stub(backdrop); await drop(["/host/a.png"], { x: 400, y: 300 }); expect(uploadFileToContainer).not.toHaveBeenCalled(); + stub(toastCard); + await drop(["/host/a.png"], { x: 700, y: 550 }); + expect(uploadFileToContainer).not.toHaveBeenCalled(); + delete (document as Partial).elementFromPoint; + toastCard.remove(); backdrop.remove(); }); diff --git a/app/src/components/projects/home/FilesTab.tsx b/app/src/components/projects/home/FilesTab.tsx index 32ff369..7994cc6 100644 --- a/app/src/components/projects/home/FilesTab.tsx +++ b/app/src/components/projects/home/FilesTab.tsx @@ -2,7 +2,7 @@ import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { getCurrentWebview } from "@tauri-apps/api/webview"; import type { FileEntry, Project } from "../../../lib/types"; import { useFileManager } from "../../../hooks/useFileManager"; -import { classifyDrop, isDropTarget } from "../../../lib/dropTarget"; +import { classifyDrop, isDropTarget, DROP_BLOCKED_TOAST } from "../../../lib/dropTarget"; import { useAppState } from "../../../store/appState"; import Button from "../../ui/Button"; import FileViewerModal from "./FileViewerModal"; @@ -240,11 +240,11 @@ export default function FilesTab({ project }: Props) { // reason `TerminalView` uses it: `dragDropEnabled` is on (the terminal needs // it), which blocks HTML5 drag inside the webview on Windows, and only the // native payload carries real file *paths*. The listener is window-wide, so - // routing is `classifyDrop` — the rect hit test *plus* the z-order question - // a rect cannot answer: is a modal or a blocking overlay painted at that - // point? (Not "is that element mine": chrome painted over a pane — a toast, - // a button — is not something that swallows a drop, and treating it as such - // made permanent dead zones.) + // routing is `classifyDrop` — the rect hit test, which says *whose* drop it + // is, plus the document-wide question a rect cannot answer: is a modal or a + // blocking overlay on screen at all? That second half is deliberately not a + // per-point z-order test; `lib/dropTarget.ts` records the two ways that went + // wrong. useEffect(() => { if (!running) return; let unlisten: (() => void) | undefined; @@ -267,16 +267,8 @@ export default function FilesTab({ project }: Props) { // Aimed at this pane and refused anyway: say so. Nothing else would — // the file just never appears in the listing. if (verdict === "blocked") { - console.warn( - "[drop] refused: an overlay is covering the drop point", - payload.position, - ); - useAppState.getState().pushToast({ - kind: "info", - message: "File drop ignored", - detail: - "A dialog or full-window overlay is covering that point. Close it and drop the file again.", - }); + console.warn("[drop] refused: a dialog or overlay is open", payload.position); + useAppState.getState().pushToast(DROP_BLOCKED_TOAST); return; } if (verdict !== "accept") return; diff --git a/app/src/components/terminal/TerminalView.test.tsx b/app/src/components/terminal/TerminalView.test.tsx index 819d4b7..900e388 100644 --- a/app/src/components/terminal/TerminalView.test.tsx +++ b/app/src/components/terminal/TerminalView.test.tsx @@ -292,9 +292,12 @@ describe("TerminalView — where a dropped file lands", () => { return view; } - /** jsdom has no `elementFromPoint`, so the z-order branch is unreachable + /** jsdom has no `elementFromPoint`, so a z-order branch is unreachable * unless a test supplies one — which is exactly how a gate that refused - * every drop under the Following toggle shipped through this file green. */ + * every drop under the Following toggle shipped through this file green. + * The gate asks no per-point question any more, but the tests below still + * install one and feed it the most misleading answer available, to pin + * that the routing does not change when it is there. */ function stubElementFromPoint(top: Element | null) { Object.defineProperty(document, "elementFromPoint", { configurable: true, @@ -369,7 +372,8 @@ describe("TerminalView — where a dropped file lands", () => { delete (document as Partial).elementFromPoint; }); - it("refuses — and says so — when a dialog is painted over the drop point", async () => { + it("refuses — and says so — while a dialog is open", async () => { + useAppState.setState({ toasts: [] }); await mountWithLayout(); const backdrop = document.createElement("div"); backdrop.setAttribute("data-blocks-drop", "true"); @@ -383,13 +387,52 @@ describe("TerminalView — where a dropped file lands", () => { expect(vi.mocked(uploadHostFileToTerminal)).not.toHaveBeenCalled(); // A refused drop is otherwise indistinguishable from a broken one. - expect( - useAppState.getState().toasts.some((t) => t.message === "File drop ignored"), - ).toBe(true); + const notice = useAppState + .getState() + .toasts.find((t) => t.message === "File drop ignored"); + expect(notice).toBeTruthy(); + // Not an error: the user has a dialog open, which is a state they chose + // and can leave with Escape. An error card would sit there until + // dismissed, and `ToastHost` paints at `z-[60]`. + expect(notice?.kind).toBe("info"); backdrop.remove(); delete (document as Partial).elementFromPoint; }); + + it("keeps refusing when the refusal's own toast is painted over the dialog", async () => { + // C1, end to end. Refusing pushes a toast; `ToastHost` is `fixed + // bottom-4 right-4 z-[60]` and the `Modal` backdrop is `z-50` in the same + // stacking context — so the toast is the topmost element over the covered + // pane, and a gate that asked "is a blocker painted here?" answered no and + // uploaded into the directory the dialog was covering. The gate had armed + // its own hole: one refused drop was all it took to open it. + useAppState.setState({ toasts: [] }); + await mountWithLayout(); + const backdrop = document.createElement("div"); + backdrop.setAttribute("data-blocks-drop", "true"); + document.body.appendChild(backdrop); + stubElementFromPoint(backdrop); + + await drop(400, 300); + expect(vi.mocked(uploadHostFileToTerminal)).not.toHaveBeenCalled(); + expect(useAppState.getState().toasts).toHaveLength(1); + + // The toast is now on screen, above the backdrop, and the user drops again + // on the very point it occupies. + const toastCard = document.createElement("div"); + document.body.appendChild(toastCard); + stubElementFromPoint(toastCard); + + await drop(700, 550); + expect(vi.mocked(uploadHostFileToTerminal)).not.toHaveBeenCalled(); + // …and a second refusal replaces the first notice rather than stacking. + expect(useAppState.getState().toasts).toHaveLength(1); + + toastCard.remove(); + backdrop.remove(); + delete (document as Partial).elementFromPoint; + }); }); describe("TerminalView — reaching the URL prompt without a mouse", () => { diff --git a/app/src/components/terminal/TerminalView.tsx b/app/src/components/terminal/TerminalView.tsx index 04ec48a..d9d1011 100644 --- a/app/src/components/terminal/TerminalView.tsx +++ b/app/src/components/terminal/TerminalView.tsx @@ -21,7 +21,7 @@ import { parseUrlRelayOsc, sanitizeRelayUrl, } from "../../lib/urlRelay"; -import { classifyDrop } from "../../lib/dropTarget"; +import { classifyDrop, DROP_BLOCKED_TOAST } from "../../lib/dropTarget"; import UrlToast, { URL_TOAST_PRIMARY_SELECTOR, URL_TOAST_SELECTOR, @@ -236,23 +236,22 @@ export default function TerminalView({ sessionId, active }: Props) { // onDragDropEvent (HTML5 ondrop on the element wouldn't expose file paths). // // The listener is window-wide, so every pane decides for itself whether a - // drop was meant for it. `isDropTarget` is that decision, shared with the - // Files pane: the payload position against this pane's rect (a hidden pane is - // `display:none`, so its zero-size rect is what stops two panes both claiming - // the drop), plus z-order — which a rect alone cannot see. An open `Modal` is - // a `fixed inset-0` portal painted *over* the window and the pane underneath - // still has its rect, so the geometric test that used to live here uploaded - // files into the directory a dialog was covering. Same for the shutdown - // overlay, which is on screen precisely while nothing should be accepting - // work at all. + // drop was meant for it. `classifyDrop` is that decision, shared with the + // Files pane, and it asks two things in order: is the payload position + // inside this pane's rect (a hidden pane is `display:none`, so its zero-size + // rect is what stops two panes both claiming the drop), and — document-wide, + // with no geometry — is a modal or blocking overlay on screen at all? An + // open `Modal` is a `fixed inset-0` portal painted *over* the window and the + // pane underneath still has its rect, so a rect alone uploaded files into + // the directory a dialog was covering. See `lib/dropTarget.ts` for why the + // blocking half is deliberately not a per-point z-order test. // // The rect asked about is the **pane wrapper**, not the xterm host inside it: // the pane is what the user sees as "the terminal", gutter included, and the // chrome painted over it (the Following toggle, the URL toast) is a sibling - // of the host rather than a child. `classifyDrop` answers "is a *blocking - // overlay* here?" rather than "is this element mine?" for the same reason — - // asking the second question turned every pixel under that chrome into a - // permanent dead zone. + // of the host rather than a child. Nothing painted over the pane refuses a + // drop on its own account — asking "is this element mine?" once turned every + // pixel under that chrome into a permanent dead zone. useEffect(() => { let unlisten: (() => void) | undefined; let cancelled = false; @@ -276,15 +275,10 @@ export default function TerminalView({ sessionId, active }: Props) { // sits above it. if (verdict === "blocked") { console.warn( - "[drop] refused: an overlay is covering the drop point", + "[drop] refused: a dialog or overlay is open", event.payload.position, ); - useAppState.getState().pushToast({ - kind: "info", - message: "File drop ignored", - detail: - "A dialog or full-window overlay is covering that point. Close it and drop the file again.", - }); + useAppState.getState().pushToast(DROP_BLOCKED_TOAST); return; } if (verdict !== "accept") return; diff --git a/app/src/components/ui/Modal.test.tsx b/app/src/components/ui/Modal.test.tsx index 2a85143..ecf72c1 100644 --- a/app/src/components/ui/Modal.test.tsx +++ b/app/src/components/ui/Modal.test.tsx @@ -109,9 +109,9 @@ describe("Modal", () => { // ------------------------------------------------------------------------- it("marks its backdrop as swallowing native file drops", async () => { - // The backdrop, not the panel, is what `elementFromPoint` returns for a - // drop released beside the dialog — so it is the element that has to carry - // the marker `lib/dropTarget` looks for. + // `lib/dropTarget` refuses every drop in the window while a dialog is on + // screen, and this marker is half of how it knows one is (the panel's + // `aria-modal` is the other half). render(

body

@@ -141,12 +141,16 @@ describe("Modal", () => { expect(backdrop.hidden).toBe(true); expect(backdrop.style.display).toBe("none"); expect(dropIsBlocked()).toBe(false); + // Two independent reasons it does not block, because they are maintained + // in two files: the marker is gone *and* `[hidden]` disqualifies it. + expect(backdrop.hasAttribute("data-blocks-drop")).toBe(false); expect(backdrop.contains(document.activeElement)).toBe(false); fireEvent.keyDown(document, { key: "Escape" }); expect(onClose).not.toHaveBeenCalled(); // Back on screen: the same dialog, still mounted, resumes everything. + // (Including the marker: it is dropped on hide, not written once at mount.) rerender( @@ -162,6 +166,46 @@ describe("Modal", () => { expect(onClose).toHaveBeenCalledTimes(1); }); + it("does not leave focus inside itself when its pane steps aside", async () => { + // The dialog goes `display:none` with the keyboard focus still inside it, + // and nothing else relocates it — so the user arrives on the tab they + // switched to with focus held by a dialog they cannot see, Tab resuming + // from inside it. jsdom does not blur on `display:none` either, so this + // is exactly the state the assertion below describes. + const { rerender } = render( + + + + + , + ); + await flushFocus(); + const panel = screen.getByRole("dialog"); + expect(panel.contains(document.activeElement)).toBe(true); + + rerender( + + + + + , + ); + await flushFocus(); + expect(panel.contains(document.activeElement)).toBe(false); + expect(document.activeElement).toBe(document.body); + + // …and coming back puts it where it was: inside the dialog. + rerender( + + + + + , + ); + await flushFocus(); + expect(screen.getByRole("dialog").contains(document.activeElement)).toBe(true); + }); + it("ignores Escape and overlay clicks when not dismissible", async () => { const onClose = vi.fn(); render( diff --git a/app/src/components/ui/Modal.tsx b/app/src/components/ui/Modal.tsx index d373f84..7755a4b 100644 --- a/app/src/components/ui/Modal.tsx +++ b/app/src/components/ui/Modal.tsx @@ -70,7 +70,7 @@ export default function Modal({ // A dialog portals to `document.body`, so the `hidden` class its pane uses to // step aside for another tab cannot reach it. `PaneVisibility` is how it // finds out, and while it is false this dialog paints nothing, traps - // nothing, and — via `[hidden]` — blocks no native file drop. + // nothing, holds no focus, and blocks no native file drop. const paneVisible = usePaneVisible(); const paneVisibleRef = useRef(paneVisible); paneVisibleRef.current = paneVisible; @@ -85,11 +85,19 @@ export default function Modal({ }; }, []); - // Move focus inside — on mount, and again whenever the pane comes back. + // Move focus inside — on mount, and again whenever the pane comes back. And + // move it *out* when the pane steps aside: the backdrop goes `display:none` + // with the keyboard focus still inside it, and nothing else relocates it, so + // the user lands on the new tab with focus held by a dialog they cannot see. + // Blurring puts it on ``, which is where a fresh Tab starts. useEffect(() => { - if (!paneVisible) return; const panel = panelRef.current; if (!panel) return; + if (!paneVisible) { + const active = panel.ownerDocument.activeElement as HTMLElement | null; + if (active && panel.contains(active)) active.blur?.(); + return; + } const target = initialFocusRef?.current ?? focusableWithin(panel)[0] ?? panel; // Defer so the panel is laid out (offsetParent) before we query it. const frame = requestAnimationFrame(() => target.focus?.()); @@ -151,10 +159,11 @@ export default function Modal({ ref={overlayRef} onClick={handleOverlayClick} className="fixed inset-0 bg-black/60 flex items-center justify-center z-50 p-4" - /* The backdrop, not the panel, is what `elementFromPoint` returns for a - drop released beside the dialog — so it is the element that has to say - "I swallow drops". See `lib/dropTarget.ts`. */ - data-blocks-drop="true" + /* This dialog swallows native file drops for as long as it is on screen. + Dropped while the owning pane is hidden, so a dialog parked on another + tab does not keep refusing drops here — `lib/dropTarget.ts` also + filters `[hidden]`, and these two must not disagree. */ + data-blocks-drop={paneVisible ? "true" : undefined} hidden={!paneVisible} aria-hidden={paneVisible ? undefined : true} /* `hidden` is a base-layer rule and `flex` is a utility-layer one, so the diff --git a/app/src/lib/dropTarget.test.ts b/app/src/lib/dropTarget.test.ts index 876913c..c173592 100644 --- a/app/src/lib/dropTarget.test.ts +++ b/app/src/lib/dropTarget.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, beforeEach, afterEach } from "vitest"; +import { describe, expect, it, beforeEach, afterEach, vi } from "vitest"; import { classifyDrop, dropIsBlocked, isDropTarget } from "./dropTarget"; function pane(rect: Partial): HTMLElement { @@ -23,16 +23,22 @@ function pane(rect: Partial): HTMLElement { /** * **jsdom has no `elementFromPoint`.** That single fact is how a z-order gate * that refused every drop under a button, a toast and a dialog shipped green - * through 81 drop tests: the branch was never entered, in any of them. So the - * tests below install one. A test that passes because the environment lacks - * the API under test is not a test. + * through 81 drop tests: the branch was never entered, in any of them. + * + * The gate no longer asks a per-point question at all, so the gap can no + * longer hide anything here — but the tests below still *install* an + * `elementFromPoint` and hand it the most misleading answer available, because + * "the module ignores it" is now a property worth pinning. `spy.mock.calls` + * proves it directly. */ -function stubElementFromPoint(top: Element | null): void { +function stubElementFromPoint(top: Element | null): ReturnType { + const spy = vi.fn(() => top); Object.defineProperty(document, "elementFromPoint", { configurable: true, writable: true, - value: () => top, + value: spy, }); + return spy; } function removeElementFromPoint(): void { @@ -125,148 +131,193 @@ describe("dropTarget", () => { }); // --------------------------------------------------------------------- - // Z-order — the branch jsdom cannot reach on its own + // The gate itself: document-wide, and provably not geometric // --------------------------------------------------------------------- - describe("z-order, with elementFromPoint actually present", () => { - it("confirms the environment gap this whole block exists for", () => { - // If jsdom ever grows layout, this fails and the stubs below can be - // reconsidered — but until then, *nothing* reaches the z-order branch - // unless a test puts the API there itself. - expect(typeof document.elementFromPoint).toBe("undefined"); - }); - - it("accepts a drop onto chrome the pane paints over itself", () => { - // The regression. `TerminalView`'s "▼ Following / ▽ Paused" toggle is - // `absolute top-2 right-4 z-50` and is rendered *unconditionally*, as a - // sibling of the xterm host rather than a child — so a gate asking "does - // the pane contain what is painted here?" turned the terminal's top-right - // corner into a dead zone that no user action could clear. - const el = pane({}); - const following = document.createElement("button"); - document.body.appendChild(following); // sibling, not a child of the pane - stubElementFromPoint(following); - - expect(classifyDrop(el, { x: 90, y: 5 }, { devicePixelRatio: 1 })).toBe("accept"); - }); - - it("accepts a drop onto a toast floating above every pane", () => { - // `ToastHost` is `fixed bottom-4 right-4 z-[60]`, 24rem wide, and its - // error cards never time out — so under the containment rule the - // bottom-right corner of both the terminal and the Files pane stopped - // accepting drops for as long as one error stayed on screen. - const el = pane({}); - const toastCard = document.createElement("div"); - document.body.appendChild(toastCard); - stubElementFromPoint(toastCard); - - expect(classifyDrop(el, { x: 95, y: 95 }, { devicePixelRatio: 1 })).toBe("accept"); - }); - - it("accepts a drop onto the pane's own content", () => { + describe("the blocking gate is document-wide, with no z-order in it", () => { + it("never consults elementFromPoint, however tempting its answer", () => { + // Round 2 asked `elementFromPoint` whether a *blocker* was painted at + // the drop point, and trusted the answer absolutely. Anything painted + // above the `z-50` backdrop in the same stacking context — `ToastHost` + // at `z-[60]`, `TerminalContextMenu` at `z-[60]` — answered "no blocker + // here" on a point the dialog was covering. The fix is not a better + // answer, it is not asking: this pins that the call is gone, so a + // future edit that reintroduces it fails here rather than in the wild. const el = pane({}); const child = document.createElement("span"); el.appendChild(child); - stubElementFromPoint(child); + const spy = stubElementFromPoint(child); expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("accept"); + expect(spy).not.toHaveBeenCalled(); }); - it("refuses a drop released onto a dialog's backdrop", () => { + it("refuses a covered drop even when a toast is painted over the dialog", () => { + // C1, exactly. A refused drop pushes a toast; `ToastHost` is + // `fixed bottom-4 right-4 z-[60]` and an error card stays until + // dismissed; the *next* drop released on that card had a topmost + // element with no blocker in it, and landed in the directory the dialog + // was covering. The gate had armed its own hole. + const el = pane({}); + openModal(); // z-50 backdrop, covering the pane + const toastCard = document.createElement("div"); // z-[60], over the backdrop + document.body.appendChild(toastCard); + const spy = stubElementFromPoint(toastCard); + + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); + expect(spy).not.toHaveBeenCalled(); + }); + + it("refuses a drop anywhere in the window while a dialog is open", () => { + // Deliberately stricter than "the points the dialog covers". A dialog + // is a state the user entered on purpose and leaves with Escape, the + // refusal is announced, and nothing is written — whereas being precise + // about coverage has silently uploaded into a covered directory twice. const el = pane({}); const { backdrop } = openModal(); - stubElementFromPoint(backdrop); + stubElementFromPoint(document.createElement("div")); // "nothing here" - expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); + expect(classifyDrop(el, { x: 5, y: 5 }, { devicePixelRatio: 1 })).toBe("blocked"); + expect(classifyDrop(el, { x: 95, y: 95 }, { devicePixelRatio: 1 })).toBe("blocked"); + + backdrop.remove(); + expect(classifyDrop(el, { x: 5, y: 5 }, { devicePixelRatio: 1 })).toBe("accept"); }); - it("refuses a drop released onto the dialog panel or anything inside it", () => { + it("refuses under the shutdown overlay, which is not a dialog", () => { const el = pane({}); - const { panel, button } = openModal(); - - stubElementFromPoint(panel); - expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); - - stubElementFromPoint(button); + const overlay = document.createElement("div"); + overlay.setAttribute("data-blocks-drop", "true"); + document.body.appendChild(overlay); expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); }); - it("refuses a drop under an unmarked overlay that wraps a dialog", () => { - // Belt to the braces: a dialog built without `ui/Modal`'s marked - // backdrop is still refused, because the element painted at the point - // *contains* something modal. + it("still refuses a dialog built without ui/Modal's marked backdrop", () => { + // Only `aria-modal` is needed; `ui/Modal` is the supported route, but a + // hand-rolled dialog must not be a hole either. const el = pane({}); const backdrop = document.createElement("div"); const panel = document.createElement("div"); panel.setAttribute("aria-modal", "true"); backdrop.appendChild(panel); document.body.appendChild(backdrop); - stubElementFromPoint(backdrop); expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); }); - it("refuses a drop under the shutdown overlay", () => { + it("counts a blocker that is only decoratively hidden from assistive tech", () => { + // `aria-hidden="true"` used to disqualify a blocker, and it is not a + // visibility statement — `ui/Modal`'s own ✕ glyph carries it while + // perfectly visible. A blocker nested inside such a wrapper would have + // silently stopped blocking. Only `[hidden]` counts now. const el = pane({}); + const wrapper = document.createElement("div"); + wrapper.setAttribute("aria-hidden", "true"); const overlay = document.createElement("div"); overlay.setAttribute("data-blocks-drop", "true"); - document.body.appendChild(overlay); - stubElementFromPoint(overlay); + wrapper.appendChild(overlay); + document.body.appendChild(wrapper); + expect(dropIsBlocked()).toBe(true); expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); }); - it("scopes a dialog's refusal to the points it actually covers", () => { - // `dropIsBlocked` is document-wide and `ui/Modal` portals to - // `document.body`, so an open dialog used to refuse every drop in the - // window. Asked per point, a dialog only swallows what lands on it. - const el = pane({}); - openModal(); - const paneContent = document.createElement("span"); - el.appendChild(paneContent); - stubElementFromPoint(paneContent); - - expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("accept"); - }); - it("ignores a blocker whose pane has stepped aside", () => { // `ui/Modal` marks itself `hidden` when the tab that owns it is not the // one on screen. A dialog left open in project A must not keep refusing - // drops in project B. + // drops in project B — this is the one case where over-refusal would be + // unbounded, since the user cannot see the dialog to close it. const el = pane({}); const { backdrop } = openModal(); backdrop.setAttribute("hidden", ""); expect(dropIsBlocked()).toBe(false); - stubElementFromPoint(backdrop); expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("accept"); }); + }); - it("falls back to the document-wide question when the point cannot be resolved", () => { - // `elementFromPoint` answers `null` for a point outside the viewport, and - // `` for a point over nothing in particular. Neither is evidence - // that the pane is clear, so the conservative answer is the old one. + // --------------------------------------------------------------------- + // Round 1's shape: chrome painted over a pane must never refuse a drop + // --------------------------------------------------------------------- + + describe("chrome over a pane, with no dialog open", () => { + /** Everything that is painted over a pane and is not a blocker. */ + const CHROME: Array<[string, () => HTMLElement]> = [ + // `TerminalView`'s "▼ Following / ▽ Paused" toggle: `absolute top-2 + // right-4 z-50`, rendered unconditionally, and a *sibling* of the xterm + // host — so "does the pane contain what is painted here?" made the + // terminal's top-right corner a dead zone no user action could clear. + ["the Following/Paused toggle", () => document.createElement("button")], + // `ToastHost`: `fixed bottom-4 right-4 z-[60]`, 24rem wide, over every + // pane, and its error cards stay until dismissed. + ["a toast card", () => document.createElement("div")], + // The drop hint is `pointer-events-none`, so a real `elementFromPoint` + // skips it — but nothing may depend on that any more. + ["the pane's own drop hint", () => document.createElement("div")], + // `Tooltip` portals to `document.body`, so it is nobody's child. + ["a portaled tooltip", () => document.createElement("div")], + ]; + + for (const [name, make] of CHROME) { + it(`accepts a drop released onto ${name}`, () => { + const el = pane({}); + const chrome = make(); + document.body.appendChild(chrome); // a sibling, not a child of the pane + stubElementFromPoint(chrome); + + expect(classifyDrop(el, { x: 90, y: 5 }, { devicePixelRatio: 1 })).toBe("accept"); + expect(classifyDrop(el, { x: 95, y: 95 }, { devicePixelRatio: 1 })).toBe("accept"); + }); + } + + it("accepts a drop on the gutter around the terminal, and on its content", () => { const el = pane({}); - openModal(); + const child = document.createElement("span"); + el.appendChild(child); + stubElementFromPoint(child); + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("accept"); - stubElementFromPoint(null); - expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); - - stubElementFromPoint(document.body); - expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); + stubElementFromPoint(el); // the gutter: the wrapper itself is topmost + expect(classifyDrop(el, { x: 1, y: 99 }, { devicePixelRatio: 1 })).toBe("accept"); }); - it("tells a refused drop apart from someone else's drop", () => { - // The caller reports one and stays silent about the other: a drop that - // was aimed at this pane and swallowed by an overlay is invisible unless - // something says so, while a drop into another pane is not ours to - // narrate. + it("accepts a drop the view could not resolve to any element", () => { + // `elementFromPoint` answers `null` outside the viewport and `` + // over nothing in particular. With no dialog open neither is a reason + // to refuse a drop that is inside the pane's rect. const el = pane({}); - const { backdrop } = openModal(); - stubElementFromPoint(backdrop); + stubElementFromPoint(null); + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("accept"); + stubElementFromPoint(document.body); + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("accept"); + }); + }); - expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); - expect(classifyDrop(el, { x: 900, y: 900 }, { devicePixelRatio: 1 })).toBe("elsewhere"); + // --------------------------------------------------------------------- + // Routing: exactly one listener speaks for a given drop + // --------------------------------------------------------------------- + + describe("routing", () => { + it("lets only the pane the drop landed on report a refusal", () => { + // Geometry is asked before the gate for this reason: both listeners are + // live for every drop, and if the gate came first they would both push + // "File drop ignored" for one drop. + const hit = pane({}); + const missed = pane({ left: 200, right: 300, top: 200, bottom: 300 }); + openModal(); + + expect(classifyDrop(hit, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); + expect(classifyDrop(missed, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe( + "elsewhere", + ); + }); + + it("says elsewhere, not blocked, for a hidden pane's zero-size rect", () => { + const hidden = pane({ right: 0, bottom: 0, width: 0, height: 0 }); + openModal(); + expect(classifyDrop(hidden, { x: 0, y: 0 }, { devicePixelRatio: 1 })).toBe( + "elsewhere", + ); }); }); diff --git a/app/src/lib/dropTarget.ts b/app/src/lib/dropTarget.ts index e4a3266..9fae17c 100644 --- a/app/src/lib/dropTarget.ts +++ b/app/src/lib/dropTarget.ts @@ -2,47 +2,58 @@ * Routing for Tauri's *native* drag-drop event. * * The listener is window-wide — every pane that wants dropped file paths gets - * the same event — so each one decides for itself whether the drop was meant - * for it. That decision used to be purely geometric: is the payload position - * inside my rect? A rect is not what the user sees, though. An open `Modal` is - * a `fixed inset-0` portal at `z-50` painted *over* the whole window, and the - * pane underneath still had its rect, so releasing a drag onto a dialog - * uploaded the file into the directory the dialog was covering. Same for the - * shutdown overlay, which is on screen precisely while nothing should be - * accepting work at all. + * the same event — so the module answers two separate questions, and keeping + * them separate is the whole design: * - * So the hit test is: the point is inside my rect, **and** nothing that - * *swallows* drops is painted at that point. + * 1. **Which pane is this drop for?** Geometry, and nothing else: is the + * payload position inside my rect? A hidden pane is `display:none` and so + * has a zero-size rect, which is what stops `TerminalView` and `FilesTab` + * both claiming the same drop. + * 2. **Should the app accept a drop at all right now?** `dropIsBlocked` — + * document-wide, no geometry, no z-order. While a modal or a blocking + * overlay is on screen anywhere, every drop is refused. * - * ## The question the z-order test asks — and the one it must not ask + * ## Why there is no z-order test here, and must not be one * - * The first version of this asked `el.contains(document.elementFromPoint(x,y))` - * — "is the thing painted here mine?" That is the wrong question, and it - * created permanent dead zones. Panes have chrome painted *over* them that is - * not part of the element handed to this function and does not swallow - * anything: `TerminalView`'s always-present "▼ Following" button, the URL - * toast, and `ToastHost`'s bottom-right stack — which is `fixed` at `z-[60]` - * over *every* pane and whose error cards stay until dismissed. Under the - * containment rule a drop onto any of them was silently refused, forever. + * A drop that lands underneath a dialog and silently uploads into the + * directory the dialog is covering is the failure mode that matters: it is + * invisible, it writes to the container, and the user did not ask for it. + * Every attempt to be *precise* about which points a dialog covers has gone + * wrong, twice, in opposite directions: * - * The question that matches the intent is "is a *blocking overlay* painted - * here?". A button, a toast or a tooltip over the pane is not one; a modal - * backdrop is. Anything else painted at the point belongs to the pane's own - * subtree or is chrome that is happy for the drop to fall through to it. + * - Asking `el.contains(document.elementFromPoint(x, y))` — "is the thing + * painted here mine?" — refused drops onto anything painted *over* a pane + * that is not part of it: `TerminalView`'s always-rendered "▼ Following" + * toggle (a sibling of the xterm host), the URL toast, `ToastHost`'s stack. + * Permanent dead zones no user action could clear. + * - Replacing that with "is a *blocking overlay* painted here?" removed the + * dead zones and opened a hole instead. `elementFromPoint` returns the + * topmost painted element, and plenty of things paint above a `z-50` modal + * backdrop in the same stacking context: `ToastHost` is `z-[60]`, so is + * `TerminalContextMenu`. A refused drop pushed a toast; the toast then sat + * over the dialog; the next drop released on that toast was reported as + * "clear" and landed in the covered directory. The gate armed its own hole. * - * That rule is also what *scopes* the blocking question. `dropIsBlocked` is - * document-wide, and `ui/Modal` portals to `document.body`, so any dialog - * anywhere used to refuse every drop in the window. Asking per-point means a - * dialog only refuses the points it actually covers. + * Both bugs are the same mistake: trusting a per-point answer to decide + * whether the app should be accepting work at all. The document-wide question + * has no z-index in it, so no future overlay can become a drop hole by being + * painted high enough, and no chrome can become a dead zone by being painted + * at all. + * + * What it costs: while any dialog is open, drops are refused *everywhere*, + * including on parts of a pane the dialog does not cover. That is a state the + * user put the app in deliberately and can leave in one keystroke, the + * refusal is announced, and nothing is written. It is strictly the better + * failure. * * ## jsdom * - * jsdom implements no layout and has no `elementFromPoint`, so the z-order - * branch cannot be exercised by simply rendering — which is exactly how the - * containment bug shipped green through 81 drop tests. Every test that cares - * about z-order therefore stubs `elementFromPoint` (see `dropTarget.test.ts`), - * and the fallback below — when there is no such API, or it cannot resolve the - * point — is the conservative document-wide question this used to ask. + * Note for future changes: jsdom implements no layout and has no + * `elementFromPoint`, which is how the first of those two bugs shipped green + * through 81 drop tests — the branch was never entered in any of them. This + * module no longer calls it (`dropTarget.test.ts` asserts that it does not), + * so the gap can no longer hide a bug here. Anything that reintroduces a + * geometric z-order test reintroduces the gap as well. */ export interface DropPoint { @@ -51,24 +62,30 @@ export interface DropPoint { } /** - * Anything that swallows a drop wherever it lands. + * Anything that swallows drops while it is on screen. * * `[aria-modal="true"]` is every dialog in the app for free — `ui/Modal` is * the only way one is built, and it sets that attribute. `data-blocks-drop` * is for full-window overlays that are not dialogs (the shutdown overlay), and - * `ui/Modal` puts it on its backdrop as well: the backdrop is what - * `elementFromPoint` returns for a point outside the dialog panel, and it is - * the element that is really covering the pane. + * `ui/Modal` puts it on its backdrop as well. */ const BLOCKING_SELECTOR = '[aria-modal="true"],[data-blocks-drop="true"]'; /** - * A blocker inside one of these is in the DOM but not on screen — `ui/Modal` - * marks itself this way when the pane that owns it is not the visible one, so - * a dialog left open in project A stops covering project B the moment the tab - * changes. + * A blocker inside this is in the DOM but not on screen — `ui/Modal` marks + * itself `hidden` when the pane that owns it is not the visible one, so a + * dialog left open in project A stops refusing drops in project B the moment + * the tab changes. + * + * `[hidden]` only, deliberately. `aria-hidden="true"` used to count too, and + * it is not a visibility statement: it is routinely put on *visible* + * decorative content (`ui/Modal`'s own ✕ glyph, every `StatusIndicator` + * dot). An overlay that happened to sit inside such a wrapper would have + * silently stopped blocking — the exact class of hole this gate exists to + * close. `ui/Modal` sets `hidden`, the `hidden` attribute, and inline + * `display:none` together, so nothing in the app depended on the aria half. */ -const OFFSCREEN_SELECTOR = '[hidden],[aria-hidden="true"]'; +const OFFSCREEN_SELECTOR = "[hidden]"; /** A blocker that is actually painted, rather than merely mounted. */ function isOnScreen(el: Element): boolean { @@ -80,6 +97,22 @@ export function dropIsBlocked(doc: Document = document): boolean { return Array.from(doc.querySelectorAll(BLOCKING_SELECTOR)).some(isOnScreen); } +/** + * What both listeners say when they refuse a drop. + * + * `kind: "info"`, so it times out on its own: a drop refused because the user + * has a dialog open is expected behaviour, not an error, and an error card + * would sit on screen until dismissed. `dedupeKey` means three refused drops + * leave one notice rather than a stack of three. + */ +export const DROP_BLOCKED_TOAST = { + kind: "info", + message: "File drop ignored", + detail: + "A dialog or full-window overlay is open, so nothing accepts dropped files. Close it and drop again.", + dedupeKey: "drop-blocked", +} as const; + export interface DropTargetOptions { doc?: Document; /** Override the ratio used to convert physical pixels to CSS pixels. */ @@ -99,10 +132,8 @@ export interface DropTargetOptions { * (`wry/src/webview2/drag_drop.rs`), while the macOS and GTK backends deliver * logical points and `tauri-runtime-wry`'s forwarding does not rescale them. * Dividing by `devicePixelRatio` unconditionally therefore halved every drop - * position on a HiDPI Mac or Linux box — which used to be a silent - * mis-aimed-but-usually-still-inside-the-pane error and, with a z-order test - * in place, becomes a drop refused because the *halved* point lands on - * something else. + * position on a HiDPI Mac or Linux box, aiming the hit test at a point the + * user never touched. * * Verified by reading the wry/tauri sources named above. **Not** verified on a * real HiDPI macOS or GTK machine — neither is available here — which is why @@ -120,15 +151,15 @@ function payloadIsPhysical( /** * Why a native drop at `pos` did or did not belong to `el`. * - * - `accept` — it is ours. - * - `blocked` — it landed on our rect, but a modal or a blocking overlay is - * painted there and swallowed it. Worth *saying* to the user: the drop - * visibly did nothing. + * - `accept` — it is ours, and the app is in a state to take it. + * - `blocked` — it was aimed at us, but a modal or a blocking overlay is on + * screen. Worth *saying* to the user: the drop visibly did nothing. * - `elsewhere` — not our drop. Silence is the right response; some other - * pane's listener is about to accept it. + * pane's listener may be about to accept it. * - * A hidden pane is `display:none` and therefore has a zero-size rect, which is - * what stops two panes both claiming the same drop. + * Geometry is asked **first**, so exactly one pane can ever answer `blocked` + * for a given drop and the refusal is announced once rather than once per + * listener. */ export type DropVerdict = "accept" | "blocked" | "elsewhere"; @@ -152,41 +183,11 @@ export function classifyDrop( return "elsewhere"; } - // Z-order, where the environment can answer it. `elementFromPoint` skips - // `pointer-events: none`, so the pane's own decorative drop hint does not - // count as something covering it. - const top = - typeof doc.elementFromPoint === "function" ? doc.elementFromPoint(x, y) : null; - if (top && top !== doc.body && top !== doc.documentElement) { - return coveredByBlocker(top, doc) ? "blocked" : "accept"; - } - - // No layout information — jsdom, or a point the view could not resolve. Fall - // back to the document-wide question, which is the conservative answer: a - // dialog somewhere refuses everything rather than risking a drop landing - // underneath one. + // Whose drop it is has been settled. Whether the app should be taking drops + // at all is a separate, document-wide question — see the header. return dropIsBlocked(doc) ? "blocked" : "accept"; } -/** - * Is the element painted at the drop point part of something that swallows - * drops? - * - * Two directions, because a dialog is two elements: the panel carries - * `aria-modal`, and the backdrop around it is what is painted over the pane. - * `ui/Modal` marks its own backdrop, so `closest` covers both; the `contains` - * half is the safety net for any overlay that wraps a dialog without marking - * itself, and is deliberately not asked of ``/`` — those contain - * every portal in the app and would make the answer "blocked" always. - */ -function coveredByBlocker(top: Element, doc: Document): boolean { - const nearest = top.closest(BLOCKING_SELECTOR); - if (nearest && isOnScreen(nearest)) return true; - if (top === doc.body || top === doc.documentElement) return false; - const inside = top.querySelector(BLOCKING_SELECTOR); - return inside !== null && isOnScreen(inside); -} - /** * Whether a native drop at `pos` (physical pixels on Windows, logical * elsewhere) belongs to `el`. diff --git a/app/src/store/appState.test.ts b/app/src/store/appState.test.ts index dae5658..ddd9027 100644 --- a/app/src/store/appState.test.ts +++ b/app/src/store/appState.test.ts @@ -77,3 +77,38 @@ describe("tab reordering", () => { expect(useAppState.getState().activeTabKey).toBe(C); }); }); + +describe("toasts", () => { + beforeEach(() => useAppState.setState({ toasts: [] })); + + const toasts = () => useAppState.getState().toasts; + + it("stacks unkeyed toasts, which are one-off reports", () => { + const { pushToast } = useAppState.getState(); + pushToast({ kind: "error", message: "one" }); + pushToast({ kind: "error", message: "two" }); + expect(toasts().map((t) => t.message)).toEqual(["one", "two"]); + }); + + it("lets a keyed toast supersede the one already on screen", () => { + // A recurring notice — "File drop ignored" is the one that exists — must + // not build a wall of identical cards when the user tries three times. + const { pushToast } = useAppState.getState(); + const first = pushToast({ kind: "info", message: "ignored", dedupeKey: "drop-blocked" }); + pushToast({ kind: "error", message: "unrelated" }); + const second = pushToast({ kind: "info", message: "ignored", dedupeKey: "drop-blocked" }); + + expect(toasts().map((t) => t.message)).toEqual(["unrelated", "ignored"]); + // A fresh id, so the card re-mounts and its dismissal timer restarts + // rather than the replacement inheriting the first one's remaining time. + expect(second).not.toBe(first); + expect(toasts().some((t) => t.id === first)).toBe(false); + }); + + it("keeps toasts with different keys apart", () => { + const { pushToast } = useAppState.getState(); + pushToast({ kind: "info", message: "a", dedupeKey: "x" }); + pushToast({ kind: "info", message: "b", dedupeKey: "y" }); + expect(toasts()).toHaveLength(2); + }); +}); diff --git a/app/src/store/appState.ts b/app/src/store/appState.ts index 29a1f1c..8dc1b45 100644 --- a/app/src/store/appState.ts +++ b/app/src/store/appState.ts @@ -47,6 +47,12 @@ export interface Toast { message: string; /** Long text (e.g. a bollard error) shown behind a "Details" disclosure. */ detail?: string; + /** + * Optional identity for a *recurring* notice. Pushing another toast with the + * same key replaces the one already on screen instead of stacking a second + * copy of it — three refused file drops leave one card, not three. + */ + dedupeKey?: string; } let toastCounter = 0; @@ -355,7 +361,14 @@ export const useAppState = create((set) => ({ toasts: [], pushToast: (toast) => { const id = `toast-${++toastCounter}`; - set((state) => ({ toasts: [...state.toasts, { ...toast, id }] })); + set((state) => { + // A keyed toast supersedes the previous one with that key. The new card + // gets a new id, so it re-mounts and its dismissal timer restarts. + const kept = toast.dedupeKey + ? state.toasts.filter((t) => t.dedupeKey !== toast.dedupeKey) + : state.toasts; + return { toasts: [...kept, { ...toast, id }] }; + }); return id; }, dismissToast: (id) =>