From a41d93ea46e9b02f06965e2bc0eb67c55c501333 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Tue, 11 Aug 2026 07:40:51 -0700 Subject: [PATCH] Fix the review's findings: drag on pointer events, read the window back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ten findings from the review of the previous commit, all applied. **The tab drag is now pointer events, not HTML5 drag-and-drop.** Two independent reasons, either one fatal. Tauri's `dragDropEnabled` blocks HTML5 drag inside the webview on Windows, and it cannot just be turned off — `TerminalView` needs Tauri's native drag-drop event, which is the only one that carries dropped *file paths*. And an HTML5 drag carries a `DataTransfer`: released over any text field in the app, the default handler types `term:` into it, and in Config that is then saved with the project. Pointer events have neither problem, and the drag is measured from the tabs on screen rather than from the event target, so the marker and the drop agree even over the marker itself. Escape abandons a drag; a press under 4px stays a click; the click that ends a drag does not select. **`Ctrl+Shift+←/→` no longer swallows word-wise selection.** It is bound on `document` in the capture phase, so in any input — the rename field, Config, Settings — it was taking the OS's extend-selection chord *and* silently reordering the strip. Guarded by `inTextField()`, which excludes xterm's helper textarea: that is an input-method shim, and the terminal is where the shortcut matters most. **The pop-out's state is read from the window, never remembered.** The pane is unmounted whenever another Project Home sub-tab is selected, so "Keep on top" came back Off over a window still floating on top. `get_browser_view_popout_state` returns both facts from the window itself, and the change event carries them. `poppedOut` is tri-state: until the answer arrives the iframe is not mounted, because guessing "not popped out" is what flashes a second viewer onto the browser. Also: `popout::close` and the off-status emit in the supervisor are behind the same epoch guard as the deregistration above them, so a supervisor whose teardown outlives a restart can no longer destroy the *new* session's window; `close()` returns its `destroy()` error instead of logging it and reporting success, since the pane restores its iframe on success; the drop marker is `pointer-events-none` and is placed before the first *visible* tab at or past the slot, so it neither refuses a drop nor vanishes when a `tabOrder` entry renders nothing; and the "Keep on top" Toggle's accessible name now matches its visible text. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 8 + HOW-TO-USE.md | 9 +- app/src-tauri/src/browser_view/commands.rs | 20 +- app/src-tauri/src/browser_view/mod.rs | 24 ++- app/src-tauri/src/browser_view/popout.rs | 73 ++++++-- app/src-tauri/src/lib.rs | 2 +- app/src/components/layout/MainTabs.test.tsx | 163 +++++++++++------ app/src/components/layout/MainTabs.tsx | 171 +++++++++++------- .../projects/home/BrowserTab.test.tsx | 41 ++++- .../components/projects/home/BrowserTab.tsx | 53 ++++-- app/src/hooks/useKeyboardShortcuts.test.tsx | 88 +++++++++ app/src/hooks/useKeyboardShortcuts.ts | 23 ++- app/src/lib/tauri-commands.ts | 14 +- app/src/lib/types.ts | 18 +- 14 files changed, 522 insertions(+), 185 deletions(-) create mode 100644 app/src/hooks/useKeyboardShortcuts.test.tsx diff --git a/CLAUDE.md b/CLAUDE.md index 0d98103..8d1fbf4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -62,6 +62,14 @@ docker exec stdout → tokio task → emit("terminal-output-{sessionId}") → li `tabOrder` is user-reorderable (drag, or `Ctrl+Shift+←/→` via `moveActiveTab`) — so **never treat a tab's position as identity**: address tabs by key, and index only through `tabOrder`. `moveTab` deliberately does not activate what it moves. + - **The tab drag is pointer events, not HTML5 drag-and-drop, and must stay that way.** Tauri's + `dragDropEnabled` blocks HTML5 drag inside the webview on Windows, and it cannot simply be + turned off: `TerminalView` needs Tauri's native drag-drop event because it is the only one + that carries dropped *file paths*. An HTML5 drag also carries a `DataTransfer`, which the + default handler types into any text field the drag is released over. + - **A new app-level shortcut must not swallow a text-editing chord.** `useKeyboardShortcuts` + binds on `document` in the capture phase, so `inTextField()` guards the arrow bindings — + excluding xterm's helper textarea, which is an input-method shim rather than a field. - **`hooks/`** — All Tauri IPC calls are encapsulated in hooks (`useTerminal`, `useProjects`, `useDocker`, `useSettings`) - **`lib/tauri-commands.ts`** — Typed `invoke()` wrappers; TypeScript types in `lib/types.ts` must match Rust models - **`components/terminal/TerminalView.tsx`** — xterm.js integration with WebGL rendering, URL detection for OAuth flow diff --git a/HOW-TO-USE.md b/HOW-TO-USE.md index 335f744..f39e058 100644 --- a/HOW-TO-USE.md +++ b/HOW-TO-USE.md @@ -192,10 +192,11 @@ Anthropic-backend project uses that token without its own login. See There is no separate terminal tab bar and no "+" button — tabs appear when you open a project or a terminal. - **Drag a tab to reorder it.** A line shows where it will land, and dropping it does not change - which tab you are looking at — so you can rearrange the strip without pulling focus away from a - terminal that is mid-run. `Ctrl+Shift+←` and `Ctrl+Shift+→` move the *active* tab the same way - without the mouse. The order is per-session: it is not saved when you quit. + **Drag a tab to reorder it.** A line shows where it will land; **Escape** abandons the drag. + Dropping does not change which tab you are looking at — so you can rearrange the strip without + pulling focus away from a terminal that is mid-run. `Ctrl+Shift+←` and `Ctrl+Shift+→` move the + *active* tab the same way without the mouse (they leave text fields alone, where that chord + still selects by word). The order is per-session: it is not saved when you quit. - **Status indicators (top right)** — Docker connection and container image availability. Each pairs a coloured dot with a word, so status is never conveyed by colour alone. The **?** button opens the built-in help. diff --git a/app/src-tauri/src/browser_view/commands.rs b/app/src-tauri/src/browser_view/commands.rs index aa20c38..f52de7b 100644 --- a/app/src-tauri/src/browser_view/commands.rs +++ b/app/src-tauri/src/browser_view/commands.rs @@ -128,23 +128,29 @@ pub async fn open_browser_view_popout( } /// Close the pop-out, putting the view back in the tab. No-op if it is closed. +/// +/// Propagates a failed close rather than reporting success: the pane restores +/// its iframe on success, and doing that with the window still up puts two +/// viewers on one browser. #[tauri::command] pub async fn close_browser_view_popout( project_id: String, app_handle: AppHandle, ) -> Result<(), String> { - popout::close(&app_handle, &project_id); - Ok(()) + popout::close(&app_handle, &project_id) } -/// Whether the pop-out is open — asked on tab open, since a window can outlive -/// the pane that spawned it. +/// Whether the pop-out is open, and whether it is pinned on top. +/// +/// Read on every pane mount: the window outlives the pane — which is unmounted +/// whenever another Project Home sub-tab is selected — so neither fact can be +/// carried in component state. #[tauri::command] -pub async fn is_browser_view_popout_open( +pub async fn get_browser_view_popout_state( project_id: String, app_handle: AppHandle, -) -> Result { - Ok(popout::is_open(&app_handle, &project_id)) +) -> Result { + Ok(popout::state(&app_handle, &project_id)) } /// Pin the pop-out above other windows, so it can be watched while working in diff --git a/app/src-tauri/src/browser_view/mod.rs b/app/src-tauri/src/browser_view/mod.rs index 597b132..ea276ea 100644 --- a/app/src-tauri/src/browser_view/mod.rs +++ b/app/src-tauri/src/browser_view/mod.rs @@ -468,17 +468,33 @@ async fn supervise( let _ = kill_dashboard(&container_id, &cli_entry).await; // Deregister, unless a newer session has already taken this project's slot. - { + let superseded = { let mut map = sessions.lock().await; - if map.get(&project_id).is_some_and(|s| s.epoch == epoch) { - map.remove(&project_id); + match map.get(&project_id) { + Some(session) if session.epoch == epoch => { + map.remove(&project_id); + false + } + // Someone else owns this project now: `stop` removes the session + // from the map *before* awaiting this task, and teardown below is + // seconds of Docker work, so a restart in that window is ordinary. + Some(_) => true, + None => false, } + }; + + // Everything past here speaks for the project as a whole, so a superseded + // supervisor must say nothing: closing the pop-out would destroy the *new* + // session's window, and the off-status would report a running view as + // stopped. + if superseded { + return; } // A pop-out outlives the tab, so nothing else would take it down: the // window would sit there showing a frozen last frame of a viewer that no // longer exists. The session owns it, and this is where the session ends. - popout::close(&app, &project_id); + let _ = popout::close(&app, &project_id); let enabled = manager().is_enabled(&project_id).await; emit(&app, &project_id, &BrowserViewStatus::off(enabled)); diff --git a/app/src-tauri/src/browser_view/popout.rs b/app/src-tauri/src/browser_view/popout.rs index fe9cca1..0ee9992 100644 --- a/app/src-tauri/src/browser_view/popout.rs +++ b/app/src-tauri/src/browser_view/popout.rs @@ -27,15 +27,36 @@ //! dead viewer is worse than no window. The reverse is not true; closing the //! window leaves the view running, and the pane takes it back into the tab. +use serde::Serialize; use tauri::{AppHandle, Emitter, Manager, WebviewUrl, WebviewWindowBuilder, WindowEvent}; -/// Emitted when a pop-out opens or closes. Payload: `{ project_id, open }`. +/// Emitted when a pop-out opens or closes. Payload: [`PopoutState`] plus the +/// project id. /// /// The window can close without the app asking it to — the user hits its X, or /// a teardown takes it — so the pane learns about it the same way it learns /// about everything else here, by listening. const POPOUT_EVENT: &str = "browser-view-popout-changed"; +/// What the pane needs to render its pop-out controls. +/// +/// Both fields are read from the window itself rather than remembered on either +/// side: the pane is unmounted whenever another Project Home sub-tab is +/// selected, so anything it merely *remembers* about the window is gone by the +/// time the user comes back, while the window is still there. +#[derive(Debug, Clone, Copy, Serialize)] +pub struct PopoutState { + pub open: bool, + pub always_on_top: bool, +} + +impl PopoutState { + const CLOSED: Self = Self { + open: false, + always_on_top: false, + }; +} + /// Tauri window labels admit `[a-zA-Z0-9-/:_]` only. Project ids are UUIDs, so /// this never fires in practice; it exists so a hand-edited `projects.json` /// cannot produce a label Tauri rejects at build time. @@ -65,7 +86,7 @@ pub fn open( let _ = window.unminimize(); let _ = window.set_focus(); let _ = window.set_always_on_top(always_on_top); - emit(app, project_id, true); + emit(app, project_id, state(app, project_id)); return Ok(()); } @@ -88,12 +109,12 @@ pub fn open( // to take the view back into the tab. window.on_window_event(move |event| { if matches!(event, WindowEvent::Destroyed) { - emit(&app_for_event, &project_id_owned, false); + emit(&app_for_event, &project_id_owned, PopoutState::CLOSED); } }); log::info!("Browser view: popped out for project {}", project_id); - emit(app, project_id, true); + emit(app, project_id, state(app, project_id)); Ok(()) } @@ -102,23 +123,39 @@ pub fn open( /// `destroy`, not `close`: `close` raises `CloseRequested`, and the app's /// window-event handler treats that as a request to quit for the main window. /// Nothing here should ever be able to be mistaken for that. -pub fn close(app: &AppHandle, project_id: &str) { +/// +/// A failure is **returned, not logged and forgotten**. The pane puts its +/// iframe back the moment it believes the window is gone, so reporting a close +/// that did not happen is how you end up with two viewers driving one browser — +/// the exact state the iframe is dropped to prevent. +pub fn close(app: &AppHandle, project_id: &str) -> Result<(), String> { if let Some(window) = app.get_webview_window(&window_label(project_id)) { - if let Err(e) = window.destroy() { + window.destroy().map_err(|e| { log::warn!( "Browser view: could not close the pop-out for project {}: {}", project_id, e ); - } + format!("Could not close the browser window: {}", e) + })?; } - // Unconditional: `Destroyed` covers the normal path, but a window that was - // already gone still owes the pane an answer. - emit(app, project_id, false); + // `Destroyed` covers the normal path; a window that was already gone still + // owes the pane an answer. + emit(app, project_id, PopoutState::CLOSED); + Ok(()) } -pub fn is_open(app: &AppHandle, project_id: &str) -> bool { - app.get_webview_window(&window_label(project_id)).is_some() +/// Whether the window exists and how it is stacked, read from the window. +pub fn state(app: &AppHandle, project_id: &str) -> PopoutState { + match app.get_webview_window(&window_label(project_id)) { + Some(window) => PopoutState { + open: true, + // A window that cannot answer is not a reason to fail the call; the + // pin is a preference, and "not pinned" is the safe reading. + always_on_top: window.is_always_on_top().unwrap_or(false), + }, + None => PopoutState::CLOSED, + } } /// Pin the pop-out above other windows, or unpin it. No-op when it is closed. @@ -128,13 +165,19 @@ pub fn set_always_on_top(app: &AppHandle, project_id: &str, on_top: bool) -> Res }; window .set_always_on_top(on_top) - .map_err(|e| format!("Could not change the window's stacking: {}", e)) + .map_err(|e| format!("Could not change the window's stacking: {}", e))?; + emit(app, project_id, state(app, project_id)); + Ok(()) } -fn emit(app: &AppHandle, project_id: &str, open: bool) { +fn emit(app: &AppHandle, project_id: &str, state: PopoutState) { let _ = app.emit( POPOUT_EVENT, - serde_json::json!({ "project_id": project_id, "open": open }), + serde_json::json!({ + "project_id": project_id, + "open": state.open, + "always_on_top": state.always_on_top, + }), ); } diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index 634f512..2f22361 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -438,7 +438,7 @@ pub fn run() { browser_view::commands::install_browser_view_browser, browser_view::commands::open_browser_view_popout, browser_view::commands::close_browser_view_popout, - browser_view::commands::is_browser_view_popout_open, + browser_view::commands::get_browser_view_popout_state, browser_view::commands::set_browser_view_popout_always_on_top, // Shared Claude Code auth token commands::auth_token_commands::acquire_claude_token, diff --git a/app/src/components/layout/MainTabs.test.tsx b/app/src/components/layout/MainTabs.test.tsx index d3162b2..0507812 100644 --- a/app/src/components/layout/MainTabs.test.tsx +++ b/app/src/components/layout/MainTabs.test.tsx @@ -45,33 +45,22 @@ const S1 = terminalTabKey("s1"); const S2 = terminalTabKey("s2"); /** - * A stand-in for the DataTransfer jsdom doesn't implement. It only has to - * carry the tab key, which is what a drop falls back to reading. + * A pointer event carrying a real `clientX`. + * + * jsdom implements no `PointerEvent`, so Testing Library's synthesized one has + * no coordinates — and the coordinate is the whole point here, since it decides + * which slot the drop lands in. `MouseEvent` has one, and React dispatches on + * the event's type name either way. */ -function dataTransfer() { - const store: Record = {}; - return { - effectAllowed: "", - dropEffect: "", - setData: (format: string, value: string) => { - store[format] = value; - }, - getData: (format: string) => store[format] ?? "", - }; +function pointer(el: Element, type: string, clientX: number) { + fireEvent(el, new MouseEvent(type, { bubbles: true, cancelable: true, clientX, button: 0 })); } -/** - * A dragover carrying a real `clientX`. - * - * jsdom has no `DragEvent`, so Testing Library's synthesized one is a plain - * `Event` with no pointer coordinates — and the coordinate is the whole point - * here, since it decides which side of a tab the drop lands on. A `MouseEvent` - * has one, and React reads it the same way. - */ -function dragOverAt(el: Element, clientX: number, dt: ReturnType) { - const event = new MouseEvent("dragover", { bubbles: true, cancelable: true, clientX }); - Object.defineProperty(event, "dataTransfer", { value: dt }); - fireEvent(el, event); +/** Press, move past the drag threshold, and release over `endX`. */ +function dragTab(el: Element, fromX: number, endX: number) { + pointer(el, "pointerdown", fromX); + pointer(el, "pointermove", endX); + pointer(el, "pointerup", endX); } /** Pin a tab's geometry so "past the midpoint" means something in jsdom. */ @@ -80,6 +69,13 @@ function place(el: Element, left: number, width = 100) { ({ left, width, right: left + width, top: 0, bottom: 30, height: 30, x: left, y: 0 }) as DOMRect; } +/** Lay the strip out as three 100px tabs starting at x=0. */ +function laidOut() { + const tabs = screen.getAllByRole("tab"); + tabs.forEach((tab, i) => place(tab, i * 100)); + return tabs; +} + const order = () => useAppState.getState().tabOrder; beforeEach(() => { @@ -95,73 +91,124 @@ beforeEach(() => { describe("MainTabs reordering", () => { it("drags a tab to the front", () => { render(); - const tabs = screen.getAllByRole("tab"); - tabs.forEach((tab, i) => place(tab, i * 100)); + const tabs = laidOut(); - const dt = dataTransfer(); - fireEvent.dragStart(tabs[2], { dataTransfer: dt }); - // Left half of the first tab — the marker sits before it. - dragOverAt(tabs[0], 10, dt); - expect(screen.getByTestId("tab-drop-marker")).toBeInTheDocument(); - fireEvent.drop(tabs[0], { dataTransfer: dt }); + // Left half of the first tab — the tab lands before it. + dragTab(tabs[2], 250, 10); expect(order()).toEqual([S2, HOME, S1]); }); it("drops after the tab when the pointer is past its midpoint", () => { render(); - const tabs = screen.getAllByRole("tab"); - tabs.forEach((tab, i) => place(tab, i * 100)); + const tabs = laidOut(); - const dt = dataTransfer(); - fireEvent.dragStart(tabs[0], { dataTransfer: dt }); - dragOverAt(tabs[1], 190, dt); - fireEvent.drop(tabs[1], { dataTransfer: dt }); + dragTab(tabs[0], 50, 190); expect(order()).toEqual([S1, HOME, S2]); }); + it("drops at the end when released past the last tab", () => { + render(); + const tabs = laidOut(); + + dragTab(tabs[0], 50, 800); + + expect(order()).toEqual([S1, S2, HOME]); + }); + it("dragging does not steal the selection", () => { render(); - const tabs = screen.getAllByRole("tab"); - tabs.forEach((tab, i) => place(tab, i * 100)); + const tabs = laidOut(); - const dt = dataTransfer(); - fireEvent.dragStart(tabs[1], { dataTransfer: dt }); - dragOverAt(tabs[2], 290, dt); - fireEvent.drop(tabs[2], { dataTransfer: dt }); + dragTab(tabs[1], 150, 290); expect(order()).toEqual([HOME, S2, S1]); expect(useAppState.getState().activeTabKey).toBe(HOME); }); - it("shows no drop marker until a drag is under way", () => { + it("shows the drop marker only while a drag is under way", () => { render(); + const tabs = laidOut(); + expect(screen.queryByTestId("tab-drop-marker")).toBeNull(); + + pointer(tabs[2], "pointerdown", 250); + pointer(tabs[2], "pointermove", 10); + expect(screen.getByTestId("tab-drop-marker")).toBeInTheDocument(); + + pointer(tabs[2], "pointerup", 10); expect(screen.queryByTestId("tab-drop-marker")).toBeNull(); }); - it("clears the marker when the drag ends without a drop", () => { + it("abandons the drag on Escape, leaving the order alone", () => { render(); - const tabs = screen.getAllByRole("tab"); - tabs.forEach((tab, i) => place(tab, i * 100)); + const tabs = laidOut(); - const dt = dataTransfer(); - fireEvent.dragStart(tabs[2], { dataTransfer: dt }); - dragOverAt(tabs[0], 10, dt); - fireEvent.dragEnd(tabs[2], { dataTransfer: dt }); + pointer(tabs[2], "pointerdown", 250); + pointer(tabs[2], "pointermove", 10); + fireEvent.keyDown(window, { key: "Escape" }); + + expect(screen.queryByTestId("tab-drop-marker")).toBeNull(); + pointer(tabs[2], "pointerup", 10); + expect(order()).toEqual([HOME, S1, S2]); + }); + + it("treats a press that barely moves as a click, not a drag", () => { + render(); + const tabs = laidOut(); + + // Two pixels of tremble, under the threshold. + pointer(tabs[2], "pointerdown", 250); + pointer(tabs[2], "pointermove", 252); + pointer(tabs[2], "pointerup", 252); + fireEvent.click(tabs[2]); + + expect(order()).toEqual([HOME, S1, S2]); + expect(useAppState.getState().activeTabKey).toBe(S2); + }); + + it("does not select the tab it just dropped", () => { + render(); + const tabs = laidOut(); + + dragTab(tabs[2], 250, 10); + // The browser fires a click after the pointerup that ended the drag. + fireEvent.click(tabs[2]); + + expect(order()).toEqual([S2, HOME, S1]); + expect(useAppState.getState().activeTabKey).toBe(HOME); + }); + + it("ignores a press that starts on the close button", () => { + render(); + const tabs = laidOut(); + const close = screen.getByRole("button", { name: "Close shell (bash)" }); + + fireEvent(close, new MouseEvent("pointerdown", { bubbles: true, clientX: 290, button: 0 })); + pointer(tabs[2], "pointermove", 10); expect(screen.queryByTestId("tab-drop-marker")).toBeNull(); expect(order()).toEqual([HOME, S1, S2]); }); - it("leaves a tab being renamed undraggable, so its text stays selectable", () => { + it("does not drag a tab that is being renamed — that drag selects text", () => { render(); - const tabs = screen.getAllByRole("tab"); - expect(tabs[1]).toHaveAttribute("draggable", "true"); - + const tabs = laidOut(); fireEvent.doubleClick(tabs[1]); - expect(screen.getByLabelText("Rename tab")).toBeInTheDocument(); - expect(screen.getAllByRole("tab")[1]).toHaveAttribute("draggable", "false"); + + dragTab(screen.getAllByRole("tab")[1], 150, 10); + + expect(order()).toEqual([HOME, S1, S2]); + }); + + it("carries no drag payload that another element could receive", () => { + // An HTML5 drag would put the tab key in a DataTransfer, and releasing over + // any text field in the app would type `term:…` into it. Pointer events + // have nothing to hand over, and the tabs are not draggable at all. + render(); + for (const tab of screen.getAllByRole("tab")) { + expect(tab).not.toHaveAttribute("draggable", "true"); + } }); }); diff --git a/app/src/components/layout/MainTabs.tsx b/app/src/components/layout/MainTabs.tsx index ab223f2..d0bf25e 100644 --- a/app/src/components/layout/MainTabs.tsx +++ b/app/src/components/layout/MainTabs.tsx @@ -18,6 +18,9 @@ interface ContextMenuState { y: number; } +/** Pixels of horizontal travel before a press becomes a drag rather than a click. */ +const DRAG_THRESHOLD = 4; + const MODE_BADGE: Record = { plan: { text: "plan", className: "bg-[var(--bg-tertiary)] text-[var(--text-secondary)]" }, default: { text: "ask", className: "bg-[var(--bg-tertiary)] text-[var(--text-secondary)]" }, @@ -29,9 +32,8 @@ const MODE_BADGE: Record = * One strip for both main-area tab kinds: Project Home views (⌂) and * terminals (▣). * - * Tabs are draggable. The drag is HTML5's, not a pointer-event - * reimplementation, so the OS supplies the drag image and the Escape-to-cancel - * behaviour for free; the only thing tracked here is where the drop would land. + * Tabs are draggable, on pointer events rather than HTML5 drag-and-drop — see + * `pointerProps` for why neither of the two obvious alternatives works. * `Ctrl+Shift+←/→` does the same thing without a mouse. */ export default function MainTabs() { @@ -53,6 +55,10 @@ export default function MainTabs() { /** The tab being dragged, and the slot it would drop into. */ const [dragKey, setDragKey] = useState(null); const [dropIndex, setDropIndex] = useState(null); + const stripRef = useRef(null); + /** A press that has not yet moved far enough to be a drag. */ + const pending = useRef<{ key: string; startX: number; dragging: boolean } | null>(null); + const suppressClick = useRef(false); useEffect(() => { if (!menu) return; @@ -72,6 +78,20 @@ export default function MainTabs() { } }, [renamingId]); + // Escape abandons a drag — the one affordance a pointer-event drag has to + // supply for itself, since the OS is not running this one. + useEffect(() => { + if (!dragKey) return; + const onKeyDown = (e: KeyboardEvent) => { + if (e.key !== "Escape") return; + pending.current = null; + setDragKey(null); + setDropIndex(null); + }; + window.addEventListener("keydown", onKeyDown); + return () => window.removeEventListener("keydown", onKeyDown); + }, [dragKey]); + if (tabOrder.length === 0) { return (
@@ -152,54 +172,91 @@ export default function MainTabs() { }${dragging ? " opacity-40" : ""}`; const endDrag = () => { + pending.current = null; setDragKey(null); setDropIndex(null); }; /** - * Drag props shared by both tab kinds. + * Which slot the pointer is currently over, as an insertion index into + * `tabOrder`. * - * A tab in rename mode is not draggable: a `draggable` ancestor swallows the - * mouse-drag that selects text inside the input, which would make the rename - * field impossible to select in. + * Measured from the tabs actually on screen rather than from the event's + * target, so the answer is the same whatever the pointer happens to be over — + * including the drop marker itself, and including a `tabOrder` entry whose + * session has already gone and which therefore renders nothing. */ - const dragProps = (key: string, index: number, renaming: boolean) => ({ - draggable: !renaming, - onDragStart: (e: React.DragEvent) => { - e.dataTransfer.effectAllowed = "move"; - // Some platforms refuse to start a drag with an empty payload. - e.dataTransfer.setData("text/plain", key); - setDragKey(key); - setDropIndex(index); + const dropIndexAt = (clientX: number): number => { + const strip = stripRef.current; + if (!strip) return tabOrder.length; + for (const el of strip.querySelectorAll("[data-tab-index]")) { + const rect = el.getBoundingClientRect(); + if (clientX < rect.left + rect.width / 2) return Number(el.dataset.tabIndex); + } + return tabOrder.length; + }; + + /** + * Dragging is done with pointer events, not HTML5 drag-and-drop. + * + * Two reasons, both load-bearing. Tauri's `dragDropEnabled` — which the + * terminal needs left on, because only the native drag-drop event carries + * dropped *file paths* — blocks HTML5 drag inside the webview on Windows, so + * an HTML5 implementation is simply dead there. And an HTML5 drag carries a + * `DataTransfer`: released over any text field in the app, the default + * handler types the payload into it. + */ + const pointerProps = (key: string, renaming: boolean) => ({ + onPointerDown: (e: React.PointerEvent) => { + // Left button only, never from the close button, and never while the + // rename input is up — that drag is a text selection. + if (e.button !== 0 || renaming) return; + if ((e.target as HTMLElement).closest("button, input")) return; + pending.current = { key, startX: e.clientX, dragging: false }; + e.currentTarget.setPointerCapture?.(e.pointerId); }, - onDragOver: (e: React.DragEvent) => { - if (!dragKey) return; // not our drag — a file dropped on the strip isn't one - e.preventDefault(); - e.dataTransfer.dropEffect = "move"; - const rect = e.currentTarget.getBoundingClientRect(); - const after = e.clientX > rect.left + rect.width / 2; - setDropIndex(index + (after ? 1 : 0)); + onPointerMove: (e: React.PointerEvent) => { + const drag = pending.current; + if (!drag) return; + // A few pixels of slop, so a click that trembles stays a click. + if (!drag.dragging && Math.abs(e.clientX - drag.startX) < DRAG_THRESHOLD) return; + drag.dragging = true; + setDragKey(drag.key); + setDropIndex(dropIndexAt(e.clientX)); }, - onDragEnd: endDrag, + onPointerUp: (e: React.PointerEvent) => { + const drag = pending.current; + e.currentTarget.releasePointerCapture?.(e.pointerId); + if (!drag?.dragging) { + pending.current = null; + return; // a plain click: leave it to `onClick` to select the tab + } + const to = dropIndexAt(e.clientX); + const from = tabOrder.indexOf(drag.key); + // `to` is a slot in the strip as it looks *now*; `moveTab` places the tab + // after pulling it out, so every slot past its own shifts down one. + if (from !== -1) moveTab(drag.key, to > from ? to - 1 : to); + // The click that follows this pointerup is the drag's, not a selection. + suppressClick.current = true; + endDrag(); + }, + onPointerCancel: endDrag, }); - /** Drop lands wherever the marker is showing, and only there. */ - const onDrop = (e: React.DragEvent) => { - const key = dragKey ?? e.dataTransfer.getData("text/plain"); - if (!key || dropIndex === null) return endDrag(); - e.preventDefault(); - const from = tabOrder.indexOf(key); - // `dropIndex` is a slot in the strip as it looks *now*; `moveTab` places the - // tab after pulling it out, so every slot past the tab's own shifts down one. - moveTab(key, dropIndex > from ? dropIndex - 1 : dropIndex); - endDrag(); + /** A drag in progress swallows the click it ends with. */ + const activateTab = (key: string) => { + if (suppressClick.current) { + suppressClick.current = false; + return; + } + setActiveTabKey(key); }; const dropMarker = ( - ) : live ? ( + ) : live && poppedOut === false ? (