From 83b74e7f8dec6d51515c0a889a41fabad6702b29 Mon Sep 17 00:00:00 2001 From: Kieran Zhang Date: Sat, 10 Oct 2026 21:44:24 +0800 Subject: [PATCH] fix(browser): keep area comments aligned with their crop and content Address the #455 review: the prompt now gives an area's position inside its cropped screenshot, area offsets track the anchor's own scroll so the outline stays on the marked content, and a drag released over a badge no longer leaves badges unclickable. --- .scratch/browser-annotation-v2/PRD.md | 1 + .../browser-annotation-overlay.test.ts | 102 ++++++++++++++++++ .../electron/browser-annotation-overlay.ts | 29 +++-- .../electron/browser-annotation.test.ts | 72 +++++++++++++ apps/desktop/electron/browser-annotation.ts | 24 ++++- apps/desktop/electron/browser-host.test.ts | 37 ------- apps/desktop/electron/browser-host.ts | 47 +------- docs/adr/0029-embedded-browser-surface.md | 2 +- packages/core/src/browser-annotation.test.ts | 74 ++++++++++++- packages/core/src/browser-annotation.ts | 80 +++++++++++++- packages/core/src/index.ts | 1 + 11 files changed, 366 insertions(+), 103 deletions(-) diff --git a/.scratch/browser-annotation-v2/PRD.md b/.scratch/browser-annotation-v2/PRD.md index 96500181..0440907f 100644 --- a/.scratch/browser-annotation-v2/PRD.md +++ b/.scratch/browser-annotation-v2/PRD.md @@ -92,6 +92,7 @@ Codex 默认是 Enter 直发、`Cmd+Enter` 攒（#22719 下大量反对）；Pac - 区域不「占有」锚点：点击锚点元素仍新建元素评论，多个区域可共享同一锚点。 - 拖动中 Esc / `pointercancel` / 松开在窗口外（`buttons === 0`）都取消框选，随后的 click 被吞；任一边小于 4px 视为误触。已保存区域用虚线轮廓，元素用实线。模式标签改为「Click or drag to annotate · Esc to exit」。 - 格式化：区域条目标题为 `#n [screenshot] area in \`selector\` (tag)`，**总是**输出 `- area: W×H at (x, y)`（截图有外扩边距、无框线，模型需要确切边界）；composer chip 为 `#n Area · 摘要`。 +- （#455 review 修订）`area` 的偏移按锚点滚动内容计：保存时加上锚点 `scrollLeft`/`scrollTop`（锚点为文档滚动元素时除外，其 rect 已随页面滚动），否则锚点自身滚动内容时轮廓会漂。带截图的区域条目同时给出截图内坐标与视口坐标（`- area: W×H at (sx, sy) in its screenshot; (x, y) in the viewport`），截图按 `resolveCommentCropRect`（已移入 `packages/core`）的同样规则裁出。 ## 切片 diff --git a/apps/desktop/electron/browser-annotation-overlay.test.ts b/apps/desktop/electron/browser-annotation-overlay.test.ts index 0d91ac18..194e4330 100644 --- a/apps/desktop/electron/browser-annotation-overlay.test.ts +++ b/apps/desktop/electron/browser-annotation-overlay.test.ts @@ -1624,4 +1624,106 @@ describe("annotation overlay — area selection", () => { vi.useRealTimers(); } }); + + it("stores the anchor's own scroll in the offset when the area is saved", async () => { + const h = harness(); + + document.body.innerHTML = + '

inside

