-
-
Notifications
You must be signed in to change notification settings - Fork 571
fix(shell): recover a drop overlay that outlived its drag #1683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
6836114
be46aa1
c44ba2c
1899ca0
6a695b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1381,7 +1381,7 @@ | |
| } | ||
| }); | ||
| return () => setKmlFileImportHandler(null); | ||
| }, [addImportedVectorLayers, confirmLargeVectorDataset, t]); | ||
|
Check warning on line 1384 in apps/geolibre-desktop/src/components/layout/DesktopShell.tsx
|
||
|
|
||
| const addDroppedPhotos = useCallback( | ||
| (result: GeotaggedPhotoResult | null): number => { | ||
|
|
@@ -1657,7 +1657,7 @@ | |
| disposed = true; | ||
| unlisten?.(); | ||
| }; | ||
| }, [ | ||
| clearDropMessageLater, | ||
| finishDrop, | ||
| addDroppedRasters, | ||
|
|
@@ -1801,7 +1801,7 @@ | |
| clearDropMessageLater(); | ||
| } | ||
| }, | ||
| [ | ||
| clearDropMessageLater, | ||
| finishDrop, | ||
| addDroppedRasters, | ||
|
|
@@ -1811,6 +1811,51 @@ | |
| ], | ||
| ); | ||
|
|
||
| // 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 two | ||
| // signals that cannot occur while a real drag is in progress: a key press and | ||
| // a pointer button. Neither fires during an HTML5 drag (the spec suppresses | ||
| // mouse events for the duration) nor during a native one (the OS holds a | ||
| // pointer grab), so this can never dismiss the overlay out from under a drag | ||
| // the user is actually performing. | ||
| 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(); | ||
|
giswqs marked this conversation as resolved.
giswqs marked this conversation as resolved.
Outdated
|
||
| }; | ||
|
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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor completeness note (low confidence): recovery only fires on
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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(); | ||
|
|
@@ -2569,7 +2614,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"> | ||
|
|
||
| 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().dispatchEvent("pointerdown"); | ||
| await expect(page.locator(OVERLAY)).toBeHidden(); | ||
| }); | ||
|
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(); | ||
| }); | ||
|
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(); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.