From 01f22c9d437ce1a8e478495bdefb4a509becae3f Mon Sep 17 00:00:00 2001 From: Hiren Kangad Date: Wed, 11 Mar 2026 17:28:52 +0530 Subject: [PATCH] fix: address code review findings from crop/text commit - CropOverlay: register move/up handlers dynamically on drag start (single handler instead of 8x per-handle), remove on drag end. Override destroy() to call _cleanup() preventing keyboard listener leaks. - Text format toolbar: re-measure text bounds (w/h) after fontSize/fontFamily changes, update spatial index and transform box. - TextEditor: add clearText() method; tools.ts uses it instead of fragile document.querySelector('textarea'). - SceneManager._updateItem: apply crop mask on remote sync for image items. - useCanvasSetup: stop TextEditor on unmount to prevent orphaned textarea. - Double-click zoom: use item.data dimensions instead of getBounds() (which includes shadow offset). --- frontend/src/canvas/CropOverlay.ts | 53 ++++++++++++++++++++-------- frontend/src/canvas/SceneManager.ts | 4 +++ frontend/src/canvas/TextEditor.ts | 8 +++++ frontend/src/canvas/tools.ts | 3 +- frontend/src/hooks/useCanvasSetup.ts | 12 ++++--- frontend/src/pages/Editor.tsx | 13 +++++++ 6 files changed, 72 insertions(+), 21 deletions(-) diff --git a/frontend/src/canvas/CropOverlay.ts b/frontend/src/canvas/CropOverlay.ts index e6b6c3b..360aaac 100644 --- a/frontend/src/canvas/CropOverlay.ts +++ b/frontend/src/canvas/CropOverlay.ts @@ -10,7 +10,6 @@ import { Container, Graphics, FederatedPointerEvent } from 'pixi.js'; import type { Viewport } from 'pixi-viewport'; import type { SceneItem } from './SceneManager'; import type { ImageObject, CropRect } from './scene-format'; -import { ImageSprite } from './sprites/ImageSprite'; const HANDLE_SIZE = 8; const HANDLE_FILL = 0xffffff; @@ -32,6 +31,9 @@ export class CropOverlay extends Container { private _onConfirm: ((item: SceneItem, crop: CropRect) => void) | null = null; private _onCancel: (() => void) | null = null; private _keyHandler: ((e: KeyboardEvent) => void) | null = null; + // Bound references for dynamic event registration + private _boundMove: ((e: FederatedPointerEvent) => void) | null = null; + private _boundUp: (() => void) | null = null; constructor(viewport: Viewport) { super(); @@ -50,7 +52,7 @@ export class CropOverlay extends Container { this._border.eventMode = 'none'; this.addChild(this._border); - // Create handles + // Create handles — only pointerdown on each handle const ids: HandleId[] = ['tl', 'tc', 'tr', 'ml', 'mr', 'bl', 'bc', 'br']; const cursors: Record = { tl: 'nwse-resize', tr: 'nesw-resize', bl: 'nesw-resize', br: 'nwse-resize', @@ -61,9 +63,6 @@ export class CropOverlay extends Container { h.eventMode = 'static'; h.cursor = cursors[id]; h.on('pointerdown', (e: FederatedPointerEvent) => this._onDown(e, id)); - h.on('globalpointermove', (e: FederatedPointerEvent) => this._onMove(e)); - h.on('pointerup', () => this._onUp()); - h.on('pointerupoutside', () => this._onUp()); this._handles.set(id, h); this.addChild(h); } @@ -94,7 +93,6 @@ export class CropOverlay extends Container { if (!this._item) return; const item = this._item; const crop = { ...this._crop }; - // If crop is full image, remove it const isFullImage = crop.x < 0.001 && crop.y < 0.001 && crop.w > 0.999 && crop.h > 0.999; this._cleanup(); this._onConfirm?.(item, isFullImage ? { x: 0, y: 0, w: 1, h: 1 } : crop); @@ -108,7 +106,9 @@ export class CropOverlay extends Container { get isActive(): boolean { return this._item !== null; } + /** Clean up all state and listeners. Safe to call multiple times. */ private _cleanup(): void { + this._removeDragListeners(); this._item = null; this._drag = null; this.visible = false; @@ -118,6 +118,12 @@ export class CropOverlay extends Container { } } + /** Override destroy to ensure cleanup runs. */ + override destroy(options?: any): void { + this._cleanup(); + super.destroy(options); + } + private _draw(): void { if (!this._item) return; const data = this._item.data; @@ -135,26 +141,21 @@ export class CropOverlay extends Container { const cw = this._crop.w * iw; const ch = this._crop.h * ih; - // Dim: draw full image rect, then cut out the crop area using inverted mask approach - // Simpler: draw 4 dim rectangles around the crop area + // Dim: draw 4 rectangles around the crop area this._dim.clear(); - // Top if (cy > iy) { this._dim.rect(ix, iy, iw, cy - iy); this._dim.fill({ color: 0x000000, alpha: DIM_ALPHA }); } - // Bottom const cropBottom = cy + ch; if (cropBottom < iy + ih) { this._dim.rect(ix, cropBottom, iw, iy + ih - cropBottom); this._dim.fill({ color: 0x000000, alpha: DIM_ALPHA }); } - // Left if (cx > ix) { this._dim.rect(ix, cy, cx - ix, ch); this._dim.fill({ color: 0x000000, alpha: DIM_ALPHA }); } - // Right const cropRight = cx + cw; if (cropRight < ix + iw) { this._dim.rect(cropRight, cy, ix + iw - cropRight, ch); @@ -201,11 +202,35 @@ export class CropOverlay extends Container { } } - // -- Handle drag -- + // -- Handle drag (single handler, registered dynamically) -- private _onDown(e: FederatedPointerEvent, id: HandleId): void { e.stopPropagation(); this._drag = { handleId: id, startCrop: { ...this._crop } }; + + // Register move/up on the stage (single handler, not per-handle) + this._removeDragListeners(); + this._boundMove = (ev: FederatedPointerEvent) => this._onMove(ev); + this._boundUp = () => this._onUp(); + const stage = this._viewport.parent; + if (stage) { + stage.on('globalpointermove', this._boundMove); + stage.on('pointerup', this._boundUp); + stage.on('pointerupoutside', this._boundUp); + } + } + + private _removeDragListeners(): void { + const stage = this._viewport.parent; + if (stage) { + if (this._boundMove) stage.off('globalpointermove', this._boundMove); + if (this._boundUp) { + stage.off('pointerup', this._boundUp); + stage.off('pointerupoutside', this._boundUp); + } + } + this._boundMove = null; + this._boundUp = null; } private _onMove(e: FederatedPointerEvent): void { @@ -216,7 +241,6 @@ export class CropOverlay extends Container { const ih = data.h * data.sy; const world = this._viewport.toWorld(e.global.x, e.global.y); - // Convert world position to normalized image coordinates const nx = (world.x - data.x) / iw; const ny = (world.y - data.y) / ih; @@ -266,5 +290,6 @@ export class CropOverlay extends Container { private _onUp(): void { this._drag = null; + this._removeDragListeners(); } } diff --git a/frontend/src/canvas/SceneManager.ts b/frontend/src/canvas/SceneManager.ts index 7cfdfb7..e9358b6 100644 --- a/frontend/src/canvas/SceneManager.ts +++ b/frontend/src/canvas/SceneManager.ts @@ -421,6 +421,10 @@ export class SceneManager { if (data.type === 'group' && obj instanceof FrameSprite) { obj.updateFromData(data as GroupObject); } + if (data.type === 'image' && obj instanceof ImageSprite) { + const imgData = data as ImageObject; + obj.applyCrop(imgData.crop); + } // Update stored data item.data = { ...data }; diff --git a/frontend/src/canvas/TextEditor.ts b/frontend/src/canvas/TextEditor.ts index f1674f2..509de34 100644 --- a/frontend/src/canvas/TextEditor.ts +++ b/frontend/src/canvas/TextEditor.ts @@ -24,6 +24,14 @@ export class TextEditor { return this._textarea !== null; } + /** Clear the current textarea content (for new text items). */ + clearText(): void { + if (this._textarea) { + this._textarea.value = ''; + this._textarea.dispatchEvent(new Event('input')); // trigger autoSize + } + } + /** * Open an inline textarea over the given text item. */ diff --git a/frontend/src/canvas/tools.ts b/frontend/src/canvas/tools.ts index 2be5519..c3a8e90 100644 --- a/frontend/src/canvas/tools.ts +++ b/frontend/src/canvas/tools.ts @@ -130,8 +130,7 @@ export function activateTool( ctx.onChange(); }); // Clear placeholder so user types from scratch - const ta = document.querySelector('textarea'); - if (ta) { ta.value = ''; } + ctx.textEditor!.clearText(); }, 50); } diff --git a/frontend/src/hooks/useCanvasSetup.ts b/frontend/src/hooks/useCanvasSetup.ts index 65ad298..4185a8c 100644 --- a/frontend/src/hooks/useCanvasSetup.ts +++ b/frontend/src/hooks/useCanvasSetup.ts @@ -132,15 +132,16 @@ export function useCanvasSetup(deps: CanvasSetupDeps) { // Double-click image: zoom-to-fit (PureRef-style focus) selection.onDoubleClickImage = (item) => { - const bounds = item.displayObject.getBounds(); const padding = 80; // screen pixels of padding around the image const screenW = viewport.screenWidth; const screenH = viewport.screenHeight; - const scaleX = (screenW - padding * 2) / bounds.width; - const scaleY = (screenH - padding * 2) / bounds.height; + const bw = item.data.w * Math.abs(item.data.sx); + const bh = item.data.h * Math.abs(item.data.sy); + const scaleX = (screenW - padding * 2) / bw; + const scaleY = (screenH - padding * 2) / bh; const targetScale = Math.min(scaleX, scaleY, 3); // cap at 3x - const cx = item.data.x + (item.data.w * item.data.sx) / 2; - const cy = item.data.y + (item.data.h * item.data.sy) / 2; + const cx = item.data.x + bw / 2; + const cy = item.data.y + bh / 2; viewport.animate({ time: 300, position: { x: cx, y: cy }, @@ -442,6 +443,7 @@ export function useCanvasSetup(deps: CanvasSetupDeps) { laserCleanupRef.current = null; dropCleanupRef.current?.(); pasteCleanupRef.current?.(); + textEditorRef.current?.stopEditing(false); if (cropOverlayRef.current) { cropOverlayRef.current.destroy(); cropOverlayRef.current = null; diff --git a/frontend/src/pages/Editor.tsx b/frontend/src/pages/Editor.tsx index 2c7b668..10aee48 100644 --- a/frontend/src/pages/Editor.tsx +++ b/frontend/src/pages/Editor.tsx @@ -700,20 +700,33 @@ export default function Editor({ isPublicView }: EditorProps) { fill={textToolbar.fill} position={textToolbar.items.length >= 2 ? 'below' : 'above'} onFontSizeChange={(size) => { + const scene = canvasRef.current?.getScene(); for (const item of textToolbar.items) { (item.data as TextObject).fontSize = size; const s = (item.displayObject as any)?.style; if (s) s.fontSize = size; + // Re-measure text bounds + const bounds = item.displayObject.getLocalBounds(); + item.data.w = bounds.width; + item.data.h = bounds.height; + if (scene) scene.updateSpatialEntry(item); } + selectionRef.current?.transformBox.update(textToolbar.items); onCanvasChange(textToolbar.items.map(i => i.id)); updateOverlays(); }} onFontFamilyChange={(family) => { + const scene = canvasRef.current?.getScene(); for (const item of textToolbar.items) { (item.data as TextObject).fontFamily = family; const s = (item.displayObject as any)?.style; if (s) s.fontFamily = family; + const bounds = item.displayObject.getLocalBounds(); + item.data.w = bounds.width; + item.data.h = bounds.height; + if (scene) scene.updateSpatialEntry(item); } + selectionRef.current?.transformBox.update(textToolbar.items); onCanvasChange(textToolbar.items.map(i => i.id)); updateOverlays(); }}