Fix the review's findings: drag on pointer events, read the window back
Build App / compute-version (pull_request) Successful in 4s
Build App / build-macos (pull_request) Successful in 2m30s
Build App / build-linux (pull_request) Successful in 5m11s
Build App / build-windows (pull_request) Successful in 5m23s
Build App / create-tag (pull_request) Skipped
Build App / sync-to-github (pull_request) Skipped
Build App / compute-version (pull_request) Successful in 4s
Build App / build-macos (pull_request) Successful in 2m30s
Build App / build-linux (pull_request) Successful in 5m11s
Build App / build-windows (pull_request) Successful in 5m23s
Build App / create-tag (pull_request) Skipped
Build App / sync-to-github (pull_request) Skipped
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:<uuid>` 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) <noreply@anthropic.com>
This commit is contained in:
@@ -15,7 +15,8 @@ const installBrowserViewSupport = vi.fn<() => Promise<BrowserSetupOutcome>>();
|
||||
const installBrowserViewBrowser = vi.fn<(id: string, b: string) => Promise<BrowserSetupOutcome>>();
|
||||
const openBrowserViewPopout = vi.fn<(id: string, onTop: boolean) => Promise<void>>();
|
||||
const closeBrowserViewPopout = vi.fn<(id: string) => Promise<void>>();
|
||||
const isBrowserViewPopoutOpen = vi.fn<() => Promise<boolean>>();
|
||||
const getBrowserViewPopoutState =
|
||||
vi.fn<() => Promise<{ open: boolean; always_on_top: boolean }>>();
|
||||
const setBrowserViewPopoutAlwaysOnTop = vi.fn<(id: string, onTop: boolean) => Promise<void>>();
|
||||
const pushToast = vi.fn();
|
||||
const setContainerProgress = vi.fn();
|
||||
@@ -28,7 +29,7 @@ vi.mock("../../../lib/tauri-commands", () => ({
|
||||
installBrowserViewBrowser: (id: string, b: string) => installBrowserViewBrowser(id, b),
|
||||
openBrowserViewPopout: (id: string, onTop: boolean) => openBrowserViewPopout(id, onTop),
|
||||
closeBrowserViewPopout: (id: string) => closeBrowserViewPopout(id),
|
||||
isBrowserViewPopoutOpen: () => isBrowserViewPopoutOpen(),
|
||||
getBrowserViewPopoutState: () => getBrowserViewPopoutState(),
|
||||
setBrowserViewPopoutAlwaysOnTop: (id: string, onTop: boolean) =>
|
||||
setBrowserViewPopoutAlwaysOnTop(id, onTop),
|
||||
}));
|
||||
@@ -120,7 +121,7 @@ beforeEach(() => {
|
||||
storeState.containerProgress = {};
|
||||
getBrowserViewStatus.mockResolvedValue(OFF);
|
||||
checkBrowserViewSupport.mockResolvedValue(READY);
|
||||
isBrowserViewPopoutOpen.mockResolvedValue(false);
|
||||
getBrowserViewPopoutState.mockResolvedValue({ open: false, always_on_top: false });
|
||||
openBrowserViewPopout.mockResolvedValue(undefined);
|
||||
closeBrowserViewPopout.mockResolvedValue(undefined);
|
||||
setBrowserViewPopoutAlwaysOnTop.mockResolvedValue(undefined);
|
||||
@@ -406,16 +407,17 @@ describe("BrowserTab", () => {
|
||||
});
|
||||
|
||||
await act(async () => {
|
||||
fireEvent.click(screen.getByRole("switch", { name: /above other windows/i }));
|
||||
// The accessible name is the visible text, as with every other Toggle.
|
||||
fireEvent.click(screen.getByRole("switch", { name: "Keep on top" }));
|
||||
});
|
||||
|
||||
expect(setBrowserViewPopoutAlwaysOnTop).toHaveBeenCalledWith("p1", true);
|
||||
});
|
||||
|
||||
it("keeps a pop-out that outlived the tab, rather than showing an empty pane", async () => {
|
||||
// The window belongs to the backend, so reopening the tab has to read its
|
||||
// The window belongs to the backend, so remounting the pane has to read its
|
||||
// state back — otherwise the pane would render an iframe alongside it.
|
||||
isBrowserViewPopoutOpen.mockResolvedValue(true);
|
||||
getBrowserViewPopoutState.mockResolvedValue({ open: true, always_on_top: true });
|
||||
checkBrowserViewSupport.mockResolvedValue({ ...READY, browsers: ["chromium-1200"] });
|
||||
getBrowserViewStatus.mockResolvedValue(LIVE);
|
||||
|
||||
@@ -423,6 +425,33 @@ describe("BrowserTab", () => {
|
||||
|
||||
expect(await screen.findByText(/in its own window/i)).toBeInTheDocument();
|
||||
expect(screen.queryByTitle(/browser view for/i)).toBeNull();
|
||||
// The pin is read from the window too — the pane is unmounted every time
|
||||
// another sub-tab is selected, so remembering it would show Off over a
|
||||
// window that is still floating on top.
|
||||
expect(screen.getByRole("switch", { name: "Keep on top" })).toBeChecked();
|
||||
});
|
||||
|
||||
it("never mounts the iframe before the window's state is known", async () => {
|
||||
// The status and the pop-out state are two separate round trips. If the
|
||||
// status wins the race, guessing "not popped out" would flash a second
|
||||
// viewer onto a browser the window is already driving.
|
||||
checkBrowserViewSupport.mockResolvedValue({ ...READY, browsers: ["chromium-1200"] });
|
||||
getBrowserViewStatus.mockResolvedValue(LIVE);
|
||||
let answer: (s: { open: boolean; always_on_top: boolean }) => void = () => {};
|
||||
getBrowserViewPopoutState.mockReturnValue(
|
||||
new Promise((resolve) => {
|
||||
answer = resolve;
|
||||
}),
|
||||
);
|
||||
|
||||
render(<BrowserTab project={project} active />);
|
||||
await waitFor(() => expect(screen.getByText("Live")).toBeInTheDocument());
|
||||
expect(screen.queryByTitle(/browser view for/i)).toBeNull();
|
||||
|
||||
await act(async () => {
|
||||
answer({ open: false, always_on_top: false });
|
||||
});
|
||||
expect(await screen.findByTitle("Playwright browser view for api-server")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("says why the window wouldn’t open instead of pretending it did", async () => {
|
||||
|
||||
@@ -15,7 +15,7 @@ import {
|
||||
getBrowserViewStatus,
|
||||
installBrowserViewBrowser,
|
||||
installBrowserViewSupport,
|
||||
isBrowserViewPopoutOpen,
|
||||
getBrowserViewPopoutState,
|
||||
openBrowserViewPopout,
|
||||
setBrowserViewEnabled,
|
||||
setBrowserViewPopoutAlwaysOnTop,
|
||||
@@ -72,8 +72,13 @@ export default function BrowserTab({ project, active }: Props) {
|
||||
const [job, setJob] = useState<SetupJob>(null);
|
||||
const [outcome, setOutcome] = useState<BrowserSetupOutcome | null>(null);
|
||||
const [setupError, setSetupError] = useState<string | null>(null);
|
||||
/** Whether the view is currently in its own window instead of this pane. */
|
||||
const [poppedOut, setPoppedOut] = useState(false);
|
||||
/**
|
||||
* Whether the view is in its own window instead of this pane, and whether
|
||||
* that window is pinned. `null` means "not asked yet" — a distinct state from
|
||||
* "not popped out", because rendering the iframe on a guess is what puts a
|
||||
* second viewer on the browser.
|
||||
*/
|
||||
const [poppedOut, setPoppedOut] = useState<boolean | null>(null);
|
||||
const [onTop, setOnTop] = useState(false);
|
||||
const pushToast = useAppState((s) => s.pushToast);
|
||||
const setContainerProgress = useAppState((s) => s.setContainerProgress);
|
||||
@@ -112,7 +117,7 @@ export default function BrowserTab({ project, active }: Props) {
|
||||
listen<BrowserViewPopoutChangedEvent>("browser-view-popout-changed", (event) => {
|
||||
if (event.payload.project_id === projectId && mounted.current) {
|
||||
setPoppedOut(event.payload.open);
|
||||
if (!event.payload.open) setOnTop(false);
|
||||
setOnTop(event.payload.always_on_top);
|
||||
}
|
||||
}).then((un) => {
|
||||
if (mounted.current) dispose = un;
|
||||
@@ -123,9 +128,15 @@ export default function BrowserTab({ project, active }: Props) {
|
||||
|
||||
useEffect(() => {
|
||||
if (!active || !running) return;
|
||||
isBrowserViewPopoutOpen(projectId)
|
||||
.then((open) => mounted.current && setPoppedOut(open))
|
||||
.catch(() => {});
|
||||
getBrowserViewPopoutState(projectId)
|
||||
.then((s) => {
|
||||
if (!mounted.current) return;
|
||||
setPoppedOut(s.open);
|
||||
setOnTop(s.always_on_top);
|
||||
})
|
||||
// Unreachable in practice, but a pane stuck at "not asked yet" would
|
||||
// never show the view at all — so fail towards the tab.
|
||||
.catch(() => mounted.current && setPoppedOut(false));
|
||||
getBrowserViewStatus(projectId)
|
||||
.then((s) => mounted.current && setStatus(s))
|
||||
.catch(() => {});
|
||||
@@ -304,22 +315,21 @@ export default function BrowserTab({ project, active }: Props) {
|
||||
</span>
|
||||
)}
|
||||
<div className="flex-1" />
|
||||
{live && poppedOut && (
|
||||
<label className="flex items-center gap-1.5 text-xs text-[var(--text-secondary)]">
|
||||
{live && poppedOut === true && (
|
||||
<span className="flex items-center gap-1.5 text-xs text-[var(--text-secondary)]">
|
||||
Keep on top
|
||||
<Toggle
|
||||
checked={onTop}
|
||||
onChange={toggleOnTop}
|
||||
label="Keep the browser window above other windows"
|
||||
/>
|
||||
</label>
|
||||
{/* The accessible name matches the visible text, as everywhere else
|
||||
a Toggle is used — a `<label>` around it would be inert anyway,
|
||||
since a Toggle renders a button. */}
|
||||
<Toggle checked={onTop} onChange={toggleOnTop} label="Keep on top" />
|
||||
</span>
|
||||
)}
|
||||
{live && !poppedOut && (
|
||||
{live && poppedOut === false && (
|
||||
<Button size="md" onClick={() => setReloadKey((k) => k + 1)}>
|
||||
Reload
|
||||
</Button>
|
||||
)}
|
||||
{live && (
|
||||
{live && poppedOut !== null && (
|
||||
<Button size="md" onClick={poppedOut ? popIn : popOut}>
|
||||
{poppedOut ? "Put back in tab" : "Open in own window"}
|
||||
</Button>
|
||||
@@ -334,7 +344,7 @@ export default function BrowserTab({ project, active }: Props) {
|
||||
</Button>
|
||||
</div>
|
||||
|
||||
{live && poppedOut ? (
|
||||
{live && poppedOut === true ? (
|
||||
// The iframe is unmounted while the window is up, on purpose. Two
|
||||
// viewers on one browser both work, but both also *drive* it — two
|
||||
// cursors taking over the same page is not a feature.
|
||||
@@ -355,7 +365,7 @@ export default function BrowserTab({ project, active }: Props) {
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
) : live ? (
|
||||
) : live && poppedOut === false ? (
|
||||
<iframe
|
||||
key={reloadKey}
|
||||
// Loopback only, and the URL carries the one-time session token the
|
||||
@@ -364,6 +374,11 @@ export default function BrowserTab({ project, active }: Props) {
|
||||
title={`Playwright browser view for ${project.name}`}
|
||||
className="flex-1 min-h-0 w-full border-0 bg-[var(--bg-primary)]"
|
||||
/>
|
||||
) : live ? (
|
||||
// Live, but the window's state hasn't come back yet. An instant, and
|
||||
// deliberately empty: guessing "not popped out" here is what would
|
||||
// flash a second viewer onto the browser.
|
||||
<div className="flex-1 min-h-0" />
|
||||
) : (
|
||||
<div className="flex-1 min-h-0 overflow-y-auto">
|
||||
{/* Setup stays on screen while an install is running and after it
|
||||
|
||||
Reference in New Issue
Block a user