Skip to content

fix(studio): align editor overlays with GSAP-transformed elements during playback#1077

Merged
miguel-heygen merged 2 commits into
mainfrom
fix/studio-overlay-position-during-gsap-playback
May 25, 2026
Merged

fix(studio): align editor overlays with GSAP-transformed elements during playback#1077
miguel-heygen merged 2 commits into
mainfrom
fix/studio-overlay-position-during-gsap-playback

Conversation

@miguel-heygen
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen commented May 25, 2026

Summary

Fix studio editor overlays (selection borders, element highlights) appearing at wrong positions when GSAP transforms are active on the composition root — e.g., during scroll animations (y: -500) or entrance animations (scale: 0.95).

Root Cause

toOverlayRect() in domEditOverlayGeometry.ts calculated overlay position as:

left: iframeRect.left - overlayRect.left + (elementRect.left - rootRect.left) * rootScaleX
top:  iframeRect.top  - overlayRect.top  + (elementRect.top  - rootRect.top)  * rootScaleY

The elementRect - rootRect subtraction cancels out the root's GSAP transform. Both elementRect and rootRect shift by the same amount when the root is transformed, so their difference always reflects the un-transformed position. The overlay stays at the element's original layout position, ignoring any GSAP animation.

Additionally, rootScaleX/Y was computed from rootRect.width/height (the transformed bounding rect), which changes when GSAP applies scale transforms — producing wrong scale factors.

Fix

Two changes:

  1. Remove rootRect subtraction — use elementRect.left/top directly. getBoundingClientRect() returns viewport-relative coordinates that already reflect GSAP transforms, which is exactly what the overlay needs.
- left: iframeRect.left - overlayRect.left + (elementRect.left - rootRect.left) * rootScaleX,
- top:  iframeRect.top  - overlayRect.top  + (elementRect.top  - rootRect.top)  * rootScaleY,
+ left: iframeRect.left - overlayRect.left + elementRect.left * rootScaleX,
+ top:  iframeRect.top  - overlayRect.top  + elementRect.top  * rootScaleY,
  1. Use data-width/data-height for scale — the composition's declared dimensions don't change with GSAP transforms, unlike rootRect.width/height.
+ const declaredWidth = readPositiveDimension(root?.getAttribute("data-width") ?? null);
+ const declaredHeight = readPositiveDimension(root?.getAttribute("data-height") ?? null);
+ const rootWidth = declaredWidth ?? rootRect?.width;
+ const rootHeight = declaredHeight ?? rootRect?.height;

Test plan

  • Select an element during GSAP scroll animation (y: -500) — overlay follows the element
  • Select an element during GSAP entrance (scale: 0.95 → 1.0) — overlay matches element size
  • Verify overlays work on compositions without GSAP transforms (normal case)
  • Verify drag/resize gestures still work correctly
  • Verify overlay positioning in nested sub-compositions

@miguel-heygen miguel-heygen changed the title fix(studio): use declared dimensions for overlay scale during GSAP playback fix(studio): align editor overlays with GSAP-transformed elements during playback May 25, 2026
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES — description doesn't match code

🚩 The "remove rootRect subtraction" change isn't actually in the diff

The PR body's Fix section claims:

- left: iframeRect.left - overlayRect.left + (elementRect.left - rootRect.left) * rootScaleX,
- top:  iframeRect.top  - overlayRect.top  + (elementRect.top  - rootRect.top)  * rootScaleY,
+ left: iframeRect.left - overlayRect.left + elementRect.left * rootScaleX,
+ top:  iframeRect.top  - overlayRect.top  + elementRect.top  * rootScaleY,

But the actual code at PR head (domEditOverlayGeometry.ts:122-123) still has:

return {
  left: iframeRect.left - overlayRect.left + (elementRect.left - rootRect.left) * rootScaleX,
  top: iframeRect.top - overlayRect.top + (elementRect.top - rootRect.top) * rootScaleY,
  ...
};

