From cbfb6ba943ea458bb79f89bbc0611cd81c817831 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Sat, 11 Jul 2026 04:07:57 -0400 Subject: [PATCH] fix(studio): crop tool stands down for clips it cannot edit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The always-on crop lifted EVERY selected element's clip-path and restored only what it could parse as a px inset — selecting an element with a circle/polygon/percentage clip visually un-clipped it, and deselecting deleted the authored clip outright. readElementCropInsets is now tri-state (zeros = no clip, null = a clip the tool can't represent): uneditable clips get no lift and no handles, and the lift restores the pre-lift inline value verbatim unless a crop gesture actually committed. --- .../components/editor/DomEditCropHandles.tsx | 65 ++++++++++--------- .../editor/domEditOverlayCrop.test.ts | 47 ++++++++++++++ .../components/editor/domEditOverlayCrop.ts | 22 +++++-- 3 files changed, 98 insertions(+), 36 deletions(-) diff --git a/packages/studio/src/components/editor/DomEditCropHandles.tsx b/packages/studio/src/components/editor/DomEditCropHandles.tsx index 9f83a90ba..746c774f9 100644 --- a/packages/studio/src/components/editor/DomEditCropHandles.tsx +++ b/packages/studio/src/components/editor/DomEditCropHandles.tsx @@ -77,36 +77,23 @@ export function DomEditCropHandles({ }: DomEditCropHandlesProps) { const gestureRef = useRef(null); const [dragging, setDragging] = useState(false); - const [state, setState] = useState(() => { - const parsed = readElementCropInsets(selection.element); - return { - element: selection.element, - insets: { - top: parsed.top, - right: parsed.right, - bottom: parsed.bottom, - left: parsed.left, - } as ClipPathInsetSides, - radius: parsed.radius, - }; - }); + // readElementCropInsets returns null for a clip this tool can't represent + // (circle/polygon/non-px inset): the crop UI must fully stand down for that + // element — no lift, no handles — or select+deselect replaces the authored + // clip with an inset (or deletes it). + const cropStateFor = (element: HTMLElement) => { + const parsed = readElementCropInsets(element); + const { radius, ...insets } = parsed ?? { top: 0, right: 0, bottom: 0, left: 0, radius: 0 }; + return { element, croppable: parsed !== null, insets: insets as ClipPathInsetSides, radius }; + }; + const [state, setState] = useState(() => cropStateFor(selection.element)); // Re-sync when the selection targets a different element (reselect, or an // undo/redo that re-keys the node): read its committed crop before the lift // effect runs. Read inside the guard so a drag's per-frame setState doesn't // re-run getComputedStyle every frame. if (state.element !== selection.element) { - const liveInsets = readElementCropInsets(selection.element); - setState({ - element: selection.element, - insets: { - top: liveInsets.top, - right: liveInsets.right, - bottom: liveInsets.bottom, - left: liveInsets.left, - }, - radius: liveInsets.radius, - }); + setState(cropStateFor(selection.element)); } const hasCrop = @@ -120,17 +107,28 @@ export function DomEditCropHandles({ committedRef.current = hasCrop ? buildInsetClipPathSides(state.insets, state.radius) : null; // Lift the clip while the element is selected so the full content shows and the - // cropped-away area can be dimmed; restore the committed crop on deselect. Keyed - // on the element so switching selections restores the previous one. Runs after - // render, so the state re-sync above still reads the element's real committed clip. + // cropped-away area can be dimmed; restore on deselect. Keyed on the element so + // switching selections restores the previous one. Runs after render, so the + // state re-sync above still reads the element's real committed clip. Restore + // prefers the pre-lift inline value VERBATIM — the rebuilt inset only replaces + // it after a crop gesture actually commits, so a mere select+deselect can + // never reformat (or drop) what the author wrote. const liftedRef = useRef(false); + const preLiftInlineClipRef = useRef(""); + const cropCommittedRef = useRef(false); useEffect(() => { const el = selection.element; + if (readElementCropInsets(el) === null) return; + preLiftInlineClipRef.current = el.style.getPropertyValue("clip-path"); + cropCommittedRef.current = false; el.style.setProperty("clip-path", "none"); liftedRef.current = true; return () => { liftedRef.current = false; - if (committedRef.current) el.style.setProperty("clip-path", committedRef.current); + const restore = cropCommittedRef.current + ? committedRef.current + : preLiftInlineClipRef.current || null; + if (restore) el.style.setProperty("clip-path", restore); else el.style.removeProperty("clip-path"); }; }, [selection.element]); @@ -198,7 +196,12 @@ export function DomEditCropHandles({ }; void Promise.resolve( onStyleCommit?.("clip-path", buildInsetClipPathSides(state.insets, state.radius)), - ).then(reLift, reLift); + ).then(() => { + // Only a landed commit makes the rebuilt inset the restore value; a + // failed one keeps restoring the pre-lift clip. + cropCommittedRef.current = true; + reLift(); + }, reLift); }; const cancelCropGesture = (event: ReactPointerEvent) => { @@ -212,6 +215,10 @@ export function DomEditCropHandles({ setState((prev) => ({ ...prev, insets: gesture.startInsets })); }; + // Uneditable clip (circle/polygon/non-px inset): the element renders exactly + // as authored and the crop tool shows nothing. All hooks above stay mounted. + if (!state.croppable) return null; + return ( <> {/* Dim the cropped-away area whenever the element is cropped and selected, diff --git a/packages/studio/src/components/editor/domEditOverlayCrop.test.ts b/packages/studio/src/components/editor/domEditOverlayCrop.test.ts index 2dd54f357..a23abc0e7 100644 --- a/packages/studio/src/components/editor/domEditOverlayCrop.test.ts +++ b/packages/studio/src/components/editor/domEditOverlayCrop.test.ts @@ -1,6 +1,8 @@ import { describe, expect, it } from "vitest"; import { cropRectFromInsets, + hugRectForElement, + readElementCropInsets, resolveCropInsetFromEdgeDrag, resolveCropInsetFromMoveDrag, } from "./domEditOverlayCrop"; @@ -104,3 +106,48 @@ describe("cropRectFromInsets", () => { expect(r.height).toBe(0); }); }); + +describe("readElementCropInsets tri-state", () => { + // Regression: a clip-path the crop tool can't represent (circle/polygon/ + // non-px inset) used to parse to ZEROS — indistinguishable from "no crop" — + // so selecting lifted the clip and deselecting removed/replaced it: the + // authored circle clip was silently destroyed by a mere select+deselect. + const fakeEl = (inlineClip: string) => + ({ + style: { getPropertyValue: (p: string) => (p === "clip-path" ? inlineClip : "") }, + ownerDocument: { defaultView: { getComputedStyle: () => ({ clipPath: "none" }) } }, + }) as unknown as HTMLElement; + + it("zeros for no clip", () => { + expect(readElementCropInsets(fakeEl(""))).toEqual({ + top: 0, + right: 0, + bottom: 0, + left: 0, + radius: 0, + }); + }); + + it("parses a px inset", () => { + expect(readElementCropInsets(fakeEl("inset(16px round 12px)"))).toEqual({ + top: 16, + right: 16, + bottom: 16, + left: 16, + radius: 12, + }); + }); + + it("null for a circle clip (uneditable, must not be lifted)", () => { + expect(readElementCropInsets(fakeEl("circle(50% at 50% 50%)"))).toBeNull(); + }); + + it("null for a non-px inset (uneditable, must not be lifted)", () => { + expect(readElementCropInsets(fakeEl("inset(10%)"))).toBeNull(); + }); + + it("hugRectForElement passes the rect through for uneditable clips", () => { + const rect = { left: 1, top: 2, width: 30, height: 40, editScaleX: 1, editScaleY: 1 }; + expect(hugRectForElement(rect, fakeEl("circle(50%)"))).toEqual(rect); + }); +}); diff --git a/packages/studio/src/components/editor/domEditOverlayCrop.ts b/packages/studio/src/components/editor/domEditOverlayCrop.ts index 7eba37ec4..4f0e7dd7f 100644 --- a/packages/studio/src/components/editor/domEditOverlayCrop.ts +++ b/packages/studio/src/components/editor/domEditOverlayCrop.ts @@ -28,15 +28,21 @@ export function cropRectFromInsets( }; } -/** Current inset crop of an element (inline first, computed fallback), or zeros. */ -export function readElementCropInsets(element: HTMLElement): ClipPathInsetSides & { - radius: number; -} { +/** + * Current inset crop of an element (inline first, computed fallback). + * Zeros = no clip (croppable, nothing cropped yet). `null` = the element + * carries a clip-path this tool cannot represent (circle/polygon/non-px + * inset) — croppers must not lift, edit, or restore it, or the clip gets + * silently replaced or destroyed on deselect. + */ +export function readElementCropInsets( + element: HTMLElement, +): (ClipPathInsetSides & { radius: number }) | null { const inline = element.style.getPropertyValue("clip-path").trim(); const value = inline || element.ownerDocument.defaultView?.getComputedStyle(element).clipPath.trim() || ""; - const parsed = parseInsetClipPathSides(value === "none" ? "" : value); - return parsed ?? { top: 0, right: 0, bottom: 0, left: 0, radius: 0 }; + if (!value || value === "none") return { top: 0, right: 0, bottom: 0, left: 0, radius: 0 }; + return parseInsetClipPathSides(value); } export interface CropInsetDragInput { @@ -111,6 +117,8 @@ export function hugRectForElement( element: HTMLElement, ): CropScreenRect { const insets = readElementCropInsets(element); - if (insets.top <= 0 && insets.right <= 0 && insets.bottom <= 0 && insets.left <= 0) return rect; + // Uneditable clip (null) can't be hugged — show the full element rect. + if (!insets || (insets.top <= 0 && insets.right <= 0 && insets.bottom <= 0 && insets.left <= 0)) + return rect; return cropRectFromInsets(rect, insets, rect.editScaleX, rect.editScaleY); }