feat(studio): off-screen element indicators + unclipped overlay#1360
Conversation
9dbe446 to
da4a238
Compare
3a4d951 to
29c439e
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Verdict: 🚨 BLOCKER — off-screen indicator click handler can't find the elements it surfaces.
Blocker — broken selection for unidentified elements
useOffScreenIndicators.ts:181setselementId = el.id || basewherebasefalls back todata-hf-id || tagName.StudioPreviewArea.tsx:285click handler resolves viaiframe.contentDocument.getElementById(id)— butgetElementByIdonly matches theidattribute, NOTdata-hf-id.- Elements without an HTML
id(most authored HF elements, perhtmlParser.ts:204which preservesdata-hf-idas canonical) will show indicators that silently fail on click.
Fix: either select by [data-hf-id="..."] in the click handler (querySelector instead of getElementById), or restrict indicators to elements with a real id.
Other concerns
- Docstring says "edge-clamped" but code doesn't clamp —
useOffScreenIndicators.ts:1-4claims "edge-clamped indicator positions" but the hook pushes rawelLeft/elTop/elW/elH— no clamp to comp edges. The "indicators at edges" UX described in the PR body relies entirely on the NLELayoutoverflow-hiddenmove letting the off-screen rect render in the unclipped region. A very-far-off element won't render an edge indicator at all — it'll render somewhere far outside the outer container or be clipped by it. Either add the clamp, or update the docstring + PR body to match what's actually built. - RAF loop runs forever —
useOffScreenIndicators.ts:113-167walks every GSAP target, callsgetBoundingClientRect()per element, every frame, regardless of whether anything is animating or any element is actually off-screen. With many elements + studio panel re-renders, this is layout thrash on the 16ms budget. Suggest gating onselectionchange + transform-commit events, or throttling to e.g. 10Hz, or short-circuiting whencompRect.widthhasn't changed AND targets array hash hasn't changed. - Pointer-handler leak risk —
DomEditOverlay.tsx:560-572attachespointermove/pointerupviael.addEventListenerbut nopointercancellistener. Touch-cancel / contextmenu / scroll-while-dragging will leak listeners + leavesetPointerCaptureun-released + leaveel.style.transformmutated. Add apointercancelcleanup path mirroringonUp. - Outside-comp click guard changes deselection behavior —
DomEditOverlay.tsx:263-278early-returns on clicks outside comp bounds. Comment says "would clear the selection" — but the previous "click empty space to deselect" behavior is now broken in the unclipped overlay region. Worth a UX check: if you select an element then click in the new unclipped margin, does selection persist forever? May be intentional, but it's a behavior change beyond the documented fix.
Determinism
collectGsapTargetElements:36-58 reads iframe.contentWindow.__timelines — wrapped in try/catch for cross-origin, good. No render-time fetch(). HF determinism rule not violated.
Test coverage
No tests added — 196 LOC of coordinate math + RAF loop + DOM mutation with zero coverage. The coordinate-math path (elLeft = iframeRect.left - overlayRect.left + elRect.left * rootScaleX) is exactly the kind of thing that breaks silently on zoom/pan changes.
Cross-stack
Introduces new onSelectElementById prop on DomEditOverlay; depends on buildDomSelectionFromTarget from useDomSelection.ts:206 (already in stack). NLELayout overflow restructure is isolated to this PR.
— Review by Rames D Jusso
da4a238 to
b4906c9
Compare
f7b33b8 to
0dede9b
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R3 verification — blocker. R2 click-resolution blocker unchanged, plus a stack-internal drive-by flag flip.
🚨 StudioPreviewArea.tsx:289 / useOffScreenIndicators.ts:464 — getElementById vs data-hf-id mismatch unchanged. The click handler is still iframe?.contentDocument?.getElementById(id); elementId = el.id || base still falls back to data-hf-id/tagName. Authored HF elements with no id attribute will still render an indicator (keyed off data-hf-id) and silently fail to select on click. Fix: swap click handler to querySelector('[data-hf-id="..."], #...') with a discriminator, OR guard elementId so we only emit indicators for elements with a real id.
🚨 manualEditingAvailability.ts:93 — stack-internal drive-by flag flip. STUDIO_GSAP_DRAG_INTERCEPT_ENABLED default flips from false → true in this PR — and #1356 (one below in the stack) correctly ships false. Not in the PR body, not in scope of "off-screen element indicators." Either an accidental ride-along (the same accidental flip I caught on monolithic #1344 R2) or an intentional flip that needs body/commit-message coverage. Same flag, same shape, same stack. Please confirm intent.
❌ RAF loop still per-frame at useOffScreenIndicators.ts:387-480. requestAnimationFrame(update) runs unconditionally every frame, calling getBoundingClientRect on every GSAP target. No throttling, no early-out on no-iframe-change. Layout thrash risk on big compositions.
❌ No pointercancel listener at DomEditOverlay.tsx:225-260. onPointerDown adds pointermove/pointerup, no pointercancel. OS-cancelled gestures (touch interrupt, alt-tab mid-drag) leak listeners + leave el.style.transform stuck.
❌ Zero tests. 196 LOC of coordinate math (declared-vs-iframe scaling, overlay vs comp-rect transforms) shipped untested.
❌ PR body vs implementation. Body says "dotted indicators at composition edges"; what ships is border: 1.5px dashed; opacity: 0.5 at ind.left/ind.top — the element's actual off-screen position, clipped by the unclipped overlay. A translucent dashed ghost at real position, not an edge indicator. Update body or implementation.
Review by Rames D Jusso
7abbbb0 to
7e8f788
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Verification pass — R3 items checked against current HEAD (7e8f788).
1. getElementById vs data-hf-id — StudioPreviewArea.tsx:290
FIXED. useOffScreenIndicators.ts now has a guard at line 172:
// Only elements with a real id attribute can be selected via getElementById
if (!el.id) continue;All surfaced indicators have elementId = el.id (a real DOM id). The getElementById(id) call in StudioPreviewArea.tsx:290 correctly resolves them. Comment explains the contract.
2. STUDIO_GSAP_DRAG_INTERCEPT_ENABLED false→true flip — manualEditingAvailability.ts:93
INTENTIONAL and explained. PR body states: "Intentional flag flip: STUDIO_GSAP_DRAG_INTERCEPT_ENABLED set to true — this stack implements and tests the drag intercept system that was gated behind false in #1341." The explanation is in the PR body. Sufficient.
Both R3 items addressed. Stamp-ready (with the same non-blocking R3 concerns Rames noted: RAF throttling, pointercancel, zero tests).
— Vai (verification pass)
7e8f788 to
ef27848
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
LGTM — Via reviewed, stamp applied.
— Vai
ef27848 to
f82b1d7
Compare
The base branch was changed.
Gesture recording uses replace-with-keyframes mutation to replace existing position-group tween. Fix N1 sign inversion and N9 wheel startPointer with pointerElementOffset subtraction.
f82b1d7 to
7f93f44
Compare

Summary
useOffScreenIndicatorshook: RAF-driven detection of GSAP elements positioned outside composition boundsoverflow-hiddenclips iframe, outer container leaves overlay unclipped for handle interaction beyond composition boundsonSelectElementByIdprop to DomEditOverlay for programmatic selectionSTUDIO_GSAP_DRAG_INTERCEPT_ENABLEDset totrue— this stack implements and tests the drag intercept system that was gated behindfalsein fix(studio): disable preview gsap-element dragging behaviour by default #1341Review feedback addressed (from R2)
window.location.hashregex withfetchAnimationscallback threaded throughGsapDragCommitCallbacksidattribute — no silent click failuresdocument.bodywith smart overflow positioningTest plan
Stack: 7/7 — depends on #1359