Skip to content

fix(studio): a rotate without GSAP saves where you let go, nested too - #4802

Merged
miguel-heygen merged 9 commits into
mainfrom
fix/studio-rotate-none-drop
Oct 1, 2026
Merged

miguel-heygen merged 9 commits into
mainfrom
fix/studio-rotate-none-drop

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

What

Rotating an element that GSAP does not turn now saves where you let go, nested compositions included:

  1. One decision, at press. gsapWritesRotation(el) sits beside the move's gsapWritesPosition(el): GSAP owns the rotate when it renders the element's transform or a tween or hold writes a rotation channel (rotation, rotate and their X/Y/Z forms). Everything else is a plain rotate, decided once when the gesture starts, before anything reads GSAP (a gsap.getProperty call would bake the CSS into GSAP's transform and flip the decision mid-gesture).
  2. The turn starts from the angle the element shows. A plain rotate's base folds the CSS rotate, scale and transform the way GSAP would parse them, so an element authored with rotate: 30deg no longer saves at the dragged amount alone (25 deg instead of 55). Ported from the approved rotation-preview change, which stays parked; only its CSS-rotation reader comes along.
  3. The save is plain CSS. savePlainRotation writes the element's own inline rotate, the same value the draft draws: the angle less what its scale and transform already turn. No script, no Studio rotation marks, no preview reload, no animation fetch. It is reached the way the move's on-element writer is, through handleDomRotationCommit passed into useGsapAwareEditing, in the Studio session and in the host wiring. A read-only preview writes nothing. An element still carrying the legacy Studio rotation marks has them removed in the same write, so the seek re-apply cannot put the old angle back.
  4. A transform-centred element turns in place. The CSS rotate property applies before transform, so a turn written there also turns a transform: translate(-50%, -50%) and the box swings around its old centre (62 px on the bench's centred box). When the element's transform translates it, the turn goes into that transform right after its leading translate (translate(-50%, -50%) rotate(25deg) scaleX(-1)), so it applies in screen space about the centre: a mirrored or stretched element still turns with the cursor and does not shear. A mirroring scale property flips the turn's sign. A second rotate replaces its own rotate(). The authored transform comes from the inline style, else the last matching stylesheet rule (specificity, !important and @media are not weighed, so a later but less specific rule can win; noted in the code, left for a follow-up). Before this PR, on a page that loads GSAP such an element rotated through GSAP and stayed put, so the plain writer must not regress it.
  5. Inline elements and the panel. An inline element becomes inline-block in the same write, as the old writer did, since an inline box does not transform. The overlay hands the rotate target it read at press to the commit, so the save draws and patches exactly what the draft did; re-reading at save time saw the inline-block the draft had already set and dropped it from the write, so the word lost its turn on reload. The GSAP-aware commit routes on that same press-time decision. The property panel shows a plain element's angle from its CSS (it read 0 deg after a plain rotate).
  6. No per-frame style read. The scale/transform share is read once at press; the draft only sets rotate.

Deleted: the legacy CSS-variable rotation writer and its patch builder (buildRotationPatches).

Stacked on #4799 (its three commits are included and drop out when it merges).

Why nested rotates were lost

A rotate on a non-GSAP element went through the GSAP route and wrote a gsap.set into the file. In a sub-composition that script lands after the </template> and never runs, so the element snapped back on release (all 18 nested rotate rows, 62 px). The CSS write lands on the element inside the template.

Score (edit accuracy bench, --grid full --filter '^rotate-none-', 36 cases, same machine)

all but smoothness tracking press drop reload render undo
main (before) 9/36 36 36 9 (worst 74.8 px) 36 36 36
this branch (after) 35/36 36 36 36 (worst 0.00 px) 35 36 35

The one miss is a nested case whose undo did not land within the bench's 15 s window (intermittent: 0 to 2 nested cases per run, only in the slowest cases); it is the nested undo path, not this writer. Transform-centred rotate rows (the bench's transform placement from #4801, 4 cases, same machine): before this change's centring step 0/4, worst swing while dragging 62.4 px; after 4/4, 0.0 px.

Smoothness is out of scope here (another track owns per-frame work); the after run shared the machine at about twice the load of the before run.

Tests

  • plainRotation.test.ts: saves only rotate, less what the transform turns, with no Studio marks; a legacy-marked element keeps the new angle through a seek re-apply; a failed save puts the live rotate back; a read-only preview writes nothing.
  • useGsapAwareEditing.test.tsx: a plain element goes to the CSS writer with no GSAP write; an element a GSAP tween turns stays on the GSAP route.
  • domEditOverlayStartGesture.test.ts: a rotate press on a page that loads GSAP never asks GSAP about an element it does not turn, and the draft draws a CSS rotate.
  • manualOffsetDrag.test.ts: the base starts from the authored CSS rotation; the draft leaves the transform's and scale's share alone; with GSAP loaded and the element not animated, nothing calls gsap.getProperty or gsap.set, and the draft does no computed-style read.
  • rotateInlineSave.test.ts: the real press, draft, hold and save order on a span made inline by a stylesheet saves display: inline-block with the turn; it fails if the save re-reads the target or the overlay drops it.
  • rotationDraft.test.ts and plainRotation.test.ts: a transform-centred element turns inside its transform after the translate; mirrored (scaleX(-1)), stretched (scale(2, 1)) and scale: -1 1 centred elements turn with the cursor, unsheared, translate kept and a second turn replaces the first; the save patches the transform, and a failed save puts it back; an inline element is saved inline-block and restored on a failed save; the panel readout shows the CSS turn, and the legacy reading once GSAP turns the element.
  • domEditOverlayStartGesture.test.ts: a press on an element a GSAP tween turns (rotation, rotate, rotateZ) reads its base from GSAP.
  • Each was run against a broken copy and fails: writer ignoring the transform, legacy marks kept, routing line removed, routing always plain, plain ignored in the draft, share re-read per frame, base back to 0 without GSAP, read-only ignored, press asking GSAP, centring never taken, trailing turn appended, transform not restored.

Before

Nested, released after a 25 deg turn: the element is back at 0 deg.

before

An inline word, turned 25 deg, after a reload: the save wrote rotate without inline-block, so the word is flat again.

before inline

After

The same case keeps its 25 deg turn.

after

Transform-centred, before: the box swings off its centre.

before centred

After: it turns in place.

after centred

The inline word after a reload keeps its turn: the save wrote display: inline-block; rotate: 24.997deg.

after inline

@miguel-heygen
miguel-heygen force-pushed the fix/studio-rotate-none-drop branch 3 times, most recently from 0b0b814 to 7792b8b Compare October 1, 2026 00:15
@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Edit accuracy: 557 passing here, 530 on the base branch

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Newly passing (27)

  • rotate-none-center-r30-root-z50
  • rotate-none-pct-r30-nested-z100
  • rotate-none-px-r30-root-z50
  • rotate-none-center-r0-nested-z50
  • rotate-none-center-r30-nested-z200
  • rotate-none-pct-r30-root-z100
  • rotate-none-px-r0-nested-z50
  • rotate-none-px-r30-nested-z200
  • rotate-none-center-r30-root-z200
  • rotate-none-pct-r0-nested-z100
  • rotate-none-px-r30-root-z200
  • rotate-none-center-r0-nested-z200
  • rotate-none-pct-r30-nested-z50
  • rotate-none-px-r0-nested-z200
  • rotate-none-center-r30-nested-z100
  • rotate-none-pct-r30-root-z50
  • rotate-none-px-r30-nested-z100
  • rotate-none-center-r30-root-z100
  • rotate-none-pct-r0-nested-z50
  • rotate-none-pct-r30-nested-z200
  • rotate-none-px-r30-root-z100
  • rotate-none-center-r0-nested-z100
  • rotate-none-pct-r30-root-z200
  • rotate-none-px-r0-nested-z100
  • rotate-none-center-r30-nested-z50
  • rotate-none-pct-r0-nested-z200
  • rotate-none-px-r30-nested-z50

@miguel-heygen
miguel-heygen force-pushed the fix/studio-rotate-none-drop branch from 7792b8b to 75e080f Compare October 1, 2026 01:19
@miguel-heygen
miguel-heygen force-pushed the fix/studio-rotate-none-drop branch 5 times, most recently from 7e28bbb to 9dedaee Compare October 1, 2026 03:27
@miguel-heygen
miguel-heygen marked this pull request as ready for review October 1, 2026 04:22

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed at 5cb66d2e. Two independent passes (a Codex pass on the raw PR, plus my own), reconciled at source.

Verdict: changes requested. There is one blocker, and it's a small fix. The core design holds: the plain-CSS route, the leading-translate centring math, and the mirror sign all check out, and the gate is real. However, the inline-element claim fails in the real gesture order, so an inline element's rotation is lost on reload and in render. That's the snap-back class this PR exists to fix. I'm holding with CR rather than COMMENT because a single approval auto-merges.

Blocker

B1. An inline element's turn is saved without display: inline-block, so it snaps back after reload. (plainRotation.ts:25, rotationDraft.ts:59 / :99)

The cause is that savePlainRotation re-derives the target at commit time, after the draft has already mutated the element:

  1. At press, readCssRotationTarget sees display: inline, so inline: true.
  2. Each draft frame, and the pointerup "hold the final angle" (useDomEditOverlayGestures.ts:432), calls applyCssRotation(…, g.plainRotation), which sets display: inline-block on the live element.
  3. The commit then calls savePlainRotation → applyCssRotation(element, next.angle) with a fresh readCssRotationTarget. Computed display is now inline-block, so inline: false and no display patch is written.

I reproduced this in happy-dom by running the real order (press → draft → hold → savePlainRotation) on an element whose display: inline comes from a stylesheet. The press target had inline: true, but the commit received only [{ property: "rotate", value: "20deg" }]. The live DOM ends as inline-block + rotate: 20deg, so it looks right until reload. After reload the span is inline again, and a non-replaced inline box ignores rotate. Codex found the same thing independently.

Inline elements can reach this path: canApplyManualRotation is true for any internal source layer (core/src/editing/affordances.ts), so a plain <span> in a heading qualifies.

The test makes an inline element inline-block… passes only because it calls savePlainRotation on an element no draft has touched. No test runs the press → draft → save order, and the bench has no inline cases.

Fix: carry the press-time decision through to the save instead of re-deriving it. Pass g.plainRotation along with the commit (or have the save take the CssRotationTarget). Then add a test that runs the real order. This also fixes S1 below, which has the same root cause.

Should-fix (non-blocking)

  • S1. The route isn't decided once at press. startGesture decides plain at domEditOverlayStartGesture.ts:235, but handleGsapAwareRotationCommit re-runs gsapWritesRotation() at useGsapAwareEditing.ts:463. If anything initializes _gsap.renderTransform mid-gesture, the draft is CSS but the commit goes through GSAP. The fix for B1 covers this.
  • S2. The property panel can flip an animated element to the GSAP route before the press. This is a pre-existing gap, and #4799's move has it too. For any element with any GSAP animation (an opacity fade is enough), readGsapRuntimeValuesForPanel (propertyPanelHelpers.ts:486→:501) calls gsap.getProperty(el, "rotation"). In GSAP 3.15, _parseTransform sets cache.renderTransform (CSSPlugin.js:1021) and bakes the CSS rotate into the inline transform (CSSPlugin.js:859–865). After that, gsapWritesRotation() returns true and the rotate takes the old GSAP route. This isn't a regression against main, which always went through GSAP. But it means the nested-comp fix doesn't reach animated-but-not-rotated elements while the panel is showing them, and the bench only covers un-animated (rotate-none) elements. I'd track it as a follow-up for move and rotate together.
  • S3. The transform path only handles leading translate*(). scaleX(-1) translate(120px, 80px) has lead = 0, so the turn is prepended and rotates the translation, and the box swings. Separately, a non-uniform scale: longhand (scale: 2 1 plus a centring transform) puts the turn inside the transform but under the stretch. That shears the box, and the drawn angle isn't the requested one. The leading-translate cases, the scaleX(-1)/scale(2,1)-in-transform cases and scale: -1 1 are correct, as the tests show.

Nits

  • Draft, save and restore drop inline !important on transform/rotate (rotationDraft.ts:102, and the snapshot stores values only). The temporary share read does preserve priority.
  • lastRuleTransform also misses rules nested in @layer/@supports, not just @media. A centring transform declared only there falls back to the rotate property and swings. The PR already notes the related limitation.
  • The banked baseline.json has all 36 rotate-none-* cases passing. The body reports 35/36, with one intermittent nested-undo miss. The 2-of-3 re-run absorbs it, but that case is banked green while it flakes.

Verified

  • Gate: ratchet.accurate() over the base and head baseline.json gives 494 → 521. All 27 newly passing cases are rotate-none-*, with 0 regressions and none missing. CI's Studio: edit accuracy gate ran all 8 shards and reports the same.
  • CI: all required checks are green at head.
  • Tests: the 7 touched studio test files pass locally with NODE_ENV=test (123 tests).
  • Mirror math: probed translate(-120px, -80px) scaleX(-1). The share is 180, the save writes translate(...) rotate(-155deg) scaleX(-1), and the matrix angle comes out at the requested 25°.
  • GSAP folding: GSAP 3.15 folds the CSS rotate/scale/translate into its own transform on first parse, so a plain CSS turn survives a later tween on another channel.
  • Union with #4805: the two PRs textually conflict in useDomGeometryCommit.ts (host wiring) and in baseline.json. Whichever lands second must keep both handleDomRotationCommit and #4805's handleDomBoxSizeCommit and re-bank the baseline. A dropped param would fail typecheck, since it's required in UseGsapAwareEditingParams. I found no semantic conflict between them.
  • Not verified: I didn't re-run the bench locally or check the transform-centred 62 px → 0 numbers myself. Those rows come from #4801's placement, which isn't in the banked baseline, so for them I'm relying on the PR's numbers and the unit tests.

To lift the CR: fix B1 (carry the press-time target through to the save) and add a press → draft → save test for an inline element. Then re-pin and I'll re-check.

— Somu

@miguel-heygen
miguel-heygen force-pushed the fix/studio-rotate-none-drop branch 3 times, most recently from a2ee9d6 to cb75cc3 Compare October 1, 2026 08:00
@somanshreddy
somanshreddy dismissed their stale review October 1, 2026 09:09

Blocker (inline rotate loses inline-block on save) verified fixed at 38f9198: the press-time target now reaches the save, the legacy-mark path re-reads after clearing, and both are pinned by tests that fail when the fix is reverted. — Somu

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review at 38f9198e (after my CHANGES_REQUESTED at 5cb66d2e, now dismissed).

B1 (inline rotate drops display: inline-block on save): fixed.

  • cb75cc37: the overlay hands the press-time CssRotationTarget to the commit (RotationCommit.plain), so savePlainRotation draws and patches what the draft drew instead of re-reading an element the draft already made inline-block. handleGsapAwareRotationCommit routes on next.plain first, so the CSS-vs-GSAP decision is no longer re-made at commit for a drag (Codex's point from round 1).
  • 38f9198e: on an element carrying legacy Studio rotation marks, clearing them restores the original display (via STUDIO_ORIGINAL_TRANSFORM_DISPLAY_ATTR) and drops the legacy turn, so the save re-reads the target after the clear. The new legacy test checks both inline-block and that the saved angle is the whole angle (plain + 15).
  • Mutation check: each of these four reverts makes a test fail: dropping plain in the overlay, ignoring next.plain in the save, always using the press target on the legacy path, and removing next.plain || from the routing.

Earlier non-blocking items: the centring-only-when-the-transform-starts-with-translate(...) limit, the scale: 2 1 shear, and dropped !important are unchanged (rotationDraft.ts is untouched apart from the new type). Still non-blocking. The panel-read item was handled by #4830.

Merge state: the PR now conflicts with main, which has #4805 and #4834. On a probe merge, the source files auto-merge. The only conflicts are additive test hunks in domEditOverlayStartGesture.test.ts and useGsapAwareEditing.test.tsx, where both sides add cases. The merged source typechecks clean outside those two files, and the rotate/move suites pass on it (99 tests). After the rebase, re-bank baseline.json from a run on the merged head.

Verified: the rotate-related studio tests pass at head (7 files, 101 tests, NODE_ENV=test). At the time of this review, studio unit tests, the edit-accuracy shards and the Windows jobs were still running. No Codex pass on this delta.

— Somu

…rawn with

The overlay passes the rotate target it read at press into the commit, so the
save draws and patches exactly what the draft did instead of re-reading an
element the draft already made inline-block. The GSAP-aware commit also routes
on that press-time decision.
baseline.json is the edit-accuracy-gate artifact of the CI run at cb75cc3,
copied byte for byte: 530 to 557 passing, all rotate-none cases, none
regressed.
… whole angle on a new rotate

Clearing the legacy rotation marks puts the element back to inline and drops
their turn, so the save reads the target again instead of using the one the
press read before the clear. A test also pins that a turn the press drew as CSS
stays on the CSS writer.
@miguel-heygen
miguel-heygen force-pushed the fix/studio-rotate-none-drop branch from 38f9198 to 86e5648 Compare October 1, 2026 09:43

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rebase delta check, 38f9198e → a6b07722: the rebase is equivalent and the baseline numbers check out. No new findings.

  • Range-diff: all 8 PR commits pair up with their pre-rebase versions. Three are identical. Five differ only where they had to fit #4834's new move-route parameters: pathOffsetCommit(selection, next, route), readMoveOffset(el, plainTranslate), and the plainTranslate param on the interface. None of the four rotate fixes changed: the press-read target, routing on next.plain, inline-block on the inline save, and the legacy re-read after clearing the old marks. The last commit a6b07722 only touches baseline.json.
  • Gate, recomputed with the bench's own accurate() from ratchet.mjs against main's baseline.json at 2eb19191: 530 → 557, 0 pass→fail, and all 27 newly passing cases are rotate-none-*.
  • Tests: the 7 touched studio test files pass at head (106/106, NODE_ENV=test). I reverted the legacy re-read fix, then dropped next.plain || from the rotation routing. Each mutant failed one targeted test (rotateInlineSave and useGsapAwareEditing rotation routing).
  • Not verified: the edit-accuracy CI shards (1–8/8) were still pending when I posted. Everything else is green. My earlier non-blocking notes stand.

— Somu

@miguel-heygen
miguel-heygen merged commit 74f5013 into main Oct 1, 2026
66 checks passed
@miguel-heygen
miguel-heygen deleted the fix/studio-rotate-none-drop branch October 1, 2026 10:38
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.

2 participants