fix(studio): a rotate without GSAP saves where you let go, nested too - #4802
Conversation
0b0b814 to
7792b8b
Compare
Edit accuracy: 557 passing here, 530 on the base branchThe gate passes. Newly passing (27)
|
7792b8b to
75e080f
Compare
7e28bbb to
9dedaee
Compare
somanshreddy
left a comment
There was a problem hiding this comment.
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:
- At press,
readCssRotationTargetseesdisplay: inline, soinline: true. - Each draft frame, and the pointerup "hold the final angle" (
useDomEditOverlayGestures.ts:432), callsapplyCssRotation(…, g.plainRotation), which setsdisplay: inline-blockon the live element. - The commit then calls
savePlainRotation→applyCssRotation(element, next.angle)with a freshreadCssRotationTarget. Computed display is nowinline-block, soinline: falseand 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.
startGesturedecidesplainatdomEditOverlayStartGesture.ts:235, buthandleGsapAwareRotationCommitre-runsgsapWritesRotation()atuseGsapAwareEditing.ts:463. If anything initializes_gsap.renderTransformmid-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) callsgsap.getProperty(el, "rotation"). In GSAP 3.15,_parseTransformsetscache.renderTransform(CSSPlugin.js:1021) and bakes the CSSrotateinto 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)haslead = 0, so the turn is prepended and rotates the translation, and the box swings. Separately, a non-uniformscale:longhand (scale: 2 1plus 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, thescaleX(-1)/scale(2,1)-in-transform cases andscale: -1 1are correct, as the tests show.
Nits
- Draft, save and restore drop inline
!importantontransform/rotate(rotationDraft.ts:102, and the snapshot stores values only). The temporaryshareread does preserve priority. lastRuleTransformalso misses rules nested in@layer/@supports, not just@media. A centring transform declared only there falls back to therotateproperty and swings. The PR already notes the related limitation.- The banked
baseline.jsonhas all 36rotate-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 headbaseline.jsongives 494 → 521. All 27 newly passing cases arerotate-none-*, with 0 regressions and none missing. CI'sStudio: edit accuracy gateran 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 writestranslate(...) rotate(-155deg) scaleX(-1), and the matrix angle comes out at the requested 25°. - GSAP folding: GSAP 3.15 folds the CSS
rotate/scale/translateinto 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 inbaseline.json. Whichever lands second must keep bothhandleDomRotationCommitand #4805'shandleDomBoxSizeCommitand re-bank the baseline. A dropped param would fail typecheck, since it's required inUseGsapAwareEditingParams. 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
a2ee9d6 to
cb75cc3
Compare
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
left a comment
There was a problem hiding this comment.
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-timeCssRotationTargetto the commit (RotationCommit.plain), sosavePlainRotationdraws and patches what the draft drew instead of re-reading an element the draft already made inline-block.handleGsapAwareRotationCommitroutes onnext.plainfirst, 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 (viaSTUDIO_ORIGINAL_TRANSFORM_DISPLAY_ATTR) and drops the legacy turn, so the save re-reads the target after the clear. The new legacy test checks bothinline-blockand that the saved angle is the whole angle (plain + 15).- Mutation check: each of these four reverts makes a test fail: dropping
plainin the overlay, ignoringnext.plainin the save, always using the press target on the legacy path, and removingnext.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
…e element's own CSS rotate
…line elements too
…t after the move landed
…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.
38f9198 to
86e5648
Compare
somanshreddy
left a comment
There was a problem hiding this comment.
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 theplainTranslateparam on the interface. None of the four rotate fixes changed: the press-read target, routing onnext.plain,inline-blockon the inline save, and the legacy re-read after clearing the old marks. The last commita6b07722only touchesbaseline.json. - Gate, recomputed with the bench's own
accurate()fromratchet.mjsagainst main'sbaseline.jsonat2eb19191: 530 → 557, 0 pass→fail, and all 27 newly passing cases arerotate-none-*. - Tests: the 7 touched studio test files pass at head (106/106,
NODE_ENV=test). I reverted the legacy re-read fix, then droppednext.plain ||from the rotation routing. Each mutant failed one targeted test (rotateInlineSaveanduseGsapAwareEditingrotation 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
What
Rotating an element that GSAP does not turn now saves where you let go, nested compositions included:
gsapWritesRotation(el)sits beside the move'sgsapWritesPosition(el): GSAP owns the rotate when it renders the element's transform or a tween or hold writes a rotation channel (rotation,rotateand their X/Y/Z forms). Everything else is a plain rotate, decided once when the gesture starts, before anything reads GSAP (agsap.getPropertycall would bake the CSS into GSAP's transform and flip the decision mid-gesture).rotate,scaleandtransformthe way GSAP would parse them, so an element authored withrotate: 30degno 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.savePlainRotationwrites the element's own inlinerotate, the same value the draft draws: the angle less what itsscaleandtransformalready 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, throughhandleDomRotationCommitpassed intouseGsapAwareEditing, 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.rotateproperty applies beforetransform, so a turn written there also turns atransform: 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 mirroringscaleproperty flips the turn's sign. A second rotate replaces its ownrotate(). The authored transform comes from the inline style, else the last matching stylesheet rule (specificity,!importantand@mediaare 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.inline-blockin 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 theinline-blockthe 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).scale/transformshare is read once at press; the draft only setsrotate.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.setinto 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)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
transformplacement 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 onlyrotate, 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 CSSrotate.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 callsgsap.getPropertyorgsap.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 savesdisplay: inline-blockwith the turn; it fails if the save re-reads the target or the overlay drops it.rotationDraft.test.tsandplainRotation.test.ts: a transform-centred element turns inside its transform after the translate; mirrored (scaleX(-1)), stretched (scale(2, 1)) andscale: -1 1centred 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.Before
Nested, released after a 25 deg turn: the element is back at 0 deg.
An inline word, turned 25 deg, after a reload: the save wrote
rotatewithoutinline-block, so the word is flat again.After
The same case keeps its 25 deg turn.
Transform-centred, before: the box swings off its centre.
After: it turns in place.
The inline word after a reload keeps its turn: the save wrote
display: inline-block; rotate: 24.997deg.