fix(studio): the properties panel no longer hands GSAP an element it does not position - #4830
Conversation
…does not position
Edit accuracy: 494 passing here, 494 on the base branchThe gate passes. |
There was a problem hiding this comment.
Verdict at 0657fe27: no blockers. This is a post-merge review: the PR merged at 05:52 UTC while I was still reviewing, with no GitHub review on it. I reviewed in two independent passes, one by Codex on the raw PR and one of my own, and checked every finding below against the source at this head.
What I verified
- The panel guard does what it says. For an element that
gsapWritesPositionreports as not positioned by GSAP,readGsapRuntimeValuesForPanelskips every key inGSAP_TRANSFORM_KEYS, so no transform getter runs. Move and nudge already route on the same predicate (createManualOffsetDragMember→plainTranslate;elementOffsetStager.ts:71). So a fade-only layer's next drag now writes its CSStranslate. GSAP_TRANSFORM_KEYSmatches GSAP 3.15.0 exactly. I checked it againstCSSPlugin.js: the 19 entries of_transformPropsminustransform, plus the 8 aliases (translateX/Y/Z,rotate,rotationZ,rotateZ/X/Y). Leaving outtransformis right, because_getskips both alias resolution and_parseTransformfor that key.- Tests. The full studio suite passes at head (561 files, 6202 tests,
NODE_ENV=test). Each of these mutations made the new test fail: dropping the guard, forcingreadsTransformto false or to true, and removing the four origin keys. - Deleting
gsapAnimatesTransformis safe. It had no consumers. - CI is green: 76 pass, 7 skipped.
Should-fix / follow-ups (non-blocking)
- The Properties panel's X/Y (and rotation) fields still write through GSAP for a fade-only layer.
propertyPanelTransformCommit.ts:50routes throughonCommitAnimatedPropertywheneverhasGsapAnimation, which is true for any tween (PropertyPanel.tsx:222). Typing X=130 into the field still adds GSAPxto the file, while dragging the same layer now writes its CSStranslate. That isn't a regression, since this was already the behaviour on main. But the two edits to the same field now land in different channels. The field should route ongsapWritesPositionlike the drag does. - The 494 = 494 gate doesn't exercise this route at all.
grid.mjsonly hasgsap: none | tween | hold, and bothtweenandholdanimatex/y. No case has a non-positioning tween, so the gate can't move either way. Adding afadevalue to thegsapaxis (an opacity-onlytl.to) would make the before/after in the description a banked regression case. - The tests prove the call list, not the folding.
getPropertyis a mock, so the test pins which keys get read. That's the right unit property, but it doesn't pin the list itself: removingsvgOrigin,skewXorrotateZfromGSAP_TRANSFORM_KEYSeach leaves the suite green (measured). A test that loads real GSAP in happy-dom could assert the property directly: for every key,getPropertysets_gsap.renderTransform, and for no other panel key does it. That would also catch a future GSAP adding a transform alias. - The origin test case only holds before the tween first renders. A real fade tween with
transformOriginparses the transform at init (CSSPlugin.js:1059, whereisTransformRelatedis true fortransformOrigin), so it setsrenderTransformitself. From then on the panel correctly reads all channels. The exclusion still matters before the tween's first render, which is what the test covers in effect. The test name could say so. - Fallback display for authored CSS transforms. For a fade-only layer with an authored
rotate: 20deg/scale: 2(ortransform: rotate() scale()), rotation now falls back to--hf-studio-rotation(0 at this head) and scale/depth to identity. This matches how a fully un-animated layer already displays on main. #4802'sreadShownRotationfixes the rotation half once it lands, but scale/3D would still show identity.
Other routes that still read GSAP at press (outside this PR, noted for the union)
At this head (base = main), two press paths still call GSAP's transform getters on a fade-only layer, and either one alone makes its next move take the GSAP route:
- Rotate press:
domEditOverlayStartGesture.ts:235callsreadGsapRotationunless a plain-translate member exists, and rotate never creates one. #4802 gates this ongsapWritesRotation. - Resize press:
domEditOverlayStartGesture.ts:180-181callsreadElementGsapNumber(x/y)on every resize. #4805 removes this read.
#4830 and #4802 test-merge cleanly onto main, and so do #4830 and #4805. #4802 and #4805 conflict with each other in useDomGeometryCommit.ts and baseline.json (already flagged on those PRs). I haven't re-verified #4805's resize commit path here.
Pre-existing, in the shared predicate (from #4799, not this PR)
matchesElement(gsapRuntimeKeyframes.ts:162) matches tweens byidacross every timeline in the window. A root#cardthat only fades counts as "GSAP-positioned" if a sub-composition's separate#cardhas anxtween. The panel now follows the move router, so the two stay consistent, but both are wrong in that case.MOVE_CHANNELSincludesleft/top, which CSSPlugin doesn't parse as transforms. Aleft-only tween therefore routes moves through GSAP and lets the panel read the transform. That's consistent with the router, but it's broader than "GSAP owns the transform".
— Somu
What changes
Selecting a layer that GSAP only fades, colours or otherwise animates without moving no longer hands that layer's position to GSAP. Its next drag or nudge then moves it by its own CSS
translate, the same way #4799 moves a layer GSAP does not animate at all.Before, the Properties panel read GSAP's x, y, rotation, scale and depth values for any layer with a tween, on every frame while it was selected. Any GSAP read of a transform value makes GSAP's CSS plugin fold the layer's CSS
translate,rotateandscaleinto GSAP's own transform. From then on GSAP counted as owning the layer's position, so selecting a fading title was enough to send its next move throughgsap.set.How
readGsapRuntimeValuesForPanel(propertyPanelHelpers.ts) still reads every non-transform value it read before, such as opacity and border radius. It reads GSAP's transform values only for a layer GSAP positions (gsapWritesPosition, the same check the move uses). For any other layer the panel falls back to the values it already shows when GSAP has none: the layer's CSS translate for X/Y, Studio's own rotation value (--hf-studio-rotation) for rotation, and identity in the 3D section.The list of GSAP transform channels is
GSAP_TRANSFORM_KEYSingsapRuntimeKeyframes.ts. It is GSAP 3.15's CSS plugin transform list, includingtransformOrigin,svgOrigin,force3DandsmoothOriginand the aliases, minustransformitself, which GSAP reads without parsing. GSAP doesn't expose that list at runtime, so it is copied, and a test pins the origin case.Test
gsapLivePreview.test.ts: "the panel reads GSAP's transform only off an element GSAP positions". For a fade-only tween the panel reads onlyopacity; for a tween that moves the layer it reads the full transform set, as before. A fade withtransformOrigin: "0 0"also reads onlyopacity. With the new guard removed, the fade case fails because the panel reads the nine transform channels too. With the four origin keys missing from the list, the origin case fails.Before
Main, a fixture with one box placed by its CSS
translate: 40px 30pxand a GSAP tween that only fades it (opacity: 0.4). Selecting the box with the Design panel open and dropping a +90/+60 move: the panel shows X 130 / Y 90, and the file gainsgsap.set("#target", { x: 130, y: 90 })in the timeline script, with no change to the box's owntranslate.After
Same fixture and gesture on this branch. The panel shows the same X 130 / Y 90, and the only change to the file is
style="translate: 130px 90px"on the box, with nogsap.set. The source view is shown after reloading Studio.