fix(core): apply position edits to SVG elements, not just HTML#2724
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
R1: APPROVE — clean, focused fix. Widening the element predicate from HTMLElement to HTMLElement | SVGElement is strictly additive across all four threading sites (applyPositionEditToElement, readCurrentTranslate, primeGsapTransformCache, lastAppliedTranslate WeakMap key), and isStylable is broadened symmetrically to admit RealmSVGElement alongside RealmHTMLElement from the document's own realm. Cross-realm safety preserved; .style.setProperty duck-type fallback still catches both.
Correctness spot-checks
- Every DOM API used on the widened type —
.style,getAttribute/hasAttribute/setAttribute/removeAttribute,.ownerDocument.defaultView,getComputedStyle— is inherited fromElementand works onSVGElement. ✓ - Modern Chrome honors the independent CSS
translatelonghand on SVG elements (px = user units, composes with existing SVGtransform) — same rationale as the HTML path, PR body cites correctly. ✓ gsap.getProperty(el, "x")accepts SVG targets (GSAP 3.x has SVG-native handling); priming is best-effort and the try/catch swallows any provider-specific issue anyway. ✓- Widening a parameter type is strictly compatible with all prior callers (SDK's
iframe.ts, runtimeinit.ts, studiomanualEdits.ts/useDomEditPreviewSync.ts/ etc.) — every existing HTMLElement call continues typing. ✓
Regression test
The new SVG <text> test in positionEdits.test.ts is the right shape — creates via createElementNS, sets data-x/data-y + baseline, asserts both count-returned-1 AND translate === "145px 1px". That's the minimum bar for a "silently dropped → now applies" fix.
Minor nit (non-blocking, optional follow-up)
The existing cross-realm regression test only covers HTML (an iframe-hosted <div>). The new RealmSVGElement !== undefined && el instanceof RealmSVGElement branch is exercised only same-realm. Symmetry with the HTML path gives strong prior that cross-realm SVG works too, but a parallel iframe-hosted SVG test (create <text> inside iframeDoc.body, verify applyPositionEdits(iframeDoc) returns 1 + writes the translate) would close the completeness gap. Truly optional — not a blocker.
— Via
86fa188 to
10220b9
Compare
What
Position edits (the SDK
moveElementconvention —data-x/data-y+ capturedbaseline) now apply to SVG elements (authored
<text>, shapes, groups),not only HTML. Previously the runtime silently dropped them on SVG.
Why
applyPositionEditsinruntime/positionEdits.tsgated each candidate withel instanceof HTMLElement, soSVGElementnodes were skipped in both theapply and orphan-reset loops — a committed move on an SVG element was never
realized. This surfaced in downstream editors (e.g. HeyGen AI Studio) where a
user moves an inner SVG
<text>label: the move persisted in the model butproduced no visual offset. It was also missing in the rendered video, since the
render injects the generated
position-edits-render-inlinebundle that is builtfrom this same module.
How
runtime/positionEdits.ts:type StylableElement = HTMLElement | SVGElementand broadenedapplyPositionEditToElement,readCurrentTranslate,primeGsapTransformCache, and thelastAppliedTranslatemap to it — everyDOM API used (
.style, attribute get/set,ownerDocument) exists on both.isStylablenow admitsSVGElementresolved from the document's own realm(
doc.defaultView.SVGElement), preserving the existing cross-realm safetyand the
.style.setPropertyduck-type fallback.translatelonghand as HTML: it composesadditively with GSAP's
transformand survives seeks (same rationale as theHTML path), and Chrome honors
translateon SVG (px = user units).src/generated/positio (bun run build:position-edits-render) so the render-time script carries the fix andcheck:position-edits-rendAdditive only — no behavior change f
Test plan
How was this tested?
(a marked SVG element is counted and receives the composedtranslate); full suite green (23/23).oxllean.composition preview.