Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 48 additions & 1 deletion apps/geolibre-desktop/src/components/layout/DesktopShell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1381,7 +1381,7 @@
}
});
return () => setKmlFileImportHandler(null);
}, [addImportedVectorLayers, confirmLargeVectorDataset, t]);

Check warning on line 1384 in apps/geolibre-desktop/src/components/layout/DesktopShell.tsx

View workflow job for this annotation

GitHub Actions / Build and test

React Hook useEffect has an unnecessary dependency: 'confirmLargeVectorDataset'. Either exclude it or remove the dependency array. Outer scope values like 'confirmLargeVectorDataset' aren't valid dependencies because mutating them doesn't re-render the component

const addDroppedPhotos = useCallback(
(result: GeotaggedPhotoResult | null): number => {
Expand Down Expand Up @@ -1657,7 +1657,7 @@
disposed = true;
unlisten?.();
};
}, [

Check warning on line 1660 in apps/geolibre-desktop/src/components/layout/DesktopShell.tsx

View workflow job for this annotation

GitHub Actions / Build and test

React Hook useEffect has a missing dependency: 't'. Either include it or remove the dependency array
clearDropMessageLater,
finishDrop,
addDroppedRasters,
Expand Down Expand Up @@ -1801,7 +1801,7 @@
clearDropMessageLater();
}
},
[

Check warning on line 1804 in apps/geolibre-desktop/src/components/layout/DesktopShell.tsx

View workflow job for this annotation

GitHub Actions / Build and test

React Hook useCallback has a missing dependency: 't'. Either include it or remove the dependency array
clearDropMessageLater,
finishDrop,
addDroppedRasters,
Expand All @@ -1811,6 +1811,50 @@
],
);

// Escape hatch for a drop overlay that outlived its drag (issue #1664).
//
// The overlay is driven by one boolean fed from two places: the webview drag
// handlers above (balanced by dragDepthRef) and, on desktop, Tauri's native
// onDragDropEvent (no counter at all, since the OS reports enter/leave/drop
// directly). Either feed can strand it. A native "leave" that never arrives
// — which is what a modal native file dialog opening mid-drag produces on
// WebKitGTK — leaves the flag set with nothing left to clear it, and an
// unbalanced webview enter/leave pair leaves dragDepthRef above zero, which
// has the same effect. The result is an overlay covering the map until the
// user happens to drag another file across the window.
//
// Rather than guess at every way the OS can swallow an event, recover on a
// pointer button, which cannot occur while a real drag is in progress: HTML
// drag-and-drop suppresses mouse events and a native drag holds an OS pointer
// grab. Escape is a separate, conventional request to cancel either the
// stranded overlay or a genuine drag.
useEffect(() => {
if (!isDraggingFiles) return;

const clear = () => {
dragDepthRef.current = 0;
setIsDraggingFiles(false);
};
const onKeyDown = (event: KeyboardEvent) => {
// Escape is the one the reporter reached for first; any key would do, but
// limiting it keeps typing in a panel from silently cancelling feedback
// for a drag that is genuinely still running.
if (event.key === "Escape") clear();
Comment thread
giswqs marked this conversation as resolved.
};
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// Capture phase, not bubble: several controls in the app stop propagation
// on these events before they reach window (startLayerPanelResize below is
// one, and a focused Radix dialog handles its own Escape), which would
// silently defeat the recovery for exactly the interaction the user is most
// likely to try first. Capturing on window runs before any of them.
window.addEventListener("keydown", onKeyDown, true);
window.addEventListener("pointerdown", clear, true);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor completeness note (low confidence): recovery only fires on Escape or a pointer press. A keyboard-only user navigating with Tab (no pointer, and not thinking to hit Escape) has no way to dismiss a stranded overlay. Given the reporter's workflow this is a big improvement either way, but it might be worth also clearing on focus-in/Tab if keyboard-only recovery matters here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this one as-is, and flagging it for a human call rather than resolving.

Two reasons:

  1. A keyboard-only user is not actually blocked. The overlay is pointer-events-none with no focusable children (DesktopShell.tsx:2616-2626), so it never traps focus or intercepts input — Tab navigation and every control keep working normally underneath it. The stranded overlay is a stale visual affordance, not a modal. And Escape, which does clear it, is the conventional dismiss key for exactly this kind of overlay.

  2. Clearing on focus-in/Tab would reintroduce the desync the previous review round just flagged. As noted in fix(shell): recover a drop overlay that outlived its drag #1683 (comment), keyboard and focus events are not spec-suppressed during a drag. Escape is safe despite that because pressing Escape mid-drag is itself the standard gesture to cancel the drag, so clearing the overlay is the correct outcome either way. Tab and focus changes have no such property: they do not cancel a drag, so clearing on them could hide the overlay while a genuine drag is still live.

Happy to add it if the accessibility angle is worth the tradeoff, but it looked like a net regression rather than a win.

return () => {
window.removeEventListener("keydown", onKeyDown, true);
window.removeEventListener("pointerdown", clear, true);
};
}, [isDraggingFiles]);

