From d8a3d200b77f33b2c1bf3d7337696045fc541ab1 Mon Sep 17 00:00:00 2001 From: Nightt <87569709+nightt5879@users.noreply.github.com> Date: Mon, 7 Sep 2026 16:48:24 +0800 Subject: [PATCH 1/2] test(drawnix): cover freehand viewport change lifecycle Extract the existing React board change effect into an internal hook and exercise it with the shared fixture, real viewport transforms, ListRender, and RoughSVG output. Cover zoom in/out, batched viewport changes, and scrolling. Document why the current post-layout active refresh remains necessary. Refs #461 --- .../freehand/freehand.component.spec.ts | 203 ++++++++++++++---- .../drawnix/testing/viewport-regressions.md | 46 ++++ .../react-board/src/hooks/use-board-change.ts | 72 +++++++ packages/react-board/src/wrapper.tsx | 62 +----- 4 files changed, 282 insertions(+), 101 deletions(-) create mode 100644 packages/drawnix/testing/viewport-regressions.md create mode 100644 packages/react-board/src/hooks/use-board-change.ts diff --git a/packages/drawnix/src/plugins/freehand/freehand.component.spec.ts b/packages/drawnix/src/plugins/freehand/freehand.component.spec.ts index 62417d7f..388b572e 100644 --- a/packages/drawnix/src/plugins/freehand/freehand.component.spec.ts +++ b/packages/drawnix/src/plugins/freehand/freehand.component.spec.ts @@ -1,57 +1,176 @@ -import { BOARD_TO_ELEMENT_HOST, createG, createTestingBoard, PlaitBoard } from '@plait/core'; +import { + ACTIVE_STROKE_WIDTH, + BOARD_TO_HOST, + BoardTransforms, + createSVG, + ListRender, + PlaitBoard, + RESIZE_HANDLE_CLASS_NAME, + SELECTION_RECTANGLE_CLASS_NAME, + Transforms, + updateViewportByScrolling, + withBoard, + withOptions, +} from '@plait/core'; +import { act, renderHook } from '@testing-library/react'; import { afterEach, describe, expect, it, vi } from 'vitest'; -import { FreehandComponent } from './freehand.component'; -import { Freehand, FreehandShape } from './type'; +import { useBoardChange } from '../../../../react-board/src/hooks/use-board-change'; +import { setupDrawnixTestingBoard } from '../../../testing'; +import { FreehandShape } from './type'; +import { createFreehandElement } from './utils'; +import { withFreehand } from './with-freehand'; -describe('FreehandComponent', () => { - let board: PlaitBoard; +// jsdom has no SVG layout. Model only the browser's viewBox/scroll geometry; +// viewport transforms, rendering, and the production change hook remain real. +const attachViewportLayout = (board: PlaitBoard) => { + const container = PlaitBoard.getBoardContainer(board); + const viewport = PlaitBoard.getViewportContainer(board); + const host = createSVG(); + const containerRect = new DOMRect(120, 80, 800, 600); + Object.defineProperty(container, 'getBoundingClientRect', { value: () => containerRect }); + Object.defineProperties(host, { + viewBox: { + get: () => { + const [x, y, width, height] = (host.getAttribute('viewBox') || '0 0 800 600') + .split(' ') + .map(Number); + return { baseVal: { x, y, width, height } }; + }, + }, + getBoundingClientRect: { + value: () => + new DOMRect( + containerRect.x - viewport.scrollLeft, + containerRect.y - viewport.scrollTop, + parseFloat(host.style.width), + parseFloat(host.style.height) + ), + }, + }); + BOARD_TO_HOST.set(board, host); + host.append(PlaitBoard.getElementHost(board)); + const activeSvg = createSVG(); + activeSvg.append(PlaitBoard.getActiveHost(board)); + viewport.append(host, activeSvg); + container.append(viewport); + return { host, viewport }; +}; + +describe('freehand active drawing on viewport changes', () => { + let dispose: (() => void) | undefined; afterEach(() => { - vi.restoreAllMocks(); - BOARD_TO_ELEMENT_HOST.delete(board); + dispose?.(); + dispose = undefined; }); - it('refreshes its active section on request', () => { - const element = { - id: 'freehand', - type: 'freehand', - shape: FreehandShape.feltTipPen, - points: [ - [0, 0], - [10, 10], - ], - } as Freehand; - board = createTestingBoard([], [element]); - const activeHost = createG(); - BOARD_TO_ELEMENT_HOST.set(board, { - lowerHost: createG(), - host: createG(), - upperHost: createG(), - topHost: createG(), - activeHost, - container: document.createElement('div'), - viewportContainer: document.createElement('div'), + const setup = async () => { + const element = createFreehandElement(FreehandShape.feltTipPen, [ + [300, 240], + [350, 270], + [420, 300], + ]); + const fixture = setupDrawnixTestingBoard([withOptions, withBoard, withFreehand], [element], { + selectedElements: [element], + withRoughSVG: true, + withHost: false, }); - const component = new FreehandComponent(); - component.context = { + const { board } = fixture; + const { host, viewport } = attachViewportLayout(board); + const listRender = new ListRender(board); + listRender.initialize(board.children, { board, - element, parent: board, - index: 0, - selected: true, - hasThemeChanged: false, + parentG: PlaitBoard.getElementHost(board), + }); + const afterChange = vi.fn(); + const { unmount } = renderHook(() => useBoardChange(board, listRender, afterChange)); + // Observe actual RoughSVG output without replacing its drawing implementation. + const rectangle = vi.spyOn(PlaitBoard.getRoughSVG(board), 'rectangle'); + dispose = () => { + unmount(); + listRender.destroy(); + rectangle.mockRestore(); + BOARD_TO_HOST.delete(board); + fixture.destroy(); }; - component.initializeGenerator(); - const processDrawing = vi - .spyOn(component.activeGenerator, 'processDrawing') - .mockImplementation(() => {}); + await act(async () => { + Transforms.setViewport(board, { zoom: 1, origination: [0, 0] }); + }); + expect(afterChange).toHaveBeenCalledOnce(); + afterChange.mockClear(); + rectangle.mockClear(); + return { board, element, host, viewport, rectangle, afterChange }; + }; + + const expectOutline = ( + { board, rectangle }: Awaited>, + [x, y, width, height]: number[] + ) => { + const padding = (ACTIVE_STROKE_WIDTH * board.viewport.zoom) / 2; + expect(rectangle).toHaveBeenLastCalledWith( + x - padding, + y - padding, + width + padding * 2, + height + padding * 2, + expect.objectContaining({ strokeWidth: ACTIVE_STROKE_WIDTH }) + ); + const activeHost = PlaitBoard.getActiveHost(board); + const outlines = activeHost.querySelectorAll(`.${SELECTION_RECTANGLE_CLASS_NAME}`); + expect(outlines).toHaveLength(1); + expect(outlines[0]).toBe(rectangle.mock.results[rectangle.mock.results.length - 1]?.value); + expect(outlines[0].querySelector('path')).not.toBeNull(); + expect(activeHost.querySelectorAll(`.${RESIZE_HANDLE_CLASS_NAME}`)).toHaveLength(4); + }; + + it.each([ + { zoom: 2, bounds: [200, 180, 240, 120] }, + { zoom: 0.5, bounds: [350, 270, 60, 30] }, + ])('keeps the outline aligned after zooming to $zoom', async ({ zoom, bounds }) => { + const state = await setup(); + const oldViewBox = state.host.getAttribute('viewBox'); + const oldScroll = [state.viewport.scrollLeft, state.viewport.scrollTop]; + + await act(async () => { + BoardTransforms.updateZoom(state.board, zoom); + expect(state.afterChange).not.toHaveBeenCalled(); + expect(state.rectangle).not.toHaveBeenCalled(); + }); - board.viewport.zoom = 2; - component.getRef().updateActiveSection(); + expect(state.board.viewport.zoom).toBe(zoom); + expect(state.host.getAttribute('viewBox')).not.toBe(oldViewBox); + expect([state.viewport.scrollLeft, state.viewport.scrollTop]).not.toEqual(oldScroll); + expect(state.afterChange).toHaveBeenCalledOnce(); + expect(state.board.operations).toEqual([]); + expectOutline(state, bounds); + }); + + it('uses the final viewport when zoom and origin change in one cycle', async () => { + const state = await setup(); + + await act(async () => { + Transforms.setViewport(state.board, { zoom: 2, origination: [200, 150] }); + Transforms.setViewport(state.board, { zoom: 1.5, origination: [240, 190] }); + }); - expect(processDrawing).toHaveBeenCalledOnce(); - expect(processDrawing).toHaveBeenCalledWith(element, activeHost, { - selected: true, + expect(state.afterChange).toHaveBeenCalledOnce(); + expect(state.board.operations).toEqual([]); + expectOutline(state, [90, 75, 180, 90]); + }); + + it('keeps the outline aligned when scrolling without rebuilding the viewBox', async () => { + const state = await setup(); + const oldViewBox = state.host.getAttribute('viewBox'); + + await act(async () => { + state.viewport.scrollLeft += 40; + state.viewport.scrollTop += 30; + updateViewportByScrolling(state.board, state.viewport.scrollLeft, state.viewport.scrollTop); }); + + expect(state.host.getAttribute('viewBox')).toBe(oldViewBox); + expect(state.board.viewport.origination).toEqual([40, 30]); + expect(state.afterChange).toHaveBeenCalledOnce(); + expectOutline(state, [260, 210, 120, 60]); }); }); diff --git a/packages/drawnix/testing/viewport-regressions.md b/packages/drawnix/testing/viewport-regressions.md new file mode 100644 index 00000000..3eb20ae8 --- /dev/null +++ b/packages/drawnix/testing/viewport-regressions.md @@ -0,0 +1,46 @@ +# Freehand viewport regression coverage + +Follow-up to [#461](https://github.com/plait-board/drawnix/issues/461) and the +selection-outline fix in [#454](https://github.com/plait-board/drawnix/pull/454). + +## Lifecycle finding + +The React board change cycle updates `ListRender`, updates the SVG viewBox, +applies the viewport scroll offset, and then refreshes selected elements' active +sections. During the first pass of a viewport change, a selected freehand element +can see the new zoom together with the previous viewBox and scroll offset. Its outline +must be redrawn after those layout inputs have been updated. + +Keep the final `updateActiveSection` pass under the current architecture. Simply +removing it, or removing the freehand component's implementation, produces wrong +outline coordinates. Both mutations were checked against the regression suite: +the zoom-in, zoom-out, and batched viewport cases fail; the scrolling case passes +because scrolling updates the layout inputs before the render cycle. + +`useBoardChange` contains the existing Wrapper effect without changing its order +or branches. It is internal to `react-board`, allowing this suite to exercise the +production lifecycle rather than reproducing it in a test callback. A shared +single-pass overlay lifecycle remains a separate design change: it needs to +account for element initialization/context updates, viewBox calculation, and all +overlay implementations, beyond the freehand behavior covered here. + +## Test boundary + +`freehand.component.spec.ts` uses `setupDrawnixTestingBoard`, explicit plugins, +real `ListRender`, the production change hook, real viewport transforms, and real +RoughSVG drawing. React's async `act` flushes the board's microtask change cycle. +The tests check outline coordinates, the actual SVG node mounted in the active +host, and resize handles after zooming, batched viewport changes, and scrolling. + +Only jsdom's missing SVG layout is supplied locally: the board container has a +fixed nonzero screen offset, the SVG viewBox reflects its current attribute, and +the SVG screen position follows the viewport's scroll offsets. Drawing calls are +observed with a call-through spy. No Plait module or drawing implementation is +mocked. These tests do not emulate browser scroll clamping, native event timing, +or pixel rasterization; those remain browser-test concerns. + +Run the focused suite from the repository root: + +```sh +npx vitest run --config packages/drawnix/vite.config.ts packages/drawnix/src/plugins/freehand/freehand.component.spec.ts +``` diff --git a/packages/react-board/src/hooks/use-board-change.ts b/packages/react-board/src/hooks/use-board-change.ts new file mode 100644 index 00000000..0aad6fa8 --- /dev/null +++ b/packages/react-board/src/hooks/use-board-change.ts @@ -0,0 +1,72 @@ +import { + BOARD_TO_AFTER_CHANGE, + BOARD_TO_ON_CHANGE, + getSelectedElements, + initializeViewBox, + isFromScrolling, + ListRender, + PlaitBoard, + PlaitElement, + setIsFromScrolling, + updateViewBox, + updateViewportOffset, +} from '@plait/core'; +import { PlaitCommonElementRef } from '@plait/common'; +import { useEffect } from 'react'; + +export const useBoardChange = ( + board: PlaitBoard, + listRender: ListRender, + onContextChange: () => void +) => { + useEffect(() => { + BOARD_TO_ON_CHANGE.set(board, () => { + const isOnlySetSelection = + board.operations.length && board.operations.every((op) => op.type === 'set_selection'); + if (isOnlySetSelection) { + listRender.update(board.children, { + board: board, + parent: board, + parentG: PlaitBoard.getElementHost(board), + }); + return; + } + const isSetViewport = + board.operations.length && board.operations.some((op) => op.type === 'set_viewport'); + if (isSetViewport && isFromScrolling(board)) { + setIsFromScrolling(board, false); + listRender.update(board.children, { + board: board, + parent: board, + parentG: PlaitBoard.getElementHost(board), + }); + return; + } + listRender.update(board.children, { + board: board, + parent: board, + parentG: PlaitBoard.getElementHost(board), + }); + if (isSetViewport) { + initializeViewBox(board); + } else { + updateViewBox(board); + } + updateViewportOffset(board); + const selectedElements = getSelectedElements(board); + selectedElements.forEach((element) => { + const elementRef = PlaitElement.getElementRef(element); + elementRef.updateActiveSection(); + }); + }); + + BOARD_TO_AFTER_CHANGE.set(board, () => { + onContextChange(); + }); + + return () => { + BOARD_TO_ON_CHANGE.delete(board); + BOARD_TO_AFTER_CHANGE.delete(board); + }; + }, [board, listRender, onContextChange]); +}; diff --git a/packages/react-board/src/wrapper.tsx b/packages/react-board/src/wrapper.tsx index 586d8637..617a4584 100644 --- a/packages/react-board/src/wrapper.tsx +++ b/packages/react-board/src/wrapper.tsx @@ -1,5 +1,4 @@ import { - BOARD_TO_ON_CHANGE, ListRender, PlaitElement, Viewport, @@ -17,26 +16,20 @@ import { type PlaitBoardOptions, type Selection, ThemeColorMode, - BOARD_TO_AFTER_CHANGE, PlaitOperation, PlaitTheme, - isFromScrolling, - setIsFromScrolling, - getSelectedElements, - updateViewportOffset, - initializeViewBox, withI18n, - updateViewBox, FLUSHING, BoardTransforms, } from '@plait/core'; import { BoardChangeData } from './plugins/board'; import { useCallback, useEffect, useRef, useState } from 'react'; import { withReact } from './plugins/with-react'; -import { PlaitCommonElementRef, withImage, withText } from '@plait/common'; +import { withImage, withText } from '@plait/common'; import { BoardContext, BoardContextValue } from './hooks/use-board'; import React from 'react'; import { withPinchZoom } from './plugins/with-pinch-zoom-plugin'; +import { useBoardChange } from './hooks/use-board-change'; export type WrapperProps = { value: PlaitElement[]; @@ -136,56 +129,7 @@ export const Wrapper: React.FC = ({ onViewportChange, ]); - useEffect(() => { - BOARD_TO_ON_CHANGE.set(board, () => { - const isOnlySetSelection = - board.operations.length && board.operations.every((op) => op.type === 'set_selection'); - if (isOnlySetSelection) { - listRender.update(board.children, { - board: board, - parent: board, - parentG: PlaitBoard.getElementHost(board), - }); - return; - } - const isSetViewport = - board.operations.length && board.operations.some((op) => op.type === 'set_viewport'); - if (isSetViewport && isFromScrolling(board)) { - setIsFromScrolling(board, false); - listRender.update(board.children, { - board: board, - parent: board, - parentG: PlaitBoard.getElementHost(board), - }); - return; - } - listRender.update(board.children, { - board: board, - parent: board, - parentG: PlaitBoard.getElementHost(board), - }); - if (isSetViewport) { - initializeViewBox(board); - } else { - updateViewBox(board); - } - updateViewportOffset(board); - const selectedElements = getSelectedElements(board); - selectedElements.forEach((element) => { - const elementRef = PlaitElement.getElementRef(element); - elementRef.updateActiveSection(); - }); - }); - - BOARD_TO_AFTER_CHANGE.set(board, () => { - onContextChange(); - }); - - return () => { - BOARD_TO_ON_CHANGE.delete(board); - BOARD_TO_AFTER_CHANGE.delete(board); - }; - }, [board, listRender, onContextChange]); + useBoardChange(board, listRender, onContextChange); const isFirstRender = useRef(true); From 4b4f306acdba529157d66bef65388e01c98ebb60 Mon Sep 17 00:00:00 2001 From: Nightt <87569709+nightt5879@users.noreply.github.com> Date: Mon, 7 Sep 2026 18:34:19 +0800 Subject: [PATCH 2/2] fix(react-board): finalize viewport before active drawing --- apps/web-e2e/src/viewport.spec.ts | 239 ++++++++++++++++++ .../freehand/freehand.component.spec.ts | 74 ++++++ .../drawnix/testing/viewport-regressions.md | 65 +++-- .../react-board/src/hooks/use-board-change.ts | 20 +- 4 files changed, 375 insertions(+), 23 deletions(-) create mode 100644 apps/web-e2e/src/viewport.spec.ts diff --git a/apps/web-e2e/src/viewport.spec.ts b/apps/web-e2e/src/viewport.spec.ts new file mode 100644 index 00000000..47445340 --- /dev/null +++ b/apps/web-e2e/src/viewport.spec.ts @@ -0,0 +1,239 @@ +import { expect, test, type Locator, type Page } from '@playwright/test'; + +test.use({ screenshot: 'only-on-failure' }); + +const text = { type: 'paragraph', children: [{ text: 'Viewport' }] }; +const points = [ + [300, 240], + [420, 300], +]; +const examples = { + freehand: { + id: 'subject', + type: 'freehand', + shape: 'feltTipPen', + points, + strokeWidth: 4, + strokeColor: '#222222', + fill: 'none', + }, + rectangle: { + id: 'subject', + type: 'geometry', + shape: 'rectangle', + points, + text, + fill: '#ddeeff', + }, + image: { + id: 'subject', + type: 'image', + points, + url: `data:image/svg+xml,${encodeURIComponent('')}`, + }, + arrow: { + id: 'subject', + type: 'arrow-line', + shape: 'straight', + points, + source: { marker: 'none' }, + target: { marker: 'arrow' }, + texts: [], + }, + mind: { + id: 'subject', + type: 'mind', + points: [[300, 240]], + layout: 'right', + data: { topic: text }, + children: [], + }, +}; + +async function loadBoard(page: Page, children: object[]) { + await page.goto('/'); + await expect(page.locator('.drawnix')).toBeVisible(); + // Seed the app's normal persistence in this test's isolated browser context. + // Interactions below use the UI, without exposing or invoking the board API. + await page.evaluate(async (children) => { + await new Promise((resolve, reject) => { + const request = indexedDB.open('Drawnix'); + request.onerror = () => reject(request.error); + request.onsuccess = () => { + const db = request.result; + const transaction = db.transaction('drawnix_store', 'readwrite'); + const store = transaction.objectStore('drawnix_store'); + store.put({ children, viewport: { zoom: 1, origination: [0, 0] } }, 'main_board_content'); + transaction.oncomplete = () => { + db.close(); + resolve(); + }; + transaction.onerror = () => { + db.close(); + reject(transaction.error); + }; + }; + }); + }, children); + await page.reload(); + await expect(page.locator('[plait-data-id="subject"]').first()).toBeVisible(); + await expect(page.locator('.zoom-menu-trigger')).toHaveText('100%'); + await page + .getByRole('radio', { name: /选择/ }) + .locator('..') + .click(); + await expect(page.getByRole('radio', { name: /选择/ })).toBeChecked(); +} + +function shape(page: Page, kind: keyof typeof examples, id = 'subject') { + return page + .locator(`.board-host-svg [plait-data-id="${id}"]`) + .locator(kind === 'image' ? '.foreign-object-image' : 'path') + .first(); +} + +async function bounds(locator: Locator) { + const box = await locator.boundingBox(); + expect(box).not.toBeNull(); + return box!; +} + +async function expectAligned(page: Page, content: Locator) { + const outline = page.locator('.active-host-g .selection-rectangle'); + await expect(outline).toHaveCount(1); + await expect + .poll(async () => { + const actual = await bounds(outline); + const expected = await bounds(content); + // Selection padding, pen thickness and endpoint handles differ by element. + // Compare all four edges in screen pixels, including native SVG layout. + return Math.max( + Math.abs(actual.x - expected.x), + Math.abs(actual.y - expected.y), + Math.abs(actual.x + actual.width - expected.x - expected.width), + Math.abs(actual.y + actual.height - expected.y - expected.height) + ); + }) + .toBeLessThan(8); +} + +for (const kind of Object.keys(examples) as (keyof typeof examples)[]) { + test(`${kind} selection follows zoom, scrolling and movement`, async ({ page }, testInfo) => { + const errors: string[] = []; + page.on('pageerror', (error) => errors.push(error.message)); + await loadBoard(page, [examples[kind]]); + const content = shape(page, kind); + const initial = await bounds(content); + await page.mouse.click(initial.x + initial.width / 2, initial.y + initial.height / 2); + await expectAligned(page, content); + + await page.locator('.zoom-in-button').click(); + await expect(page.locator('.zoom-menu-trigger')).toHaveText('110%'); + await expectAligned(page, content); + await page.locator('.zoom-out-button').click(); + await page.locator('.zoom-out-button').click(); + await expect(page.locator('.zoom-menu-trigger')).toHaveText('90%'); + await expectAligned(page, content); + + // A large mouse-wheel step reproduces the original visible drift better + // than tiny trackpad deltas. Wait for the event's observable zoom change. + await page.mouse.move(550, 420); + await page.keyboard.down('Control'); + await page.mouse.wheel(0, -120); + await expect(page.locator('.zoom-menu-trigger')).not.toHaveText('90%'); + await page.keyboard.up('Control'); + await expectAligned(page, content); + + const beforeScroll = await bounds(content); + await page.mouse.move(160, 550); + await page.mouse.wheel(40, 70); + await expect.poll(async () => (await bounds(content)).y).toBeLessThan(beforeScroll.y - 20); + await expectAligned(page, content); + + const beforeMove = await bounds(content); + // Avoid arrow midpoint handles and shape text editors while dragging. + const x = beforeMove.x + beforeMove.width / 4; + const y = beforeMove.y + beforeMove.height / 4; + await page.mouse.move(x, y); + await page.mouse.down(); + await page.mouse.move(x + 90, y + 60, { steps: 10 }); + await page.mouse.up(); + await expect.poll(async () => (await bounds(content)).x).toBeGreaterThan(beforeMove.x + 60); + await expectAligned(page, content); + const previousZoom = await page.locator('.zoom-menu-trigger').innerText(); + await page.locator('.zoom-in-button').click(); + await expect(page.locator('.zoom-menu-trigger')).not.toHaveText(previousZoom); + await expectAligned(page, content); + + const screenshotPath = testInfo.outputPath(`${kind}-selection.png`); + await page.screenshot({ path: screenshotPath }); + await testInfo.attach(`${kind}-selection`, { path: screenshotPath, contentType: 'image/png' }); + expect(errors).toEqual([]); + }); +} + +test('mind selection follows a text layout change and subsequent zoom', async ({ page }) => { + await loadBoard(page, [examples.mind]); + const content = shape(page, 'mind'); + const initial = await bounds(content); + await page.mouse.dblclick(initial.x + initial.width / 2, initial.y + initial.height / 2); + const editor = page.locator('[plait-data-id="subject"] [contenteditable="true"]'); + await expect(editor).toBeVisible(); + await editor.fill('A wider mind topic after editing'); + await page.mouse.click(160, 550); + await expect.poll(async () => (await bounds(content)).width).toBeGreaterThan(initial.width + 30); + const resized = await bounds(content); + await page.mouse.click(resized.x + resized.width / 2, resized.y + resized.height / 2); + await expectAligned(page, content); + await page.locator('.zoom-in-button').click(); + await expect(page.locator('.zoom-menu-trigger')).toHaveText('110%'); + await expectAligned(page, content); +}); + +test('multiple selection bounds follow zoom and native scrolling', async ({ page }) => { + await loadBoard(page, [ + examples.freehand, + { + ...examples.rectangle, + id: 'second', + points: [ + [500, 340], + [620, 400], + ], + }, + ]); + const first = shape(page, 'freehand'); + const second = shape(page, 'rectangle', 'second'); + for (const [index, content] of [first, second].entries()) { + const box = await bounds(content); + if (index) await page.keyboard.down('Shift'); + await page.mouse.click(box.x + box.width / 2, box.y + box.height / 2); + if (index) await page.keyboard.up('Shift'); + } + const outline = page.locator('.selection-rectangle-bounding'); + const check = async () => { + await expect(outline).toHaveCount(1); + await expect + .poll(async () => { + const a = await bounds(first); + const b = await bounds(second); + const actual = await bounds(outline); + return Math.max( + Math.abs(actual.x - Math.min(a.x, b.x)), + Math.abs(actual.y - Math.min(a.y, b.y)), + Math.abs(actual.x + actual.width - Math.max(a.x + a.width, b.x + b.width)), + Math.abs(actual.y + actual.height - Math.max(a.y + a.height, b.y + b.height)) + ); + }) + .toBeLessThan(8); + }; + await check(); + await page.locator('.zoom-in-button').click(); + await expect(page.locator('.zoom-menu-trigger')).toHaveText('110%'); + await check(); + const beforeScroll = await bounds(first); + await page.mouse.move(160, 550); + await page.mouse.wheel(40, 70); + await expect.poll(async () => (await bounds(first)).y).toBeLessThan(beforeScroll.y - 20); + await check(); +}); diff --git a/packages/drawnix/src/plugins/freehand/freehand.component.spec.ts b/packages/drawnix/src/plugins/freehand/freehand.component.spec.ts index 388b572e..ce177c01 100644 --- a/packages/drawnix/src/plugins/freehand/freehand.component.spec.ts +++ b/packages/drawnix/src/plugins/freehand/freehand.component.spec.ts @@ -8,6 +8,7 @@ import { RESIZE_HANDLE_CLASS_NAME, SELECTION_RECTANGLE_CLASS_NAME, Transforms, + isFromScrolling, updateViewportByScrolling, withBoard, withOptions, @@ -142,6 +143,7 @@ describe('freehand active drawing on viewport changes', () => { expect([state.viewport.scrollLeft, state.viewport.scrollTop]).not.toEqual(oldScroll); expect(state.afterChange).toHaveBeenCalledOnce(); expect(state.board.operations).toEqual([]); + expect(state.rectangle).toHaveBeenCalledOnce(); expectOutline(state, bounds); }); @@ -155,6 +157,7 @@ describe('freehand active drawing on viewport changes', () => { expect(state.afterChange).toHaveBeenCalledOnce(); expect(state.board.operations).toEqual([]); + expect(state.rectangle).toHaveBeenCalledOnce(); expectOutline(state, [90, 75, 180, 90]); }); @@ -171,6 +174,77 @@ describe('freehand active drawing on viewport changes', () => { expect(state.host.getAttribute('viewBox')).toBe(oldViewBox); expect(state.board.viewport.origination).toEqual([40, 30]); expect(state.afterChange).toHaveBeenCalledOnce(); + expect(state.rectangle).toHaveBeenCalledOnce(); + expect(isFromScrolling(state.board)).toBe(false); expectOutline(state, [260, 210, 120, 60]); }); + + it('finalizes a zoom batched after native scrolling', async () => { + const state = await setup(); + await act(async () => { + state.viewport.scrollLeft += 40; + state.viewport.scrollTop += 30; + updateViewportByScrolling(state.board, state.viewport.scrollLeft, state.viewport.scrollTop); + BoardTransforms.updateZoom(state.board, 2); + }); + expect(state.afterChange).toHaveBeenCalledOnce(); + expect(state.rectangle).toHaveBeenCalledOnce(); + expect(isFromScrolling(state.board)).toBe(false); + expectOutline(state, [120, 120, 240, 120]); + }); + + it('applies an origin change batched after native scrolling', async () => { + const state = await setup(); + await act(async () => { + state.viewport.scrollLeft += 40; + state.viewport.scrollTop += 30; + updateViewportByScrolling(state.board, state.viewport.scrollLeft, state.viewport.scrollTop); + Transforms.setViewport(state.board, { zoom: 1, origination: [80, 60] }); + }); + expect(state.rectangle).toHaveBeenCalledOnce(); + expectOutline(state, [220, 180, 120, 60]); + }); + + it('refreshes after rendering when an element change is batched with zoom', async () => { + const state = await setup(); + await act(async () => { + Transforms.setNode( + state.board, + { + points: [ + [1000, 800], + [1120, 860], + ], + }, + [0] + ); + BoardTransforms.updateZoom(state.board, 2); + }); + expect(state.afterChange).toHaveBeenCalledOnce(); + expectOutline(state, [1600, 1300, 240, 120]); + }); + + it('does not skip element layout in a batch that includes native scrolling', async () => { + const state = await setup(); + const oldViewBox = state.host.getAttribute('viewBox'); + await act(async () => { + state.viewport.scrollLeft += 40; + state.viewport.scrollTop += 30; + updateViewportByScrolling(state.board, state.viewport.scrollLeft, state.viewport.scrollTop); + Transforms.setNode( + state.board, + { + points: [ + [1000, 800], + [1120, 860], + ], + }, + [0] + ); + }); + expect(state.host.getAttribute('viewBox')).not.toBe(oldViewBox); + expect(state.afterChange).toHaveBeenCalledOnce(); + expect(isFromScrolling(state.board)).toBe(false); + expectOutline(state, [960, 770, 120, 60]); + }); }); diff --git a/packages/drawnix/testing/viewport-regressions.md b/packages/drawnix/testing/viewport-regressions.md index 3eb20ae8..6e3460f4 100644 --- a/packages/drawnix/testing/viewport-regressions.md +++ b/packages/drawnix/testing/viewport-regressions.md @@ -1,46 +1,67 @@ -# Freehand viewport regression coverage +# Viewport lifecycle and selection regressions Follow-up to [#461](https://github.com/plait-board/drawnix/issues/461) and the selection-outline fix in [#454](https://github.com/plait-board/drawnix/pull/454). ## Lifecycle finding -The React board change cycle updates `ListRender`, updates the SVG viewBox, -applies the viewport scroll offset, and then refreshes selected elements' active -sections. During the first pass of a viewport change, a selected freehand element -can see the new zoom together with the previous viewBox and scroll offset. Its outline -must be redrawn after those layout inputs have been updated. - -Keep the final `updateActiveSection` pass under the current architecture. Simply -removing it, or removing the freehand component's implementation, produces wrong -outline coordinates. Both mutations were checked against the regression suite: -the zoom-in, zoom-out, and batched viewport cases fail; the scrolling case passes -because scrolling updates the layout inputs before the render cycle. - -`useBoardChange` contains the existing Wrapper effect without changing its order -or branches. It is internal to `react-board`, allowing this suite to exercise the -production lifecycle rather than reproducing it in a test callback. A shared -single-pass overlay lifecycle remains a separate design change: it needs to -account for element initialization/context updates, viewBox calculation, and all -overlay implementations, beyond the freehand behavior covered here. +Previously, the React board change cycle updated `ListRender`, updated the SVG +viewBox, applied the viewport scroll offset, and then refreshed selected elements' +active sections. The first draw could see the new zoom together with the previous +viewBox and scroll offset. The final `updateActiveSection` pass corrected it, but +also redrew the same selection outline. + +`useBoardChange` now distinguishes batches containing only `set_viewport` +operations. Element layout is unchanged in these batches, so it finalizes the +viewBox and scroll offset **before** `ListRender.update()`. Selected components +draw against those final inputs once, without a corrective active-section pass. +Native scrolling keeps its existing viewBox unless the same batch changes zoom; +the final origin is still applied if another viewport transform follows scrolling. + +Selection-only updates retain their existing path. Batches containing element +changes retain rendering before viewport calculation and the final active-section +refresh: mind nodes can recompute their layout during component context updates. +A batch containing both scrolling and element changes must also take this path, +instead of the previous scrolling shortcut that skipped layout updates. + +This resolves the redundant pass for viewport-only updates in the React board. +It does not remove the component `updateActiveSection` contract or provide a new +overlay lifecycle for every kind of data change. Removing that contract requires +coordinated changes to the Plait components' layout and rendering phases. ## Test boundary `freehand.component.spec.ts` uses `setupDrawnixTestingBoard`, explicit plugins, real `ListRender`, the production change hook, real viewport transforms, and real RoughSVG drawing. React's async `act` flushes the board's microtask change cycle. -The tests check outline coordinates, the actual SVG node mounted in the active -host, and resize handles after zooming, batched viewport changes, and scrolling. +The tests check exact outline coordinates, the actual SVG node mounted in the +active host, resize handles, and exactly one draw for pure viewport updates. +They cover zoom in/out, batched zoom/origin changes, native scrolling, scrolling +followed by a zoom/origin override, and element changes batched with zoom/scrolling. +Moving the pure-viewport render back before viewport finalization makes five of +these tests fail on incorrect coordinates; the production order was restored +after this mutation check. Only jsdom's missing SVG layout is supplied locally: the board container has a fixed nonzero screen offset, the SVG viewBox reflects its current attribute, and the SVG screen position follows the viewport's scroll offsets. Drawing calls are observed with a call-through spy. No Plait module or drawing implementation is mocked. These tests do not emulate browser scroll clamping, native event timing, -or pixel rasterization; those remain browser-test concerns. +or pixel rasterization. + +`apps/web-e2e/src/viewport.spec.ts` complements them with Chromium, Firefox and +WebKit tests. Each test seeds the normal IndexedDB board content in an isolated +browser context, then selects elements and operates the real toolbar, mouse wheel, +and drag interactions. Freehand, rectangle, image, straight arrow, mind root, +multiple selection, and mind text layout changes are covered. Assertions compare +the selection outline to rendered SVG geometry on screen; the browser tolerance +allows selection padding, stroke width and endpoint handles. These checks do not +replace the unit tests' exact coordinate and draw-count assertions. Final +single-element screenshots are attached to the Playwright report. Run the focused suite from the repository root: ```sh npx vitest run --config packages/drawnix/vite.config.ts packages/drawnix/src/plugins/freehand/freehand.component.spec.ts +npx playwright test --config apps/web-e2e/playwright.config.ts --workers=3 ``` diff --git a/packages/react-board/src/hooks/use-board-change.ts b/packages/react-board/src/hooks/use-board-change.ts index 0aad6fa8..8f6aa2f8 100644 --- a/packages/react-board/src/hooks/use-board-change.ts +++ b/packages/react-board/src/hooks/use-board-change.ts @@ -33,8 +33,24 @@ export const useBoardChange = ( } const isSetViewport = board.operations.length && board.operations.some((op) => op.type === 'set_viewport'); - if (isSetViewport && isFromScrolling(board)) { + const isOnlySetViewport = + isSetViewport && board.operations.every((op) => op.type === 'set_viewport'); + const fromScrolling = isFromScrolling(board); + if (fromScrolling) { setIsFromScrolling(board, false); + } + if (isOnlySetViewport) { + // Element layout is unchanged. Finalize the viewport before components + // draw their active sections, so no corrective second render is needed. + const hasZoomChanged = board.operations.some( + (op) => op.type === 'set_viewport' && op.properties.zoom !== op.newProperties.zoom + ); + if (!fromScrolling || hasZoomChanged) { + initializeViewBox(board); + } + // Usually a no-op for native scrolling, but a later transform in the + // same batch may have changed the origin again. + updateViewportOffset(board); listRender.update(board.children, { board: board, parent: board, @@ -42,6 +58,8 @@ export const useBoardChange = ( }); return; } + // Element changes can recompute geometry (for example, a mind layout) + // during rendering. Preserve the post-layout refresh for these batches. listRender.update(board.children, { board: board, parent: board,