From 2f84e8f3b1849e0361bc18911492fe806bbb14ce Mon Sep 17 00:00:00 2001 From: nova28 Date: Thu, 30 Jul 2026 23:24:10 +0800 Subject: [PATCH] fix(web): stop a closed dialog from leaving the page click-dead (#2056) --- apps/packages/ui/src/dialog.tsx | 48 +++++--- apps/packages/ui/src/lib/dialog-body-lock.ts | 73 ++++++++++++ .../tests/task/dialog-body-lock-helpers.ts | 68 +++++++++++ .../e2e/tests/task/dialog-body-lock.spec.ts | 6 + .../task/mobile-dialog-body-lock.spec.ts | 6 + .../lib/dialog-body-lock-component.test.tsx | 31 +++++ apps/web/lib/dialog-body-lock.test.ts | 110 ++++++++++++++++++ 7 files changed, 328 insertions(+), 14 deletions(-) create mode 100644 apps/packages/ui/src/lib/dialog-body-lock.ts create mode 100644 apps/web/e2e/tests/task/dialog-body-lock-helpers.ts create mode 100644 apps/web/e2e/tests/task/dialog-body-lock.spec.ts create mode 100644 apps/web/e2e/tests/task/mobile-dialog-body-lock.spec.ts create mode 100644 apps/web/lib/dialog-body-lock-component.test.tsx create mode 100644 apps/web/lib/dialog-body-lock.test.ts diff --git a/apps/packages/ui/src/dialog.tsx b/apps/packages/ui/src/dialog.tsx index 85fbcb375b..5eb9e32a60 100644 --- a/apps/packages/ui/src/dialog.tsx +++ b/apps/packages/ui/src/dialog.tsx @@ -7,22 +7,42 @@ import { cn } from "./lib/utils"; import { Button } from "./button"; import { IconX } from "@tabler/icons-react"; import { handleDialogDefaultActionKeyDown } from "./lib/dialog-default-action"; +import { + DIALOG_CLOSE_RECOVERY_MS, + finishClosingDialogAnimations, + subscribeDialogCloseRecovery, +} from "./lib/dialog-body-lock"; -function Dialog({ ...props }: React.ComponentProps) { - React.useEffect(() => { - return () => { - // Safety cleanup: Radix Dialog sets pointer-events: none on body when - // modal. If the dialog unmounts mid-close (e.g. onOpenChange(false) - // followed immediately by router.push), Radix never finishes its - // animation-based cleanup. Check the actual body state rather than - // tracking the open prop, which may already be false by unmount time. - if (document.body.style.pointerEvents === "none") { - document.body.style.removeProperty("pointer-events"); - } - }; - }, []); +function Dialog({ onOpenChange, ...props }: React.ComponentProps) { + const finishClosingAnimations = React.useCallback( + () => finishClosingDialogAnimations(document), + [], + ); - return ; + // Unmount is one way to strand the lock — Radix's animation-based cleanup + // never runs when the dialog disappears mid-close. + React.useEffect( + () => () => { + finishClosingAnimations(); + }, + [finishClosingAnimations], + ); + + // Becoming visible again is the other. Dialog roots share one document-level + // listener because the body lock and closing portals are document-global. + React.useEffect(() => subscribeDialogCloseRecovery(document), []); + + // And the general case: once a close has been requested, the page must be + // usable shortly afterwards whether or not animationend ever arrives. + const handleOpenChange = React.useCallback( + (open: boolean) => { + onOpenChange?.(open); + if (!open) window.setTimeout(finishClosingAnimations, DIALOG_CLOSE_RECOVERY_MS); + }, + [finishClosingAnimations, onOpenChange], + ); + + return ; } function DialogTrigger({ ...props }: React.ComponentProps) { diff --git a/apps/packages/ui/src/lib/dialog-body-lock.ts b/apps/packages/ui/src/lib/dialog-body-lock.ts new file mode 100644 index 0000000000..875e17ab3b --- /dev/null +++ b/apps/packages/ui/src/lib/dialog-body-lock.ts @@ -0,0 +1,73 @@ +/** + * Recovery for a stuck modal body lock. + * + * Radix sets `pointer-events: none` on while a modal dialog is open and + * releases it when the content unmounts. Presence defers that unmount until the + * exit animation fires `animationend` — so anything that stops that event from + * arriving leaves every control on the page dead, with no visible dialog to + * close. Two ways in, both observed: + * + * - the dialog unmounts mid-close (onOpenChange(false) then router.push), so + * Radix's own cleanup never runs; + * - the document is not being rendered when the dialog closes, which freezes + * CSS animation timelines. `document.timeline.currentTime` stays at 0 and a + * 100ms exit animation reports playState "running" indefinitely. + * + * Recovery finishes only closing dialog animations. The resulting + * `animationend` event lets Radix Presence unmount the closing owner and release + * its own pointer and scroll-lock bookkeeping. + */ + +/** Grace period after a close before sweeping. Must exceed the exit duration. */ +export const DIALOG_CLOSE_RECOVERY_MS = 400; + +const CLOSING_DIALOG_PARTS = [ + '[data-state="closed"][data-slot="dialog-content"]', + '[data-state="closed"][data-slot="dialog-overlay"]', +].join(","); + +type VisibilitySubscription = { + subscribers: number; + onVisibilityChange: () => void; +}; + +const visibilitySubscriptions = new WeakMap(); + +/** + * Ask Radix Presence to finish closing dialog owners through its normal + * `animationend` path. Returns true when at least one animation was finished. + */ +export function finishClosingDialogAnimations(doc: Document): boolean { + let finished = false; + for (const element of doc.querySelectorAll(CLOSING_DIALOG_PARTS)) { + for (const animation of element.getAnimations()) { + animation.finish(); + finished = true; + } + } + return finished; +} + +/** Share one document-level foreground recovery listener across dialog roots. */ +export function subscribeDialogCloseRecovery(doc: Document): () => void { + let subscription = visibilitySubscriptions.get(doc); + if (!subscription) { + const onVisibilityChange = () => { + if (doc.visibilityState === "visible") finishClosingDialogAnimations(doc); + }; + subscription = { subscribers: 0, onVisibilityChange }; + visibilitySubscriptions.set(doc, subscription); + doc.addEventListener("visibilitychange", onVisibilityChange); + } + subscription.subscribers += 1; + + let subscribed = true; + return () => { + if (!subscribed) return; + subscribed = false; + subscription.subscribers -= 1; + if (subscription.subscribers > 0) return; + doc.removeEventListener("visibilitychange", subscription.onVisibilityChange); + visibilitySubscriptions.delete(doc); + }; +} diff --git a/apps/web/e2e/tests/task/dialog-body-lock-helpers.ts b/apps/web/e2e/tests/task/dialog-body-lock-helpers.ts new file mode 100644 index 0000000000..132456d3b3 --- /dev/null +++ b/apps/web/e2e/tests/task/dialog-body-lock-helpers.ts @@ -0,0 +1,68 @@ +import { expect, type Page } from "@playwright/test"; +import { KanbanPage } from "../../pages/kanban-page"; + +async function bodyLockState(page: Page) { + return page.evaluate(() => ({ + pointerEvents: document.body.style.pointerEvents, + scrollLocked: document.body.hasAttribute("data-scroll-locked"), + })); +} + +export async function verifyStalledDialogCloseRecovery(page: Page, touch: boolean) { + const kanban = new KanbanPage(page); + await kanban.goto(); + await page.addStyleTag({ + content: ` + [data-state="closed"][data-slot="dialog-content"], + [data-state="closed"][data-slot="dialog-overlay"] { + animation-duration: 60s !important; + } + `, + }); + await page.evaluate(() => { + const accordion = document.createElement("div"); + accordion.dataset.slot = "accordion-content"; + accordion.dataset.state = "open"; + accordion.dataset.testid = "body-lock-open-accordion"; + document.body.appendChild(accordion); + }); + + const createTask = touch + ? page.getByTestId("mobile-fab") + : kanban.createTaskButton.filter({ visible: true }).first(); + if (touch) await createTask.tap(); + else await createTask.click(); + + const dialog = page.getByTestId("create-task-dialog"); + await expect(dialog).toBeVisible(); + await expect + .poll(() => bodyLockState(page)) + .toEqual({ + pointerEvents: "none", + scrollLocked: true, + }); + + const close = dialog.getByRole("button", { name: "Close" }); + if (touch) await close.tap(); + else await close.click(); + + await expect(dialog).not.toBeAttached(); + await expect(page.locator('[data-slot="dialog-overlay"]')).not.toBeAttached(); + await expect + .poll(() => bodyLockState(page)) + .toEqual({ + pointerEvents: "", + scrollLocked: false, + }); + + if (touch) await createTask.tap(); + else await createTask.click(); + + await expect(dialog).toBeVisible(); + await expect + .poll(() => bodyLockState(page)) + .toEqual({ + pointerEvents: "none", + scrollLocked: true, + }); +} diff --git a/apps/web/e2e/tests/task/dialog-body-lock.spec.ts b/apps/web/e2e/tests/task/dialog-body-lock.spec.ts new file mode 100644 index 0000000000..d1027510a2 --- /dev/null +++ b/apps/web/e2e/tests/task/dialog-body-lock.spec.ts @@ -0,0 +1,6 @@ +import { test } from "../../fixtures/test-base"; +import { verifyStalledDialogCloseRecovery } from "./dialog-body-lock-helpers"; + +test("a stalled dialog close releases and can reacquire the body lock", async ({ testPage }) => { + await verifyStalledDialogCloseRecovery(testPage, false); +}); diff --git a/apps/web/e2e/tests/task/mobile-dialog-body-lock.spec.ts b/apps/web/e2e/tests/task/mobile-dialog-body-lock.spec.ts new file mode 100644 index 0000000000..3c1c6719eb --- /dev/null +++ b/apps/web/e2e/tests/task/mobile-dialog-body-lock.spec.ts @@ -0,0 +1,6 @@ +import { test } from "../../fixtures/test-base"; +import { verifyStalledDialogCloseRecovery } from "./dialog-body-lock-helpers"; + +test("a stalled dialog close releases and can reacquire the body lock", async ({ testPage }) => { + await verifyStalledDialogCloseRecovery(testPage, true); +}); diff --git a/apps/web/lib/dialog-body-lock-component.test.tsx b/apps/web/lib/dialog-body-lock-component.test.tsx new file mode 100644 index 0000000000..c4b0a15da5 --- /dev/null +++ b/apps/web/lib/dialog-body-lock-component.test.tsx @@ -0,0 +1,31 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { render } from "@testing-library/react"; +import { Dialog } from "@kandev/ui/dialog"; + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("Dialog body-lock recovery subscription", () => { + it("shares one visibility listener across mounted dialog roots", () => { + const addEventListener = vi.spyOn(document, "addEventListener"); + const removeEventListener = vi.spyOn(document, "removeEventListener"); + + const view = render( + <> + + + , + ); + + expect( + addEventListener.mock.calls.filter(([type]) => type === "visibilitychange"), + ).toHaveLength(1); + + view.unmount(); + + expect( + removeEventListener.mock.calls.filter(([type]) => type === "visibilitychange"), + ).toHaveLength(1); + }); +}); diff --git a/apps/web/lib/dialog-body-lock.test.ts b/apps/web/lib/dialog-body-lock.test.ts new file mode 100644 index 0000000000..1eede12f54 --- /dev/null +++ b/apps/web/lib/dialog-body-lock.test.ts @@ -0,0 +1,110 @@ +import { describe, it, expect, beforeEach, vi } from "vitest"; +import { finishClosingDialogAnimations } from "@kandev/ui/lib/dialog-body-lock"; + +/** + * Unit coverage for the modal body-lock recovery used by the base Dialog. + * + * Radix releases `pointer-events: none` on when the dialog content + * unmounts, and Presence defers that unmount until the exit animation fires + * `animationend`. When that event never arrives — the document was hidden while + * closing, so animation timelines were frozen, or the dialog unmounted mid-close + * — the lock outlives its dialog and every control on the page stops responding + * with nothing visible to close. These tests pin the two halves of the contract: + * recovery terminates only closing modal animations and leaves Radix to release + * its own body-lock bookkeeping when those modal owners unmount. + */ +const SCROLL_LOCKED = "data-scroll-locked"; +const DIALOG_CONTENT = "dialog-content"; + +function lockBody() { + document.body.style.pointerEvents = "none"; + document.body.setAttribute(SCROLL_LOCKED, "1"); +} + +function isLocked() { + return document.body.style.pointerEvents === "none" || document.body.hasAttribute(SCROLL_LOCKED); +} + +function animation() { + return { finish: vi.fn() } as unknown as Animation; +} + +/** Mirrors modal/content primitives so selectors see realistic markup. */ +function mountContent( + state: "open" | "closed", + slot = DIALOG_CONTENT, + animations: Animation[] = [], +) { + const el = document.createElement("div"); + el.setAttribute("role", "dialog"); + el.setAttribute("data-slot", slot); + el.setAttribute("data-state", state); + el.getAnimations = () => animations; + document.body.appendChild(el); + return el; +} + +beforeEach(() => { + document.body.innerHTML = ""; + document.body.removeAttribute(SCROLL_LOCKED); + document.body.style.removeProperty("pointer-events"); +}); + +describe("finishClosingDialogAnimations", () => { + it("finishes a closing dialog animation without mutating its owner's body lock", () => { + const exit = animation(); + mountContent("closed", DIALOG_CONTENT, [exit]); + lockBody(); + + expect(finishClosingDialogAnimations(document)).toBe(true); + expect(exit.finish).toHaveBeenCalledOnce(); + expect(isLocked()).toBe(true); + }); + + it("finishes a closing overlay so an invisible portal cannot intercept clicks", () => { + const exit = animation(); + mountContent("closed", "dialog-overlay", [exit]); + + expect(finishClosingDialogAnimations(document)).toBe(true); + expect(exit.finish).toHaveBeenCalledOnce(); + }); + + it("ignores open dialog animations", () => { + const open = animation(); + mountContent("open", DIALOG_CONTENT, [open]); + lockBody(); + + expect(finishClosingDialogAnimations(document)).toBe(false); + expect(open.finish).not.toHaveBeenCalled(); + expect(isLocked()).toBe(true); + }); + + it("finishes the closing owner while preserving a second open modal's lock", () => { + const closing = animation(); + const open = animation(); + mountContent("closed", DIALOG_CONTENT, [closing]); + mountContent("open", "alert-dialog-content", [open]); + lockBody(); + + expect(finishClosingDialogAnimations(document)).toBe(true); + expect(closing.finish).toHaveBeenCalledOnce(); + expect(open.finish).not.toHaveBeenCalled(); + expect(isLocked()).toBe(true); + }); + + it("does not let unrelated open content suppress dialog recovery", () => { + const closing = animation(); + mountContent("closed", DIALOG_CONTENT, [closing]); + mountContent("open", "accordion-content"); + mountContent("open", "collapsible-content"); + lockBody(); + + expect(finishClosingDialogAnimations(document)).toBe(true); + expect(closing.finish).toHaveBeenCalledOnce(); + expect(isLocked()).toBe(true); + }); + + it("reports nothing to do without a closing dialog animation", () => { + expect(finishClosingDialogAnimations(document)).toBe(false); + }); +});