Skip to content

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

Open
miguel-heygen wants to merge 6 commits into
mainfrom
fix/studio-rotate-none-drop
Open

miguel-heygen wants to merge 6 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 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.
  • 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

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

@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: 521 passing here, 494 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 4 times, most recently from 17f6e71 to 7e28bbb Compare October 1, 2026 02:51
@miguel-heygen
miguel-heygen force-pushed the fix/studio-rotate-none-drop branch 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

This branch has not been deployed

No deployments
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