'; + const box = place(document.getElementById("scrollbox")!, 40, 40, 200, 200); + const start = place(document.getElementById("start")!, 50, 50, 20, 10); + vi.spyOn(box, "scrollTop", "get").mockReturnValue(100); + + h.overlay.setDesignMode(true); + drag(start, 60, 60, 160, 110); + typeDraft(h, "inside the scroller"); + pressKey(editorInput(h), "Enter"); + await nextFrames(); + + // The box does not move when its content scrolls, so the offset has to + // carry scrollTop to keep naming the same content. + expect(h.saved[0]!.annotation).toMatchObject({ + selector: "#scrollbox", + rect: { x: 60, y: 60, width: 100, height: 50 }, + area: { x: 20, y: 120, width: 100, height: 50 }, + }); + }); + + it("moves the outline with the content when the anchor scrolls its content", async () => { + const h = harness(); + + document.body.innerHTML = '

x

'; + const box = place(document.getElementById("scrollbox")!, 40, 40, 200, 200); + const scrollTop = vi.spyOn(box, "scrollTop", "get").mockReturnValue(100); + + // area.y = 120 in content coords: 40 + 120 - 100 = 60 on screen. + h.overlay.syncAnnotations([ + { + ...areaAnnotation("srv-a", 1, "#scrollbox"), + tag: "div", + rect: { x: 50, y: 60, width: 40, height: 20 }, + area: { x: 10, y: 120, width: 40, height: 20 }, + }, + ]); + + const outline = h.shadow().querySelector( + '[data-slot="annotation-outline"]', + )!; + expect(outline.style.top).toBe("60px"); + + // The anchor's rect is unchanged but its content moved: a mark pinned + // to the box alone would slide over different content. + scrollTop.mockReturnValue(130); + window.dispatchEvent(new Event("scroll")); + await nextFrames(); + + expect(outline.style.top).toBe("30px"); + }); + + it("lets a badge open after a drag's trailing click landed on it", async () => { + const h = harness(); + place(document.querySelector("main")!, 0, 0, 600, 400); + const button = place(document.getElementById("cta")!); + + h.overlay.setDesignMode(true); + await annotate(h, button, "saved mark"); + const badge = badges(h)[0]!; + + // A drag released under the area threshold, with its trailing click + // landing on a saved badge: the badge swallows it — and the swallow + // clears the flag itself, so the next click is not eaten. + pointer(button, "pointerdown", 50, 50, { button: 0, buttons: 1 }); + pointer(button, "pointermove", 200, 150, { buttons: 1 }); + pointer(button, "pointerup", 200, 52, { button: 0, buttons: 0 }); + badge.click(); + + expect(editorAbsent(h)).toBe(true); + + badge.click(); + + expect(editorVisible(h)).toBe(true); + expect(editorInput(h).value).toBe("saved mark"); + }); + + it("re-arms the click swallow on a pointerdown over overlay chrome", async () => { + const h = harness(); + place(document.querySelector("main")!, 0, 0, 600, 400); + const button = place(document.getElementById("cta")!); + + h.overlay.setDesignMode(true); + await annotate(h, button, "saved mark"); + const badge = badges(h)[0]!; + + // Same dead drag, but the next gesture starts on the badge itself: a + // pointerdown over overlay chrome must still reset the swallow — the + // page-target early return cannot be where it is re-armed. + pointer(button, "pointerdown", 50, 50, { button: 0, buttons: 1 }); + pointer(button, "pointermove", 200, 150, { buttons: 1 }); + pointer(button, "pointerup", 200, 52, { button: 0, buttons: 0 }); + pointer(badge, "pointerdown", 0, 0, { button: 0, buttons: 1 }); + badge.click(); + + expect(editorVisible(h)).toBe(true); + expect(editorInput(h).value).toBe("saved mark"); + }); }); diff --git a/apps/desktop/electron/browser-annotation-overlay.ts b/apps/desktop/electron/browser-annotation-overlay.ts index 1e81d57f..9f00a7e2 100644 --- a/apps/desktop/electron/browser-annotation-overlay.ts +++ b/apps/desktop/electron/browser-annotation-overlay.ts @@ -1,4 +1,5 @@ import { + anchorScroll, describeAnnotatedArea, describeAnnotatedElement, maxCommentLength, @@ -870,16 +871,19 @@ export function createAnnotationOverlay(options: { * The mark's viewport rect: the element's own box, or for an area the box * the saved offset lands at from wherever the anchor re-measures — what * keeps a region pinned to its place inside a scrolled or reflowed anchor. + * Offsets are stored in the anchor's scrolled-content coordinates, so its + * own scroll comes back out here. */ function markedRect( element: Element, area: BrowserAnnotationElement["area"] | null, ): ViewportRect { const anchor = element.getBoundingClientRect(); + const scroll = area ? anchorScroll(element) : { x: 0, y: 0 }; return toViewportRect( - anchor.left + (area?.x ?? 0), - anchor.top + (area?.y ?? 0), + anchor.left + (area?.x ?? 0) - scroll.x, + anchor.top + (area?.y ?? 0) - scroll.y, area?.width ?? anchor.width, area?.height ?? anchor.height, ); @@ -1111,8 +1115,10 @@ export function createAnnotationOverlay(options: { styleBadge(badge); badge.addEventListener("click", () => { // A drag releasing over a badge fires this too — its click is - // swallowed like the one that lands on page elements. + // swallowed like the one that lands on page elements. The flag is + // consumed, not held: it guards only this one trailing click. if (suppressClick) { + suppressClick = false; return; } const entry = entries.get(annotation.id); @@ -1651,6 +1657,13 @@ export function createAnnotationOverlay(options: { return; } + // A new press re-arms the trailing-click swallow even when it lands on + // our own chrome — the early return below must not leave a stale flag + // eating the badge's next click. + if (event.type === "pointerdown") { + suppressClick = false; + } + const target = pageTarget(event); // Our own badges and editor have to keep working, so their events are @@ -1665,8 +1678,7 @@ export function createAnnotationOverlay(options: { if (event.type === "pointerdown") { // A press on a page target arms the drag: whether it stays a click is - // the next pointermove's call. The trailing-click swallow re-arms too. - suppressClick = false; + // the next pointermove's call. const pointer = event as MouseEvent; if (pointer.button === 0) { dragStart = { x: pointer.clientX, y: pointer.clientY, target }; @@ -1730,10 +1742,13 @@ export function createAnnotationOverlay(options: { const anchor = resolveAreaAnchor(start.target, rect); const anchorRect = anchor.getBoundingClientRect(); + // Scrolled-content coordinates, same rule as describeAnnotatedArea: the + // anchor's own scroll belongs to the offset, not the box position. + const scroll = anchorScroll(anchor); requestEditor(anchor, undefined, { - x: rect.left - anchorRect.left, - y: rect.top - anchorRect.top, + x: rect.left - anchorRect.left + scroll.x, + y: rect.top - anchorRect.top + scroll.y, width: rect.width, height: rect.height, }); diff --git a/apps/desktop/electron/browser-annotation.test.ts b/apps/desktop/electron/browser-annotation.test.ts index ce4452d4..49ed20b6 100644 --- a/apps/desktop/electron/browser-annotation.test.ts +++ b/apps/desktop/electron/browser-annotation.test.ts @@ -220,6 +220,78 @@ describe("describeAnnotatedArea", () => { // The anchor's text would describe the element, not the marked region. expect(annotation).not.toHaveProperty("text"); }); + + it("counts the anchor's own scroll in the offset — the mark tracks content, not the box", () => { + mount( + '

