From c229c75ca613d35a9e4f37fd0f79feba8a81af40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20T=C3=B6rcsv=C3=A1ri?= Date: Thu, 9 Jul 2026 16:31:41 +0200 Subject: [PATCH] =?UTF-8?q?fix(collab):=20re-push=20viewport=20on=20canvas?= =?UTF-8?q?=20resize=20=E2=80=94=20comment=20pins=20sat=20offset=20until?= =?UTF-8?q?=20pan/zoom?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit emitViewportIfChanged deduped on scale+center only, so a canvas SIZE change (boot layout settling after the bind-time seed, window resize) never re-pushed {w,h} to JS. CommentLayer's worldToScreen maps through h/2, so every DOM pin hit target (and its hover ring) sat vertically offset from its GAL dot by exactly delta-h/2 css-px until the next pan/zoom finally passed the dedupe. Size now participates in the dedupe and wxEVT_SIZE re-pushes post-layout (CallAfter, after the GAL's own onSize). New regression e2e comments-viewport-resize.spec.ts asserts the DOM pin re-aligns with GAL truth (fresh kicadCollabGetViewport) across a window resize with no pan/zoom. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011x3h5AzkWDbHmCkeVYAuzU --- tests/web/comments-viewport-resize.spec.ts | 111 +++++++++++++++++++++ wasm/bindings/collab_presence_core.h | 28 +++++- 2 files changed, 134 insertions(+), 5 deletions(-) create mode 100644 tests/web/comments-viewport-resize.spec.ts diff --git a/tests/web/comments-viewport-resize.spec.ts b/tests/web/comments-viewport-resize.spec.ts new file mode 100644 index 0000000..69baf96 --- /dev/null +++ b/tests/web/comments-viewport-resize.spec.ts @@ -0,0 +1,111 @@ +import { test, expect, type Page } from '@playwright/test'; + +/** + * Comment pins vs. canvas resize (collab-presence 0005 regression): the DOM + * hit targets map world→CSS through the viewport transform pushed from C++ + * ({cx,cy,scale,w,h} → worldToScreen maps through w/2,h/2). The push used to + * dedupe on scale+center only, so a canvas RESIZE (window resize, boot layout + * settling after the bind-time seed) left a stale w/h in React and every pin + * target sat vertically offset from its GAL dot — by exactly Δh/2 css-px — + * until the next pan/zoom. Now wxEVT_SIZE re-pushes and size participates in + * the dedupe, so pins must re-align after a resize with no pan/zoom at all. + * + * "GAL truth" is computed from a FRESH kicadCollabGetViewport() read + the + * live glcanvas rect — the same mapping the GAL dot itself renders through. + */ + +const SCOPE = 'default'; +const ROUTE = 'demo.kicad_sch'; +const TITLE = /demo — Schematic Editor/i; + +async function bootAs(page: Page, user: string): Promise { + await page.goto(`/${SCOPE}/projects/demo/${ROUTE}?user=${user}`); + await expect(page.locator('#canvas')).toBeVisible({ timeout: 120000 }); + await expect + .poll(() => page.title(), { + message: `${user}: editor never reached the expected title`, + timeout: 120000, + intervals: [1000], + }) + .toMatch(TITLE); + await expect(page.getByTestId('comment-bar-toggle')).toBeVisible({ timeout: 30000 }); + await page.getByTestId('comment-bar-toggle').click(); + await expect(page.getByTestId('comment-mode-toggle')).toBeVisible(); +} + +/** Comments persist in the room's ydoc — start each run from a clean slate. */ +async function deleteAllThreads(page: Page): Promise { + await page.evaluate(() => { + const ctl = (window as unknown as { + __pcbjamComments?: { + threads(): Array<{ id: string }>; + deleteThread(id: string): void; + }; + }).__pcbjamComments; + ctl?.threads().forEach((t) => ctl.deleteThread(t.id)); + }); + await expect(page.getByTestId('comment-pin')).toHaveCount(0); +} + +/** First pin's DOM center minus its GAL-truth CSS position (fresh transform). */ +async function pinDelta(page: Page): Promise<{ dx: number; dy: number }> { + return page.evaluate(() => { + const win = window as unknown as { + Module: { kicadCollabGetViewport(): string }; + __pcbjamComments: { threads(): Array<{ world: { x: number; y: number } }> }; + }; + const vp = JSON.parse(win.Module.kicadCollabGetViewport()) as { + cx: number; cy: number; scale: number; w: number; h: number; + }; + const gl = Array.from(document.querySelectorAll('[id^="glcanvas-"]')).find((c) => { + const r = (c as HTMLElement).getBoundingClientRect(); + return getComputedStyle(c as HTMLElement).display !== 'none' && r.width > 0; + }) as HTMLElement; + const r = gl.getBoundingClientRect(); + const ratio = r.width / vp.w; + const world = win.__pcbjamComments.threads()[0].world; + const truthX = r.x + ((world.x - vp.cx) * vp.scale + vp.w / 2) * ratio; + const truthY = r.y + ((world.y - vp.cy) * vp.scale + vp.h / 2) * ratio; + const pin = document.querySelector('[data-testid="comment-pin"]') as HTMLElement; + const pr = pin.getBoundingClientRect(); + return { dx: pr.x + pr.width / 2 - truthX, dy: pr.y + pr.height / 2 - truthY }; + }); +} + +test('comment pin targets track the GAL dots across a canvas resize', async ({ page }) => { + test.setTimeout(300000); // one full tool boot + + await bootAs(page, 'alice'); + await deleteAllThreads(page); + + // Place a comment mid-canvas via comment mode. + await page.getByTestId('comment-mode-toggle').click(); + const catcher = page.getByTestId('comment-click-catcher'); + await expect(catcher).toBeVisible(); + const box = (await catcher.boundingBox())!; + await catcher.click({ position: { x: box.width / 2, y: box.height / 2 } }); + await page.getByTestId('comment-composer').locator('textarea').fill('resize pin'); + await page.getByTestId('comment-submit').click(); + await expect(page.getByTestId('comment-pin')).toHaveCount(1); + + // Aligned at the boot size (the composer click and the pin share the same + // transform, so this mostly guards the harness itself). + await expect + .poll(async () => Math.abs((await pinDelta(page)).dy), { + message: 'pin never aligned with GAL truth at the boot size', + timeout: 15000, + }) + .toBeLessThan(3); + + // Shrink the window: the canvas height changes while scale/center stay put. + // The wxEVT_SIZE re-push must realign the DOM target with the GAL dot — + // before the fix it stayed exactly Δh/2 css-px below until a pan/zoom. + await page.setViewportSize({ width: 1280, height: 620 }); + await expect + .poll(async () => Math.abs((await pinDelta(page)).dy), { + message: 'pin target did not re-align after the canvas resize (stale viewport w/h?)', + timeout: 15000, + }) + .toBeLessThan(3); + expect(Math.abs((await pinDelta(page)).dx)).toBeLessThan(3); +}); diff --git a/wasm/bindings/collab_presence_core.h b/wasm/bindings/collab_presence_core.h index 4341c31..77e66e4 100644 --- a/wasm/bindings/collab_presence_core.h +++ b/wasm/bindings/collab_presence_core.h @@ -142,6 +142,7 @@ struct CORE long long lastCursorEmitMs = 0; double lastVpScale = 0.0; VECTOR2D lastVpCenter; + VECTOR2I lastVpSize; // ── local state emit (selection / cursor / viewport) ────────────────── @@ -203,17 +204,23 @@ struct CORE if( !fr ) return; - KIGFX::VIEW* view = fr->GetCanvas()->GetView(); - double scale = view->GetScale(); // zoom — cheap change detector only - VECTOR2D c = view->GetCenter(); + KIGFX::VIEW* view = fr->GetCanvas()->GetView(); + double scale = view->GetScale(); // zoom — cheap change detector only + VECTOR2D c = view->GetCenter(); + const VECTOR2I& sz = view->GetScreenPixelSize(); - if( scale == lastVpScale && c == lastVpCenter ) + // Size participates in the dedupe: the JS worldToScreen maps through + // w/2,h/2, so a canvas resize (or the boot layout settling after the + // bind-time seed) with an unchanged scale/center must still re-push — + // else every DOM pin target is vertically offset until the next + // pan/zoom. + if( scale == lastVpScale && c == lastVpCenter && sz == lastVpSize ) return; lastVpScale = scale; lastVpCenter = c; + lastVpSize = sz; - const VECTOR2I& sz = view->GetScreenPixelSize(); // px per IU via the GAL matrix — GetScale() is the zoom, not px/IU. pcbjam_collab::emitViewport( c.x, c.y, view->ToScreen( 1.0 ), sz.x, sz.y ); @@ -453,6 +460,17 @@ struct CORE canvas->Bind( wxEVT_RIGHT_UP, [selAndViewport]( wxMouseEvent& e ) { selAndViewport( e ); } ); canvas->Bind( wxEVT_KEY_UP, [selAndViewport]( wxKeyEvent& e ) { selAndViewport( e ); } ); canvas->Bind( wxEVT_MOUSEWHEEL, [selAndViewport]( wxMouseEvent& e ) { selAndViewport( e ); } ); + + // Canvas resizes change the w/h half of the world↔screen transform + // without touching scale/center — re-push post-layout (CallAfter runs + // after the GAL's own onSize updated the screen size). + canvas->Bind( wxEVT_SIZE, [this]( wxSizeEvent& e ) + { + e.Skip(); + + if( EDA_DRAW_FRAME* f = frame() ) + f->CallAfter( [this]() { emitViewportIfChanged(); } ); + } ); } /** kicadCollabSetRemote: full remote-peers snapshot — `{peers:[{id,name,