fix(studio): resizing a cropped element keeps the same part of it in view - #4743
Conversation
…agging and once saved
080dfec to
48ee967
Compare
…first after a soft reload
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 4175ab48. I read the diff against the full files. I did not run the tests.
What checks out
- One source of truth for the crop.
liftElementCrop(domEditOverlayCrop.ts:47) hides the crop with aclip-path:none!importantrule keyed on the element. The element's inlineclip-pathis no longer touched while it is selected. That removes the whole class of stale-ref bugs the oldpreLiftInlineClipRef/committedClipRefpair had with undo and redo. Deselect just removes the rule (DomEditCropHandles.tsx:140). readElementClipPathwhile lifted takes the rule out, reads the stylesheet crop once, and puts the rule back where it was (domEditOverlayCrop.ts:76-79). Both the hover hug anduseCropOverlayskip a lifted element, so the selection box keeps the full box as before.- Every size write route stages the crop.
- CSS route:
useDomGeometryCommits.ts:107-128. The patch goes into the same commit as the size, andcrop?.revert()runs on failure. - GSAP route:
useGsapAwareEditing.ts:358,388,432-437. The crop is saved only after the script writes land, under the resize'scoalesceKey. The DOM fallback leavescropUndoKeynull, so the crop is not scaled twice. - Animated property route:
writeSizeWithCrop(cropResize.ts:125-142) keys both the single and the batched mutation to one undo step.commitAnimatedPropertywas already a one-prop wrapper aroundcommitAnimatedProperties(useAnimatedPropertyCommit.ts:589), so routing it through the wrapped version changes nothing else.
- CSS route:
prepareCropResizemeasures the end box instead of trusting the request (cropResize.ts:71), and returns null when the inset string is unchanged. So a resize saved as ascalereally does leave the crop alone, as the PR body says. A crop edited mid-save (untouched) and a GSAP-drivenclipPathare skipped too.- The draft base is cleared on every exit:
stage(),restoreStudioBoxSizeandclearStudioBoxSize(manualEditsSnapshot.ts:118,295). So a failed or cancelled resize does not leave a stale base for the next one. - The
hasNonHoldTweenchange is a real fix for the existing preflights too. Before, it read only the first registered timeline, so after a soft reload re-added main last, main's tweens were missed. The resize, position and rotation preflights (gsapResizePreflight.ts:46,gsapRuntimeBridge.ts:190,312,397) all benefit. The{ width: [...] }keyframe form is matched by key, and the object form's keys are percentages, so there is no false positive there.gsapRuntimeKeyframes.test.ts:170pins the later-timeline case.
Nits (not blocking)
- Toast and telemetry say "position" for a failed crop save. The crop patch goes through
commitPositionPatchToHtml, so a failed crop save toasts "Failed to save position" and recordsmutationType: "position"(useDomEditPositionPatchCommit.ts:44,50). This is the same path the anchor move uses, so it is consistent, but the wording is slightly off for a crop. - Cost of
readCropFollowingResizewhile dragging. It runs on every render ofDomEditCropHandles, and while a resize draft is livecropFollowswalks every timeline's children twice (cropResize.ts:26-27). It is fine at normal sizes. If a large composition ever shows resize-drag jank, this is the first thing to memoise.
CI
The one red job was attempt 1 of run 36727661720, "Studio: timeline viewport gate". The default arm passed 5/5 at about 33 ms. The virtualization-off control arm (1000 mounted rows) passed 3 of 5 measured runs, and it needs 4. The two misses were interaction p95 80.8 ms and 82.7 ms against the 75 ms budget. The rerun (attempt 2) passed the off arm 5/5 at about 47-49 ms, and the job is green at this head. The "Test" failure in that run was the studio shard being cancelled, not a test failure. Nothing in this diff touches the timeline rows, so this reads as runner timing noise on the unvirtualized arm.
Verdict: APPROVE
Reasoning: Every size write route now scales the crop in the same undo step and puts it back on failure, and moving the crop's source of truth onto the element's own clip-path removes the stale-crop bugs instead of patching them. The only remaining points are wording and performance nits.
— Rames Jusso
jrusso1020
left a comment
There was a problem hiding this comment.
Re-approving at f75c3f19. My earlier approval was at 4175ab48.
This head is only a merge of main, with parents 4175ab48 and 5b79038c. I checked that it adds nothing else:
5b79038cis on main.git diff 5b79038c f75c3f19has the same stable patch-id (c43a95b6) as the patch I reviewed (git diff ae8d1780 4175ab48). Both show 23 files, +838/-209. So the merge resolved nothing by hand and the PR's own change is byte-identical.- The commits it brings in from main (#4784, #4770, #4769, #4760, #4782, #4759, #4775, #4756, #4768) touch none of the PR's 23 files. They add no calls to what this PR changes:
hasNonHoldTweenForElement, the crop readers,commitAnimatedProperties,applyStudioBoxSizeDraft,handleDomBoxSizeCommit,commitPositionPatchToHtml, or anyclip-pathwrites.
My earlier findings and the two non-blocking nits still apply as written.
Verdict: APPROVE
Reasoning: The new head is a clean merge of main with an identical PR patch, and nothing on main newly interacts with the code this PR changes.
— Rames Jusso
What
Resizing a cropped element now scales its crop with it, so the same part of the element stays in view, just larger or smaller. Before, the crop kept its pixel insets, so growing a cropped element showed more of it than was cropped (a 300 px wide element cropped to 81% showed 88% after being resized to 473 px).
This holds for every size write:
handleGsapAwareBoxSizeCommit, with its CSS fallback): a corner drag, the design panel's W/H fields on an element with no animation, the agent resize tool, and a host's geometry commit.commitAnimatedProperties): the W/H fields on an element that has any GSAP animation, and any other width/height edit made through the animated property path.Both routes use the same pair,
prepareCropResizethensaveCropResize, from one module.Why
A crop is
clip-path: inset(...)in pixels, measured against the element's box. Nothing on the resize path touched it, so every resize silently changed what the crop showed. Design tools treat a crop as part of the element: resizing it scales the crop too.Related work
Rebased onto #4745 and #4730. The crop release keeps #4745's rule that a drag ending where it started saves nothing (it compares against the press-time insets). This PR adds two things to it: the press-time insets are read from the element when you press, and the radius rides on the gesture.
How
clip-path. While an element is selected, the crop UI shows it uncropped so it can dim what is cut away. That used to be done by writingclip-path: noneonto the element and keeping the real crop in component refs. Undo, redo and panel edits rewrite the element's style in place, so that held copy went stale.clip-path: none !importantrule keyed on the element'sdata-hf-id(orid), added while it is selected. The element's inlineclip-pathstays the one record of the crop, and everything reads it.clip-pathpatch rides in the same patch commit as the size.scaleleaves the crop alone; it already scales with the transform.clip-path(a percent crop already scales with the box), or when the crop was edited while the resize was still saving; that edit was drawn in the new box already. An inline!importantcrop keeps its priority.set) does not count as animated. The drag preview and the save ask the same question, through the existing live-tween check (hasNonHoldTweenForElement), which now also takes an element and understands thekeyframes: { width: [...] }form. The answer is taken before the write lands, because a W edit can add a width keyframe of its own.Known limits
clip-pathis!importantis not shown uncropped while selected, because an inline important declaration beats the lift rule. Its crop still scales.Test plan
{ width: [...] }keyframes) keeps them; an instantsetstill scales.!important, a crop edited during the save, and a tweened crop.scaleresize leaving the crop alone, and the lift never rewriting the element's clip.Walks: headless Chrome, edit fixture,
#medium300x200. "Share" is the visible width fraction.inset(0 57.75px 0 0)inset(0 91.05px 0 0)inset(0 86.63px 0 0); same after reloadinset(0 115.5px 0 0), share 0.8075, in the file and the previewinset(0 115.5px 0 0), share 0.8075; one undo puts back both the width and 57.75pxinset(0 0 115.5px 0), vertical share 0.7113clip-path: … !important, corner resizeinset(0px 94.6px 0px 0px) !importantclip-pathtween, corner resize#cropped400x300)inset(28.33px 42.45px 56.67px 70.75px)at 566x425, share 0.8inset(0 120px 0 0), share 0.8 at t=0, 2 and 4 after reloadkeyframes: { width: [300, 600] }, 60 px cropscale(element with a scale tween)Before
On main, the right edge of the red box is cropped, then a corner resize makes it larger. Mid-drag, the dimmed cropped strip stays thin while the box grows:
After deselecting, the crop kept its old pixel width, so more of the box shows than before the resize:
After
With the fix, the dimmed strip grows with the box during the drag:
After deselecting, the crop scaled with the box, so the same part of it stays in view: