fix(studio): move and nudge an element without GSAP by its own CSS translate - #4799
Conversation
Edit accuracy: 494 passing here, 494 on the base branchThe gate passes. |
… meet the file checks
…not position alone
baseline.json is the edit-accuracy-gate artifact of the CI run at 2a84c10, copied byte for byte: 60 newly passing cases, none regressed. The commits since change no behaviour (a reader split and tests).
2a84c10 to
2ea7bab
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 2ea7bab8. Read the full changed files plus the callers of the new gate (drag press, nudge, single and group commit, stager, panel X/Y), and the GSAP 3.15 CSSPlugin code the design depends on.
Verdict: APPROVE. No blockers. The routing decision fails safe in the cases that matter, and the edit accuracy numbers check out. Nits below. None of them needs to block tonight.
Is "un-animated" detection right?
gsapWritesPosition (gsapRuntimeKeyframes.ts:438-440) says yes if el._gsap.renderTransform is set, or if any child of any window.__timelines entry targets the element and writes x/y/xPercent/yPercent/left/top/translateX/translateY/motionPath (:420, :422-433).
- GSAP loaded but the element is not tweened: no matching tween and no
renderTransform, so it takes the CSS route. Covered by the newdomEditOverlayStartGesturetests. - Tween targets the element through a selector or class, or a stagger: GSAP resolves targets when the tween is created, and
matchesElementcompares them by identity (:165). So.box,[data-x]andgsap.utils.toArrayall match. A shared stagger is still detected, which keeps the shared-tween left/top path intact (elementOffsetStager.ts:71only branches off when GSAP does not position the element). - Ancestor or descendant tweens: these don't match, and they shouldn't. A parent transform composes with the child's own
translate, so moving the child bytranslateis correct. Covered for the parent case. - Nested compositions: detection reads
el.ownerDocument.defaultView.__timelines, the same sourceelementHasNonHoldTweenalready uses.getChildren(true)is deep, so a sub-timeline added to a parent is included. The id fallback inmatchesElementcan only produce a false "animated" (two instances sharing an authored id), and that falls back to the old path. The*-nestedmove/nudge cases are among the 60 newly passing. - Later in the timeline:
getChildren(true)returns every child regardless of the playhead, so a position tween starting at 5s is detected at t=0.
I found no false "un-animated" for a position tween registered on a timeline. See nit 1 for the remaining gaps.
Can a GSAP-animated element get a translate that fights the tween?
- Position tween anywhere on a registered timeline: no. The press (
manualOffsetDrag.ts:342), commit (useGsapAwareEditing.ts:156,:249) and stager (elementOffsetStager.ts:71) all keep the GSAP writer. - Opacity-only or other non-transform tweens: these get the CSS route. GSAP never touches
transform/translatethere, so nothing competes. - A scale/rotation tween that has not rendered yet (it starts after the playhead): this gets the CSS route. At that tween's init, CSSPlugin's
_parseTransformfolds the computedtranslateintotransformand setstranslate: none(CSSPlugin.js 859-865), so x/y start at the moved value and the position holds. It is folded once, not fought. One side effect: whether the next move routes CSS or GSAP depends on whether that tween has rendered at the current playhead. Both routes land in the right place. - A position tween added after a move: the same fold applies at init. It matches main's semantics, where the move had become
gsap.set x/y. The next move routes through GSAP. - Existing
transform: the writer touches onlytranslate. Individualtranslatecomposes ahead oftransform, so rotate/scale intransformare not overwritten. The r30 fixtures cover this. - Runtime not booted: detection reads the live runtime only, so an empty
__timelinesreads as un-animated. In practice the commit path refuses before the preview has booted (useDomGeometryCommit.ts:149), and the press needs a rendered overlay.
Press-jump fix
An earlier head resolved %/calc() by writing a temporary inline transform, and that showed for a frame. readTranslatePx (plainTranslate.ts:59-67) now resolves px, %, and Chrome's calc(P% ± Lpx) arithmetically against the border box, which is the reference box for HTML elements. It only falls back to the transform probe for min()/max()/clamp(). That probe is set and restored synchronously, so nothing paints in between. The rotation read was moved after member creation and is skipped for plain moves (domEditOverlayStartGesture.ts:235). That is needed, because getProperty would run _parseTransform and bake the translate. Looks right.
baseline.json and the 494/434 claim
I recomputed this with the gate's own accurate() from ratchet.mjs against main's baseline: 434 → 494 accurate, 0 regressed, 60 newly passing, all move-none-*/nudge-none-*. That matches the body. At this head, Studio: edit accuracy gate is success and the sticky comment reads the same. A second run of the shards was still in progress when I checked.
The composite pass field (which includes smoothness) flips true→false on 58 non-"none" cases. All of those flips are smoothness only: work is null in 88 entries vs 16 before, and no gated metric got worse by more than 0.5px. The ratchet does not gate on smoothness, so this is harmless. It does make the raw pass column read worse than main.
Tests
bun install --frozen-lockfile, then built parsers, lint, core and studio-server.vitest run src/components/editor src/hooks: 254 files, 2700 tests, all pass.tsc --noEmit -p packages/studio: clean.- Mutations on
gsapWritesPosition:- Dropping the timeline channel scan fails "keeps the GSAP writer for an element a timeline hold positions".
- Dropping the
renderTransformcheck fails 10 tests. - Dropping
motionPathfromMOVE_CHANNELSpasses everything. So does dropping thekeyframeVarsCarryChannelbranch (nit 2).
Nits
-
Detection gaps that land in the dangerous direction (all edge cases):
- A tween outside
window.__timelinesthat hasn't rendered yet (a standalonegsap.to(el, {x, delay})). - A tween written as
transform: "translateX(..)"ortranslate: "...", which aren't inMOVE_CHANNELS. - A not-yet-rendered transform tween with
clearProps: "transform"|"all". CSSPlugin.js:598 setstranslate: noneat completion, which would wipe a saved move.
Adding
"transform"and"translate"toMOVE_CHANNELS(gsapRuntimeKeyframes.ts:420) is cheap, and it only fails toward the old path. - A tween outside
-
No test pins the
keyframeVarsCarryChannelormotionPathbranches ofgsapWritesChannels. Akeyframes: { x: [...] }tween starting after the playhead is the case that would quietly get a plain translate if either branch regressed. Worth one test each. -
The route is decided three times, independently: at press (
manualOffsetDrag.ts:342), at commit (useGsapAwareEditing.ts:156) and in the stager (elementOffsetStager.ts:71). In the plain route,nextis absolute translate px. In the GSAP route it is a legacy offset. I could not find a way for the answers to diverge mid-gesture, becauserenderTransformis sticky and the panel'sgetPropertyreads run at selection, before the press. Still, carryingmember.plainTranslatethrough to the commit would make that true by construction rather than by sequencing. -
The group commit returns early when
gsapCommitMutationis absent (useGsapAwareEditing.ts:194), before the plain-member check. The single-move path checks plain first (:156). IfgsapCommitMutationcan ever be absent, a group of GSAP-free members would silently not save while a single move would. Moving the plain partition above that guard would make the two paths consistent. -
readTranslatePxprefers the inline value over the computed one (plainTranslate.ts:61). A stylesheettranslate: ... !importantwould make both the read and the write ineffective. That's rare, so a comment is enough.
— Rames
What changes
When GSAP does not position an element, dragging it or nudging it with the arrow keys now moves it by its own CSS
translate. The drag preview, the drop and the saved file all use the same literal: inlinetranslate: 130px 90pxin plain px. Nothing else is written: nogsap.set, no timeline, and no GSAP script from the CDN is added to a GSAP-free file. The save goes through the existing inline-style patch, with no preview reload and no read of the file's animations.Before this change, every move went through the GSAP writer:
gsap.setto a CSS-only composition. The first save took 2-3 s and failed offline.translateintox/y, then the set overwrote them, so the element landed short of the drop by its own translate (50 px on the fixture below).</template>, where it never runs, so a nested move or nudge was lost.How
gsapWritesPosition(el)(gsapRuntimeKeyframes.ts): GSAP positions the element if a live timeline tween or hold writes x/y/xPercent/yPercent/left/top/motionPath on it, or if GSAP already renders its transform. It is synchronous and does no fetch. The press, the drop and the save all ask it. A GSAP-positioned element keeps today's route unchanged.readTranslatePx(plainTranslate.ts): the element's translate in px as Chrome resolves it.%,calc(),min()/max()/clamp(),var()andemare resolved by Chrome itself, not by parsing.elementOffsetStager.ts): a GSAP-free move is saved on the element itself, as a shared-tween word already was, and puts the live value back if the save fails. It serves single moves and group members.gsap.getPropertyon these elements, because that makes GSAP bake the CSS translate into its transform.translateon spaces and loses acalc().translate: -50% -50%is stored in px at its current size once moved.Before
Main, edit accuracy fixture
move-none-px-r0-root-z100(translate: 40px 30px, no GSAP), after dropping a +90/+60 move. The box lands 50 px short of the drop, and the file gains a CDN GSAP script plusgsap.set("#target", { x: 90, y: 60 }).After
Same fixture and gesture on this branch. The box stays where it was dropped, and the only change to the file is
style="translate: 129.998px 90px"on the element. The Lint badge counts the fixture's own two findings (no__timelinesregistration), which the GSAP bootstrap used to hide.Edit accuracy slice
^(move|nudge)-none-(72 cases)Before: main 299670e, devbox.
After: this branch merged with main, CI edit accuracy gate (8 shards, each case 2 of 3 runs).
All 72 pass every metric except smoothness; 59 of 72 pass smoothness too, which its own track scores. Across the whole 660-case grid the gate counts 494 passing (main: 434), 60 newly passing and none regressed.
baseline.jsonbanks them byte for byte from that run's artifact.The press reads a
%orcalc()translate arithmetically against the border box and writes nothing to the element. An earlier head resolved it through a temporary inlinetransform, and that showed for the first frame after the press: CI saw a press jump equal to the element's own translate on every percent case.Tests
translatepatch and makes no GSAP request. A failed save puts the live translate back (useDomGeometryCommit.test.tsx).manualOffsetDrag.test.ts).readTranslatePxresolves px, %,calc(P% ± Lpx)and padding plus border without writing to the element, and hands min()/max()/clamp() to Chrome with the style put back (plainTranslate.test.ts). A drag press on a page that loads GSAP but does not position the element (nothing animated, or only its parent) never asks GSAP and keeps the CSS route. A centred element starts from its -50% translate in px (domEditOverlayStartGesture.test.ts).useGsapAwareEditing.test.tsx).Not in this PR
Resize and rotate routing for GSAP-free elements, the nudge's layout read, and deleting the left/top and legacy offset channels come in follow-ups.