fix(studio): read the rotate property when measuring an element's angle - #3163
Conversation
Turning an element with Studio's rotate handle left every piece of overlay chrome square across it: the selection box, the crop outline and the child outlines all drew upright while the element underneath was clearly rotated. The handle writes the CSS `rotate` property. `rotate` is an individual transform property, not part of `transform`, so `getComputedStyle(el).transform` reports nothing for it and both places that measure an element's angle — the overlay geometry and the crop frame — read the element as upright. Both now read `rotate` alongside `transform` and compose them the way CSS does, individual properties first. A rotation about any axis but z has no single in-plane angle, so it reports nothing and the caller keeps its axis-aligned fallback rather than drawing chrome at a plausible wrong angle.
The crop outline still drew square on a rotated element after the rotate- property fix, because it refused the transform outright: it accepted only `matrix(...)`, and GSAP writes `matrix3d(...)` for an ordinary 2D move or spin (force3D). A composition that mirrors an element writes one with a negative z scale, and the negative determinant that follows was refused too. Both are ordinary planar transforms. The outline now reads the same 2D projection the rest of the chrome takes through DOMMatrix, and sizes a flipped element from the magnitude of its determinant. Only a perspective term still falls back, because that is where the mapping stops being affine and no single angle describes it. The test that asserted "a 3D matrix means give up" asserted the bug: its fixture was the identity written as matrix3d, which is as planar as a transform gets. It now checks the behaviour that replaced it, alongside the perspective case, which still falls back.
Selecting a text layer inside a rotated card drew its crop outline across the text at roughly a right angle. The outline read the element's own transform, but what the user sees is that composed with every ancestor's — the layer carries its own spin and its parent turns it again. It now walks to the composition root and composes each level, the element's `rotate` property before its `transform` and an ancestor outside its child, which is the order CSS applies them in. Nothing transformed anywhere still falls back to the caller's axis-aligned rect, since that comes from real layout and describes the element exactly. The chrome test stubbed getComputedStyle to answer "rotated 30deg" for every node in the document, so composing read the same turn once per ancestor. The stub now answers per element, which is what it always meant.
677a11d to
83cb9f5
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 83cb9f5e4.
Cleanly diagnosed and cleanly fixed. The rotate property is separate from transform, both are read together on every node, individual-transform ordering (rotate before transform) is applied correctly, and the ancestor walk stops at the composition root using the same convention the sibling readElementTransformSnapshot already uses. The matrix3d rehabilitation is scoped to affine matrices — perspective terms at m[3]/m[7]/m[11] still fall back to axis-aligned, which is honest (there is no single angle for a projective mapping). Flipped elements route through |det| so a negative-determinant composition still reports a real size.
One meaningful concern, one nit.
Concern
Divergence risk: two implementations of the same ancestor-with-rotate composition now live in this directory. domEditOverlayCrop.ts:212-289 composes spinMatrix(rotate) before a hand-parsed {a,b,c,d} from matrix/matrix3d, walks parentElement, stops at data-composition-id. domEditOverlayGeometry.ts:150-172 does the equivalent walk with DOMMatrix.rotateSelf(spin).multiply(...) for the same boundary. Both fold rotate in on the left, both stop at the same attribute, but the code paths are independent.
That's fine today — this PR fixes both — but it's the same shape as the bug this PR closes. When someone adds support for CSS translate or scale (the other individual transform properties), or the composition-root boundary changes (e.g. to data-composition-file for nested compositions), the fix must land in both files, and a miss in one puts the crop outline back to drawing at the wrong angle while the selection box draws at the right one — which is the divergence class this PR exists to close.
The crop file can be expressed on DOMMatrix too: DOMMatrix exposes .a/.b/.c/.d directly, so the angle/scale derivation at :322-333 runs unchanged, and both files could share the walk + individualRotateDegrees fold as one helper. Not asking for it in this PR, but worth flagging so the next individual-transform work doesn't rediscover the same class of bug.
Nit
Ancestor composition is only covered by one automated test, and that one's ancestors have no transform of their own. domEditOverlayCrop.test.ts uses a synthetic fakeEl at :164-169 with no parentElement, so every test in that suite exercises readPlanarTransform at exactly one node — the walk never gets to a second iteration. The DomEditSelectionChrome.test.tsx:73-79 multi-element mock is the only test that runs the walk against a real DOM, and it stubs every non-target node to transform: "none", so composition of two rotations (element and ancestor) isn't locked either.
The PR body names the exact case that isn't in the automated coverage: "Selecting the text layer inside it — the outline has to follow the angle the layer paints under, which is its own spin composed with its parent's." That is a "child at 20deg inside parent at 30deg → composed angle 50deg, both scales combine" test which today only exists in the manual test plan. It's what would catch a future regression where the composition order flips or the parent walk breaks. Low priority — the code is verifiable from reading — but the composed-rotation case is the load-bearing invariant of this whole change, and it's the one shape the suite doesn't exercise.
What lands cleanly
individualRotateDegreesatdomEditOverlayCrop.ts:123-138— planar-only, honors axis sign for0 0 -1 <angle>, returns 0 for any 3D axis so the caller falls back to axis-aligned rather than drawing at a plausible-looking wrong angle. Absent/unparseable → 0. Test coverage at:279-311locks all four branches.parseMatrixComponents— thematrix3drehabilitation only accepts matrices wherem[3]/m[7]/m[11]are withinPERSPECTIVE_EPSILON, keeping the fall-back-square behavior only for the case where a single angle stops describing the transform. Projection to{a,b,c,d}fromm[0]/m[1]/m[4]/m[5]is correct for the CSS column-major convention.elScaleY = Math.abs(det) / elScaleXat:328— the sign of the determinant no longer collapses the height to zero on a mirrored element. Existing behavior for scale-only transforms preserved.- Composition-root boundary at
:296matches the geometry file's convention, so the crop outline and the selection chrome measure to the same root. - Two existing tests that asserted the old, refuse-
matrix3dbehavior were updated in place, not deleted — the fixtures moved from "3D means give up" to "2D-written-as-3D is measurable" and "perspective still falls back", which is the right split. - Chrome test's per-element
getComputedStylestub atDomEditSelectionChrome.test.tsx:73-79— necessary now that the walk sees more than one node, and the comment names why.
Ready from where I sit — stamp routing per standing rule.
vanceingalls
left a comment
There was a problem hiding this comment.
Approving. The change is correct, the tests lock in exactly the regressions the commits describe, and the before/after screenshots line up with what the code does.
What I checked
- Composition order in both files — the
rotateproperty composes on the left oftransformat each node (matches CSS: individual properties applied beforetransform, soM_final = M_transform × M_rotateand ancestor-composed asM_parent × M_child). BothdomEditOverlayGeometry.ts:158-167(DOMMatrix,own.multiply(matrix)) anddomEditOverlayCrop.ts:composeMatricesare symmetric. parseMatrixComponentsplanar-vs-3D gate — checksm[3], m[7], m[11]for perspective terms, drops thematrix3dz-projection to a 2D{a,b,c,d}. Correct for the "GSAPmatrix3dfor ordinary 2D moves" case and for negative-determinant flipped elements. Genuine 3D rotations without perspective (e.g.rotateX(30deg)) will get through the gate and produce a "plausibly foreshortened" 2D box rather than falling back — a soft behavior change frommain, but arguably no worse than the axis-aligned fallback for elements Studio never authors with 3D turns today.individualRotateDegreesaxis-parsing — the0 0 -1 30degsign-honouring case is the one that's easy to get wrong; the unit test locks it in. Non-z axes correctly return 0 so callers fall back rather than drawing a plausible-looking wrong angle.- Test fixture correction in
DomEditSelectionChrome.test.tsx— moving from a blanketgetComputedStylemock to per-element is exactly the adjustment the composition walk requires; the note about "rotation would compound once per ancestor" is well captured. - All required CI green on windows-latest (Tests, Render, Preview parity, Studio load smoke).
MergeStateStatus: BLOCKEDis approval-count only.
One nit (non-blocking)
packages/studio/src/components/editor/domEditOverlayCrop.ts:305—node.hasAttribute?.("data-composition-id"):hasAttributeis guaranteed onHTMLElement | null(already narrowed toHTMLElementat that point). The?.is dead code. Consistent withdomEditOverlayGeometry.ts:166which just usesnode.hasAttribute(...).
Follow-ups outside this PR's scope — same missing-rotate-read pattern lives in a few other places worth a separate ticket
The individualRotateDegrees helper is exported now; two sites derive angles/matrices from .transform alone and would benefit from picking it up (or from a shared helper):
packages/studio/src/components/editor/manualEditsDom.ts:222-238—stripGsapTranslateFromTransformreads onlyelement.style.getPropertyValue("transform"), then derives an angle viaMath.atan2(m.b, m.a)to counter-rotate the Studio offset when un-baking GSAP's translate. On an element rotated via CSSrotate:, the derived angle is wrong and the un-bake writes back an incorrectm41/m42. Symmetric to the bug this PR fixes, on the write-back side.packages/engine/src/services/threeDProjection.ts:766(readTransform) — readsgetComputedStyle(el).transformand multiplies into the view matrix at 815-817 without anycomposeIndividualTransforms-style step.videoFrameInjector.ts:605already has the correct composition helper in the repo and could be reused. A group whose root carries CSSrotate:renders in the 3D projection without that rotation.
Lower priority (root-only or fallback paths, low current blast radius): drawElementService.ts:394/787/1033 (composition-root capture), videoFrameInjector.ts:331/689 (raw computed-transform export), CaptionOverlayUtils.ts:155-164 (GSAP-absent fallback that regexes rotate(...) out of the transform string only).
Nothing in this list should block #3163 — flagging so the pattern-fix work doesn't stop at these two files.
A composition lives under this package's root, so Vite's HMR saw a write to one as an html page dependency changing and full-reloaded the browser. That reload is the flash after every edit in the canvas: the whole app remounts, taking the preview iframe with it. The decision was never Vite's to make. Studio already knows whether a write was its own — that is what the write receipt is for — and refreshes the preview itself when it needs to. Vite's watcher now ignores the project data, and the dev plugin watches it on a watcher of its own, announcing changes as hf:file-change exactly as before. Measured on a drag: Vite full reloads went from one per edit to none, and the receipt now reports 'suppressed: own write token' where it previously never saw a matching path.
Review: the crop frame hand-composed ancestor matrices while the geometry file did the same walk through DOMMatrix. Both were right, but the next individual transform property CSS grows — `translate`, `scale` — would have to land in both, and a miss puts the crop outline back at the wrong angle while the selection box draws the right one. The walk now lives in one place and takes the arithmetic as a parameter. The geometry file keeps DOMMatrix, because it goes on to transform corner points and needs the translation; the crop frame keeps plain 2D components, because it only needs an angle and a scale. Which transforms count, and in what order, is stated once. Also from review: the nested case was verified by hand only, so the composed walk is now covered on both sides — a child inside a rotated parent reports the angle it paints at, the parent's rotation alone when the child has none, and the walk stopping at the composition root. And `hasAttribute?.` was dead on a narrowed HTMLElement; it only survived because the crop test's fake element was not one. The fake now models an element and the guard is gone.
|
Thanks both — all four review points are addressed, and the two follow-ups have answers. Two of them changed the diff, so this is worth a re-read rather than a re-approve on the old state. Divergence between the two walks (Rames) — fixed, and you were right that it was the real risk here. The walk now lives in one place ( Composed case only manually verified (Rames) — fixed. Covered on both sides now: a child inside a rotated parent reporting the angle it paints at, the parent's rotation alone when the child has none, and the walk stopping at the composition root rather than climbing to the document.
Follow-up 1, The reason it holds: Caveat: my first attempt at that repro was wrong and passed for the wrong reason — it never set a baked matrix, so the strip early-returned on Follow-up 2, New in this PR since your reviews: a fix for the canvas flashing after every edit. It was not Studio — a composition lives under the studio package root, so Vite's HMR treated a write as an html page dependency changing and full-reloaded the browser, remounting the app and its iframe. Vite's watcher now ignores the project data and the dev plugin watches it itself. Measured on a drag: Vite full reloads went from one per edit to zero, and the write receipt now reports |
vanceingalls
left a comment
There was a problem hiding this comment.
Approving. Rames's divergence concern addressed exactly and my hasAttribute?. nit picked up on the way through.
What the refactor does
packages/studio/src/components/editor/domEditOverlayTransform.ts (new, +88) hoists the ancestor walk into composeElementTransform<M>(element, ops, getStyle) parameterized over PlanarTransformOps<M> — an algebra with identity, fromTransform(value), fromRotate(deg), compose(outer, inner). Both callers pass their own M:
- Crop supplies
PLANAR_2D_OPS: PlanarTransformOps<{a,b,c,d}>— plain 2×2 components, drops translation (only the angle and scale are needed for the crop frame). - Geometry supplies
ops: PlanarTransformOps<DOMMatrix>— keeps the full matrix because the corner math transforms points.
Same walk, different algebra. The composition order (rotate on the left of transform within a node; ancestor on the left of child across nodes) and the data-composition-id stop are now single-sourced in one function.
Verified equivalence to the pre-refactor code
- Composition order preserved.
ops.compose(ops.fromRotate(spin), own)andops.compose(own, acc)atdomEditOverlayTransform.ts:82-84compose in the same order the previous per-file walks did (rotate-outside-transform within a node; ancestor-outside-child across). Concrete caserotate: 45; transform: translate(100px, 0)walks toR × T— matches CSS "individual properties beforetransform". data-composition-idstop. Extracted asCOMPOSITION_ROOT_ATTRand checked withnode.hasAttribute(COMPOSITION_ROOT_ATTR)— no optional chain (fixes my earlier nit; symmetric to the sibling file's convention).parseMatrixComponentsunchanged. Still guardsm[3]/m[7]/m[11]for perspective and projects{m[0], m[1], m[4], m[5]}for planarmatrix3d.- Failure paths.
ops.fromTransformreturning null aborts the walk in both algebras (planar-2D returns null on perspective; DOMMatrix throws on unparseable, caught at the outertryinreadElementTransformSnapshot).getStylereturning null also aborts, which is a slight strictening of the crop side's prior "undefined-style soft-through as identity" — but that path only fires on a detached document with nodefaultView, and axis-aligned fallback is the honest choice there. Not a behavior regression that would touch real Studio users.
Tests added — cover the exact case that was missing coverage
domEditOverlayCrop.test.ts — 3 new tests under "readElementCropFrame — the composed walk" using a nested(childTransform, parentTransform) fixture with a proper parentElement + hasAttribute returning true for data-composition-id:
- Parent 60° × child 30° → composed 90° (this is the "text layer inside rotated card" case from the PR body, now locked in automation).
- Child none + parent 30° → 30° (parent-only contribution).
- Composition-root stop marker present (the two tests are observationally equivalent under this fixture — same result would be produced by the walk terminating at the null grandparent — so this one only names intent rather than independently proving the stop; a grandparent with a distinct transform would prove it. Tiny nit, non-blocking; the geometry-side test picks up the rest.)
domEditOverlayGeometry.test.ts:167-176 — asserts orientedOverlayRect sees the composed angle when the parent has ROTATE_30DEG_MATRIX and the child has none. Symmetric to the crop-side coverage; confirms the two files' walks land the same answer.
CI at 384b1aa
All required + Preview parity + Perf lanes + Studio load smoke + Studio timeline viewport gate + Render/Tests on windows-latest → SUCCESS (Tests on windows-latest final run in progress at time of review; every prior run at that head is SUCCESS). MergeStateStatus BLOCKED remains approval-count only.
Outside this PR's scope (still open, still worth a follow-up ticket)
individualRotateDegrees is now importable from domEditOverlayTransform — the two same-bug sibling sites named in the R1 review can pick it up without going through the crop module:
packages/studio/src/components/editor/manualEditsDom.ts:222-238(stripGsapTranslateFromTransform— derives angle viaatan2(m.b, m.a)from.transformalone).packages/engine/src/services/threeDProjection.ts:766(readTransform— multiplies into view matrix without individual-transform composition).
Ready to merge from where I sit. Nicely done.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 384b1aac4 (delta from 83cb9f5e4).
Both R1 findings closed at the shape I asked for. One scope note on the R2 restack.
Concern (divergence risk) — closed. New domEditOverlayTransform.ts at packages/studio/src/components/editor/domEditOverlayTransform.ts:29-88 is exactly the one-walk-two-algebras factoring: PlanarTransformOps<M> interface with identity/fromTransform/fromRotate/compose, composeElementTransform<M> runs the ancestor walk against it, individualRotateDegrees co-located. The crop file's PLANAR_2D_OPS at domEditOverlayCrop.ts:198-211 and the geometry file's inline PlanarTransformOps<DOMMatrix> at domEditOverlayGeometry.ts:152-157 are the two consumers, and both share the composition-root stop, the "own is rotate composed on the left of transform" rule, and the "return null → abort walk → caller falls back" semantics. Adding a future individual transform property (translate, scale) now means one new step in composeElementTransform and one method per ops implementation — the divergence class this PR closed can't reappear silently.
Nit (composed-walk test) — closed. domEditOverlayCrop.test.ts:317-377 — "readElementCropFrame — the composed walk" — pins the exact "text layer inside a rotated card" case my nit named, with three specific arithmetic assertions:
adds the parent's rotation to the child's— 30° child inside 60° parent → 90° at the frame. This is the load-bearing invariant of the whole change.takes the parent's rotation when the child has none of its own— 30° parent, identity child → 30° at the frame.stops at the composition root rather than walking the whole document— the composition-root boundary is now tested behavior rather than a comment.
Geometry side gets its own composed test at domEditOverlayGeometry.test.ts:172-179 — composes the parent's rotation into the child's angle — so both algebras are locked against the same expected composition, and if one drifts the other test still catches it.
Concern — R2 scope
vite.config.ts + package.json + bun.lock changes are unrelated to the rotate-property fix and would land under a misleading title. The rewrite at vite.config.ts:145-190 replaces Vite's built-in watcher for data/projects/** with a chokidar watcher (new ^4.0.3 devDep), adds awaitWriteFinish: { stabilityThreshold: 40, pollInterval: 10 } so half-written composition files aren't announced, tells Vite's own watcher to ignore the same paths (server.watch.ignored: ["**/data/projects/**"]), and cleans up on server close. Comment at :213-219 names why: "That reload is the flash after every edit in the canvas, and it is not Studio's to make."
That's a real fix, and it's load-bearing for the preview-reload suppression contract — Vite's built-in HMR was full-reloading on composition writes and bypassing the write-token / self-write-echo suppression the client relies on to avoid the flash. But it's not the change the PR title advertises, and if HMR regresses in the future, bisect lands on a "read the rotate property" commit instead of a dev-server watcher commit. Consider splitting into a follow-up so the two behaviour changes have their own commit trails; if you prefer to keep them together, at least call the watcher change out in the PR body under a second "What" section so future spelunkers see it.
One fringe: realProjectPaths follows symlinks via lstatSync(full).isSymbolicLink() ? realpathSync(full) : full, so a project symlinked out of packages/studio/data/projects/ resolves to its target path. chokidar will watch the target (good), but the ignored: ["**/data/projects/**"] glob only masks the in-tree path from Vite's watcher — the resolved symlink target isn't matched. So on symlinked projects Vite's built-in HMR is still active and will still full-reload on their writes. Uncommon setup, but the guard is asymmetric with the intent.
What lands cleanly (R2 delta)
- Shared
COMPOSITION_ROOT_ATTR = "data-composition-id"constant in the transform module — one place to update if the boundary attribute ever changes. PlanarTransformOps<M>.fromTransformreturnsM | null, and the walk aborts (returns null) on the first unusable node — the crop'sparseMatrixComponentsperspective rejection still routes to the axis-aligned fallback.- Crop file's own
readPlanarTransformatdomEditOverlayCrop.ts:230-239now just callscomposeElementTransform(element, PLANAR_2D_OPS, (node) => …)— the walk logic is genuinely one place, not two implementations that happen to agree. - Geometry file's DOMMatrix ops define
identity/fromTransform/fromRotate/composein terms ofnew DOMMatrixCtor(),new DOMMatrixCtor(value),.rotateSelf,.multiply— clean adapter, no logic duplication. individualRotateDegreestest suite kept unchanged and moved with the function; imports re-point to the transform module.
Rotate fix is ready from my side; splitting the watcher change is a soft ask, not a block. Stamp routing per standing rule.
What
Overlay chrome now follows an element turned with Studio's rotate handle. Before, the selection box, crop outline and child outlines all drew square across a visibly rotated element.
Before / after
Same composition, same element.
mainon the left, this branch on the right.Selecting the card — the dashed crop outline sat square across a rotated element:
Selecting the text layer inside it — the outline has to follow the angle the layer paints under, which is its own spin composed with its parent's:
Why
The rotate handle writes the CSS
rotateproperty. That is an individual transform property, not part oftransform, sogetComputedStyle(el).transformreports nothing for it. Both places that measure an element's angle read onlytransform, so a rotated element measured as upright and every box derived from that measurement drew axis-aligned.Two more refusals sat behind it. The crop outline accepted only
matrix(...), and GSAP writesmatrix3d(...)for an ordinary 2D move or spin (force3D); a composition that mirrors an element writes one with a negative determinant, which was refused too. And it read the element's own transform rather than the one it paints under, so a layer inside a rotated card was drawn at its own angle instead of the combination.How
The overlay geometry and the crop frame both read
rotatealongsidetransformand compose them the way CSS does — individual properties beforetransform, an ancestor outside its child. The crop frame walks to the composition root instead of reading the element alone, and takes the same 2D projection the rest of the chrome reads through DOMMatrix. Only a perspective term still falls back to axis-aligned, because that is where the mapping stops being affine and no single angle describes it.Test plan
rotate: -22deg. Before: selection box 0, crop outline 0, both child outlines 0. After: all four report -22. Reverting the one-line read puts them back to 0.matrix3d, a flipped element, perspective still falling back)matrix3das its "3D means give up" fixture, and one stubbedgetComputedStyleto answer "rotated 30deg" for every node in the document, which made composing read the same turn once per ancestor