Skip to content

fix(photo-annotator): drop callout, unify line/arrow/measurement #1529

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
steilerDev merged 2 commits into from
May 19, 2026
Merged

fix(photo-annotator): drop callout, unify line/arrow/measurement #1529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
64fedeb
fix(photo-annotator): drop callout, unify line/arrow/measurement prim…
May 19, 2026
3c7d88f
test(photo-annotator): remove callout references from tests
May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 0 additions & 41 deletions client/src/components/photos/PhotoAnnotator/PhotoAnnotator.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,6 @@ describe('PhotoAnnotator', () => {
'tool-line',
'tool-ellipse',
'tool-text',
'tool-callout',
'tool-measurement',
'tool-freehand',
];
Expand Down Expand Up @@ -676,28 +675,6 @@ describe('PhotoAnnotator', () => {
expect(screen.getByRole('region', { name: /annotation tool/i })).toBeInTheDocument();
});

// ─── Callout tool ──────────────────────────────────────────────────────────
//
// Story #1476: Callout text tool with two-phase interaction.
// The Phase 1 → Phase 2 flow uses Konva Stage mouse events which are not
// simulatable in JSDOM. We verify that the tool can be selected without error.

it('callout tool button can be selected without errors', async () => {
await renderAnnotator({ width: 800, height: 600 });

const calloutBtn = screen.getByTestId('tool-callout');
expect(calloutBtn).toBeInTheDocument();
fireEvent.click(calloutBtn);
expect(calloutBtn).toHaveAttribute('aria-pressed', 'true');

// Component still rendered (no error from tool switch)
expect(screen.getByRole('region', { name: /annotation tool/i })).toBeInTheDocument();
});

it.todo(
'callout Phase 1→Phase 2 transition does not discard draft (E2E covers Konva pointer flow)',
);

// ── Canvas bake uses naturalWidth/naturalHeight (not photo.width/height) ────
//
// Fix: canvas dimensions now come from `img.naturalWidth` / `img.naturalHeight`
Expand Down Expand Up @@ -777,24 +754,6 @@ describe('PhotoAnnotator', () => {
expect(capturedCanvasHeight).toBe(1800);
});

// ── Callout text commitment regression (fix for: callout disappears after text entry) ──
//
// Bug fix: missing return statement in commitInlineInput() after empty text handling.
// The fix prevents fall-through. E2E tests fully exercise the callout text flow.
it('photoAnnotator renders and processes callout tool without errors', async () => {
await renderAnnotator({ width: 800, height: 600 });

// Verify the component renders
expect(screen.getByRole('region', { name: /annotation tool/i })).toBeInTheDocument();

// Verify we can switch to the callout tool without errors
fireEvent.click(screen.getByTestId('tool-callout'));
expect(screen.getByTestId('tool-callout')).toHaveAttribute('aria-pressed', 'true');

// Verify action buttons are still present
expect(screen.getByRole('button', { name: /^Cancel$/i })).toBeInTheDocument();
});

// ── Coordinate-transform tests ────────────────────────────────────────────
//
// The SVG-overlay coordinate transform tests (imgRef/svgRef sibling structure,
Expand Down
156 changes: 57 additions & 99 deletions client/src/components/photos/PhotoAnnotator/PhotoAnnotator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import {
import type {
AnnotationShape,
TextShape,
CalloutShape,
MeasurementShape,
FreehandShape,
} from './useUndoStack.js';
Expand Down Expand Up @@ -144,7 +143,7 @@ export function PhotoAnnotator({ photo, onSave, onCancel }: PhotoAnnotatorProps)
: null;
const originalText = (() => {
if (!existingShape) return '';
if (existingShape.type === 'text' || existingShape.type === 'callout')
if (existingShape.type === 'text')
return existingShape.text;
if (existingShape.type === 'measurement') return existingShape.label;
return '';
Expand Down Expand Up @@ -188,7 +187,7 @@ export function PhotoAnnotator({ photo, onSave, onCancel }: PhotoAnnotatorProps)

if (inlineInput.editingShapeId !== null) {
const shape = state.shapes.find((s) => s.id === inlineInput.editingShapeId);
if (shape && (shape.type === 'text' || shape.type === 'callout')) {
if (shape && shape.type === 'text') {
const updated = { ...shape, text };
undoStack.commit(state.shapes.map((s) => (s.id === updated.id ? updated : s)));
} else if (shape && shape.type === 'measurement') {
Expand All @@ -207,27 +206,6 @@ export function PhotoAnnotator({ photo, onSave, onCancel }: PhotoAnnotatorProps)
};
undoStack.commit([...undoStack.shapes, newShape]);
dispatch({ type: 'SELECT_SHAPE', id: newShape.id });
} else if (state.selectedTool === 'callout' && draftShape?.type === 'callout') {
const committed: CalloutShape = {
type: 'callout',
id: nanoid(),
x: Math.min(draftShape.startX, draftShape.endX),
y: Math.min(draftShape.startY, draftShape.endY),
w: Math.abs(draftShape.endX - draftShape.startX),
h: Math.abs(draftShape.endY - draftShape.startY),
text,
tailX: draftShape.endX,
tailY: draftShape.endY,
stroke: state.activeColor,
fill: state.activeColor,
fontSize,
color: state.activeColor,
strokeWidth: resolveStrokeWidth(state.activeStrokeWidthKey, photo.width!, photo.height!),
};
const newShapes = [...undoStack.shapes, committed];
setDraftShape(null);
undoStack.commit(newShapes);
dispatch({ type: 'SELECT_SHAPE', id: committed.id });
} else if (state.selectedTool === 'measurement' && draftShape?.type === 'measurement') {
const committed: MeasurementShape = {
type: 'measurement',
Expand Down Expand Up @@ -372,12 +350,6 @@ export function PhotoAnnotator({ photo, onSave, onCancel }: PhotoAnnotatorProps)
x: Math.max(0, Math.min(selectedShape.x + dx, photo.width!)),
y: Math.max(0, Math.min(selectedShape.y + dy, photo.height!)),
};
} else if (selectedShape.type === 'callout') {
updated = {
...selectedShape,
x: Math.max(0, Math.min(selectedShape.x + dx, photo.width! - selectedShape.w)),
y: Math.max(0, Math.min(selectedShape.y + dy, photo.height! - selectedShape.h)),
};
} else if (selectedShape.type === 'measurement') {
updated = {
...selectedShape,
Expand Down Expand Up @@ -502,21 +474,16 @@ export function PhotoAnnotator({ photo, onSave, onCancel }: PhotoAnnotatorProps)
} else if (state.selectedTool === 'text') {
// Text tool: click-to-place, no drag size requirement
openInlineInput(draftShape.startX, draftShape.startY);
} else if (state.selectedTool === 'callout') {
// Callout: requires minimum drag (rect with tail)
if (w > MIN_SIZE && h > MIN_SIZE) {
openInlineInput(draftShape.startX, draftShape.startY);
} else {
setDraftShape(null);
}
} else if (state.selectedTool === 'measurement') {
// Measurement: line-based, use Euclidean distance gate
const distance = Math.hypot(
draftShape.endX - draftShape.startX,
draftShape.endY - draftShape.startY,
);
if (distance > MIN_SIZE) {
openInlineInput(draftShape.startX, draftShape.startY);
const midX = (draftShape.startX + draftShape.endX) / 2;
const midY = (draftShape.startY + draftShape.endY) / 2;
openInlineInput(midX, midY);
} else {
setDraftShape(null);
}
Expand All @@ -532,7 +499,6 @@ export function PhotoAnnotator({ photo, onSave, onCancel }: PhotoAnnotatorProps)
line: t('shapeAddedLine'),
ellipse: t('shapeAddedEllipse'),
text: '',
callout: '',
measurement: '',
freehand: '',
select: '',
Expand Down Expand Up @@ -700,7 +666,7 @@ export function PhotoAnnotator({ photo, onSave, onCancel }: PhotoAnnotatorProps)
let editingShape = null;
if (inlineInput.editingShapeId) {
editingShape = state.shapes.find((s) => s.id === inlineInput.editingShapeId);
if (editingShape && (editingShape.type === 'text' || editingShape.type === 'callout')) {
if (editingShape && editingShape.type === 'text') {
textColor = editingShape.color;
}
}
Expand All @@ -713,25 +679,17 @@ export function PhotoAnnotator({ photo, onSave, onCancel }: PhotoAnnotatorProps)
let imgH = screenFontSizePx / scale;
let textAlign: 'left' | 'center' = 'left';

if (shapeType === 'callout' && draftShape?.type === 'callout') {
const inset = 6;
imgX = draftShape.startX + inset;
imgY = draftShape.startY + inset;
imgW = Math.max(1, Math.abs(draftShape.endX - draftShape.startX) - 2 * inset);
imgH = Math.max(1, Math.abs(draftShape.endY - draftShape.startY) - 2 * inset);
} else if (shapeType === 'measurement' && draftShape?.type === 'measurement') {
if (shapeType === 'measurement' && draftShape?.type === 'measurement') {
const fontSize = getActiveFontSizePx();
const dx = draftShape.endX - draftShape.startX;
const dy = draftShape.endY - draftShape.startY;
const len = Math.sqrt(dx * dx + dy * dy) || 1;
const len = Math.hypot(dx, dy) || 1;
const nx = -dy / len;
const ny = dx / len;
const midX = (draftShape.startX + draftShape.endX) / 2;
const midY = (draftShape.startY + draftShape.endY) / 2;
const labelOffsetX = -nx * fontSize * 0.6;
const labelOffsetY = -ny * fontSize * 0.6;
imgX = midX + labelOffsetX - fontSize * 2;
imgY = midY + labelOffsetY - fontSize * 0.5;
const labelOffsetX = nx * fontSize * 1.2;
const labelOffsetY = ny * fontSize * 1.2;
imgX = inlineInput.anchorImageX + labelOffsetX - fontSize * 2;
imgY = inlineInput.anchorImageY + labelOffsetY - fontSize * 0.5;
imgW = fontSize * 4;
imgH = fontSize;
textAlign = 'center';
Expand Down Expand Up @@ -812,7 +770,7 @@ export function PhotoAnnotator({ photo, onSave, onCancel }: PhotoAnnotatorProps)
const shape = state.shapes.find((s) => s.id === state.selectedShapeId);
if (shape) {
const updated =
shape.type === 'text' || shape.type === 'callout'
shape.type === 'text'
? { ...shape, color }
: { ...shape, stroke: color };
undoStack.commit(state.shapes.map((s) => (s.id === updated.id ? updated : s)));
Expand All @@ -837,7 +795,7 @@ export function PhotoAnnotator({ photo, onSave, onCancel }: PhotoAnnotatorProps)
const shape = state.shapes.find((s) => s.id === state.selectedShapeId);
if (
shape &&
(shape.type === 'text' || shape.type === 'callout' || shape.type === 'measurement')
(shape.type === 'text' || shape.type === 'measurement')
) {
const newFontSize = resolveFontSize(key as FontSizeKey, photo.width!, photo.height!);
const updated = { ...shape, fontSize: newFontSize };
Expand Down Expand Up @@ -1033,9 +991,11 @@ function renderKonvaShape(
points={[shape.x1, shape.y1, shape.x2, shape.y2]}
stroke={shape.stroke}
strokeWidth={shape.strokeWidth}
fill="none"
fill={shape.stroke}
draggable={selectedTool === 'select'}
onClick={() => onSelect(shape.id)}
pointerLength={Math.max(8, shape.strokeWidth * 3)}
pointerWidth={Math.max(8, shape.strokeWidth * 3)}
ref={(node) => {
if (node) {
shapesNodesRef.current.set(shape.id, node);
Expand Down Expand Up @@ -1125,45 +1085,16 @@ function renderKonvaShape(
);
}

if (shape.type === 'callout') {
return (
<Group
key={shape.id}
id={`shape-${shape.id}`}
draggable={selectedTool === 'select'}
ref={(node) => {
if (node) {
shapesNodesRef.current.set(shape.id, node);
}
}}
>
<Rect
x={shape.x}
y={shape.y}
width={shape.w}
height={shape.h}
stroke={shape.stroke}
fill={shape.fill}
fillOpacity={0.2}
/>
<Line
points={[shape.x, shape.y, shape.tailX, shape.tailY]}
stroke={shape.stroke}
strokeWidth={shape.strokeWidth}
/>
<Text
x={shape.x + 6}
y={shape.y + 6}
text={shape.text}
fontSize={shape.fontSize}
fill={shape.color}
fontFamily={ANNOTATION_FONT_FAMILY}
/>
</Group>
);
}

if (shape.type === 'measurement') {
const dx = shape.x2 - shape.x1;
const dy = shape.y2 - shape.y1;
const len = Math.hypot(dx, dy) || 1;
const nx = -dy / len;
const ny = dx / len;
const midX = (shape.x1 + shape.x2) / 2;
const midY = (shape.y1 + shape.y2) / 2;
const offset = shape.fontSize * 1.2;

return (
<Group
key={shape.id}
Expand All @@ -1175,18 +1106,25 @@ function renderKonvaShape(
}
}}
>
<Line
<Arrow
points={[shape.x1, shape.y1, shape.x2, shape.y2]}
stroke={shape.stroke}
strokeWidth={shape.strokeWidth}
fill={shape.stroke}
pointerAtBeginning
pointerAtEnding
pointerLength={Math.max(8, shape.strokeWidth * 3)}
pointerWidth={Math.max(8, shape.strokeWidth * 3)}
/>
<Text
x={(shape.x1 + shape.x2) / 2 - 20}
y={(shape.y1 + shape.y2) / 2 - shape.fontSize}
x={midX + nx * offset}
y={midY + ny * offset - shape.fontSize / 2}
text={shape.label}
fontSize={shape.fontSize}
fill={shape.color}
fontFamily={ANNOTATION_FONT_FAMILY}
align="center"
offsetX={shape.fontSize}
/>
</Group>
);
Expand Down Expand Up @@ -1271,7 +1209,9 @@ function renderDraftShape(draft: DraftShape, state: any): React.ReactNode {
points={[draft.startX, draft.startY, draft.endX, draft.endY]}
stroke={state.activeColor}
strokeWidth={1}
fill="none"
fill={state.activeColor}
pointerLength={8}
pointerWidth={8}
opacity={0.8}
listening={false}
/>
Expand All @@ -1291,6 +1231,24 @@ function renderDraftShape(draft: DraftShape, state: any): React.ReactNode {
);
}

if (draft.type === 'measurement') {
return (
<Arrow
key="draft"
points={[draft.startX, draft.startY, draft.endX, draft.endY]}
stroke={state.activeColor}
strokeWidth={1}
fill={state.activeColor}
pointerAtBeginning
pointerAtEnding
pointerLength={8}
pointerWidth={8}
opacity={0.8}
listening={false}
/>
);
}

if (draft.type === 'ellipse') {
return (
<Ellipse
Expand Down
Loading
Loading