Only the data-width/height scale change (fix #2) landed. The rootRect-subtraction removal (fix #1) is missing.

This matters because fix #1 is the load-bearing one for the bug being claimed. With the subtraction still present:

  • GSAP applies y: -500 to root → rootRect.top shifts by -500 → elementRect.top shifts by -500 → (elementRect.top - rootRect.top) stays UN-shifted.
  • The data-width scale fix (#2) makes rootScaleY stable, but multiplying stable scale by un-shifted delta = un-shifted position. The overlay still doesn't follow GSAP transforms.

Either:

  1. Miguel forgot to commit the left/top formula change → push it
  2. The data-width fix alone DOES fix the bug for some reason I'm missing → the PR description should describe what actually shipped

Worth verifying by running the test plan item ("Select an element during GSAP scroll animation (y: -500)") on the current branch. The PR description says [x] for that test — would be surprising if it passes without fix #1.

🚩 CI red — file-size check

This PR includes hf#1076's playhead-clamp change (visible in the useTimelinePlayer.ts diff), but apparently without hf#1076's blank-line-trimming follow-up. useTimelinePlayer.ts is at 611 lines again, tripping the 600-line gate.

If hf#1076 lands first, this PR will need rebase + the blank-line trim follow-up. Or this PR should drop the duplicated useTimelinePlayer.ts change.

Sanity checks on what IS in the diff

The data-width/height substitution for scale (declaredWidth ?? rootRect?.width) is correctly shaped — falls back to rootRect when data-width is missing or invalid. ✓

if (!rootWidth || !rootHeight || !rootRect) return null; — the new !rootRect guard is necessary because rootRect is used below for the subtraction; correct addition. ✓

Summary

Two pre-merge asks:

  1. Push the missing left/top formula change OR explain why the data-width fix alone suffices
  2. Resolve the file-size CI failure (rebase off hf#1076 + trim, OR drop the duplicate useTimelinePlayer.ts change)

— Rames Jusso

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to @jrusso1020's findings — both blockers are confirmed. Adding two gaps and a rebase note not covered in the existing review.

Audited: packages/studio/src/components/editor/domEditOverlayGeometry.ts (read end-to-end), packages/studio/src/player/hooks/useTimelinePlayer.ts (read end-to-end), existing test files for both.


Strengths

  • domEditOverlayGeometry.ts:99–101 — using data-width/data-height as the canonical scale reference is exactly right; this dimension is stable across GSAP transforms unlike rootRect.width/height.
  • domEditOverlayGeometry.ts:104 — adding !rootRect to the null guard is a correct and necessary addition now that rootRect is read below even when declaredWidth/declaredHeight is present.

Blocker 1 (confirmed, adds angle) — position formula unchanged; fix #1 from the PR description was never committed

domEditOverlayGeometry.ts:121-122. As @jrusso1020 notes, the return {} block is byte-identical to main. I verified against both base and head SHAs directly.

To add to the analysis: even with the scale fix in place, a translate-only GSAP animation (the y: -500 case explicitly listed in the test plan) will still misplace overlays. The PR description's own root-cause analysis explains why: (elementRect.top - rootRect.top) cancels the transform. Scale-only GSAP (scale: 0.95) might happen to work if the root transforms from center and scale doesn't shift the viewport-relative top of the element — but that's incidental, not fixed. The two GSAP test plan items marked [ ] are almost certainly still failing.

Either commit the missing hunk or update the description + test plan to accurately describe what this PR actually does.

Blocker 2 (confirmed, adds detail) — useTimelinePlayer.ts change is a duplicate of the already-merged hf#1076

hf#1076 ("fix(studio): clamp playhead to composition duration in RAF loop") merged 2026-05-25 23:18 UTC — about 6 minutes before this PR's CI run. The two-line clamp change in this PR's diff is an exact duplicate of that commit. useTimelinePlayer.ts at main already has the clamping. Rebasing off main will drop this hunk automatically, and the 611-line file will come back under the 600-line gate.

Action: rebase off main (or merge main into this branch).

Important — no regression test for toOverlayRect under transforms

DomEditOverlay.test.ts mocks out toOverlayRect entirely (line 76 stubs its return). domEditOverlayGeometry.ts has no direct test file. There's no coverage proving the scale fix works for a root with data-width set, and no coverage that will catch a regression if the position formula is changed. A unit test for toOverlayRect with a mocked getBoundingClientRect simulating a scaled/translated root would cover both parts of this fix. At minimum, it would make the [x] test plan item verifiable without manual QA.

Nit — comment block on lines 95–99 is thorough but slightly over-explains

The inline comment (4 lines) already describes the problem well; the PR description has the same explanation. One sentence is enough at the call site.


Verdict: REQUEST CHANGES
Reasoning: The load-bearing position-formula fix described in the PR body is not in the diff. The scale fix alone does not address the translate case the PR explicitly claims to fix. Rebase off main and land the missing hunk.

— Vai

…ayback

When GSAP applies transforms (scale, translate) to the root composition
element during playback, rootRect.width/height from getBoundingClientRect()
changes to reflect the transformed size. The overlay scale calculation
(rootScaleX/Y = iframeRect / rootRect) then produces wrong values,
causing overlays to appear at incorrect positions during animated
playback — especially visible during scroll animations (y transform)
and entrance animations (scale transform).

Fix: use the composition's declared data-width/data-height attributes
for scale calculation. These are the canonical dimensions that don't
change with GSAP transforms. Falls back to rootRect dimensions when
the attributes aren't present (non-composition elements).
elementRect.left/top from getBoundingClientRect() already reflects GSAP
transforms in viewport coordinates. Subtracting rootRect.left/top
cancels the transform, pinning overlays to the un-animated layout
position. Use elementRect directly so overlays track elements during
scroll (y: -500) and entrance (scale: 0.95) animations.
@miguel-heygen miguel-heygen force-pushed the fix/studio-overlay-position-during-gsap-playback branch from 720c48d to 45e03ab Compare May 25, 2026 23:40
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified at 45e03ab9:

  • Position formula change committed ✓ — elementRect.left/top * rootScaleX/Y (no rootRect subtraction). The fix the PR description claimed is now actually in the code. With the data-width scale and the direct elementRect projection, GSAP y: -500 translates flow through getBoundingClientRect() into elementRect, are no longer cancelled by rootRect subtraction, and the overlay tracks the element.
  • Rebase ✓ — single-file diff now, no useTimelinePlayer.ts duplication.
  • CI ✓ — 10 success / 14 in_progress / 0 failures.

Re-approved.

— Rames Jusso

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both blockers addressed. LGTM.

Blocker 1 — Position formula fixed
rootRect.left/top subtraction removed at lines 122–123. getBoundingClientRect() returns viewport-absolute coords that already incorporate GSAP transforms, so the subtraction was double-canceling them. Clean.

Blocker 2 — Rebase clean
useTimelinePlayer.ts is entirely out of the diff. Only file touched is domEditOverlayGeometry.ts (219 lines, well under the 600-line threshold flagged earlier).

Bonus: declared-dimension fallback — nice fix; using data-width/data-height for scale instead of rootRect.width/height is the right call since GSAP transforms skew the rect dimensions while the composition's canonical size stays stable.

CI is still running; no failures at time of review. Ship it once green.

— Vai

@miguel-heygen miguel-heygen merged commit 66e90b7 into main May 25, 2026
35 checks passed
@miguel-heygen miguel-heygen deleted the fix/studio-overlay-position-during-gsap-playback branch May 25, 2026 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants