fix(studio): a rotate without GSAP saves where you let go, nested too - #4802
miguel-heygen wants to merge 6 commits into
Conversation
0b0b814 to
7792b8b
Compare
Edit accuracy: 521 passing here, 494 on the base branchThe gate passes. Newly passing (27)
|
7792b8b to
75e080f
Compare
17f6e71 to
7e28bbb
Compare
…e element's own CSS rotate
…line elements too
…t after the move landed
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
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 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.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.
After
The same case keeps its 25 deg turn.
Transform-centred, before: the box swings off its centre.
After: it turns in place.