inside

', + ); + const anchor = document.getElementById("scrollbox")!; + vi.spyOn(anchor, "getBoundingClientRect").mockReturnValue({ + x: 40, + y: 40, + left: 40, + top: 40, + width: 200, + height: 200, + right: 240, + bottom: 240, + toJSON: () => ({}), + } as DOMRect); + vi.spyOn(anchor, "scrollTop", "get").mockReturnValue(120); + vi.spyOn(anchor, "scrollLeft", "get").mockReturnValue(10); + + // A scrolled anchor's border box does not move — without the scroll + // term the offset would point at different content after every scroll. + const annotation = describeAnnotatedArea( + anchor, + { left: 60, top: 90, width: 50, height: 30 }, + 1, + "a1", + ); + + expect(annotation.area).toEqual({ x: 30, y: 170, width: 50, height: 30 }); + }); + + it("skips the scroll term when the anchor is the document's scrolling element", () => { + // The scrolling element's rect already moves with the page scroll, so + // adding scrollTop would count it twice. jsdom leaves + // document.scrollingElement unimplemented — it is mocked directly. + const anchor = document.documentElement; + vi.spyOn(anchor, "getBoundingClientRect").mockReturnValue({ + x: 0, + y: -100, + left: 0, + top: -100, + width: 800, + height: 800, + right: 800, + bottom: 700, + toJSON: () => ({}), + } as DOMRect); + vi.spyOn(anchor, "scrollTop", "get").mockReturnValue(100); + const own = Object.getOwnPropertyDescriptor(document, "scrollingElement"); + Object.defineProperty(document, "scrollingElement", { + configurable: true, + value: anchor, + }); + + try { + const annotation = describeAnnotatedArea( + anchor, + { left: 10, top: 20, width: 50, height: 30 }, + 1, + "a1", + ); + + expect(annotation.area).toEqual({ x: 10, y: 120, width: 50, height: 30 }); + } finally { + if (own) { + Object.defineProperty(document, "scrollingElement", own); + } else { + Reflect.deleteProperty(document, "scrollingElement"); + } + } + }); }); describe("acceptBrowserAnnotationMessage", () => { diff --git a/apps/desktop/electron/browser-annotation.ts b/apps/desktop/electron/browser-annotation.ts index 64c1ff77..3bb92a9b 100644 --- a/apps/desktop/electron/browser-annotation.ts +++ b/apps/desktop/electron/browser-annotation.ts @@ -306,12 +306,25 @@ export function describeAnnotatedElement( }; } +/** + * How much of an anchor's scrolled content sits hidden above-left of its + * border box: its own scroll offsets — except for the document's scrolling + * element, whose box already rides the page scroll, so adding its scrollTop + * would count the same scroll twice. + */ +export function anchorScroll(anchor: Element) { + return anchor === anchor.ownerDocument.scrollingElement + ? { x: 0, y: 0 } + : { x: anchor.scrollLeft, y: anchor.scrollTop }; +} + /** * An area comment: `selector`/`tag`/`source` describe the anchor — the * smallest element fully containing the dragged rect — while `rect` is the - * area's own viewport box and `area` its offset from the anchor's, so a - * restore can re-derive the rect from wherever the anchor re-measures. No - * `text`: the anchor's content would describe the element, not the region. + * area's own viewport box and `area` its offset from the anchor's border + * box in scrolled-content coordinates (`anchorScroll`, so the mark tracks + * the content, not the box). No `text`: the anchor's content would describe + * the element, not the region. */ export function describeAnnotatedArea( anchor: Element, @@ -320,6 +333,7 @@ export function describeAnnotatedArea( id: string, ): BrowserAnnotationElement { const anchorRect = anchor.getBoundingClientRect(); + const scroll = anchorScroll(anchor); const source = readSource(anchor); return { @@ -334,8 +348,8 @@ export function describeAnnotatedArea( height: Math.round(areaRect.height), }, area: { - x: Math.round(areaRect.left - anchorRect.left), - y: Math.round(areaRect.top - anchorRect.top), + x: Math.round(areaRect.left - anchorRect.left + scroll.x), + y: Math.round(areaRect.top - anchorRect.top + scroll.y), width: Math.round(areaRect.width), height: Math.round(areaRect.height), }, diff --git a/apps/desktop/electron/browser-host.test.ts b/apps/desktop/electron/browser-host.test.ts index e16a5eb9..b4dc6e60 100644 --- a/apps/desktop/electron/browser-host.test.ts +++ b/apps/desktop/electron/browser-host.test.ts @@ -14,7 +14,6 @@ import { maxSessionComments, normalizeBrowserUrl, resolveBrowserViewBounds, - resolveCommentCropRect, urlKey, type BrowserHostView, } from "./browser-host"; @@ -1258,40 +1257,4 @@ describe("urlKey", () => { }); }); -describe("resolveCommentCropRect", () => { - it("expands the element by 48px and grows to the minimum size", () => { - expect( - resolveCommentCropRect( - { x: 400, y: 300, width: 40, height: 20 }, - { width: 1440, height: 900 }, - ), - ).toEqual({ x: 260, y: 210, width: 320, height: 200 }); - }); - - it("keeps a large element's expanded bounds", () => { - expect( - resolveCommentCropRect( - { x: 400, y: 300, width: 500, height: 400 }, - { width: 1440, height: 900 }, - ), - ).toEqual({ x: 352, y: 252, width: 596, height: 496 }); - }); - it("shifts an off-edge crop inside before shrinking it", () => { - expect( - resolveCommentCropRect( - { x: 1380, y: 10, width: 40, height: 20 }, - { width: 1440, height: 900 }, - ), - ).toEqual({ x: 1120, y: 0, width: 320, height: 200 }); - }); - - it("clips a crop bigger than the viewport to it", () => { - expect( - resolveCommentCropRect( - { x: 100, y: 100, width: 900, height: 800 }, - { width: 400, height: 300 }, - ), - ).toEqual({ x: 0, y: 0, width: 400, height: 300 }); - }); -}); diff --git a/apps/desktop/electron/browser-host.ts b/apps/desktop/electron/browser-host.ts index bb4eb8ba..50945b3a 100644 --- a/apps/desktop/electron/browser-host.ts +++ b/apps/desktop/electron/browser-host.ts @@ -1,4 +1,5 @@ import { randomUUID } from "node:crypto"; +import { resolveCommentCropRect } from "@pace/core"; import type { BrowserAnnotationElement, BrowserAnnotationPalette, @@ -192,52 +193,6 @@ export function urlKey(url: string) { /** Comments a Session may keep at once — beyond it, saves are ignored. */ export const maxSessionComments = 200; -/** - * The crop saved with a comment: the element plus enough surroundings to - * read, grown to a useful minimum, kept inside the viewport it was measured - * in. Shifted before clipped — a crop near an edge keeps its full size if it - * can be moved inside rather than shrunk. - */ -export function resolveCommentCropRect( - rect: { x: number; y: number; width: number; height: number }, - viewport: { width: number; height: number }, -): BrowserViewRect { - const margin = 48; - let x = rect.x - margin; - let y = rect.y - margin; - let width = rect.width + margin * 2; - let height = rect.height + margin * 2; - - if (width < 320) { - x -= (320 - width) / 2; - width = 320; - } - if (height < 200) { - y -= (200 - height) / 2; - height = 200; - } - - if (width <= viewport.width) { - x = clamp(x, 0, viewport.width - width); - } else { - x = 0; - width = viewport.width; - } - if (height <= viewport.height) { - y = clamp(y, 0, viewport.height - height); - } else { - y = 0; - height = viewport.height; - } - - return { - x: Math.round(x), - y: Math.round(y), - width: Math.round(width), - height: Math.round(height), - }; -} - export type BrowserHostSession = { setPermissionRequestHandler(handler: (permission: string) => boolean): void; setPermissionCheckHandler(handler: (permission: string) => boolean): void; diff --git a/docs/adr/0029-embedded-browser-surface.md b/docs/adr/0029-embedded-browser-surface.md index 8a18ede1..f5fd10b7 100644 --- a/docs/adr/0029-embedded-browser-surface.md +++ b/docs/adr/0029-embedded-browser-surface.md @@ -71,7 +71,7 @@ ADR-0013 把「内嵌浏览器 + DOM 标注」定为切换 Electron 外壳的承 - **评论仓归主进程、Session 级**：`browser-host.ts` 每 Session 持一个评论仓，仓内顺序即 `#n`（读时重算）；每条 `BrowserComment` = 标注字段 + `tabId` / `url` / `title` / `viewport` / `stale` / `hasImage` / `createdAt`。上限 200 条（`maxSessionComments`），超出拒收并发 `comment-rejected`。新评论保存时 overlay 整层隐藏两帧再上报，主进程随即 `capturePage` 裁局部截图：标记矩形外扩 48 CSS px、最小 320×200、先平移再夹进视口；图片不进状态广播，按 id 由 `browser_comment_images` 取。关闭 tab 只移除页上标记，评论仍在仓里；关窗/退出即丢，不落盘。命令：`browser_list_comments`、`browser_settle_comments`（等在途截图落定再读，发送用它）、`browser_comment_images`、`browser_delete_comment`、`browser_consume_comments`；`browser_clear_annotations` 只清当前 tab 的评论。事件：`comments-changed`（带 revision，旧答复丢弃）、`comment-rejected`、`submit-requested`。composer 订阅读取，不持有第二份副本。 - **跨文档保留**：新文档 `ready` 与 `did-navigate-in-page`（SPA 路由切换）时，主进程下发 `sync-annotations`：本 tab 中 URL 键（origin + pathname + search，忽略 hash）相同的评论。overlay 用 selector 唯一解析元素，解析不到时用 MutationObserver 再等最多 5s，仍失败上报 `stale`；stale 评论仍可发送（文本与保存时的截图仍有效）。页上只显示当前 URL 的标记，其他页面的评论只在 composer 里。 - **覆盖层交互**：点击即打开已聚焦的草稿编辑器（textarea），评论必填，保存才成为标注；点已保存的元素或徽章进入编辑（带 Delete），不产生重复；一次只有一个编辑器，有未保存内容时点别处编辑器抖动而不切走。目标收敛：跳过行内装饰节点、SVG 内部与小于 8×8 的元素，优先最近的交互控件；`↑` / `↓` 沿祖先链手动微调。悬停标签 `tag.firstClass · W×H`（有 source 时加 `file:line`）。已保存标注画细轮廓 + 外侧右上角编号徽章，相邻徽章重叠时错开。模式可感知：视口内侧 2px 主题色描边 + 顶部居中标签「Click or drag to annotate · Esc to exit」。颜色取渲染层语义 token，随 `set-design-mode` / `set-annotation-palette` 下发，overlay 用 `CSS.supports` 逐个校验、不合法的回落内置默认。工具条按钮名 `Annotate`。 -- **区域框选**：标注模式下按住拖动超过 4px 进入框选，松开打开编辑器；任一边小于 4px 视为误触；Esc / pointercancel / 窗口外松开取消并吞掉随后的 click；不拦截滚动。`BrowserAnnotationElement.area`（相对锚点边框盒左上角的偏移，CSS px）存在即区域评论，`rect` 为保存时区域的视口矩形；锚点取按下点祖先链上第一个完整包含矩形且非 SVG 内部的元素，兜底 `body`；区域不带 `text`、不占有锚点（点锚点仍新建元素评论，多个区域可共享锚点）；已保存区域画虚线轮廓。格式化无论有无截图都输出 `- area: W×H at (x, y)`。 +- **区域框选**：标注模式下按住拖动超过 4px 进入框选，松开打开编辑器；任一边小于 4px 视为误触；Esc / pointercancel / 窗口外松开取消并吞掉随后的 click；不拦截滚动。`BrowserAnnotationElement.area`（相对锚点边框盒左上角、按其滚动内容计的偏移，CSS px——保存时加上锚点 `scrollLeft`/`scrollTop`，锚点为文档滚动元素时除外）存在即区域评论，`rect` 为保存时区域的视口矩形；锚点取按下点祖先链上第一个完整包含矩形且非 SVG 内部的元素，兜底 `body`；区域不带 `text`、不占有锚点（点锚点仍新建元素评论，多个区域可共享锚点）；已保存区域画虚线轮廓。格式化总输出 `- area:` 行：带截图时同时给出截图内坐标与视口坐标（`- area: W×H at (sx, sy) in its screenshot; (x, y) in the viewport`），无截图时只给视口坐标。 - **键位**： | 位置 | 键 | 行为 | diff --git a/packages/core/src/browser-annotation.test.ts b/packages/core/src/browser-annotation.test.ts index 87e41bdc..55888917 100644 --- a/packages/core/src/browser-annotation.test.ts +++ b/packages/core/src/browser-annotation.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { formatBrowserComments, + resolveCommentCropRect, type BrowserComment, } from "./browser-annotation"; @@ -138,9 +139,10 @@ describe("formatBrowserComments", () => { "#1 [screenshot] area in `#panel` (section)", " move this down", " - source: `src/hero.tsx:4`", - // The crop has margins and no drawn box, so the bounds are always - // printed — screenshot or not — and never as an element `rect:`. - " - area: 60×30 at (36, 352)", + // The crop has margins and no drawn box, so the bounds are printed + // in the screenshot's own coordinates as well as the viewport's — + // the crop here was shifted by the left edge to (0, 267, 320, 200). + " - area: 60×30 at (36, 85) in its screenshot; (36, 352) in the viewport", "", "#2 area in `#panel` (section)", " no shot", @@ -149,6 +151,34 @@ describe("formatBrowserComments", () => { ); }); + it("subtracts a crop the viewport edge shifted, not the margin it asked for", () => { + expect( + formatBrowserComments([ + comment({ + index: 1, + hasImage: true, + viewport: { width: 400, height: 600, dpr: 1 }, + area: { x: 4, y: 300, width: 40, height: 30 }, + rect: { x: 10, y: 400, width: 40, height: 30 }, + comment: "edge", + }), + ]), + ).toBe( + [ + "Browser comments from the embedded preview. A comment marked [screenshot] has a cropped screenshot of its element attached; those screenshots are the last images in this message, in comment order.", + "", + "Page: Alpha — http://localhost:5173/a", + "Viewport: 400×600 @1x", + "", + "#1 [screenshot] area in `#cta` (button)", + " edge", + // The crop wanted (-130, 315) but the edge pushed it to (0, 315): + // in-screenshot is the rect minus what was actually captured. + " - area: 40×30 at (10, 85) in its screenshot; (10, 400) in the viewport", + ].join("\n"), + ); + }); + it("prints the page without a title and a comment without text", () => { expect( formatBrowserComments([ @@ -168,3 +198,41 @@ describe("formatBrowserComments", () => { ); }); }); + +describe("resolveCommentCropRect", () => { + it("expands the element by 48px and grows to the minimum size", () => { + expect( + resolveCommentCropRect( + { x: 400, y: 300, width: 40, height: 20 }, + { width: 1440, height: 900 }, + ), + ).toEqual({ x: 260, y: 210, width: 320, height: 200 }); + }); + + it("keeps a large element's expanded bounds", () => { + expect( + resolveCommentCropRect( + { x: 400, y: 300, width: 500, height: 400 }, + { width: 1440, height: 900 }, + ), + ).toEqual({ x: 352, y: 252, width: 596, height: 496 }); + }); + + it("shifts an off-edge crop inside before shrinking it", () => { + expect( + resolveCommentCropRect( + { x: 1380, y: 10, width: 40, height: 20 }, + { width: 1440, height: 900 }, + ), + ).toEqual({ x: 1120, y: 0, width: 320, height: 200 }); + }); + + it("clips a crop bigger than the viewport to it", () => { + expect( + resolveCommentCropRect( + { x: 100, y: 100, width: 900, height: 800 }, + { width: 400, height: 300 }, + ), + ).toEqual({ x: 0, y: 0, width: 400, height: 300 }); + }); +}); diff --git a/packages/core/src/browser-annotation.ts b/packages/core/src/browser-annotation.ts index f5cab7ce..22319135 100644 --- a/packages/core/src/browser-annotation.ts +++ b/packages/core/src/browser-annotation.ts @@ -38,9 +38,12 @@ export type BrowserAnnotationElement = { rect: { x: number; y: number; width: number; height: number }; /** * Present only for an area comment: the dragged rectangle relative to the - * anchor element's border-box top-left, in CSS px. `selector`/`tag`/`source` - * then describe the anchor (smallest element fully containing the area); - * `rect` is the area's own viewport rect when saved. + * anchor element's border-box top-left in its scrolled content — i.e. plus + * the anchor's `scrollLeft`/`scrollTop` at save time, except when the + * anchor is the document's scrolling element (its rect already rides the + * page scroll). `selector`/`tag`/`source` then describe the anchor + * (smallest element fully containing the area); `rect` is the area's own + * viewport rect when saved. */ area?: { x: number; y: number; width: number; height: number }; source?: { file: string; line: number; column?: number }; @@ -73,6 +76,59 @@ export type BrowserComment = BrowserAnnotationElement & { createdAt: string; }; +function clamp(value: number, min: number, max: number) { + return Math.min(Math.max(value, min), Math.max(min, max)); +} + +/** + * The crop saved with a comment: the element plus enough surroundings to + * read, grown to a useful minimum, kept inside the viewport it was measured + * in. Shifted before clipped — a crop near an edge keeps its full size if it + * can be moved inside rather than shrunk. + * + * Lives in core because both ends of the crop need it: main to take the shot, + * the formatter to name an area's bounds in the shot's own coordinates. + */ +export function resolveCommentCropRect( + rect: { x: number; y: number; width: number; height: number }, + viewport: { width: number; height: number }, +): { x: number; y: number; width: number; height: number } { + const margin = 48; + let x = rect.x - margin; + let y = rect.y - margin; + let width = rect.width + margin * 2; + let height = rect.height + margin * 2; + + if (width < 320) { + x -= (320 - width) / 2; + width = 320; + } + if (height < 200) { + y -= (200 - height) / 2; + height = 200; + } + + if (width <= viewport.width) { + x = clamp(x, 0, viewport.width - width); + } else { + x = 0; + width = viewport.width; + } + if (height <= viewport.height) { + y = clamp(y, 0, viewport.height - height); + } else { + y = 0; + height = viewport.height; + } + + return { + x: Math.round(x), + y: Math.round(y), + width: Math.round(width), + height: Math.round(height), + }; +} + /** * One line of it. Main folds the same fields on the way in, but the template's * promise — one row per mark — is core's to keep for every caller of a public @@ -158,8 +214,24 @@ export function formatBrowserComments(comments: readonly BrowserComment[]) { } // An area always prints its bounds: its crop has margins and no drawn // box, so even a screenshot cannot say where exactly the region was. + // With a screenshot the bounds come twice — in the shot's own + // coordinates (1 image px is 1 CSS px after the CSS-width downsample, + // and the crop may have been shifted by a viewport edge) and in the + // viewport's. if (comment.area) { - lines.push(` - area: ${formatRect(comment.rect)}`); + if (comment.hasImage) { + const crop = resolveCommentCropRect(comment.rect, comment.viewport); + const inCrop = { + ...comment.rect, + x: comment.rect.x - crop.x, + y: comment.rect.y - crop.y, + }; + lines.push( + ` - area: ${formatRect(inCrop)} in its screenshot; (${comment.rect.x}, ${comment.rect.y}) in the viewport`, + ); + } else { + lines.push(` - area: ${formatRect(comment.rect)}`); + } } else if (!comment.hasImage) { lines.push(` - rect: ${formatRect(comment.rect)}`); } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 09baccd9..506e63be 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -171,6 +171,7 @@ export { export { formatBrowserComments, + resolveCommentCropRect, type BrowserAnnotationElement, type BrowserAnnotationViewport, type BrowserComment,