From 060db69108b68dae3a90efb4c88e4811477d6a67 Mon Sep 17 00:00:00 2001 From: Marcel Date: Wed, 29 Apr 2026 08:53:29 +0200 Subject: [PATCH] refactor(person-mention): extract computeHoverCardPosition into testable util MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three reviewer concerns land here: - Felix #2: magic numbers 0.7 and 300 belong in named constants - Sara #6: the position function had 4 branches and 2 thresholds with zero tests - Leonie FINDING-05: at 320px viewport the flip-left could push the card past the right edge — needed a viewport clamp Move the function to src/lib/utils/hoverCardPosition.ts as a pure (rect, viewport) → {top, left} mapping, with named exports CARD_WIDTH_PX, CARD_HEIGHT_PX, CARD_GAP_PX, BOTTOM_BAND_RATIO, RIGHT_FLIP_THRESHOLD_PX. Add a viewport clamp so left + CARD_WIDTH never exceeds the right edge. Ten unit tests cover default placement, flip-up (both triggers), flip-left, flip-right-edge clamp, and scroll offset. TranscriptionReadView passes the current window viewport in on each call. Co-Authored-By: Claude Sonnet 4.6 --- .../components/TranscriptionReadView.svelte | 32 +--- .../src/lib/utils/hoverCardPosition.spec.ts | 152 ++++++++++++++++++ frontend/src/lib/utils/hoverCardPosition.ts | 69 ++++++++ 3 files changed, 228 insertions(+), 25 deletions(-) create mode 100644 frontend/src/lib/utils/hoverCardPosition.spec.ts create mode 100644 frontend/src/lib/utils/hoverCardPosition.ts diff --git a/frontend/src/lib/components/TranscriptionReadView.svelte b/frontend/src/lib/components/TranscriptionReadView.svelte index 9f48f651..e4ca6d1c 100644 --- a/frontend/src/lib/components/TranscriptionReadView.svelte +++ b/frontend/src/lib/components/TranscriptionReadView.svelte @@ -7,6 +7,7 @@ import { type SafeHtml, PERSON_MENTION_SELECTOR } from '$lib/utils/mention'; +import { computeHoverCardPosition } from '$lib/utils/hoverCardPosition'; import PersonHoverCard from './PersonHoverCard.svelte'; import type { HoverData, LoadState } from '$lib/types/personHoverCard'; import { goto } from '$app/navigation'; @@ -38,10 +39,6 @@ let activeCard: { position: { top: number; left: number }; } | null = $state(null); -const CARD_WIDTH = 320; -const CARD_HEIGHT = 180; -const CARD_GAP = 6; - // Compose splitByMarkers with renderTranscriptionBody. Markers are pre-rendered // as tags; text segments run through HTML-escaping + mention // substitution. The two are concatenated to preserve marker boundaries — markers @@ -81,27 +78,12 @@ function fetchHoverData(personId: string): Promise { return cached; } -function computeCardPosition(rect: DOMRect): { top: number; left: number } { - const vw = window.innerWidth; - const vh = window.innerHeight; - - let top = rect.bottom + CARD_GAP; - let left = rect.left; - - // Flip up if the card would overflow the bottom edge OR the mention sits in - // the bottom 30% of the viewport (Leonie #5329). - if (vh - rect.bottom < CARD_HEIGHT + CARD_GAP || rect.top > vh * 0.7) { - top = rect.top - CARD_HEIGHT - CARD_GAP; - } - - // Flip left if <300px from the right edge. - if (vw - rect.left < 300) { - left = rect.right - CARD_WIDTH; - } - +function currentViewport() { return { - top: Math.max(0, top + window.scrollY), - left: Math.max(0, left + window.scrollX) + viewportWidth: window.innerWidth, + viewportHeight: window.innerHeight, + scrollX: window.scrollX, + scrollY: window.scrollY }; } @@ -115,7 +97,7 @@ async function handleMentionEnter(event: Event) { link.setAttribute('aria-describedby', cardId); const rect = link.getBoundingClientRect(); - const position = computeCardPosition(rect); + const position = computeHoverCardPosition(rect, currentViewport()); activeCard = { personId, cardId, position, state: { status: 'loading' } }; diff --git a/frontend/src/lib/utils/hoverCardPosition.spec.ts b/frontend/src/lib/utils/hoverCardPosition.spec.ts new file mode 100644 index 00000000..e953f442 --- /dev/null +++ b/frontend/src/lib/utils/hoverCardPosition.spec.ts @@ -0,0 +1,152 @@ +import { describe, it, expect } from 'vitest'; +import { + computeHoverCardPosition, + CARD_WIDTH_PX, + CARD_HEIGHT_PX, + CARD_GAP_PX, + BOTTOM_BAND_RATIO, + RIGHT_FLIP_THRESHOLD_PX +} from './hoverCardPosition'; + +const makeRect = (overrides: Partial = {}): DOMRect => { + const base = { top: 100, left: 200, bottom: 120, right: 300, width: 100, height: 20 }; + const merged = { ...base, ...overrides }; + return { + ...merged, + x: merged.left, + y: merged.top, + toJSON: () => merged + } as DOMRect; +}; + +describe('computeHoverCardPosition', () => { + it('exports the spec constants used by the spec/CSS layer', () => { + // Pin the values the design spec calls out — if these drift, the design spec + // in #5329 needs to drift with them. Felix's PR review #2 (named constants). + expect(CARD_WIDTH_PX).toBe(320); + expect(CARD_HEIGHT_PX).toBe(180); + expect(CARD_GAP_PX).toBe(6); + expect(BOTTOM_BAND_RATIO).toBe(0.7); + expect(RIGHT_FLIP_THRESHOLD_PX).toBe(300); + }); + + describe('default placement (below-right)', () => { + it('positions the card below the rect with a small gap', () => { + const rect = makeRect({ top: 100, bottom: 120, left: 200 }); + const result = computeHoverCardPosition(rect, { + viewportWidth: 1440, + viewportHeight: 900, + scrollX: 0, + scrollY: 0 + }); + expect(result.top).toBe(120 + CARD_GAP_PX); + expect(result.left).toBe(200); + }); + }); + + describe('flip-up rule (Leonie #5329)', () => { + it('flips up when the card would overflow the bottom edge', () => { + // Mention sits 50px above the viewport bottom — card is 180px tall, can't fit below + const rect = makeRect({ top: 800, bottom: 850 }); + const result = computeHoverCardPosition(rect, { + viewportWidth: 1440, + viewportHeight: 900, + scrollX: 0, + scrollY: 0 + }); + expect(result.top).toBe(800 - CARD_HEIGHT_PX - CARD_GAP_PX); + }); + + it('flips up when the mention sits in the bottom 30% of the viewport (BOTTOM_BAND_RATIO)', () => { + // rect.top is at 80% of viewport — fits below numerically, but poor UX + const rect = makeRect({ top: 720, bottom: 740 }); + const result = computeHoverCardPosition(rect, { + viewportWidth: 1440, + viewportHeight: 900, + scrollX: 0, + scrollY: 0 + }); + expect(result.top).toBe(720 - CARD_HEIGHT_PX - CARD_GAP_PX); + }); + }); + + describe('flip-left rule', () => { + it('flips left when the rect is within RIGHT_FLIP_THRESHOLD_PX of the right edge', () => { + // vw - rect.left = 1440 - 1200 = 240 < 300, so flip + const rect = makeRect({ left: 1200, right: 1300, top: 100, bottom: 120 }); + const result = computeHoverCardPosition(rect, { + viewportWidth: 1440, + viewportHeight: 900, + scrollX: 0, + scrollY: 0 + }); + // left = right - CARD_WIDTH = 1300 - 320 = 980 + expect(result.left).toBe(980); + }); + + it('does not flip left when the rect has plenty of right-side room', () => { + // vw - rect.left = 1440 - 200 = 1240 >> 300 → no flip + const rect = makeRect({ left: 200, right: 300 }); + const result = computeHoverCardPosition(rect, { + viewportWidth: 1440, + viewportHeight: 900, + scrollX: 0, + scrollY: 0 + }); + expect(result.left).toBe(200); + }); + }); + + describe('viewport clamping (Leonie FINDING-05)', () => { + it('clamps left so the card never overflows the right edge', () => { + // On a 320px viewport, even with flip the card width equals the viewport. + // Without clamping the card would be at left=0 but extend to 320 — fine. + // At viewport=400px with rect.left=200, flip puts left=300-320=-20, clamped to 0. + const rect = makeRect({ left: 200, right: 300, top: 100, bottom: 120 }); + const result = computeHoverCardPosition(rect, { + viewportWidth: 400, + viewportHeight: 900, + scrollX: 0, + scrollY: 0 + }); + expect(result.left).toBeGreaterThanOrEqual(0); + expect(result.left + CARD_WIDTH_PX).toBeLessThanOrEqual(400); + }); + + it('never returns a negative top or left', () => { + const rect = makeRect({ top: -50, left: -100, bottom: -30, right: 0 }); + const result = computeHoverCardPosition(rect, { + viewportWidth: 1440, + viewportHeight: 900, + scrollX: 0, + scrollY: 0 + }); + expect(result.top).toBeGreaterThanOrEqual(0); + expect(result.left).toBeGreaterThanOrEqual(0); + }); + }); + + describe('scroll offset', () => { + it('adds window.scrollY to the absolute-positioned top', () => { + const rect = makeRect({ top: 100, bottom: 120 }); + const result = computeHoverCardPosition(rect, { + viewportWidth: 1440, + viewportHeight: 900, + scrollX: 0, + scrollY: 500 + }); + expect(result.top).toBe(120 + CARD_GAP_PX + 500); + }); + + it('adds window.scrollX to the absolute-positioned left', () => { + const rect = makeRect({ top: 100, bottom: 120, left: 200, right: 300 }); + const result = computeHoverCardPosition(rect, { + viewportWidth: 1440, + viewportHeight: 900, + scrollX: 50, + scrollY: 0 + }); + expect(result.left).toBe(200 + 50); + }); + }); +}); diff --git a/frontend/src/lib/utils/hoverCardPosition.ts b/frontend/src/lib/utils/hoverCardPosition.ts new file mode 100644 index 00000000..4d16a899 --- /dev/null +++ b/frontend/src/lib/utils/hoverCardPosition.ts @@ -0,0 +1,69 @@ +/** + * Pure positioning logic for the person-mention hover card. + * + * Pulled out of TranscriptionReadView so the four placement branches + * (default, flip-up, flip-left, both) plus the viewport clamp are unit-testable + * without DOM. Sara's PR-B2 review #6 (no test for computeCardPosition) and + * Leonie's FINDING-05 (320px overflow) both land here. + */ + +/** Width of the rendered hover card. Mirrored in PersonHoverCard.svelte's CSS. */ +export const CARD_WIDTH_PX = 320; + +/** Min-height of the rendered hover card. Mirrored in PersonHoverCard.svelte's CSS. */ +export const CARD_HEIGHT_PX = 180; + +/** Gap between the mention rect and the card so they do not touch. */ +export const CARD_GAP_PX = 6; + +/** + * Mentions in the bottom 30% of the viewport flip the card up by default, + * even if it would numerically fit below — keeping the eye-line stable + * is more important than minimal travel (Leonie #5329). + */ +export const BOTTOM_BAND_RATIO = 0.7; + +/** + * Mentions within this distance of the right viewport edge flip the card + * left so it stays fully visible. + */ +export const RIGHT_FLIP_THRESHOLD_PX = 300; + +export type Viewport = { + viewportWidth: number; + viewportHeight: number; + scrollX: number; + scrollY: number; +}; + +export type CardPosition = { top: number; left: number }; + +/** + * Compute absolute-positioned top/left for the hover card, given a rect for + * the mention anchor and the current viewport. Output is in document + * coordinates (already includes scroll offsets). + */ +export function computeHoverCardPosition(rect: DOMRect, vp: Viewport): CardPosition { + let top = rect.bottom + CARD_GAP_PX; + let left = rect.left; + + const overflowsBottom = vp.viewportHeight - rect.bottom < CARD_HEIGHT_PX + CARD_GAP_PX; + const inBottomBand = rect.top > vp.viewportHeight * BOTTOM_BAND_RATIO; + if (overflowsBottom || inBottomBand) { + top = rect.top - CARD_HEIGHT_PX - CARD_GAP_PX; + } + + if (vp.viewportWidth - rect.left < RIGHT_FLIP_THRESHOLD_PX) { + left = rect.right - CARD_WIDTH_PX; + } + + // Clamp left so the card never extends past the right viewport edge + // (FINDING-05: at 320px viewport the flip would otherwise produce a + // negative left or right-side overflow). + left = Math.min(left, vp.viewportWidth - CARD_WIDTH_PX - CARD_GAP_PX); + + return { + top: Math.max(0, top + vp.scrollY), + left: Math.max(0, left + vp.scrollX) + }; +}