Repository navigation
fix(studio): plain elements keep their place and size when keyframed - #5045
Merged
Merged
Conversation
Edit accuracy: accurate 2055 (base branch 2040), smooth 1576 of thoseThe gate passes. Newly passing (15)
Quarantined, measured but not gated (0) |
miguel-heygen
force-pushed
the
fix/studio-plain-element-keyframes
branch
4 times, most recently
from
October 5, 2026 10:38
4d5a271 to
15cb22c
Compare
miguel-heygen
force-pushed
the
fix/studio-plain-element-keyframes
branch
4 times, most recently
from
October 5, 2026 12:35
7c60d90 to
a486fda
Compare
…a seek matches playback
…esize is one undo
…y placed tweens alone
…ges frames left alone
…script-only ones too
miguel-heygen
force-pushed
the
fix/studio-plain-element-keyframes
branch
from
October 5, 2026 13:02
a486fda to
4ad5721
Compare
miguel-heygen
force-pushed
the
fix/studio-plain-element-keyframes
branch
from
October 5, 2026 13:33
4ad5721 to
2273dbc
Compare
miguel-heygen
marked this pull request as ready for review
October 5, 2026 14:42
jrusso1020
approved these changes
Oct 5, 2026
jrusso1020
left a comment
Collaborator
There was a problem hiding this comment.
Approving at ebad31f4.
What I checked
- Tests: the changed unit tests pass three runs in a row (parsers 238, studio-server 113, studio 133), and
settle.test.mjspasses under bun (4/4). All 20 edit-accuracy shards are green at this head. - Trim vs real GSAP 3.15: keys
0%: 0, 50%: 100over 2 s, compared before and after the trim at 1.25 / 1.5 / 1.75 / 2 / 2.5 s, played through and on a direct seek. The values are identical, including under a timelinedefaults: { ease: "power2.out" }, which GSAP does not apply to the keyframe run. A tween with an explicitease: "power2.out"is left untrimmed, as intended. - Moves after a trimmed tween's end: a move after the last key now lands outside the tween, so I followed that path. It goes through the existing "outside the tween, add one there" branch (
buildExtendedKeyframes), so trimming does not strand later keys. - Mutation testing: 16 of 17 guards caught by the unit tests. Caught: lone-key hold scope, untouched-tween hold, size in holds, the untouched skip in the trim, the timeline regex,
placedAfter, loops, ease, unsized root, hostdata-end, draft clear, auto-record gate, frozen stamp, shown-restore saving, preview-change count, and lone-at-0 hold.
Non-blocking
- The one survivor is the headline fix. Replacing
readTranslatePxLeavingPercent(...)in Add keyframe with{ x: 0, y: 0 }passes every unit test; only theseqplainkeysbench catches it. A smalluseEnableKeyframestest (plain element withtranslate: 90px -40px→ first keyx: 90, y: -40) would pin it without a browser. - Can it be simpler: the PR has two notions of "the edit touched this tween".
holdScopecompares parsed JSON signatures;trimTrailingKeyframeSpanscompares call source text. They could share one, so holds and trims can't disagree on what was touched. - Reuse:
readTranslatePxLeavingPercentrepeats most ofreadTranslatePx. It could bereadTranslatePxwith a zero reference box. lengthIsTimelineparses every file in the root's closure on each keyframe save. That's fine at today's project sizes.- The CodeQL polynomial-regex alert on
PLACED_BY_TIMELINE.test(script)looks like a false positive. A 2.2 MB adversarial input runs in 2 ms, and the whole trim on an 864 KBtimeline({{-repeated script runs in 2 ms, so it is safe to dismiss.
— Rames
7 of 10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Turning a plain element into an animated one through auto-record and "Add keyframe" should leave it where it is, at the size you gave it, at every time.
translate. Add keyframe (toolbar button or K) wrote its first key asx: 0, y: 0, and GSAP takes the element'stranslateover once it animates x/y, so the box jumped back by its offset in the preview, after reload and in the render. The first key now starts from the offset the move left intranslate(px, read with the same length evaluator a move uses, socalc(-50% - 120px)gives -120; a percentage stays with xPercent, as GSAP reads it)."0%"keyframe by itself). The hold stays when that tween gains more keys. Rule: an edit never changes frames the user did not touch, so these holds are only added for a tween the edit touched (a split counts; moving a clip does not), and a tween the edit left alone keeps only the hold made from its own first key (an authored size tween keeps its own lead-in, even after a Studio key on the same element is deleted). Elements whose only keyframes fade them, or whose tweens are flat, keep plain CSS size, as before.0%: 440, 50%: 340over 2 s at 2 s show 390 on a direct seek to 3 s and 340 after playing through 2 s. Add keyframe makes a single-key tween that runs to the clip end, so every later key landed inside it. The server now trims the keyless tail of each tween a keyframe edit wrote or changed (duration scaled, keys rescaled; the same render once played, no new keyframe). Tweens the edit did not touch keep their bytes (a chained.to().to()link is compared by its own arguments). No trim when anything in the script loops (repeat:,yoyo:), sets the timeline's length or speed, or is placed relative to another tween's end (no position,>,+=,tl.add,tl.call,addLabel), since a shorter tween would move it; staticgsap.setcalls do not count. No trim either when a trim could end the film early: anywhere in a film whose root composition has nodata-duration(its length follows every timeline it plays), in a sub-composition whose host clip has neitherdata-durationnordata-end, or in a root no host mounts and that has nodata-duration. A host that gives the length keeps the fix.Bench
seqplainkeys(12 cases): move a plain element, turn on auto-record, Add keyframe, then move and resize at and between keyframes, then seek. Every step waits for its save and is scored against where it put the box; a seek also checks the size the size keys hold there (before the first key: that key's size). The moves stay inside the nested composition's frame, which clips the box in preview and render alike.seqresizedrag-tweenandseqresizeundodrag-tween, root and nested, settled per step: a resize on a flat tween saves its size and its anchor separately (one undo), which the unsettled stack model counted as two edits.Not in this PR
An edited tween can take over a size hold on its element that another tween made (for example an authored tween moved ahead of Studio's lone size key); frames before it change, as they already do on main.
Under the opt-in recast writer, moving a clip still counts as touching its tweens for holds; the default writer does not.
In a project whose root file is not
index.html, a sub-composition is judged by its own root rather than its host when deciding whether to trim.A size tween moved to start at 0 drops its hold (nothing to hold before it), and moving it later again does not bring the hold back.
A drag pressed while a resize's save is still pending: the resize's teardown clears the drag's own start stamp. Same family as the case below; fixed in the follow-up.
Add keyframe, then a move pressed before the new tween reaches the preview: the move is saved as plain CSS
translate, which the new keys then override (40.8 px off). It routes the gesture on the preview's old state and is not this writer; it is the follow-up's second red case.The keyframe writes behind the SDK cutover flags, and the split path, do not run the trim yet.
Resize, undo, then drag on a nested element (
seqresizeundodrag-tween-px-r0-nested-z100): after the undo settles, the drag does not move the box. It fails here as on main and stays out of the bank; it is the red case of the follow-up PR.Before
Move a plain element, turn on auto-record, Add keyframe, then resize at and between keyframes (repo fixture, edit bench case
seqplainkeys-none-px-r0-root-z100). On Add keyframe the box jumps back by the offset of the move, and the resizes change its size at every time.add-keyframe-before.webm
After
The same steps: the box stays where it was moved and keeps each resize at its keyframe.
add-keyframe-after.webm