const startLayerPanelResize = useCallback(
(event: ReactPointerEvent<HTMLDivElement>) => {
event.preventDefault();
Expand Down Expand Up @@ -2569,7 +2613,10 @@
className="pointer-events-none fixed bottom-7 top-11 z-50 hidden w-px bg-primary shadow-[0_0_0_1px_hsl(var(--primary)/0.25)]"
/>
{isDraggingFiles ? (
<div className="pointer-events-none absolute inset-0 z-50 flex items-center justify-center bg-background/70 backdrop-blur-sm">
<div
data-testid="file-drop-overlay"
className="pointer-events-none absolute inset-0 z-50 flex items-center justify-center bg-background/70 backdrop-blur-sm"
>
<div className="max-w-sm rounded-md border bg-background px-4 py-3 text-center shadow-lg">
<p className="text-sm font-medium">{t("toolbar.fileDrop.overlayTitle")}</p>
<p className="mt-1 text-xs text-muted-foreground">
Expand Down
89 changes: 89 additions & 0 deletions e2e/drop-overlay.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import { expect, test, type Page } from "@playwright/test";
import { dropGeoJson, layerRow, readFixture, waitForMap } from "./helpers";

/**
* Regression cover for GeoLibre#1664: the drag-and-drop overlay could outlive
* its drag and sit over the map forever.
*
* The overlay is one boolean fed from two places — the webview drag handlers
* (balanced by a depth counter) and, on desktop, Tauri's native drag events (no
* counter). Either feed can strand it: a native "leave" the OS never delivers,
* or an unbalanced webview enter/leave pair that leaves the counter above zero.
* These tests strand it the second way, which is the one a browser can drive,
* and assert the recovery paths that clear it either way.
*/

const OVERLAY = '[data-testid="file-drop-overlay"]';

/** Fires one file-drag event at the shell, without completing a drop. */
async function fireFileDrag(page: Page, type: "dragenter" | "dragleave"): Promise<void> {
const dataTransfer = await page.evaluateHandle(() => {
const dt = new DataTransfer();
dt.items.add(new File(["{}"], "style.json", { type: "application/json" }));
return dt;
});
await page.dispatchEvent('[data-testid="map-canvas"]', type, { dataTransfer });
await dataTransfer.dispose();
}

/**
* Leaves the overlay showing with no drag in progress, by sending one more
* `dragenter` than `dragleave` — the shape a swallowed leave event produces.
*/
async function strandOverlay(page: Page): Promise<void> {
await fireFileDrag(page, "dragenter");
await fireFileDrag(page, "dragenter");
await fireFileDrag(page, "dragleave");
await expect(page.locator(OVERLAY)).toBeVisible();
}

test("Escape dismisses a drop overlay that outlived its drag", async ({ page }) => {
await waitForMap(page);
await strandOverlay(page);

await page.keyboard.press("Escape");
await expect(page.locator(OVERLAY)).toBeHidden();
});

test("a pointer press dismisses a drop overlay that outlived its drag", async ({ page }) => {
await waitForMap(page);
await strandOverlay(page);

// A mouse button cannot go down during a real drag, so this is the signal
// that heals the overlay without the user knowing to press anything.
await page.mouse.click(400, 400);
await expect(page.locator(OVERLAY)).toBeHidden();
});

test("a press on a control that stops propagation still dismisses the overlay", async ({
page,
}) => {
await waitForMap(page);
await strandOverlay(page);

// The panel resize handles call stopPropagation() on pointerdown, so a bubble
// phase listener on window would never see this press. The recovery listens
// in the capture phase precisely so it does.
await page.locator('[aria-label="Resize Layers panel"]').first().click();
await expect(page.locator(OVERLAY)).toBeHidden();
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

test("a balanced drag still shows and hides the overlay", async ({ page }) => {
await waitForMap(page);

await fireFileDrag(page, "dragenter");
await expect(page.locator(OVERLAY)).toBeVisible();
await fireFileDrag(page, "dragleave");
await expect(page.locator(OVERLAY)).toBeHidden();
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

test("a real file drop still imports a layer and clears the overlay", async ({ page }) => {
await waitForMap(page);

// The recovery listeners are only mounted while the overlay is up, so guard
// that they cannot swallow the drop that the overlay exists to invite.
await dropGeoJson(page, "dropped", readFixture("smoke.geojson"));

await expect(layerRow(page, "dropped")).toBeVisible();
await expect(page.locator(OVERLAY)).toBeHidden();
});
Loading