feat: remove bound-arrows from frames (#7157)

dev-docs/docs/codebase/frames.mdx

@@ -20,3 +20,13 @@ Frames should be ordered where frame children come first, followed by the frame
 ```
 
 If not oredered correctly, the editor will still function, but the elements may not be rendered and clipped correctly. Further, the renderer relies on this ordering for performance optimizations.
+
+# Arrows
+
+An arrow can be a child of a frame only if it has no binding (either start or end) to any other element, regardless of whether the bound element is inside the frame or not.
+
+This ensures that when an arrow is bound to an element outside the frame, it's rendered and behaves correctly.
+
+Therefore, when an arrow (that's a child of a frame) gets bound to an element, it's automatically removed from the frame.
+
+Bound-arrow is duplicated alongside a frame only if the arrow start is bound to an element within that frame.

src/actions/actionDuplicateSelection.tsx

@@ -155,7 +155,12 @@ const duplicateElements = (
             groupId,
           ).flatMap((element) =>
             isFrameElement(element)
-              ? [...getFrameElements(elements, element.id), element]
+              ? [
+                  ...getFrameElements(elements, element.id, {
+                    includeBoundArrows: true,
+                  }),
+                  element,
+                ]
               : [element],
           );
 
@@ -181,7 +186,9 @@ const duplicateElements = (
           continue;
         }
         if (isElementAFrame) {
-          const elementsInFrame = getFrameElements(sortedElements, element.id);
+          const elementsInFrame = getFrameElements(sortedElements, element.id, {
+            includeBoundArrows: true,
+          });
 
           elementsWithClones.push(
             ...markAsProcessed([

src/components/App.tsx

@@ -2431,18 +2431,12 @@ class App extends React.Component<AppProps, AppState> {
 
         const lineHeight = getDefaultLineHeight(textElementProps.fontFamily);
         if (text.length) {
-          const topLayerFrame = this.getTopLayerFrameAtSceneCoords({
-            x,
-            y: currentY,
-          });
-
           const element = newTextElement({
             ...textElementProps,
             x,
             y: currentY,
             text,
             lineHeight,
-            frameId: topLayerFrame ? topLayerFrame.id : null,
           });
           acc.push(element);
           currentY += element.height + LINE_GAP;
@@ -3456,6 +3450,11 @@ class App extends React.Component<AppProps, AppState> {
     return getElementsAtPosition(elements, (element) =>
       hitTest(element, this.state, this.frameNameBoundsCache, x, y),
     ).filter((element) => {
+      // arrows don't clip even if they're children of frames,
+      // so always allow hitbox regardless of beinging contained in frame
+      if (isArrowElement(element)) {
+        return true;
+      }
       // hitting a frame's element from outside the frame is not considered a hit
       const containingFrame = getContainingFrame(element);
       return containingFrame &&

src/data/restore.ts

@@ -43,6 +43,7 @@ import {
   measureBaseline,
 } from "../element/textElement";
 import { normalizeLink } from "./url";
+import { isValidFrameChild } from "../frame";
 
 type RestoredAppState = Omit<
   AppState,
@@ -396,7 +397,7 @@ const repairBoundElement = (
 };
 
 /**
- * Remove an element's frameId if its containing frame is non-existent
+ * resets `frameId` if no longer applicable.
  *
  * NOTE mutates elements.
  */
@@ -404,12 +405,16 @@ const repairFrameMembership = (
   element: Mutable<ExcalidrawElement>,
   elementsMap: Map<string, Mutable<ExcalidrawElement>>,
 ) => {
-  if (element.frameId) {
-    const containingFrame = elementsMap.get(element.frameId);
+  if (!element.frameId) {
+    return;
+  }
 
-    if (!containingFrame) {
-      element.frameId = null;
-    }
+  if (
+    !isValidFrameChild(element) ||
+    // target frame not exists
+    !elementsMap.get(element.frameId)
+  ) {
+    element.frameId = null;
   }
 };
 
@@ -453,6 +458,8 @@ export const restoreElements = (
   // repair binding. Mutates elements.
   const restoredElementsMap = arrayToMap(restoredElements);
   for (const element of restoredElements) {
+    // repair frame membership *after* bindings we do in restoreElement()
+    // since we rely on bindings to be correct
     if (element.frameId) {
       repairFrameMembership(element, restoredElementsMap);
     }

src/element/binding.ts

@@ -27,6 +27,7 @@ import { LinearElementEditor } from "./linearElementEditor";
 import { arrayToMap, tupleToCoors } from "../utils";
 import { KEYS } from "../keys";
 import { getBoundTextElement, handleBindTextResize } from "./textElement";
+import { isValidFrameChild } from "../frame";
 
 export type SuggestedBinding =
   | NonDeleted<ExcalidrawBindableElement>
@@ -211,6 +212,15 @@ export const bindLinearElement = (
       }),
     });
   }
+  if (linearElement.frameId && !isValidFrameChild(linearElement)) {
+    mutateElement(
+      linearElement,
+      {
+        frameId: null,
+      },
+      false,
+    );
+  }
 };
 
 // Don't bind both ends of a simple segment

src/frame.ts

@@ -323,7 +323,24 @@ export const groupByFrames = (elements: readonly ExcalidrawElement[]) => {
 export const getFrameElements = (
   allElements: ExcalidrawElementsIncludingDeleted,
   frameId: string,
-) => allElements.filter((element) => element.frameId === frameId);
+  opts?: { includeBoundArrows?: boolean },
+) => {
+  return allElements.filter((element) => {
+    if (element.frameId === frameId) {
+      return true;
+    }
+    if (opts?.includeBoundArrows && element.type === "arrow") {
+      const bindingId = element.startBinding?.elementId;
+      if (bindingId) {
+        const boundElement = Scene.getScene(element)?.getElement(bindingId);
+        if (boundElement?.frameId === frameId) {
+          return true;
+        }
+      }
+    }
+    return false;
+  });
+};
 
 export const getElementsInResizingFrame = (
   allElements: ExcalidrawElementsIncludingDeleted,
@@ -451,6 +468,14 @@ export const getContainingFrame = (
   return null;
 };
 
+export const isValidFrameChild = (element: ExcalidrawElement) => {
+  return (
+    element.type !== "frame" &&
+    // arrows that are bound to elements cannot be frame children
+    (element.type !== "arrow" || (!element.startBinding && !element.endBinding))
+  );
+};
+
 // --------------------------- Frame Operations -------------------------------
 
 /**
@@ -489,6 +514,9 @@ export const addElementsToFrame = (
     elementsToAdd,
   )) {
     if (!currTargetFrameChildrenMap.has(element.id)) {
+      if (!isValidFrameChild(element)) {
+        continue;
+      }
       finalElementsToAdd.push(element);
     }
 

src/renderer/renderScene.ts

@@ -69,6 +69,7 @@ import {
 } from "../element/Hyperlink";
 import { renderSnaps } from "./renderSnaps";
 import {
+  isArrowElement,
   isEmbeddableElement,
   isFrameElement,
   isLinearElement,
@@ -984,7 +985,10 @@ const _renderStaticScene = ({
 
           // TODO do we need to check isElementInFrame here?
           if (frame && isElementInFrame(element, elements, appState)) {
-            frameClip(frame, context, renderConfig, appState);
+            // do not clip arrows
+            if (!isArrowElement(element)) {
+              frameClip(frame, context, renderConfig, appState);
+            }
           }
           renderElement(element, rc, context, renderConfig, appState);
           context.restore();