Redesign document comments with responsive sheets and motion

apps/web/src/project-sidebar-chrome.tsx

@@ -23,7 +23,7 @@ export function ProjectSidebarExpandButton(
 			ref={buttonRef}
 			type="button"
 		>
-			<img alt="" height="18" src={sidebarOpenIcon} width="18" />
+			<img alt="" height="14" src={sidebarOpenIcon} width="14" />
 		</button>
 	);
 }

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",

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}

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);
 });

e2e/responsive-comments.e2e.ts

@@ -1,26 +1,92 @@
-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(
 	{ length: 40 },
 	(_, 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,

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 }) => {