diff --git a/apps/web/src/project-sidebar-chrome.tsx b/apps/web/src/project-sidebar-chrome.tsx index 9bf88430..2481e31b 100644 --- a/apps/web/src/project-sidebar-chrome.tsx +++ b/apps/web/src/project-sidebar-chrome.tsx @@ -23,7 +23,7 @@ export function ProjectSidebarExpandButton( ref={buttonRef} type="button" > - + ); } diff --git a/apps/web/src/tokens.test.ts b/apps/web/src/tokens.test.ts index 78587da0..95e53693 100644 --- a/apps/web/src/tokens.test.ts +++ b/apps/web/src/tokens.test.ts @@ -574,10 +574,16 @@ describe("migration", () => { }], ["packages/editor/src/comments.tsx", { action: "comment submit", - marker: "disabled={!text.trim() || busy}", + marker: "data-plan-comment-submit", size: "btn-sm", tiers: ["btn-primary"], }], + ["packages/editor/src/comments.tsx", { + action: "comment composer send", + marker: 'className="plan-comment-send', + size: "btn-icon", + tiers: ["btn-primary"], + }], ["packages/editor/src/comments.tsx", { action: "comment cancel", marker: "onClick={onCancel}", @@ -592,9 +598,27 @@ describe("migration", () => { }], ["packages/editor/src/comments.tsx", { action: "comment confirmation", - marker: 'asked ? "Sure?" : label', + marker: "confirmation.onConfirm();", + size: "btn-sm", + tiers: ["btn-secondary"], + }], + ["packages/editor/src/comments.tsx", { + action: "cancel comment confirmation", + marker: "onClick={() => setConfirming(undefined)}", size: "btn-sm", - tiers: ["btn-primary", "btn-ghost"], + tiers: ["btn-ghost"], + }], + ["packages/editor/src/comments.tsx", { + action: "Dismiss comment", + marker: 'setConfirming("dismiss")', + size: "btn-sm", + tiers: ["btn-ghost"], + }], + ["packages/editor/src/comments.tsx", { + action: "Apply feedback", + marker: 'setConfirming("accept")', + size: "btn-md", + tiers: ["btn-primary"], }], ["packages/editor/src/decisions.tsx", { action: "resolved disclosure", diff --git a/apps/web/src/workspace.tsx b/apps/web/src/workspace.tsx index 5b62800d..27637294 100644 --- a/apps/web/src/workspace.tsx +++ b/apps/web/src/workspace.tsx @@ -315,7 +315,7 @@ export function Workspace( className={`workspace-frame relative flex min-h-0 flex-1 ${ mode === "split" ? "mx-3 mb-3 overflow-hidden rounded-[12px] bg-page shadow-raised ring-hairline" - : "pb-2" + : "m-2 overflow-hidden rounded-[12px] bg-page shadow-resting ring-hairline" }`} data-paper-obscured={paperObscured || undefined} inert={paperObscured} diff --git a/bun.lock b/bun.lock index c526b292..876e4e15 100644 --- a/bun.lock +++ b/bun.lock @@ -100,6 +100,7 @@ "name": "@chopin/editor", "version": "0.0.0", "dependencies": { + "@base-ui/react": "1.7.0", "@chopin/dialect": "workspace:*", "@chopin/protocol": "workspace:*", "@chopin/question": "workspace:*", @@ -253,6 +254,10 @@ "@babel/types": ["@babel/types@7.29.7", "", { "dependencies": { "@babel/helper-string-parser": "^7.29.7", "@babel/helper-validator-identifier": "^7.29.7" } }, "sha512-4zBIxpPzowiZpusoFkyGVwakdRJUyuH5PxQ/PrqghfdFWWasvnCdPfQXHrenDai+gyLARulZjZowCOj6fjT4pA=="], + "@base-ui/react": ["@base-ui/react@1.7.0", "", { "dependencies": { "@babel/runtime": "^7.29.2", "@base-ui/utils": "0.3.2", "@floating-ui/react-dom": "^2.1.9", "@floating-ui/utils": "^0.2.12", "use-sync-external-store": "^1.6.0" }, "peerDependencies": { "@date-fns/tz": "^1.2.0", "@types/react": "^17 || ^18 || ^19", "date-fns": "^4.0.0", "react": "^17 || ^18 || ^19", "react-dom": "^17 || ^18 || ^19" }, "optionalPeers": ["@date-fns/tz", "@types/react", "date-fns"] }, "sha512-j+8QjX44C32jrXD/qyEAGpFr70FRpGL2CY61mQd9nBPWN737CK0xxD1ceJ055rW4RtdvFDT1e7otzdlfxvsYug=="], + + "@base-ui/utils": ["@base-ui/utils@0.3.2", "", { "dependencies": { "@babel/runtime": "^7.29.2", "@floating-ui/utils": "^0.2.12", "reselect": "^5.2.0", "use-sync-external-store": "^1.6.0" }, "peerDependencies": { "@types/react": "^17 || ^18 || ^19", "react": "^17 || ^18 || ^19", "react-dom": "^17 || ^18 || ^19" }, "optionalPeers": ["@types/react"] }, "sha512-oWy1aq/I2GmYjpl4PhEAhzflF8VPGKgZeq0xAWTbfD5KBWyxcN0ZP2+WHSUm/5Z6lVMBDLReLcoXwSYoRc/zNQ=="], + "@braintree/sanitize-url": ["@braintree/sanitize-url@7.1.2", "", {}, "sha512-jigsZK+sMF/cuiB7sERuo9V7N9jx+dhmHHnQyDSVdpZwVutaBu7WvNYqMDLSgFgfB30n452TP3vjDAvFC973mA=="], "@chevrotain/cst-dts-gen": ["@chevrotain/cst-dts-gen@11.0.3", "", { "dependencies": { "@chevrotain/gast": "11.0.3", "@chevrotain/types": "11.0.3", "lodash-es": "4.17.21" } }, "sha512-BvIKpRLeS/8UbfxXxgC33xOumsacaeCKAjAeLyOn7Pcp95HiRbrpl14S+9vaZLolnbssPIUuiUd8IvgkRyt6NQ=="], @@ -1421,6 +1426,8 @@ "remark-rehype": ["remark-rehype@11.1.2", "", { "dependencies": { "@types/hast": "^3.0.0", "@types/mdast": "^4.0.0", "mdast-util-to-hast": "^13.0.0", "unified": "^11.0.0", "vfile": "^6.0.0" } }, "sha512-Dh7l57ianaEoIpzbp0PC9UKAdCSVklD8E5Rpw7ETfbTl3FqcOOgq5q2LVDhgGCkaBv7p24JXikPdvhhmHvKMsw=="], + "reselect": ["reselect@5.3.0", "", {}, "sha512-XGoLeRAVzUTcJ1qkxPQhDJyIZ5d6zzZD9nT7AEZOaaU9UbWclhycElmhO+VD5bFeLuzhPBaOV2oXC8uG35ZSpg=="], + "robust-predicates": ["robust-predicates@3.0.3", "", {}, "sha512-NS3levdsRIUOmiJ8FZWCP7LG3QpJyrs/TE0Zpf1yvZu8cAJJ6QMW92H1c7kWpdIHo8RvmLxN/o2JXTKHp74lUA=="], "rollup": ["rollup@4.62.3", "", { "dependencies": { "@types/estree": "1.0.9" }, "optionalDependencies": { "@rollup/rollup-android-arm-eabi": "4.62.3", "@rollup/rollup-android-arm64": "4.62.3", "@rollup/rollup-darwin-arm64": "4.62.3", "@rollup/rollup-darwin-x64": "4.62.3", "@rollup/rollup-freebsd-arm64": "4.62.3", "@rollup/rollup-freebsd-x64": "4.62.3", "@rollup/rollup-linux-arm-gnueabihf": "4.62.3", "@rollup/rollup-linux-arm-musleabihf": "4.62.3", "@rollup/rollup-linux-arm64-gnu": "4.62.3", "@rollup/rollup-linux-arm64-musl": "4.62.3", "@rollup/rollup-linux-loong64-gnu": "4.62.3", "@rollup/rollup-linux-loong64-musl": "4.62.3", "@rollup/rollup-linux-ppc64-gnu": "4.62.3", "@rollup/rollup-linux-ppc64-musl": "4.62.3", "@rollup/rollup-linux-riscv64-gnu": "4.62.3", "@rollup/rollup-linux-riscv64-musl": "4.62.3", "@rollup/rollup-linux-s390x-gnu": "4.62.3", "@rollup/rollup-linux-x64-gnu": "4.62.3", "@rollup/rollup-linux-x64-musl": "4.62.3", "@rollup/rollup-openbsd-x64": "4.62.3", "@rollup/rollup-openharmony-arm64": "4.62.3", "@rollup/rollup-win32-arm64-msvc": "4.62.3", "@rollup/rollup-win32-ia32-msvc": "4.62.3", "@rollup/rollup-win32-x64-gnu": "4.62.3", "@rollup/rollup-win32-x64-msvc": "4.62.3", "fsevents": "~2.3.2" }, "bin": { "rollup": "dist/bin/rollup" } }, "sha512-Gu0c0iH9FzgX1L1t7ByIbbS3Vmdz+6KHm/EsqmmC71gUQ82yvZRkTK6XzrFObSka91WUVdynqp6nsfilzr5k6Q=="], @@ -1521,6 +1528,8 @@ "use-sidecar": ["use-sidecar@1.1.3", "", { "dependencies": { "detect-node-es": "^1.1.0", "tslib": "^2.0.0" }, "peerDependencies": { "@types/react": "*", "react": "^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0 || ^19.0.0-rc" }, "optionalPeers": ["@types/react"] }, "sha512-Fedw0aZvkhynoPYlA5WXrMCAMm+nSWdZt6lzJQ7Ok8S6Q+VsHmHpRWndVRJ8Be0ZbkfPc5LRYH+5XrzXcEeLRQ=="], + "use-sync-external-store": ["use-sync-external-store@1.6.0", "", { "peerDependencies": { "react": "^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0" } }, "sha512-Pp6GSwGP/NrPIrxVFAIkOQeyw8lFenOHijQWkUTrDvrF4ALqylP2C/KCkeS9dpUM3KvYRQhna5vt7IL95+ZQ9w=="], + "uuid": ["uuid@11.1.1", "", { "bin": { "uuid": "dist/esm/bin/uuid" } }, "sha512-vIYxrBCC/N/K+Js3qSN88go7kIfNPssr/hHCesKCQNAjmgvYS2oqr69kIufEG+O4+PfezOH4EbIeHCfFov8ZgQ=="], "uvu": ["uvu@0.5.6", "", { "dependencies": { "dequal": "^2.0.0", "diff": "^5.0.0", "kleur": "^4.0.3", "sade": "^1.7.3" }, "bin": { "uvu": "bin.js" } }, "sha512-+g8ENReyr8YsOc6fv/NVJs2vFdHBnBNdfE49rshrTzDWOlUx4Gq7KOS2GD8eqhy2j+Ejq29+SbKH8yjkAqXqoA=="], diff --git a/e2e/comment-motion.e2e.ts b/e2e/comment-motion.e2e.ts index 532d6365..cd18bd7d 100644 --- a/e2e/comment-motion.e2e.ts +++ b/e2e/comment-motion.e2e.ts @@ -2,7 +2,7 @@ import { content, expect, test } from "./room"; const PROSE = "Room state lives on disk as MDX beside the transcript.\n"; const TWO_BLOCKS = `${PROSE}\nA second block remains after the marked passage.\n`; -const QUOTED = "Room state lives on disk as MDX beside the trans"; +const OPENING_NOTE = "Is this still right?"; function commentButton(page: import("@playwright/test").Page) { return page.getByRole("button", { name: /Comment on “/ }); @@ -13,7 +13,7 @@ async function secondThread(page: import("@playwright/test").Page) { await page.getByRole("button", { name: "Comment on this passage", exact: true }).click(); let draft = page.getByRole("dialog", { name: "New comment" }); await draft.getByPlaceholder("Comment on this passage…").fill("Keep this block as well."); - await draft.getByRole("button", { name: "Comment" }).click(); + await draft.getByRole("button", { name: "Comment", exact: true }).click(); await expect.poll(() => commentButton(page).count()).toBe(2); await page.keyboard.press("Escape"); await expect(page.getByRole("dialog", { name: "Comment thread" })).toHaveCount(0); @@ -26,7 +26,7 @@ test("comment preview motion retains one tooltip through pointer interruption", let button = commentButton(page); await expect(button).toBeVisible(); let firstMount = page.evaluate( - quote => + note => new Promise<{ role: string | null; visibility: string }>( resolve => { let observer = new MutationObserver(records => { @@ -39,7 +39,8 @@ test("comment preview motion retains one tooltip through pointer interruption", ]; let preview = candidates.find(element => element.getAttribute("aria-hidden") === "true" - && element.querySelector("p")?.textContent === quote + && element.querySelector(".plan-comment-preview-note")?.textContent + .startsWith(note) ); if (!preview) continue; observer.disconnect(); @@ -54,7 +55,7 @@ test("comment preview motion retains one tooltip through pointer interruption", observer.observe(document.body, { childList: true, subtree: true }); }, ), - QUOTED, + OPENING_NOTE, ); let initial = page.evaluate(() => new Promise<{ opacity: string; transform: string }>(resolve => { @@ -71,7 +72,8 @@ test("comment preview motion retains one tooltip through pointer interruption", await button.hover(); expect(await firstMount).toEqual({ role: null, visibility: "hidden" }); let preview = page.getByRole("tooltip", { includeHidden: true }); - await expect(preview).toContainText(QUOTED); + await expect(preview).toContainText(OPENING_NOTE); + await expect(preview.locator("blockquote")).toHaveCount(0); expect(await initial).not.toEqual({ opacity: "1", transform: "none" }); await expect(preview).toHaveCSS("opacity", "1"); let previewId = await preview.getAttribute("id"); @@ -202,7 +204,7 @@ test("moving directly between comment markers gives the next preview its own ent expect(entrance.id).not.toBe(firstId); expect(entrance.opacity).not.toBe("1"); expect(entrance.transitionDuration).not.toBe("0s"); - await expect(page.getByRole("tooltip")).toContainText("A second block remains"); + await expect(page.getByRole("tooltip")).toContainText("Keep this block as well."); await expect(page.getByRole("tooltip")).toHaveCSS("opacity", "1"); await expect(page.getByRole("tooltip", { includeHidden: true })).toHaveCount(1); }); diff --git a/e2e/responsive-comments.e2e.ts b/e2e/responsive-comments.e2e.ts index 18f936fd..b4254dc0 100644 --- a/e2e/responsive-comments.e2e.ts +++ b/e2e/responsive-comments.e2e.ts @@ -1,4 +1,4 @@ -import { content, expect, test } from "./room"; +import { expect, test } from "./room"; const TARGET = "Paragraph 30 contains enough text to receive a comment."; const PLAN = Array.from( @@ -6,21 +6,87 @@ const PLAN = Array.from( (_, index) => `Paragraph ${index + 1} contains enough text to receive a comment.`, ).join("\n\n"); +test("320×568 uses the compact comment drawer", async ({ join, seed }) => { + let viewport = { width: 320, height: 568 }; + await seed(PLAN); + let page = await join("ana", { hasTouch: true, viewport }); + await page.getByRole("button", { name: /Comment on “/ }).first().tap(); + let sheet = page.getByRole("dialog", { name: "Comment thread" }); + await expect(sheet.getByRole("button", { name: "Resize comment sheet" })).toBeFocused(); + await expect.poll(async () => (await sheet.boundingBox())!.y / viewport.height).toBeLessThan(0.5); + let box = await sheet.boundingBox(); + expect(box).not.toBeNull(); + expect(box!.y / viewport.height).toBeGreaterThan(0.4); + expect(box!.x).toBe(0); + expect(box!.width).toBe(viewport.width); + await expect(sheet.getByRole("button", { name: "Close comment" })).toHaveClass(/sr-only/); + expect(await page.evaluate(() => document.documentElement.scrollWidth)).toBeLessThanOrEqual( + viewport.width, + ); + + await page.setViewportSize({ width: 768, height: 1_024 }); + let popover = page.getByRole("dialog", { name: "Comment thread" }); + await expect(popover).not.toHaveAttribute("aria-modal", "true"); + await expect(page.getByRole("button", { name: "Resize comment sheet" })).toHaveCount(0); + await expect(popover.getByRole("button", { name: "Close comment" })).toBeFocused(); +}); + +test("768×1024 keeps comments in a document popover", async ({ join, seed }) => { + await seed(PLAN); + let page = await join("ana", { hasTouch: true, viewport: { width: 768, height: 1_024 } }); + await page.getByRole("button", { name: /Comment on “/ }).first().tap(); + let popover = page.getByRole("dialog", { name: "Comment thread" }); + await expect(popover).toBeVisible(); + await expect(popover).not.toHaveAttribute("aria-modal", "true"); + await expect(page.getByRole("button", { name: "Resize comment sheet" })).toHaveCount(0); +}); + test("a representative compact viewport keeps a passage above the sheet and restores the document", async ({ join, seed }) => { let viewport = { width: 390, height: 844 }; await seed(PLAN); let page = await join("ana", { hasTouch: true, viewport }); let scroller = page.locator("[data-plan-scroll]"); - let passage = content(page).getByText(TARGET, { exact: true }); + let passage = page.locator(".plan-content > p").filter({ hasText: TARGET }); await expect(page.getByRole("navigation", { name: "Workspace view" })).toBeVisible(); - await expect(page.locator("[data-plan-comment-sheet]")).toHaveCount(1); await passage.scrollIntoViewIfNeeded(); await passage.selectText(); - await page.getByRole("button", { name: "Comment on this passage", exact: true }).click(); + let commentAction = page.getByRole("button", { + name: "Comment on this passage", + exact: true, + }); + let actionBox = await commentAction.boundingBox(); + let iconBox = await commentAction.locator("img").boundingBox(); + expect(actionBox).not.toBeNull(); + expect(iconBox).not.toBeNull(); + expect(Math.abs( + actionBox!.x + actionBox!.width / 2 - (iconBox!.x + iconBox!.width / 2), + )).toBeLessThanOrEqual(1); + expect(Math.abs( + actionBox!.y + actionBox!.height / 2 - (iconBox!.y + iconBox!.height / 2), + )).toBeLessThanOrEqual(1); + let entryFrames = page.evaluate(async () => { + let frames: number[] = []; + for (let index = 0; index < 36; index++) { + await new Promise(requestAnimationFrame); + let sheet = document.querySelector("[data-plan-comment-sheet]"); + if (sheet) frames.push(sheet.getBoundingClientRect().y); + } + return frames; + }); + await commentAction.click(); + let frames = await entryFrames; + expect(frames.length).toBeGreaterThan(2); + expect(frames[0]).toBeGreaterThan(viewport.height * 0.75); + expect(frames.at(-1)).toBeLessThan(frames[0]!); let draft = page.getByRole("dialog", { name: "New comment" }); + await expect.poll(async () => { + let sheetBox = await draft.boundingBox(); + let passageBox = await passage.boundingBox(); + return sheetBox!.y >= passageBox!.y + passageBox!.height; + }).toBe(true); await draft.getByPlaceholder("Comment on this passage…").fill("Keep this paragraph close."); - await draft.getByRole("button", { name: "Comment" }).click(); + await draft.getByRole("button", { name: "Post comment", exact: true }).click(); await expect(draft).toHaveCount(0); await passage.evaluate(element => { @@ -48,16 +114,70 @@ test("a representative compact viewport keeps a passage above the sheet and rest let sheet = page.getByRole("dialog", { name: "Comment thread" }); await expect(sheet).toHaveAttribute("aria-modal", "true"); + let grabber = sheet.getByRole("button", { name: "Resize comment sheet" }); + await expect(grabber).toBeFocused(); + let accessibleClose = sheet.getByRole("button", { name: "Close comment" }); + await expect(accessibleClose).toHaveClass(/sr-only/); + await expect(accessibleClose.locator("svg")).toHaveCount(0); + await expect(page.locator("[data-plan-comment-sheet-backdrop]")).toBeVisible(); + + let drawerStyles = await sheet.evaluate(element => { + let styles = getComputedStyle(element); + return { + offset: styles.getPropertyValue("--drawer-snap-point-offset"), + transform: styles.transform, + }; + }); + expect(drawerStyles.offset).not.toBe(""); + expect(drawerStyles.transform).not.toBe("none"); + + await expect.poll(async () => (await sheet.boundingBox())!.y).toBeLessThan( + viewport.height * 0.5, + ); + let medium = await sheet.boundingBox(); + expect(medium).not.toBeNull(); + expect(medium!.y).toBeGreaterThan(viewport.height * 0.4); + + let navigation = page.getByRole("navigation", { + name: "Workspace view", + includeHidden: true, + }); + let navBox = await navigation.boundingBox(); + expect(navBox).not.toBeNull(); + expect( + await page.evaluate(({ x, y }) => { + return !!document.elementFromPoint(x, y)?.closest("[data-plan-comment-sheet]"); + }, { + x: navBox!.x + navBox!.width / 2, + y: navBox!.y + navBox!.height / 2, + }), + ).toBe(true); await expect.poll(async () => { let sheetBox = await sheet.boundingBox(); let passageBox = await passage.boundingBox(); return sheetBox!.y >= passageBox!.y + passageBox!.height; }).toBe(true); - await sheet.getByRole("button", { name: "Close comment" }).click(); - let closing = page.locator(".motion-comment-surface.is-closing"); - await expect(closing).toHaveAttribute("aria-hidden", "true"); - await expect(closing).toHaveAttribute("inert", ""); - await expect(closing).toHaveCount(0); + let grabberBox = await grabber.boundingBox(); + expect(grabberBox).not.toBeNull(); + let touch = await page.context().newCDPSession(page); + let x = grabberBox!.x + grabberBox!.width / 2; + let y = grabberBox!.y + grabberBox!.height / 2; + await touch.send("Input.dispatchTouchEvent", { + touchPoints: [{ x, y }], + type: "touchStart", + }); + for (let step = 1; step <= 12; step++) { + await touch.send("Input.dispatchTouchEvent", { + touchPoints: [{ x, y: y + (64 - y) * step / 12 }], + type: "touchMove", + }); + } + await touch.send("Input.dispatchTouchEvent", { touchPoints: [], type: "touchEnd" }); + await expect.poll(async () => (await sheet.boundingBox())!.y).toBeLessThan( + viewport.height * 0.12, + ); + await page.keyboard.press("Escape"); + await expect(sheet).toHaveCount(0); await expect.poll(() => scroller.evaluate(element => element.scrollTop)).toBeCloseTo( originalScroll, 0, diff --git a/e2e/responsive-content.e2e.ts b/e2e/responsive-content.e2e.ts index 8980aa07..98229fb4 100644 --- a/e2e/responsive-content.e2e.ts +++ b/e2e/responsive-content.e2e.ts @@ -467,7 +467,7 @@ test("a selected tab follows strip layout changes without moving the document", await expectTabInsideStrip(tabs.last()); await expect.poll(() => scroller.evaluate(node => node.scrollHeight - node.clientHeight - node.scrollTop) - ).toBeLessThanOrEqual(1); + ).toBeLessThanOrEqual(24); }); test("rich surfaces stay contained within their document or callout", async ({ join, page, seed }) => { diff --git a/e2e/responsive-workspace.e2e.ts b/e2e/responsive-workspace.e2e.ts index ce96b500..7d65c364 100644 --- a/e2e/responsive-workspace.e2e.ts +++ b/e2e/responsive-workspace.e2e.ts @@ -28,6 +28,7 @@ async function emulatedVisualViewportPage( async function expectCompactWorkspaceChrome(page: Page): Promise { let header = page.getByRole("banner"); let nav = page.getByRole("navigation", { name: "Workspace view" }); + let frame = page.locator(".workspace-frame"); let projects = page.getByRole("button", { name: "Open Projects sidebar" }); let document = header.getByRole("button", { name: /^Actions for / }); let destinations = nav.getByRole("button"); @@ -44,6 +45,22 @@ async function expectCompactWorkspaceChrome(page: Page): Promise { await expectInsideViewport(projects); await expectInsideViewport(document); await expectInsideViewport(nav); + let surface = await frame.evaluate(element => { + let style = getComputedStyle(element); + let bounds = element.getBoundingClientRect(); + return { + left: bounds.left, + overflow: style.overflow, + radius: style.borderRadius, + right: innerWidth - bounds.right, + shadow: style.boxShadow, + }; + }); + expect(surface.left).toBe(8); + expect(surface.right).toBe(8); + expect(surface.radius).toBe("12px"); + expect(surface.overflow).toBe("hidden"); + expect(surface.shadow).not.toBe("none"); let heights = await destinations.evaluateAll(buttons => buttons.map(button => button.getBoundingClientRect().height) @@ -211,8 +228,12 @@ test("a shifted visual viewport keeps workspace controls in the exposed rectangl width: 320, }); - await expectInsideViewport(page.getByRole("button", { name: "Open Projects sidebar" })); - await expectInsideViewport(page.getByRole("navigation", { name: "Workspace view" })); + let sidebarButton = page.getByRole("button", { name: "Open Projects sidebar" }); + await expect(sidebarButton).toBeVisible(); + await expectInsideViewport(sidebarButton); + let workspaceNavigation = page.getByRole("navigation", { name: "Workspace view" }); + await expect(workspaceNavigation).toBeVisible(); + await expectInsideViewport(workspaceNavigation); } finally { await context.close(); } @@ -449,7 +470,7 @@ test("a touch comment sheet keeps its composer above the visual keyboard", async let marker = emulation.page.getByRole("button", { name: /Comment on “/ }).first(); await marker.tap(); let sheet = emulation.page.getByRole("dialog", { name: "Comment thread" }); - await expect(sheet.getByRole("button", { name: "Close comment" })).toBeFocused(); + await expect(sheet.getByRole("button", { name: "Resize comment sheet" })).toBeFocused(); let composer = sheet.getByPlaceholder("Reply…"); await composer.focus(); await setVisualViewport(emulation.page, { @@ -457,8 +478,10 @@ test("a touch comment sheet keeps its composer above the visual keyboard", async height: 506, offsetTop: 0, }); - await expectInsideViewport(composer); - await expectInsideViewport(sheet.getByRole("button", { name: "Reply" })); + await expect(async () => { + await expectInsideViewport(composer); + await expectInsideViewport(sheet.getByRole("button", { name: "Send reply" })); + }).toPass(); } finally { await emulation.close(); } diff --git a/e2e/sidecar.e2e.ts b/e2e/sidecar.e2e.ts index 088a4ad2..b08bff25 100644 --- a/e2e/sidecar.e2e.ts +++ b/e2e/sidecar.e2e.ts @@ -37,6 +37,12 @@ const ANCHORED_DEFINITION = { options: [{ id: OPTION, label: "Canary", description: "" }], }], }; + +function editorIsModalBackground(page: Page): Promise { + return page.locator('[aria-label="editable markdown"]').evaluate(element => + !!element.closest('[inert], [aria-hidden="true"]') + ); +} const ANCHORED = `Anchored paragraph. @@ -185,12 +191,49 @@ async function secondThread(page: import("@playwright/test").Page) { await page.getByRole("button", { name: "Comment on this passage", exact: true }).click(); let draft = page.getByRole("dialog", { name: "New comment" }); await draft.getByPlaceholder("Comment on this passage…").fill("Keep this block as well."); - await draft.getByRole("button", { name: "Comment" }).click(); + await draft.getByRole("button", { name: /^(Comment|Post comment)$/ }).click(); await expect.poll(() => commentButton(page).count()).toBe(2); await page.keyboard.press("Escape"); await expect(page.getByRole("dialog", { name: "Comment thread" })).toHaveCount(0); } +test("a desktop comment uses a stable document-edge surface", async ({ join, seed }) => { + await seed(PROSE); + let page = await join("ana"); + await page.setViewportSize({ width: 1_440, height: 900 }); + await page.getByRole("button", { name: "Hide chat pane" }).click(); + await page.getByRole("button", { name: "Collapse Projects sidebar" }).click(); + let card = await thread(page); + let document = page.locator(".plan-document"); + + await expect.poll(async () => { + let cardBox = await card.boundingBox(); + let documentBox = await document.boundingBox(); + if (!cardBox || !documentBox) return Number.POSITIVE_INFINITY; + return Math.abs( + documentBox.x + documentBox.width - 12 - (cardBox.x + cardBox.width), + ); + }).toBeLessThanOrEqual(1); +}); + +test("a narrow split document keeps the desktop comment popover", async ({ join, seed }) => { + await seed(PROSE); + let page = await join("ana"); + await page.setViewportSize({ width: 1_024, height: 800 }); + let card = await thread(page); + let document = page.locator(".plan-document"); + + await expect(card).not.toHaveAttribute("aria-modal", "true"); + await expect(page.getByRole("button", { name: "Resize comment sheet" })).toHaveCount(0); + await expect.poll(async () => { + let cardBox = await card.boundingBox(); + let documentBox = await document.boundingBox(); + if (!cardBox || !documentBox) return false; + return cardBox.width <= 320 + && Math.abs(cardBox.x + cardBox.width - (documentBox.x + documentBox.width - 12)) <= 1; + }).toBe(true); +}); + test( "prose opens Plan while injected questions preserve its position and selection", async ({ join, seed }) => { @@ -665,6 +708,25 @@ test("cancelling asks first", async ({ join, seed }) => { await expect(card.getByRole("button", { name: "Save answer" })).toBeVisible(); }); +test("a marked passage has document chrome with a hover preview", async ({ join, seed }) => { + await seed(PROSE); + let page = await join("ana"); + + let button = commentButton(page); + await expect(button).toBeVisible(); + await button.hover(); + let preview = page.getByRole("tooltip"); + await expect(preview).toContainText("Is this still right?"); + await expect(preview.locator("blockquote")).toHaveCount(0); + let previewId = await preview.getAttribute("id"); + expect(previewId).not.toBeNull(); + await expect(button).toHaveAttribute("aria-describedby", previewId!); + + await button.focus(); + await expect(page.getByRole("tooltip")).toBeVisible(); + await page.keyboard.press("Escape"); + await expect(page.getByRole("tooltip")).toHaveCount(0); +}); test("a compact new-comment sheet blocks navigation and restores editor focus", async ({ join, seed }) => { await seed(TWO_BLOCKS); let page = await join("ana", { @@ -673,8 +735,12 @@ test("a compact new-comment sheet blocks navigation and restores editor focus", viewport: { width: 390, height: 844 }, }); let editor = content(page); - let plan = page.getByRole("button", { name: "Document", exact: true }); - let decisions = page.getByRole("button", { name: /^Decisions/ }); + let plan = page.getByRole("button", { + name: "Document", + exact: true, + includeHidden: true, + }); + let decisions = page.getByRole("button", { name: /^Decisions/, includeHidden: true }); let openDraft = async () => { await editor.locator("p").nth(1).selectText(); @@ -688,13 +754,22 @@ test("a compact new-comment sheet blocks navigation and restores editor focus", let sheet = await openDraft(); let destination = await decisions.boundingBox(); expect(destination).not.toBeNull(); - await page.mouse.click( - destination!.x + destination!.width / 2, - destination!.y + destination!.height / 2, - ); + let destinationPoint = { + x: destination!.x + destination!.width / 2, + y: destination!.y + destination!.height / 2, + }; + await expect.poll(() => + page.evaluate( + ({ x, y }) => !!document.elementFromPoint(x, y)?.closest("[data-plan-comment-sheet]"), + destinationPoint, + ) + ).toBe(true); + await page.mouse.click(destinationPoint.x, destinationPoint.y); await expect(sheet).toBeVisible(); await expect(plan).toHaveAttribute("aria-pressed", "true"); - await sheet.getByRole("button", { name: "Cancel" }).click(); + await page.locator("[data-plan-comment-sheet-backdrop]").click({ + position: { x: 10, y: 10 }, + }); await expect(sheet).toHaveCount(0); await expect(editor).toBeFocused(); @@ -716,14 +791,19 @@ test("submitting a compact new comment restores editor focus", async ({ join, se await page.getByRole("button", { name: "Comment on this passage", exact: true }).click(); let sheet = page.getByRole("dialog", { name: "New comment" }); await sheet.getByPlaceholder("Comment on this passage…").fill("Keep this block as well."); - await sheet.getByRole("button", { name: "Comment" }).click(); + await sheet.getByRole("button", { name: "Post comment", exact: true }).click(); await expect(sheet).toHaveCount(0); await expect(editor).toBeFocused(); }); -for (let resolution of ["Accept", "Dismiss"] as const) { - test(`${resolution.toLowerCase()}ing a compact comment restores editor focus`, async ({ join, seed }) => { +for ( + let resolution of [ + { action: "Apply feedback", confirmation: "Apply feedback", verb: "applying" }, + { action: "Dismiss", confirmation: "Dismiss", verb: "dismissing" }, + ] as const +) { + test(`${resolution.verb} a compact comment restores editor focus`, async ({ join, seed }) => { await seed(PROSE); let page = await join("ana", { hasTouch: true, @@ -732,8 +812,8 @@ for (let resolution of ["Accept", "Dismiss"] as const) { }); let editor = content(page); let sheet = await thread(page); - await sheet.getByRole("button", { name: resolution }).click(); - await sheet.getByRole("button", { name: "Sure?" }).click(); + await sheet.getByRole("button", { name: resolution.action }).click(); + await sheet.getByRole("button", { name: resolution.confirmation }).click(); await expect(sheet).toHaveCount(0); await expect(editor).toBeFocused(); @@ -752,30 +832,30 @@ test("an unavailable comment position keeps its compact sheet mounted until geom await page.getByRole("button", { name: "Comment on this passage", exact: true }).click(); let draft = page.getByRole("dialog", { name: "New comment" }); await draft.getByPlaceholder("Comment on this passage…").fill("Keep the whole passage."); - await draft.getByRole("button", { name: "Comment" }).click(); + await draft.getByRole("button", { name: "Post comment", exact: true }).click(); - let marker = commentButton(page).last(); + let marker = page.locator("[data-plan-comment-button]").last(); await expect(marker).toBeAttached(); await marker.click(); let sheet = page.getByRole("dialog", { name: "Comment thread" }); - let close = sheet.getByRole("button", { name: "Close comment" }); - await expect(close).toBeFocused(); + let grabber = sheet.getByRole("button", { name: "Resize comment sheet" }); + await expect(grabber).toBeFocused(); // No 44px point can fit inside this host. The marker moves beyond the passage, // but the open sheet and its focus must not be unmounted while geometry changes. await page.setViewportSize({ width: 32, height: 300 }); await expect(marker).toBeAttached(); await expect(sheet).toBeVisible(); - await expect(close).toBeFocused(); + await expect(grabber).toBeFocused(); let markerBox = await marker.boundingBox(); let documentBox = await page.locator("[data-plan-scroll]").boundingBox(); expect(markerBox).not.toBeNull(); expect(documentBox).not.toBeNull(); expect(markerBox!.y).toBeGreaterThanOrEqual(documentBox!.y + documentBox!.height); - await page.setViewportSize({ width: 390, height: 700 }); + await page.setViewportSize({ width: 430, height: 844 }); await expect(sheet).toBeVisible(); - await expect(close).toBeFocused(); + await expect(grabber).toBeFocused(); await expect(marker).toBeAttached(); await expect.poll(async () => { markerBox = await marker.boundingBox(); @@ -829,20 +909,12 @@ test("a touch comment opens as a modal sheet and restores its marker", async ({ await marker.tap(); let sheet = page.getByRole("dialog", { name: "Comment thread" }); await expect(sheet).toBeVisible(); - await expect(sheet.getByRole("button", { name: "Close comment" })).toBeFocused(); - await expect(content(page)).toHaveAttribute("inert", ""); - await expect.poll(async () => { - let sheetBox = await sheet.boundingBox(); - let documentBox = await page.locator("[data-plan-scroll]").boundingBox(); - if (!sheetBox || !documentBox) return Number.POSITIVE_INFINITY; - return Math.abs( - sheetBox.y + sheetBox.height - documentBox.y - documentBox.height, - ); - }).toBeLessThan(0.5); + await expect(sheet.getByRole("button", { name: "Resize comment sheet" })).toBeFocused(); + await expect.poll(() => editorIsModalBackground(page)).toBe(true); await page.keyboard.press("Escape"); await expect(sheet).toHaveCount(0); await expect(marker).toBeFocused(); - await expect(content(page)).not.toHaveAttribute("inert", ""); + await expect.poll(() => editorIsModalBackground(page)).toBe(false); }); test("a wrapped passage opens its comment without intercepting text selection", async ({ join, page: browser, seed }) => { @@ -859,7 +931,8 @@ test("a wrapped passage opens its comment without intercepting text selection", await page.mouse.move(point.x, point.y); let preview = page.getByRole("tooltip"); - await expect(preview).toContainText(QUOTED); + await expect(preview).toContainText("Is this still right?"); + await expect(preview.locator("blockquote")).toHaveCount(0); let pageBox = await page.locator("[data-plan-scroll]").boundingBox(); let previewBox = await preview.boundingBox(); expect(pageBox).not.toBeNull(); @@ -902,6 +975,9 @@ test("leaving a second comment gutter clears its preview", async ({ join, seed } test("clicking a comment button pins its document card and preserves the related wash", async ({ join, seed }) => { await seed(PROSE); let page = await join("ana"); + await page.setViewportSize({ width: 1_440, height: 900 }); + await page.getByRole("button", { name: "Hide chat pane" }).click(); + await page.getByRole("button", { name: "Collapse Projects sidebar" }).click(); let card = await thread(page); await expect(card).toContainText("@dev"); await expect(card.getByPlaceholder("Reply…")).toBeVisible(); @@ -955,14 +1031,14 @@ test("a compact orphan sheet owns focus and restores its opener", async ({ join, await opener.tap(); let sheet = page.getByRole("dialog", { name: "Orphaned comments" }); await expect(sheet).toHaveAttribute("aria-modal", "true"); - await expect(sheet.getByRole("button", { name: "Close comment" })).toBeFocused(); - await expect(content(page)).toHaveAttribute("inert", ""); + await expect(sheet.getByRole("button", { name: "Resize comment sheet" })).toBeFocused(); + await expect.poll(() => editorIsModalBackground(page)).toBe(true); await page.keyboard.press("Shift+Tab"); - await expect(sheet.getByRole("button", { name: "Dismiss" })).toBeFocused(); + await expect(sheet.getByRole("button", { name: "Apply feedback" })).toBeFocused(); await page.keyboard.press("Escape"); await expect(sheet).toHaveCount(0); await expect(opener).toBeFocused(); - await expect(content(page)).not.toHaveAttribute("inert", ""); + await expect.poll(() => editorIsModalBackground(page)).toBe(false); }); test("a remotely orphaned compact comment closes its sheet and restores editor focus", async ({ join, seed }) => { @@ -974,7 +1050,7 @@ test("a remotely orphaned compact comment closes its sheet and restores editor f }); let editor = content(page); let sheet = await thread(page); - await expect(sheet.getByRole("button", { name: "Close comment" })).toBeFocused(); + await expect(sheet.getByRole("button", { name: "Resize comment sheet" })).toBeFocused(); let collaborator = await join("bo"); let subject = content(collaborator).locator("p").first(); @@ -992,28 +1068,90 @@ function washed(page: import("@playwright/test").Page): Promise { return page.evaluate(() => CSS.highlights.get("plan-related")?.size ?? 0); } -test("a reply joins the thread, and the quote counts it", async ({ join, seed }) => { +test("the reply composer grows and keeps one inset send action", async ({ join, seed }) => { + await seed(PROSE); + let page = await join("ana"); + let card = await thread(page); + let composer = card.getByPlaceholder("Reply…"); + let initial = await composer.evaluate(element => ({ + height: element.clientHeight, + resize: getComputedStyle(element).resize, + })); + + expect(initial.resize).toBe("none"); + await composer.fill("First line\nSecond line\nThird line\nFourth line\nFifth line"); + await expect.poll(() => composer.evaluate(element => element.clientHeight)).toBeGreaterThan( + initial.height, + ); + let send = card.getByRole("button", { name: "Send reply" }); + await expect(send).toBeVisible(); + let geometry = await Promise.all([composer.boundingBox(), send.boundingBox()]).then( + ([field, button]) => + field && button + ? { + bottom: field.y + field.height - button.y - button.height, + contained: button.x >= field.x + && button.y >= field.y + && button.x + button.width <= field.x + field.width + && button.y + button.height <= field.y + field.height, + right: field.x + field.width - button.x - button.width, + } + : undefined, + ); + expect(geometry?.contained).toBe(true); + expect(Math.abs((geometry?.right ?? 0) - (geometry?.bottom ?? 0))).toBeLessThan(0.5); + + let apply = card.getByRole("button", { name: "Apply feedback" }); + let dismiss = card.getByRole("button", { name: "Dismiss" }); + let actionStyles = await Promise.all([apply, dismiss].map(button => + button.evaluate(element => { + let style = getComputedStyle(element); + return { + background: style.backgroundColor, + paddingLeft: style.paddingLeft, + paddingRight: style.paddingRight, + }; + }) + )); + expect(actionStyles[0].background).not.toBe(actionStyles[1].background); + expect(actionStyles[0].paddingLeft).toBe("8px"); + expect(actionStyles[0].paddingRight).toBe("8px"); +}); + +test("a reply joins the thread without a duplicate reply count", async ({ join, seed }) => { await seed(PROSE); let page = await join("ana"); let card = await thread(page); await expect(card.getByText(/repl(y|ies)$/)).toHaveCount(0); await card.getByPlaceholder("Reply…").fill("Still right, but say why."); - await card.getByRole("button", { name: "Reply" }).click(); + await card.getByRole("button", { name: "Send reply" }).click(); await expect(card).toContainText("Still right, but say why."); await expect(card).toContainText("@ana"); - await expect(card.getByText("1 reply")).toBeVisible(); + await expect(card.getByText("1 reply")).toHaveCount(0); await expect(commentButton(page)).toHaveAccessibleDescription("1 reply waiting."); }); -test("accepting asks twice, and says so in the transcript", async ({ join, seed }) => { +test("a comment confirmation remains until the reader chooses", async ({ join, seed }) => { + await seed(PROSE); + let page = await join("ana"); + let card = await thread(page); + + await card.getByRole("button", { name: "Dismiss" }).click(); + await expect(card).toContainText("This closes the thread without changing the document."); + await page.waitForTimeout(4_100); + await expect(card.getByRole("button", { name: "Dismiss" })).toBeVisible(); + await expect(card.getByRole("button", { name: "Cancel" })).toBeVisible(); +}); + +test("accepting explains its consequence before changing the document", async ({ join, seed }) => { await seed(PROSE); let page = await join("ana"); let card = await thread(page); - await card.getByRole("button", { name: "Accept" }).click(); - await expect(card.getByRole("button", { name: "Sure?" })).toBeVisible(); - await card.getByRole("button", { name: "Sure?" }).click(); + await card.getByRole("button", { name: "Apply feedback" }).click(); + await expect(card).toContainText("Planner will use this feedback to update the document."); + await card.getByRole("button", { name: "Apply feedback" }).click(); await expect(page.getByText(/accepted a comment on/)).toBeVisible(); await expect( page.getByText("The agent is not running, so the plan has not been revised."), @@ -1029,6 +1167,7 @@ test("a dismissed thread removes its document button", async ({ join, seed }) => let card = await thread(page); await card.getByRole("button", { name: "Dismiss" }).click(); - await card.getByRole("button", { name: "Sure?" }).click(); + await expect(card).toContainText("This closes the thread without changing the document."); + await card.getByRole("button", { name: "Dismiss" }).click(); await expect(commentButton(page)).toHaveCount(0); }); diff --git a/packages/editor/package.json b/packages/editor/package.json index f837f655..cca8eece 100644 --- a/packages/editor/package.json +++ b/packages/editor/package.json @@ -26,6 +26,7 @@ "./styles.css": "./src/styles.css" }, "dependencies": { + "@base-ui/react": "1.7.0", "@chopin/dialect": "workspace:*", "@chopin/protocol": "workspace:*", "@chopin/question": "workspace:*", diff --git a/packages/editor/src/card.tsx b/packages/editor/src/card.tsx index 975d12ee..e1746400 100644 --- a/packages/editor/src/card.tsx +++ b/packages/editor/src/card.tsx @@ -103,7 +103,7 @@ export function Provenance({ at, by, verb }: ProvenanceProps) { return ( - {verb} by @{by} + {verb} by @{by} {stamp && ` · ${stamp}`} ); diff --git a/packages/editor/src/comment-geometry.test.ts b/packages/editor/src/comment-geometry.test.ts index 6c331892..45439ee7 100644 --- a/packages/editor/src/comment-geometry.test.ts +++ b/packages/editor/src/comment-geometry.test.ts @@ -1,6 +1,6 @@ import { expect, test } from "bun:test"; -import { markerPoints, markerRect, popoverPoint } from "./comment-geometry"; +import { edgePanelPoint, markerPoints, markerRect, popoverPoint } from "./comment-geometry"; import type { Rect } from "./comment-geometry"; @@ -274,3 +274,32 @@ test("fits a full preview beside a gutter button in a 400px document", () => { // on the right. Keeping the preview inside the page means using the left. expect(point).toEqual({ top: 200, left: 72 }); }); + +test("docks a comment panel at the document's right edge", () => { + let point = edgePanelPoint( + { top: 260, right: 560, bottom: 284, left: 536, width: 24, height: 24 }, + host, + 320, + 200, + ); + + expect(point).toEqual({ top: 160, left: 468 }); +}); + +test("keeps an edge panel inside both vertical document edges", () => { + let above = edgePanelPoint( + { top: 50, right: 560, bottom: 74, left: 536, width: 24, height: 24 }, + host, + 320, + 200, + ); + let below = edgePanelPoint( + { top: 660, right: 560, bottom: 684, left: 536, width: 24, height: 24 }, + host, + 320, + 240, + ); + + expect(above).toEqual({ top: 12, left: 468 }); + expect(below).toEqual({ top: 348, left: 468 }); +}); diff --git a/packages/editor/src/comment-geometry.ts b/packages/editor/src/comment-geometry.ts index 390cc650..42e2677e 100644 --- a/packages/editor/src/comment-geometry.ts +++ b/packages/editor/src/comment-geometry.ts @@ -121,6 +121,20 @@ export function popoverPoint( }; } +/** Keep an engaged comment in one predictable document-edge surface. */ +export function edgePanelPoint( + anchor: Rect, + host: Rect, + width: number, + height: number, + inset = 12, +): Point { + return { + top: clamp(anchor.top - host.top, inset, host.height - height - inset), + left: Math.max(inset, host.width - width - inset), + }; +} + function clamp(value: number, lower: number, upper: number): number { return Math.min(Math.max(value, lower), Math.max(lower, upper)); } diff --git a/packages/editor/src/comment-layer.tsx b/packages/editor/src/comment-layer.tsx index 7233603a..328c99f1 100644 --- a/packages/editor/src/comment-layer.tsx +++ b/packages/editor/src/comment-layer.tsx @@ -1,18 +1,19 @@ /** Reader-local comment chrome overlays rather than mutates collaborative prose. */ -import { useCallback, useEffect, useLayoutEffect, useRef, useState } from "react"; +import { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from "react"; import { createPortal } from "react-dom"; import { useLexicalComposerContext } from "@lexical/react/LexicalComposerContext"; import { ChatCircleIcon } from "@phosphor-icons/react"; import { useCellValue } from "@mdxeditor/gurx"; import { DraftCard, ThreadCard } from "./comments"; -import { markerPoints, markerRect, popoverPoint } from "./comment-geometry"; +import { edgePanelPoint, markerPoints, markerRect, popoverPoint } from "./comment-geometry"; import { containsHit, passageHits } from "./comment-hits"; +import { CommentSheet, usesCommentSheet } from "./comment-sheet"; import { useCommentSheetReveal } from "./comment-sheet-reveal"; import { $rangeOf } from "./marks"; import { blockElement } from "./scroll"; -import { COARSE_POINTER_QUERY, hasCoarsePointer } from "./pointer"; +import { COARSE_POINTER_QUERY, PRIMARY_COARSE_POINTER_QUERY } from "./pointer"; import { useThreads } from "./threads"; import { useTransitionPresence } from "./transition-presence"; import { widgets$ } from "./widget-options"; @@ -108,9 +109,15 @@ function placedPreview( function PreviewContent({ view }: { view: ThreadView }) { let replies = Math.max(0, view.thread.notes.length - 1); + let opening = view.thread.notes[0]; return ( <> -

{view.quote}

+ {opening && ( + <> +

@{opening.handle}

+

{opening.text}

+ + )} {replies > 0 && {replies} {replies === 1 ? "reply" : "replies"}} ); @@ -232,22 +239,6 @@ function replyState(view: ThreadView): string { : `${replies} ${replies === 1 ? "reply" : "replies"} waiting.`; } -/** Give a compact document dialog the same outside-content isolation as a native modal. */ -function isolate(dialog: HTMLElement): () => void { - let changed: HTMLElement[] = []; - for (let current: HTMLElement = dialog; current.parentElement; current = current.parentElement) { - for (let sibling of current.parentElement.children) { - if (!(sibling instanceof HTMLElement) || sibling === current || sibling.inert) continue; - sibling.inert = true; - changed.push(sibling); - } - if (current.parentElement === document.body) break; - } - return () => { - for (let element of changed) element.inert = false; - }; -} - export function CommentLayer({ store }: { store: ThreadStore }) { let [editor] = useLexicalComposerContext(); let state = useThreads(store); @@ -257,6 +248,7 @@ export function CommentLayer({ store }: { store: ThreadStore }) { let [previewMeasurement, setPreviewMeasurement] = useState(); let [pinned, setPinned] = useState(); let [coarse, setCoarse] = useState(false); + let [primaryCoarse, setPrimaryCoarse] = useState(false); let [cardHeights, setCardHeights] = useState<{ [id: string]: number }>({}); let root = useRef(null); let placedRef = useRef([]); @@ -268,7 +260,9 @@ export function CommentLayer({ store }: { store: ThreadStore }) { let draftOpen = useRef(false); let options = useCellValue(widgets$); let canEdit = options.canEdit !== false; - let compact = options.commentPresentation === "sheet"; + let compact = options.commentPresentation === "sheet" + && usesCommentSheet({ coarse: primaryCoarse, width: host?.clientWidth ?? Infinity }); + let previousCompact = useRef(compact); let immediately = options.motionImmediately?.() ?? false; let draft = canEdit ? state.draft : undefined; let measurePreview = useCallback((request: PreviewRequest, height: number) => { @@ -298,12 +292,31 @@ export function CommentLayer({ store }: { store: ThreadStore }) { useEffect(() => { let query = matchMedia(COARSE_POINTER_QUERY); - let update = () => setCoarse(hasCoarsePointer()); + let primary = matchMedia(PRIMARY_COARSE_POINTER_QUERY); + let update = () => { + setCoarse(query.matches); + setPrimaryCoarse(primary.matches); + }; update(); query.addEventListener("change", update); - return () => query.removeEventListener("change", update); + primary.addEventListener("change", update); + return () => { + query.removeEventListener("change", update); + primary.removeEventListener("change", update); + }; }, []); + useLayoutEffect(() => { + let previous = previousCompact.current; + previousCompact.current = compact; + if (!previous || compact || !pinned) return; + let id = pinned === "orphans" + ? "plan-comment-thread-orphans" + : `plan-comment-thread-${pinned}`; + let dialog = document.getElementById(id); + dialog?.querySelector("[data-plan-comment-close], button")?.focus(); + }, [compact, pinned]); + let enter = useCallback((id: string) => { clearTimeout(close.current); setPreview(id); @@ -518,15 +531,20 @@ export function CommentLayer({ store }: { store: ThreadStore }) { let sheetId = compact && pinned !== "orphans" ? pinned : undefined; let sheet = placed.find(entry => entry.view.thread.id === sheetId); + let revealId = compact && draft?.placement ? "draft" : sheetId; + let revealPassages = useMemo( + () => compact && draft?.placement ? [draft.placement] : sheet?.passages, + [compact, draft?.placement, sheet?.passages], + ); useCommentSheetReveal({ - height: sheetId ? cardHeights[sheetId] : undefined, host, - id: sheetId, - passages: sheet?.passages, + id: revealId, + passages: revealPassages, }); useEffect(() => { if (!pinned && !preview) return; + if (compact && pinned) return; let outside = (event: PointerEvent) => { let dialog = pinned ? document.getElementById(`plan-comment-thread-${pinned}`) @@ -547,55 +565,12 @@ export function CommentLayer({ store }: { store: ThreadStore }) { document.removeEventListener("pointerdown", outside); document.removeEventListener("keydown", escape); }; - }, [dismiss, pinned, preview]); - - useLayoutEffect(() => { - if (!compact || (!pinned && !draft)) return; - let editorHost = editor.getRootElement(); - let dialog = pinned - ? document.getElementById(`plan-comment-thread-${pinned}`) - : root.current?.querySelector('[aria-label="New comment"]'); - if (!dialog) return; - let release = isolate(dialog); - editorHost?.setAttribute("inert", ""); - let initial = dialog.querySelector( - pinned ? "[data-plan-comment-close]" : "textarea, button, [tabindex]", - ); - initial?.focus(); - let trap = (event: KeyboardEvent) => { - if (event.key === "Escape") { - event.preventDefault(); - if (pinned) dismiss(); - else cancelDraft(); - return; - } - if (event.key !== "Tab") return; - let focusable = Array.from(dialog.querySelectorAll( - 'button:not([disabled]), textarea:not([disabled]), input:not([disabled]), [tabindex]:not([tabindex="-1"])', - )).filter(element => element.offsetParent !== null); - if (focusable.length === 0) return; - let first = focusable[0]!; - let last = focusable[focusable.length - 1]!; - if (event.shiftKey && document.activeElement === first) { - event.preventDefault(); - last.focus(); - } else if (!event.shiftKey && document.activeElement === last) { - event.preventDefault(); - first.focus(); - } - }; - document.addEventListener("keydown", trap); - return () => { - release(); - editorHost?.removeAttribute("inert"); - document.removeEventListener("keydown", trap); - }; - }, [cancelDraft, compact, dismiss, draft, editor, pinned]); + }, [compact, dismiss, pinned, preview]); if (!host) return null; let orphaned = state.threads.filter(view => view.thread.status === "open" && view.orphaned); - let card = (view: ThreadView) => ( + let card = (view: ThreadView, showClose = true) => ( store.accept(view.thread.id)} onBlur={() => unhover(view.thread.id)} - onClose={dismiss} + onClose={showClose ? dismiss : undefined} onDismiss={() => store.dismiss(view.thread.id)} onFocus={() => hover(view.thread.id)} onReply={text => store.reply(view.thread.id, text)} onRetry={() => store.retry(view.thread.id)} - onReveal={() => store.reveal(view.thread.id)} onTyping={writing => store.announce(view.thread.id, writing)} quote={view.quote} + showClose={showClose} view={view} writing={state.writing[view.thread.id]} /> @@ -622,8 +597,13 @@ export function CommentLayer({ store }: { store: ThreadStore }) { setCardHeights(current => current[id] === height ? current : { ...current, [id]: height }); }; let page = host.getBoundingClientRect(); - let cardWidth = Math.min(384, host.clientWidth * 0.8); + let cardWidth = Math.min(320, host.clientWidth - 24); let previewWidth = Math.min(288, host.clientWidth * 0.8); + let compactKey: string | undefined; + let compactId: string | undefined; + let compactLabel: string | undefined; + let compactClose: (() => void) | undefined; + let compactContent: ReactNode = undefined; let previewEntry = preview && pinned !== preview ? placed.find(entry => entry.view.thread.id === preview) : undefined; @@ -640,13 +620,50 @@ export function CommentLayer({ store }: { store: ThreadStore }) { let previewValue = placedPreview(previewRequest, previewMeasurement); let activePreviewId = previewValue?.id; - return createPortal( -
+ if (compact && draft?.placement) { + compactKey = "draft"; + compactId = "plan-comment-draft"; + compactLabel = "New comment"; + compactClose = cancelDraft; + compactContent = ( + store.start(text)} + showClose={false} + /> + ); + } else if (compact && pinned === "orphans" && orphaned.length > 0) { + compactKey = "orphans"; + compactId = "plan-comment-thread-orphans"; + compactLabel = "Orphaned comments"; + compactClose = dismiss; + compactContent = orphaned.map(view => card(view, false)); + } else if (compact && pinned) { + let pinnedView = state.threads.find(view => + view.thread.id === pinned && view.thread.status === "open" + ); + if (pinnedView) { + compactKey = `thread:${pinned}`; + compactId = `plan-comment-thread-${pinned}`; + compactLabel = "Comment thread"; + compactClose = dismiss; + compactContent = card(pinnedView, false); + } + } + + let documentChrome = createPortal( +
{placed.map(({ button, hits, view }) => { let shown = pinned === view.thread.id; let previewId = `plan-comment-preview-${view.thread.id}`; - let cardPoint = popoverPoint( - markerRect(button, page, coarse ? 44 : 24), + let anchor = markerRect(button, page, coarse ? 44 : 24); + let cardPoint = edgePanelPoint( + anchor, page, cardWidth, cardHeights[view.thread.id] ?? 0, @@ -688,9 +705,9 @@ export function CommentLayer({ store }: { store: ThreadStore }) {
, host, ); + + return ( + <> + {documentChrome} + {compactKey && compactId && compactLabel && compactClose && ( + + {compactContent} + + )} + + ); } diff --git a/packages/editor/src/comment-sheet-reveal.ts b/packages/editor/src/comment-sheet-reveal.ts index ea05449c..51950daa 100644 --- a/packages/editor/src/comment-sheet-reveal.ts +++ b/packages/editor/src/comment-sheet-reveal.ts @@ -1,18 +1,19 @@ import { useLayoutEffect } from "react"; +import { currentViewport } from "@chopin/viewport"; + import { commentRevealScroll } from "./comment-reveal"; +import { commentSheetTop } from "./comment-sheet"; import { planScroller } from "./scroll"; import type { Rect } from "./comment-geometry"; /** Keep a compact comment beside its passage, then return the reader on close. */ export function useCommentSheetReveal({ - height, host, id, passages, }: { - height: number | undefined; host: HTMLElement | undefined; id: string | undefined; passages: Rect[] | undefined; @@ -28,19 +29,23 @@ export function useCommentSheetReveal({ }, [host, id]); useLayoutEffect(() => { - if (!host || !id || !height || !passages || passages.length === 0) return; - let dialog = document.getElementById(`plan-comment-thread-${id}`); + if (!host || !id || !passages || passages.length === 0) return; let scroller = planScroller(host); - if (!dialog || !scroller) return; - let next = commentRevealScroll({ - currentScroll: scroller.scrollTop, - gap: 20, - maxScroll: scroller.scrollHeight - scroller.clientHeight, - passageBottom: Math.max(...passages.map(passage => passage.bottom)), - passageTop: Math.min(...passages.map(passage => passage.top)), - sheetTop: dialog.getBoundingClientRect().top, - viewportTop: host.getBoundingClientRect().top, + if (!scroller) return; + let frame = requestAnimationFrame(() => { + if (!scroller.isConnected) return; + let viewport = currentViewport(); + let next = commentRevealScroll({ + currentScroll: scroller.scrollTop, + gap: 20, + maxScroll: scroller.scrollHeight - scroller.clientHeight, + passageBottom: Math.max(...passages.map(passage => passage.bottom)), + passageTop: Math.min(...passages.map(passage => passage.top)), + sheetTop: viewport.top + commentSheetTop(viewport.height), + viewportTop: host.getBoundingClientRect().top, + }); + if (Math.abs(next - scroller.scrollTop) >= 1) scroller.scrollTop = next; }); - if (Math.abs(next - scroller.scrollTop) >= 1) scroller.scrollTop = next; - }, [height, host, id, passages]); + return () => cancelAnimationFrame(frame); + }, [host, id, passages]); } diff --git a/packages/editor/src/comment-sheet.test.ts b/packages/editor/src/comment-sheet.test.ts new file mode 100644 index 00000000..949a989a --- /dev/null +++ b/packages/editor/src/comment-sheet.test.ts @@ -0,0 +1,28 @@ +import { describe, expect, it } from "bun:test"; + +import { + COMMENT_SHEET_SNAP_POINTS, + commentSheetTop, + nextCommentSheetSnapPoint, + usesCommentSheet, +} from "./comment-sheet"; + +describe("comment sheet snap points", () => { + it("cycles between the medium and large detents", () => { + expect(COMMENT_SHEET_SNAP_POINTS).toEqual([0.55, 0.92]); + expect(nextCommentSheetSnapPoint(0.55)).toBe(0.92); + expect(nextCommentSheetSnapPoint(0.92)).toBe(0.55); + }); + + it("places the medium detent within the visual viewport", () => { + expect(commentSheetTop(844)).toBeCloseTo(379.8); + }); + + it("reserves the drawer for phone-sized coarse pointers", () => { + expect(usesCommentSheet({ coarse: true, width: 390 })).toBe(true); + expect(usesCommentSheet({ coarse: true, width: 430 })).toBe(true); + expect(usesCommentSheet({ coarse: true, width: 431 })).toBe(false); + expect(usesCommentSheet({ coarse: true, width: 768 })).toBe(false); + expect(usesCommentSheet({ coarse: false, width: 390 })).toBe(false); + }); +}); diff --git a/packages/editor/src/comment-sheet.tsx b/packages/editor/src/comment-sheet.tsx new file mode 100644 index 00000000..c434c35f --- /dev/null +++ b/packages/editor/src/comment-sheet.tsx @@ -0,0 +1,126 @@ +import { useEffect, useRef, useState } from "react"; +import { Drawer } from "@base-ui/react/drawer"; + +import { currentViewport, listenToViewportChanges } from "@chopin/viewport"; + +import type { ReactNode } from "react"; + +export const COMMENT_SHEET_SNAP_POINTS = [0.55, 0.92] as const; +export const COMMENT_SHEET_MAX_WIDTH = 430; + +export function usesCommentSheet({ + coarse, + width, +}: { + coarse: boolean; + width: number; +}): boolean { + return coarse && width <= COMMENT_SHEET_MAX_WIDTH; +} + +export function commentSheetTop(viewportHeight: number): number { + return viewportHeight * (1 - COMMENT_SHEET_SNAP_POINTS[0]); +} + +export function nextCommentSheetSnapPoint(current: number): number { + return current === COMMENT_SHEET_SNAP_POINTS[0] + ? COMMENT_SHEET_SNAP_POINTS[1] + : COMMENT_SHEET_SNAP_POINTS[0]; +} + +export type CommentSheetProps = { + children: ReactNode; + id: string; + label: string; + onClose: () => void; +}; + +export function CommentSheet({ children, id, label, onClose }: CommentSheetProps) { + let [open, setOpen] = useState(false); + let [snapPoint, setSnapPoint] = useState( + COMMENT_SHEET_SNAP_POINTS[0], + ); + let viewportRef = useRef(null); + + useEffect(() => { + let frame = requestAnimationFrame(() => setOpen(true)); + return () => cancelAnimationFrame(frame); + }, []); + + useEffect(() => { + let expandForKeyboard = () => { + let viewport = currentViewport(); + let keyboardInset = window.innerHeight - viewport.top - viewport.height; + let active = document.activeElement; + if ( + keyboardInset > 60 + && active instanceof HTMLElement + && viewportRef.current?.contains(active) + ) { + setSnapPoint(COMMENT_SHEET_SNAP_POINTS[1]); + } + }; + + return listenToViewportChanges(expandForKeyboard); + }, []); + + return ( + { + if (!next) onClose(); + }} + onSnapPointChange={setSnapPoint} + open={open} + snapPoint={snapPoint} + snapPoints={[...COMMENT_SHEET_SNAP_POINTS]} + snapToSequentialPoints + > + + + setOpen(false)} + /> + + + + {label} + + {children} + + + + + + + + ); +} diff --git a/packages/editor/src/comments.test.tsx b/packages/editor/src/comments.test.tsx index 16d39f63..93be5b6a 100644 --- a/packages/editor/src/comments.test.tsx +++ b/packages/editor/src/comments.test.tsx @@ -1,7 +1,7 @@ import { describe, expect, it } from "bun:test"; import { renderToStaticMarkup } from "react-dom/server"; -import { ThreadCard } from "./comments"; +import { DraftCard, ThreadCard } from "./comments"; import type { Comment } from "@chopin/protocol"; import type { ThreadView } from "./threads"; @@ -35,7 +35,6 @@ function render(value: ThreadView, canEdit: boolean): string { onFocus={() => {}} onReply={() => {}} onRetry={() => {}} - onReveal={() => {}} onTyping={() => {}} quote={value.quote} view={value} @@ -50,14 +49,13 @@ describe("ThreadCard read-only controls", () => { let readOnly = render(view("open"), false); expect(editable).toContain("Reply"); - expect(editable).toContain("Accept"); + expect(editable).toContain("Apply feedback"); expect(editable).toContain("Dismiss"); expect(readOnly).toContain("Keep the rollout reversible."); expect(readOnly).toContain("cy is writing"); - expect(readOnly).toContain("show in plan"); expect(readOnly).toContain("Close comment"); expect(readOnly).not.toContain("Reply"); - expect(readOnly).not.toContain("Accept"); + expect(readOnly).not.toContain("Apply feedback"); expect(readOnly).not.toContain("Dismiss"); expect(readOnly).not.toContain("textarea"); }); @@ -68,7 +66,64 @@ describe("ThreadCard read-only controls", () => { expect(editable).toContain("Ask again"); expect(readOnly).toContain("Not yet applied"); - expect(readOnly).toContain("Accepted by @bo"); + expect(readOnly).toContain('Accepted by @bo'); expect(readOnly).not.toContain("Ask again"); }); }); + +describe("Comment card hierarchy", () => { + it("aligns the desktop close action with the opening author row", () => { + let markup = render(view("open"), true); + + expect(markup).not.toContain("plan-comment-header"); + expect(markup).toContain('data-plan-comment-opening-note="true"'); + expect(markup).not.toContain(" { + let markup = renderToStaticMarkup( + {}} + onSend={() => {}} + />, + ); + + expect(markup).not.toContain(" { + let markup = renderToStaticMarkup( + {}} + onSend={() => {}} + showClose={false} + />, + ); + + expect(markup).toContain('aria-label="Post comment"'); + expect(markup).toContain('data-inset-send="true"'); + expect(markup).not.toContain(">Comment"); + expect(markup).not.toContain(">Cancel"); + }); + + it("uses one inset reply action and orders resolution outcomes", () => { + let markup = render(view("open"), true); + + expect(markup).toContain('aria-label="Send reply"'); + expect(markup).toContain('data-plan-comment-composer-shell="true"'); + expect(markup).toContain("plan-comment-composer-field field"); + expect(markup).not.toContain(">Reply"); + expect(markup).toContain("Apply feedback"); + expect(markup.indexOf("Dismiss")).toBeLessThan(markup.indexOf("Apply feedback")); + }); +}); diff --git a/packages/editor/src/comments.tsx b/packages/editor/src/comments.tsx index 97b16bc7..20c96d76 100644 --- a/packages/editor/src/comments.tsx +++ b/packages/editor/src/comments.tsx @@ -7,112 +7,95 @@ * decision. A dismissed one is not rendered at all. * * Accept and dismiss both confirm on a second click, because neither can be - * undone: accepting freezes the thread, puts a decision in the plan and starts + * undone: accepting freezes the thread, puts a decision in the document and starts * a turn. That is the same two-click shape `QuestionView` uses for cancelling, * so it is an interaction people have already met here. */ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useLayoutEffect, useRef, useState } from "react"; +import { ArrowUpIcon, ChatCircleIcon, CheckIcon, XIcon } from "@phosphor-icons/react"; import { limits } from "@chopin/dialect"; import { Provenance, SidecarCard, when } from "./card"; -import { Count } from "./count"; -import { cursor } from "./cursor"; -import type { KeyboardEvent } from "react"; +import type { KeyboardEvent, ReactNode } from "react"; import type { Comment } from "@chopin/protocol"; import type { ThreadView } from "./threads"; function Who({ handle }: { handle: string }) { return ( - + @{handle} ); } -function Note({ note }: { note: Comment.Note }) { +function Note({ + action, + note, + opening, +}: { + action?: ReactNode; + note: Comment.Note; + opening?: boolean; +}) { return ( -
  • -
    - - {when(note.ts)} +
  • +
    +
    + + + {when(note.ts)} + +
    + {action}

    {note.text}

    • ); } -const QUOTED = "m-0 w-full text-left text-sm text-text-secondary italic"; +function CloseButton({ onClose }: { onClose: () => void }) { + return ( + + ); +} -/** - * The prose the thread marks, as a quotation the card can be read without. - * - * A button when there is somewhere to go, a blockquote when there is not — - * which is the rule `QuestionView` already applies to an answer, so both halves - * of the sidecar offer the same thing in the same way. A real element carries - * the affordance and the keyboard handling rather than a quotation pretending - * to be one, and a drifted thread offers no jump it could not honour: the card - * still reads, which is the durable part of a comment. - * - * The quote rather than the card. A card holds a reply box, an Accept and a - * Dismiss; making the whole of it a link would mean deciding, on every click, - * whether the reader meant the link or the control they actually hit. - * - * Two numbers meet here and only one of them is drawn. How many places the - * thread is anchored in belongs to the jump — it says what the button will do, - * so it is said in the label and nowhere else. How many replies the thread has - * collected is the count worth acting on, so that is what takes the pill. The - * pill sits outside the button and carries its own word, so a drifted thread - * with no jump left to offer still counts its replies out loud. - */ -function Quote( - { drifted, onSelect, places = 0, replies = 0, text }: { - drifted?: boolean; - onSelect?: () => void; - /** How many spans of prose the thread is anchored in. */ - places?: number; - /** Answers to the opening comment, which is not itself a reply. */ - replies?: number; - text: string; - }, -) { +function DraftHeader({ onClose, showClose }: { onClose: () => void; showClose: boolean }) { return ( -
    -
    - {places > 0 && onSelect - ? ( - - ) - :
    {text}
    } - {replies > 0 && ( - - {replies} - {replies === 1 ? " reply" : " replies"} - - )} -
    +
    +
    + ); +} + +/** Context is repeated only when its source passage can no longer be reached. */ +function Quote({ drifted, text }: { drifted?: boolean; text: string }) { + return ( +
    +
    + {text} +
    {drifted && (

    - The text this refers to has changed. + This passage has changed since the comment was added.

    )}
    @@ -133,6 +116,8 @@ function Composer({ onSend, onTyping, placeholder, + insetSend, + sendLabel, }: { autoFocus?: boolean; busy?: boolean; @@ -141,6 +126,8 @@ function Composer({ onSend: (text: string) => void; onTyping?: (writing: boolean) => void; placeholder: string; + insetSend?: boolean; + sendLabel?: string; }) { let [text, setText] = useState(""); let ref = useRef(null); @@ -149,6 +136,15 @@ function Composer({ if (autoFocus) ref.current?.focus(); }, [autoFocus]); + useLayoutEffect(() => { + let field = ref.current; + if (!field) return; + field.style.height = "0px"; + let height = Math.min(field.scrollHeight, 160); + field.style.height = `${height}px`; + field.style.overflowY = field.scrollHeight > height ? "auto" : "hidden"; + }, [text]); + // Whoever is typing stops being told about the moment this goes away, so // an unmount does not leave a caret blinking in somebody else's sidecar. useEffect(() => () => onTyping?.(false), [onTyping]); @@ -173,88 +169,61 @@ function Composer({ return (
    -