diff --git a/.codesandbox/Dockerfile b/.codesandbox/Dockerfile index fd5b38d1e8..ce8c857650 100644 --- a/.codesandbox/Dockerfile +++ b/.codesandbox/Dockerfile @@ -1,4 +1,4 @@ -FROM node:18-bullseye +FROM node:24-bullseye # Vite wants to open the browser using `open`, so we # need to install those utils. diff --git a/.github/workflows/semantic-pr-title.yml b/.github/workflows/semantic-pr-title.yml index 8bfc1327b4..0dc2c879f4 100644 --- a/.github/workflows/semantic-pr-title.yml +++ b/.github/workflows/semantic-pr-title.yml @@ -6,11 +6,97 @@ on: - opened - edited - synchronize + - labeled + - unlabeled jobs: semantic: runs-on: ubuntu-latest + permissions: + pull-requests: read steps: - uses: amannn/action-semantic-pull-request@e32d7e603df1aa1ba07e981f2a23455dee596825 # v5 + with: + requireScope: true + scopes: | + app + editor + packages/excalidraw + packages/utils + docker + repo + ignoreLabels: | + skip-semantic-title env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + label-scope: + needs: semantic + if: github.event.pull_request.head.repo.full_name == github.repository + runs-on: ubuntu-latest + permissions: + issues: write + pull-requests: write + steps: + - name: Label scoped PR + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_TITLE: ${{ github.event.pull_request.title }} + REPOSITORY: ${{ github.repository }} + run: | + set -euo pipefail + + scope_labels=(s-app s-editor s-package) + + readarray -t desired_labels < <( + node <<'NODE' + const title = process.env.PR_TITLE; + const match = title.match(/^[a-z]+(?:\(([^)]+)\))?!?:/i); + const scopes = match?.[1]?.split(",").map((scope) => scope.trim()) ?? []; + const labels = new Set(); + + for (const scope of scopes) { + if (scope === "app") { + labels.add("s-app"); + } else if (scope === "editor") { + labels.add("s-editor"); + } else if (scope.startsWith("packages/")) { + labels.add("s-package"); + } + } + + process.stdout.write([...labels].join("\n")); + NODE + ) + + should_apply_label() { + local label="$1" + + for desired_label in "${desired_labels[@]}"; do + if [[ "$desired_label" == "$label" ]]; then + return 0 + fi + done + + return 1 + } + + for label in "${scope_labels[@]}"; do + if ! should_apply_label "$label"; then + gh api \ + --method DELETE \ + "repos/${REPOSITORY}/issues/${PR_NUMBER}/labels/${label}" \ + --silent 2>/dev/null || true + fi + done + + for label in "${desired_labels[@]}"; do + if ! gh api \ + --method POST \ + "repos/${REPOSITORY}/issues/${PR_NUMBER}/labels" \ + --field "labels[]=${label}" \ + --silent; then + echo "::warning::Could not apply ${label}. The workflow token likely does not have issues:write permission for this PR." + fi + done diff --git a/Dockerfile b/Dockerfile index c08385d654..e15b425704 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,4 @@ -FROM --platform=${BUILDPLATFORM} node:18 AS build +FROM --platform=${BUILDPLATFORM} node:24 AS build WORKDIR /opt/node_app @@ -13,7 +13,7 @@ ARG NODE_ENV=production RUN npm_config_target_arch=${TARGETARCH} yarn build:app:docker -FROM --platform=${TARGETPLATFORM} nginx:1.27-alpine +FROM nginx:1.27-alpine COPY --from=build /opt/node_app/excalidraw-app/build /usr/share/nginx/html diff --git a/excalidraw-app/data/LocalData.ts b/excalidraw-app/data/LocalData.ts index 634fdc4f1c..d09d8aa88e 100644 --- a/excalidraw-app/data/LocalData.ts +++ b/excalidraw-app/data/LocalData.ts @@ -26,7 +26,6 @@ import { get, } from "idb-keyval"; -import { appJotaiStore, atom } from "excalidraw-app/app-jotai"; import { getNonDeletedElements } from "@excalidraw/element"; import type { LibraryPersistedData } from "@excalidraw/excalidraw/data/library"; @@ -39,6 +38,7 @@ import type { } from "@excalidraw/excalidraw/types"; import type { MaybePromise } from "@excalidraw/common/utility-types"; +import { appJotaiStore, atom } from "../app-jotai"; import { SAVE_TO_LOCAL_STORAGE_TIMEOUT, STORAGE_KEYS } from "../app_constants"; import { FileManager } from "./FileManager"; diff --git a/excalidraw-app/vite.config.mts b/excalidraw-app/vite.config.mts index fa8d63d956..dfb213ef3a 100644 --- a/excalidraw-app/vite.config.mts +++ b/excalidraw-app/vite.config.mts @@ -75,6 +75,13 @@ export default defineConfig(({ mode }) => { find: /^@excalidraw\/utils\/(.*?)/, replacement: path.resolve(__dirname, "../packages/utils/src/$1"), }, + { + find: /^@excalidraw\/fractional-indexing$/, + replacement: path.resolve( + __dirname, + "../packages/fractional-indexing/src/index.ts", + ), + }, ], }, build: { diff --git a/package.json b/package.json index 65ff221a49..df5c4e8a79 100644 --- a/package.json +++ b/package.json @@ -56,7 +56,8 @@ "build:element": "yarn --cwd ./packages/element build:esm", "build:excalidraw": "yarn --cwd ./packages/excalidraw build:esm", "build:math": "yarn --cwd ./packages/math build:esm", - "build:packages": "yarn build:common && yarn build:math && yarn build:element && yarn build:excalidraw", + "build:fractional-indexing": "yarn --cwd ./packages/fractional-indexing build:esm", + "build:packages": "yarn build:common && yarn build:fractional-indexing && yarn build:math && yarn build:element && yarn build:excalidraw", "build:version": "yarn --cwd ./excalidraw-app build:version", "build": "yarn --cwd ./excalidraw-app build", "build:preview": "yarn --cwd ./excalidraw-app build:preview", diff --git a/packages/common/src/constants.ts b/packages/common/src/constants.ts index 4ff50335ef..0e94df5af6 100644 --- a/packages/common/src/constants.ts +++ b/packages/common/src/constants.ts @@ -337,9 +337,10 @@ export const MAX_DECIMALS_FOR_SVG_EXPORT = 2; export const EXPORT_SCALES = [1, 2, 3]; export const DEFAULT_EXPORT_PADDING = 10; // px -export const DEFAULT_MAX_IMAGE_WIDTH_OR_HEIGHT = 1440; - -export const MAX_ALLOWED_FILE_BYTES = 4 * 1024 * 1024; +export const DEFAULT_IMAGE_OPTIONS: AppProps["imageOptions"] = { + maxWidthOrHeight: 1440, + maxFileSizeBytes: 4 * 1024 * 1024, +}; export const SVG_NS = "http://www.w3.org/2000/svg"; export const SVG_DOCUMENT_PREAMBLE = ` diff --git a/packages/element/package.json b/packages/element/package.json index 7dff00a750..408f2c9d10 100644 --- a/packages/element/package.json +++ b/packages/element/package.json @@ -64,6 +64,7 @@ }, "dependencies": { "@excalidraw/common": "0.18.0", - "@excalidraw/math": "0.18.0" + "@excalidraw/math": "0.18.0", + "@excalidraw/fractional-indexing": "3.3.0" } } diff --git a/packages/element/src/Scene.ts b/packages/element/src/Scene.ts index 4ba663ceba..35b0cf4b95 100644 --- a/packages/element/src/Scene.ts +++ b/packages/element/src/Scene.ts @@ -338,29 +338,20 @@ export class Scene { this.callbacks.clear(); } - insertElementAtIndex(element: ExcalidrawElement, index: number) { - if (!Number.isFinite(index) || index < 0) { - throw new Error( - "insertElementAtIndex can only be called with index >= 0", - ); - } - - const nextElements = [ - ...this.elements.slice(0, index), - element, - ...this.elements.slice(index), - ]; - - syncMovedIndices(nextElements, arrayToMap([element])); - - this.replaceAllElements(nextElements); - } - - insertElementsAtIndex(elements: ExcalidrawElement[], index: number) { + /** low-level - generally use app.insertNewElements() */ + insertElementsAtIndex( + elements: ExcalidrawElement[], + /** null indicates end of the array */ + index: number | null, + ) { if (!elements.length) { return; } + if (index === null) { + index = this.elements.length; + } + if (!Number.isFinite(index) || index < 0) { throw new Error( "insertElementAtIndex can only be called with index >= 0", @@ -378,24 +369,9 @@ export class Scene { this.replaceAllElements(nextElements); } + /** low-level - generally use app.insertNewElement() */ insertElement = (element: ExcalidrawElement) => { - const index = element.frameId - ? this.getElementIndex(element.frameId) - : this.elements.length; - - this.insertElementAtIndex(element, index); - }; - - insertElements = (elements: ExcalidrawElement[]) => { - if (!elements.length) { - return; - } - - const index = elements[0]?.frameId - ? this.getElementIndex(elements[0].frameId) - : this.elements.length; - - this.insertElementsAtIndex(elements, index); + this.insertElementsAtIndex([element], null); }; getElementIndex(elementId: string) { diff --git a/packages/element/src/binding.ts b/packages/element/src/binding.ts index 566ef3c4e4..e21542ebb8 100644 --- a/packages/element/src/binding.ts +++ b/packages/element/src/binding.ts @@ -734,12 +734,11 @@ const getBindingStrategyForDraggingBindingElementEndpoints_simple = ( }); // Handle outside-outside binding to the same element - if (otherBinding && otherBinding.elementId === hit?.id) { - invariant( - !opts?.newArrow || appState.selectedLinearElement?.initialState.origin, - "appState.selectedLinearElement.initialState.origin must be defined for new arrows", - ); - + if ( + otherBinding && + otherBinding.elementId === hit?.id && + (!opts?.newArrow || appState.selectedLinearElement?.initialState.origin) + ) { return { start: { mode: "inside", @@ -1943,9 +1942,9 @@ export const calculateFixedPointForElbowArrowBinding = ( return { fixedPoint: normalizeFixedPoint([ (nonRotatedSnappedGlobalPoint[0] - hoveredElement.x) / - hoveredElement.width, + Math.max(hoveredElement.width, PRECISION), (nonRotatedSnappedGlobalPoint[1] - hoveredElement.y) / - hoveredElement.height, + Math.max(hoveredElement.height, PRECISION), ]), }; }; @@ -1976,9 +1975,11 @@ export const calculateFixedPointForNonElbowArrowBinding = ( // Calculate the ratio relative to the element's bounds const fixedPointX = - (nonRotatedPoint[0] - hoveredElement.x) / hoveredElement.width; + (nonRotatedPoint[0] - hoveredElement.x) / + Math.max(hoveredElement.width, PRECISION); const fixedPointY = - (nonRotatedPoint[1] - hoveredElement.y) / hoveredElement.height; + (nonRotatedPoint[1] - hoveredElement.y) / + Math.max(hoveredElement.height, PRECISION); return { fixedPoint: normalizeFixedPoint([fixedPointX, fixedPointY]), diff --git a/packages/element/src/collision.ts b/packages/element/src/collision.ts index c260ae5267..6999512b44 100644 --- a/packages/element/src/collision.ts +++ b/packages/element/src/collision.ts @@ -61,6 +61,8 @@ import { distanceToElement } from "./distance"; import { getBindingGap } from "./binding"; +import { hasBackground } from "./comparisons"; + import type { ElementsMap, ExcalidrawArrowElement, @@ -83,7 +85,7 @@ export const shouldTestInside = (element: ExcalidrawElement) => { } const isDraggableFromInside = - !isTransparent(element.backgroundColor) || + (hasBackground(element.type) && !isTransparent(element.backgroundColor)) || hasBoundTextElement(element) || isIframeLikeElement(element) || isTextElement(element); @@ -324,7 +326,10 @@ export const getAllHoveredElementAtPoint = ( ) { candidateElements.push(element); - if (!isTransparent(element.backgroundColor)) { + if ( + hasBackground(element.type) && + !isTransparent(element.backgroundColor) + ) { break; } } diff --git a/packages/element/src/duplicate.ts b/packages/element/src/duplicate.ts index c2cee4c089..24135c0879 100644 --- a/packages/element/src/duplicate.ts +++ b/packages/element/src/duplicate.ts @@ -111,6 +111,9 @@ export const duplicateElements = ( * user interaction. */ type: "everything"; + // TODO remove/review this once we add frame children order migration + // and invariant checks + preserveFrameChildrenOrder?: boolean; } | { /** @@ -170,6 +173,8 @@ export const duplicateElements = ( opts.type === "in-place" ? opts.idsOfElementsToDuplicate : new Map(elements.map((el) => [el.id, el])); + const preserveFrameChildrenOrder = + opts.type === "everything" && opts.preserveFrameChildrenOrder; // For sanity if (opts.type === "in-place") { @@ -250,6 +255,9 @@ export const duplicateElements = ( elementsWithDuplicates.splice(index + 1, 0, ...castArray(elements)); }; + // main + // --------------------------------------------------------------------------- + const frameIdsToDuplicate = new Set( elements .filter( @@ -274,7 +282,7 @@ export const duplicateElements = ( if (groupId) { const groupElements = getElementsInGroup(elements, groupId).flatMap( (element) => - isFrameLikeElement(element) + isFrameLikeElement(element) && !preserveFrameChildrenOrder ? [...getFrameChildren(elements, element.id), element] : [element], ); @@ -290,13 +298,25 @@ export const duplicateElements = ( // frame duplication // ------------------------------------------------------------------------- - if (element.frameId && frameIdsToDuplicate.has(element.frameId)) { + if ( + !preserveFrameChildrenOrder && + element.frameId && + frameIdsToDuplicate.has(element.frameId) + ) { continue; } if (isFrameLikeElement(element)) { const frameId = element.id; + if (preserveFrameChildrenOrder) { + insertBeforeOrAfterIndex( + findLastIndex(elementsWithDuplicates, (el) => el.id === frameId), + copyElements(element), + ); + continue; + } + const frameChildren = getFrameChildren(elements, frameId); const targetIndex = findLastIndex(elementsWithDuplicates, (el) => { diff --git a/packages/element/src/elbowArrow.ts b/packages/element/src/elbowArrow.ts index 9543b4182f..024b846b12 100644 --- a/packages/element/src/elbowArrow.ts +++ b/packages/element/src/elbowArrow.ts @@ -2124,8 +2124,8 @@ const normalizeArrowElementUpdate = ( offsetY < -MAX_POS || offsetY > MAX_POS || offsetX + points[points.length - 1][0] < -MAX_POS || - offsetY + points[points.length - 1][0] > MAX_POS || - offsetX + points[points.length - 1][1] < -MAX_POS || + offsetX + points[points.length - 1][0] > MAX_POS || + offsetY + points[points.length - 1][1] < -MAX_POS || offsetY + points[points.length - 1][1] > MAX_POS ) { console.error( diff --git a/packages/element/src/fractionalIndex.ts b/packages/element/src/fractionalIndex.ts index 44ca523c80..90a2e7c217 100644 --- a/packages/element/src/fractionalIndex.ts +++ b/packages/element/src/fractionalIndex.ts @@ -1,7 +1,10 @@ -import { generateNKeysBetween } from "fractional-indexing"; - import { arrayToMap } from "@excalidraw/common"; +import { + validateOrderKey, + generateNKeysBetween, +} from "@excalidraw/fractional-indexing"; + import { mutateElement, newElementWith } from "./mutateElement"; import { getBoundTextElement } from "./textElement"; import { hasBoundTextElement } from "./typeChecks"; @@ -382,6 +385,13 @@ const isValidFractionalIndex = ( return false; } + try { + // Format validation + validateOrderKey(index); + } catch { + return false; + } + if (predecessor && successor) { return predecessor < index && index < successor; } diff --git a/packages/element/src/frame.ts b/packages/element/src/frame.ts index 787c2692f9..3d1449a072 100644 --- a/packages/element/src/frame.ts +++ b/packages/element/src/frame.ts @@ -19,9 +19,11 @@ import { getElementAbsoluteCoords, doBoundsIntersect, getElementBounds, + boundsContainBounds, } from "./bounds"; import { mutateElement } from "./mutateElement"; import { getBoundTextElement, getContainerElement } from "./textElement"; +import { syncMovedIndices } from "./fractionalIndex"; import { isFrameElement, isFrameLikeElement, @@ -101,8 +103,9 @@ export const isElementContainingFrame = ( frame: ExcalidrawFrameLikeElement, elementsMap: ElementsMap, ) => { - return getElementsWithinSelection([frame], element, elementsMap).some( - (e) => e.id === frame.id, + return boundsContainBounds( + getElementBounds(element, elementsMap), + getElementBounds(frame, elementsMap), ); }; @@ -489,10 +492,44 @@ export const filterElementsEligibleAsFrameChildren = ( return eligibleElements; }; +export const getCommonFrameId = (elements: readonly ExcalidrawElement[]) => { + let commonFrameId: ExcalidrawElement["frameId"] | undefined; + + for (const element of elements) { + if (isFrameLikeElement(element) || !element.frameId) { + return null; + } + + if (commonFrameId === undefined) { + commonFrameId = element.frameId; + } else if (commonFrameId !== element.frameId) { + return null; + } + } + + return commonFrameId ?? null; +}; + +export const getFrameChildrenInsertionIndex = ( + elements: readonly ExcalidrawElement[], + frameId: ExcalidrawFrameLikeElement["id"], +): number | null => { + for (let index = elements.length - 1; index >= 0; index--) { + const element = elements[index]; + + if (element.id === frameId) { + return index; + } else if (element.frameId === frameId) { + return index + 1; + } + } + + return null; +}; + /** - * Retains (or repairs for target frame) the ordering invriant where children - * elements come right before the parent frame: - * [el, el, child, child, frame, el] + * Adds elements and their bound elements to frame. Reorders added elements to + * be just below frame, or just above its highest child (whichever is higher). * * @returns mutated allElements (same data structure) */ @@ -500,19 +537,11 @@ export const addElementsToFrame = ( allElements: T, elementsToAdd: NonDeletedExcalidrawElement[], frame: ExcalidrawFrameLikeElement, - appState: AppState, ): T => { const elementsMap = arrayToMap(allElements); - const currTargetFrameChildrenMap = new Map(); - for (const element of allElements.values()) { - if (element.frameId === frame.id) { - currTargetFrameChildrenMap.set(element.id, true); - } - } + const commonFrameId = getCommonFrameId(elementsToAdd); - const suppliedElementsToAddSet = new Set(elementsToAdd.map((el) => el.id)); - - const finalElementsToAdd: ExcalidrawElement[] = []; + const finalElementsToAdd = new Set(); const otherFrames = new Set(); @@ -523,7 +552,8 @@ export const addElementsToFrame = ( } // - add bound text elements if not already in the array - // - filter out elements that are already in the frame + // - keep elements already in the frame so mixed selections can be reordered + // together for (const element of omitGroupsContainingFrameLikes( allElements, elementsToAdd, @@ -536,38 +566,68 @@ export const addElementsToFrame = ( continue; } - // if the element is already in another frame (which is also in elementsToAdd), - // it means that frame and children are selected at the same time - // => keep original frame membership, do not add to the target frame - if ( - element.frameId && - appState.selectedElementIds[element.id] && - appState.selectedElementIds[element.frameId] - ) { + if (element.frameId && element.frameId !== frame.id) { continue; } - if (!currTargetFrameChildrenMap.has(element.id)) { - finalElementsToAdd.push(element); - } + finalElementsToAdd.add(element); const boundTextElement = getBoundTextElement(element, elementsMap); - if ( - boundTextElement && - !suppliedElementsToAddSet.has(boundTextElement.id) && - !currTargetFrameChildrenMap.has(boundTextElement.id) - ) { - finalElementsToAdd.push(boundTextElement); + if (boundTextElement && !finalElementsToAdd.has(boundTextElement)) { + finalElementsToAdd.add(boundTextElement); } } for (const element of finalElementsToAdd) { - mutateElement(element, elementsMap, { - frameId: frame.id, - }); + // we don't always need to update the element if it's already in the frame, + // but we still need to accumulate in finalElementsToAdd so we potentially + // reorder them if added together + if (element.frameId !== frame.id) { + mutateElement(element, elementsMap, { + frameId: frame.id, + }); + } } - return allElements; + // (re)order elements to be just below the frame, + // or just above the highest child if that is higher + // (latter case is denormalized order until we migrate) + // --------------------------------------------------------------------------- + + if ( + !finalElementsToAdd.size || + // if all elements to add already belong to the frame, then we don't want to + // reorder (case: we're dragging element children within the frame) + commonFrameId === frame.id + ) { + return allElements; + } + + const otherElements = Array.from(allElements.values()).filter( + (element) => !finalElementsToAdd.has(element), + ); + const insertionIndex = getFrameChildrenInsertionIndex( + otherElements, + frame.id, + ); + + if (insertionIndex === null) { + return allElements; + } + + const reorderedElements = [ + ...otherElements.slice(0, insertionIndex), + ...finalElementsToAdd, + ...otherElements.slice(insertionIndex), + ]; + + syncMovedIndices(reorderedElements, arrayToMap([...finalElementsToAdd])); + + return ( + Array.isArray(allElements) + ? reorderedElements + : new Map(reorderedElements.map((element) => [element.id, element])) + ) as T; }; export const removeElementsFromFrame = ( @@ -621,13 +681,11 @@ export const replaceAllElementsInFrame = ( allElements: readonly T[], nextElementsInFrame: ExcalidrawElement[], frame: ExcalidrawFrameLikeElement, - app: AppClassProperties, ): T[] => { return addElementsToFrame( removeAllElementsFromFrame(allElements, frame), nextElementsInFrame, frame, - app.state, ).slice(); }; diff --git a/packages/element/src/linearElementEditor.ts b/packages/element/src/linearElementEditor.ts index 8af8ecd858..d2f80a50b3 100644 --- a/packages/element/src/linearElementEditor.ts +++ b/packages/element/src/linearElementEditor.ts @@ -486,7 +486,7 @@ export class LinearElementEditor { selectedPointsIndices, )}) points(0..${ element.points.length - 1 - }) lastClickedPoint(${lastClickedPoint})`, + }) lastClickedPoint(${lastClickedPoint}) isElbowArrow: ${elbowed}`, ); // Fall back to the actual last point as a last resort. @@ -2139,13 +2139,13 @@ const pointDraggingUpdates = ( } => { const naiveDraggingPoints = new Map( selectedPointsIndices.map((pointIndex) => { + // NOTE: Avoid stale point index issue potentially caused by elbow + // arrows unpredictably changing the number of points during dragging + const point = element.points[pointIndex] ?? element.points.at(-1); return [ pointIndex, { - point: pointFrom( - element.points[pointIndex][0] + deltaX, - element.points[pointIndex][1] + deltaY, - ), + point: pointFrom(point[0] + deltaX, point[1] + deltaY), isDragging: true, }, ]; diff --git a/packages/element/src/selection.ts b/packages/element/src/selection.ts index 8e9c4e8086..a65d1d3ce3 100644 --- a/packages/element/src/selection.ts +++ b/packages/element/src/selection.ts @@ -34,7 +34,6 @@ import { elementOverlapsWithFrame, getContainingFrame, getFrameChildren, - isElementIntersectingFrame, } from "./frame"; import { LinearElementEditor } from "./linearElementEditor"; @@ -130,13 +129,24 @@ export const getElementsWithinSelection = ( const framesInSelection = excludeElementsInFrames ? new Set() : null; - let elementsInSelection: NonDeletedExcalidrawElement[] = []; + const groups: Record = {}; + const elementsInSelection: Set = new Set(); for (const element of elements) { if (shouldIgnoreElementFromSelection(element)) { continue; } + // Track only selectable top-level group members, so ignored elements such + // as bound text and locked elements don't affect group selection. + const groupId = element.groupIds.at(-1); + if (groupId) { + if (!groups[groupId]) { + groups[groupId] = []; + } + groups[groupId].push(element); + } + const strokeWidth = element.strokeWidth; let labelAABB: Bounds | null = null; let elementAABB = getElementBounds(element, elementsMap); @@ -170,7 +180,7 @@ export const getElementsWithinSelection = ( const associatedFrame = getContainingFrame(element, elementsMap); if ( associatedFrame && - isElementIntersectingFrame(element, associatedFrame, elementsMap) + elementOverlapsWithFrame(element, associatedFrame, elementsMap) ) { const frameAABB = getElementBounds(associatedFrame, elementsMap); elementAABB = [ @@ -209,10 +219,9 @@ export const getElementsWithinSelection = ( if (boundsContainBounds(selectionBounds, commonAABB)) { if (framesInSelection && isFrameLikeElement(element)) { framesInSelection.add(element.id); - } else { - elementsInSelection.push(element); - continue; } + elementsInSelection.add(element); + continue; } // 2. Handle the case where the label is overlapped by the selection box @@ -221,7 +230,7 @@ export const getElementsWithinSelection = ( labelAABB && doBoundsIntersect(selectionBounds, labelAABB) ) { - elementsInSelection.push(element); + elementsInSelection.add(element); continue; } @@ -311,7 +320,7 @@ export const getElementsWithinSelection = ( framesInSelection.add(element.id); } - elementsInSelection.push(element); + elementsInSelection.add(element); continue; } } @@ -320,21 +329,41 @@ export const getElementsWithinSelection = ( // as it is separately handled in App. } - elementsInSelection = framesInSelection - ? excludeElementsFromFrames(elementsInSelection, framesInSelection) - : elementsInSelection; + if (framesInSelection) { + elementsInSelection.forEach((element) => { + if (element.frameId && framesInSelection.has(element.frameId)) { + elementsInSelection.delete(element); + } + }); + } - elementsInSelection = elementsInSelection.filter((element) => { - const containingFrame = getContainingFrame(element, elementsMap); + if (boxSelectionMode === "overlap") { + Array.from(elementsInSelection).forEach((element) => { + const groupId = element.groupIds.at(-1); + const group = groupId ? groups[groupId] : null; - if (containingFrame) { - return elementOverlapsWithFrame(element, containingFrame, elementsMap); - } + group?.forEach((groupElement) => elementsInSelection.add(groupElement)); + }); + } else if (boxSelectionMode === "contain") { + elementsInSelection.forEach((element) => { + // note: currently we only support top-level group handling since + // we don't support box selecting while editing the group/subgroup + // see https://github.com/excalidraw/excalidraw/pull/11234#issuecomment-4387654451 + const groupId = element.groupIds.at(-1); - return true; - }); + const group = groupId ? groups[groupId] : null; - return elementsInSelection; + if ( + group && + !group.every((groupElement) => elementsInSelection.has(groupElement)) + ) { + elementsInSelection.delete(element); + } + }); + } + + // to maintain original order elements (namely for group selection) + return elements.filter((element) => elementsInSelection.has(element)); }; export const getVisibleAndNonSelectedElements = ( diff --git a/packages/element/src/sortElements.ts b/packages/element/src/sortElements.ts index c98ff9d523..0f9e8da0f1 100644 --- a/packages/element/src/sortElements.ts +++ b/packages/element/src/sortElements.ts @@ -1,59 +1,56 @@ -import { arrayToMapWithIndex } from "@excalidraw/common"; +import { arrayToMap } from "@excalidraw/common"; import type { ExcalidrawElement } from "./types"; -const normalizeGroupElementOrder = (elements: readonly ExcalidrawElement[]) => { - const origElements: ExcalidrawElement[] = elements.slice(); - const sortedElements = new Set(); - - const orderInnerGroups = ( - elements: readonly ExcalidrawElement[], - ): ExcalidrawElement[] => { - const firstGroupSig = elements[0]?.groupIds?.join(""); - const aGroup: ExcalidrawElement[] = [elements[0]]; - const bGroup: ExcalidrawElement[] = []; - for (const element of elements.slice(1)) { - if (element.groupIds?.join("") === firstGroupSig) { - aGroup.push(element); - } else { - bGroup.push(element); - } - } - return bGroup.length ? [...aGroup, ...orderInnerGroups(bGroup)] : aGroup; +const defragmentGroups = (elements: readonly ExcalidrawElement[]) => { + const groupIdAtLevel = (element: ExcalidrawElement, level: number) => { + return element.groupIds[element.groupIds.length - level - 1]; }; - const groupHandledElements = new Map(); + const orderLevel = ( + levelElements: readonly ExcalidrawElement[], + level: number, + ): ExcalidrawElement[] => { + const buckets = new Map(); + // Slots preserve first-occurrence order: a groupId reserves its slot + // the first time one of its members is seen; loose elements occupy + // their own slot. Groups are then expanded (and recursed into) in place. + const slots: (ExcalidrawElement | string)[] = []; - origElements.forEach((element, idx) => { - if (groupHandledElements.has(element.id)) { - return; - } - if (element.groupIds?.length) { - const topGroup = element.groupIds[element.groupIds.length - 1]; - const groupElements = origElements.slice(idx).filter((element) => { - const ret = element?.groupIds?.some((id) => id === topGroup); - if (ret) { - groupHandledElements.set(element!.id, true); - } - return ret; - }); - - for (const elem of orderInnerGroups(groupElements)) { - sortedElements.add(elem); + for (const element of levelElements) { + const groupId = groupIdAtLevel(element, level); + if (groupId === undefined) { + slots.push(element); + continue; } - } else { - sortedElements.add(element); + let bucket = buckets.get(groupId); + if (!bucket) { + bucket = []; + buckets.set(groupId, bucket); + slots.push(groupId); + } + bucket.push(element); } - }); + + return slots.flatMap((slot) => + typeof slot === "string" + ? orderLevel(buckets.get(slot)!, level + 1) + : [slot], + ); + }; + + // `groupIds` is stored innermost-first, so the outermost group is the + // last entry. We recurse from level 0 (outermost) inward. + const sortedElements = orderLevel(elements, 0); // if there's a bug which resulted in losing some of the elements, return // original instead as that's better than losing data - if (sortedElements.size !== elements.length) { - console.error("normalizeGroupElementOrder: lost some elements... bailing!"); + if (sortedElements.length !== elements.length) { + console.error("defragmentGroups: lost some elements... bailing!"); return elements; } - return [...sortedElements]; + return sortedElements; }; /** @@ -68,39 +65,40 @@ const normalizeGroupElementOrder = (elements: readonly ExcalidrawElement[]) => { const normalizeBoundElementsOrder = ( elements: readonly ExcalidrawElement[], ) => { - const elementsMap = arrayToMapWithIndex(elements); + const elementsMap = arrayToMap(elements); - const origElements: (ExcalidrawElement | null)[] = elements.slice(); const sortedElements = new Set(); - origElements.forEach((element, idx) => { - if (!element) { - return; + for (const element of elements) { + if (sortedElements.has(element)) { + continue; } + if (element.boundElements?.length) { sortedElements.add(element); - origElements[idx] = null; - element.boundElements.forEach((boundElement) => { + for (const boundElement of element.boundElements) { const child = elementsMap.get(boundElement.id); if (child && boundElement.type === "text") { - sortedElements.add(child[0]); - origElements[child[1]] = null; + sortedElements.add(child); } - }); - } else if (element.type === "text" && element.containerId) { - const parent = elementsMap.get(element.containerId); - if (!parent?.[0].boundElements?.find((x) => x.id === element.id)) { - sortedElements.add(element); - origElements[idx] = null; - - // if element has a container and container lists it, skip this element - // as it'll be taken care of by the container } - } else { - sortedElements.add(element); - origElements[idx] = null; + continue; } - }); + + // if element has a container and container lists it, skip this element + // as it'll be taken care of by the container + if ( + element.type === "text" && + element.containerId && + elementsMap + .get(element.containerId) + ?.boundElements?.some((el) => el.id === element.id) + ) { + continue; + } + + sortedElements.add(element); + } // if there's a bug which resulted in losing some of the elements, return // original instead as that's better than losing data @@ -117,5 +115,5 @@ const normalizeBoundElementsOrder = ( export const normalizeElementOrder = ( elements: readonly ExcalidrawElement[], ) => { - return normalizeBoundElementsOrder(normalizeGroupElementOrder(elements)); + return normalizeBoundElementsOrder(defragmentGroups(elements)); }; diff --git a/packages/element/src/typeChecks.ts b/packages/element/src/typeChecks.ts index b609cc3f8a..3a8f5e36ef 100644 --- a/packages/element/src/typeChecks.ts +++ b/packages/element/src/typeChecks.ts @@ -392,3 +392,23 @@ export const canBecomePolygon = ( (points.length === 3 && !pointsEqual(points[0], points[points.length - 1])) ); }; + +export const isEligibleFrameChildType = (type: ElementOrToolType) => { + switch (type) { + case "rectangle": + case "diamond": + case "ellipse": + case "arrow": + case "line": + case "freedraw": + case "text": + case "image": + case "frame": + case "embeddable": { + return true; + } + default: { + return false; + } + } +}; diff --git a/packages/element/tests/fractionalIndex.test.ts b/packages/element/tests/fractionalIndex.test.ts index 1cc3ca5af3..2834a831e1 100644 --- a/packages/element/tests/fractionalIndex.test.ts +++ b/packages/element/tests/fractionalIndex.test.ts @@ -1,9 +1,8 @@ /* eslint-disable no-lone-blocks */ -import { generateKeyBetween } from "fractional-indexing"; - import { arrayToMap } from "@excalidraw/common"; import { + InvalidFractionalIndexError, syncInvalidIndices, syncMovedIndices, validateFractionalIndices, @@ -13,13 +12,34 @@ import { deepCopyElement } from "@excalidraw/element"; import { API } from "@excalidraw/excalidraw/tests/helpers/api"; +import { + generateKeyBetween, + validateOrderKey, +} from "@excalidraw/fractional-indexing"; + import type { ElementsMap, ExcalidrawElement, FractionalIndex, } from "@excalidraw/element/types"; -import { InvalidFractionalIndexError } from "../src/fractionalIndex"; +describe("fractional index format validation", () => { + it("should reject malformed base62 order keys", () => { + expect(() => validateOrderKey("a!")).toThrow(); + expect(() => validateOrderKey("a_")).toThrow(); + expect(() => validateOrderKey("a1!")).toThrow(); + expect(() => validateOrderKey("a1_")).toThrow(); + expect(() => validateOrderKey("zd0032")).toThrow(); + }); + + it("should accept valid base62 order keys", () => { + expect(() => validateOrderKey("Zz")).not.toThrow(); + expect(() => validateOrderKey("a0")).not.toThrow(); + expect(() => validateOrderKey("a1")).not.toThrow(); + expect(() => validateOrderKey("a1V")).not.toThrow(); + expect(() => validateOrderKey("z".padEnd(28, "z"))).not.toThrow(); + }); +}); describe("sync invalid indices with array order", () => { describe("should NOT sync empty array", () => { @@ -104,6 +124,46 @@ describe("sync invalid indices with array order", () => { }); }); + describe("should sync when fractional index is malformed", () => { + // "zd0032" has head "z" which requires length 28 per getIntegerLength, + // but the string is far too short, so validateOrderKey throws for it + testInvalidIndicesSync({ + elements: [{ id: "A", index: "zd0032" }], + expect: { + unchangedElements: [], + }, + }); + + testInvalidIndicesSync({ + elements: [ + { id: "A", index: "a1" }, + { id: "B", index: "zd0032" }, + { id: "C", index: "a3" }, + ], + expect: { + unchangedElements: ["A", "C"], + }, + }); + + testInvalidIndicesSync({ + elements: [{ id: "A", index: "a!" }], + expect: { + unchangedElements: [], + }, + }); + + testInvalidIndicesSync({ + elements: [ + { id: "A", index: "a1" }, + { id: "B", index: "a!" }, + { id: "C", index: "a2" }, + ], + expect: { + unchangedElements: ["A", "C"], + }, + }); + }); + describe("should sync when fractional indices are duplicated", () => { testInvalidIndicesSync({ elements: [ diff --git a/packages/element/tests/frame.test.tsx b/packages/element/tests/frame.test.tsx index 47f2160ac3..b419d00f02 100644 --- a/packages/element/tests/frame.test.tsx +++ b/packages/element/tests/frame.test.tsx @@ -2,15 +2,24 @@ import { convertToExcalidrawElements, Excalidraw, } from "@excalidraw/excalidraw"; +import { arrayToMap } from "@excalidraw/common"; import { API } from "@excalidraw/excalidraw/tests/helpers/api"; -import { Keyboard, Pointer } from "@excalidraw/excalidraw/tests/helpers/ui"; +import { Keyboard, Pointer, UI } from "@excalidraw/excalidraw/tests/helpers/ui"; +import { getTextEditor } from "@excalidraw/excalidraw/tests/queries/dom"; import { getCloneByOrigId, render, } from "@excalidraw/excalidraw/tests/test-utils"; -import type { ExcalidrawElement } from "../src/types"; +import { getSelectedElements } from "@excalidraw/excalidraw/scene"; + +import { elementOverlapsWithFrame } from "../src/frame"; + +import type { + ExcalidrawElement, + ExcalidrawFrameLikeElement, +} from "../src/types"; const { h } = window; const mouse = new Pointer("mouse"); @@ -125,6 +134,250 @@ describe("adding elements to frames", () => { }); }); + it("should treat an element fully containing a frame as overlapping the frame", () => { + const containingRect = API.createElement({ + type: "rectangle", + x: -50, + y: -50, + width: 250, + height: 250, + }); + + API.setElements([containingRect, frame]); + + expect( + elementOverlapsWithFrame( + containingRect, + frame as ExcalidrawFrameLikeElement, + arrayToMap(h.elements), + ), + ).toBe(true); + }); + + it("should not add a newly created element to a frame behind a non-frame element", () => { + const cover = API.createElement({ + id: "cover", + type: "rectangle", + x: 10, + y: 10, + width: 80, + height: 80, + backgroundColor: "#ffc9c9", + }); + + API.setElements([frame, cover]); + + UI.clickTool("rectangle"); + mouse.downAt(20, 20); + mouse.moveTo(40, 40); + mouse.upAt(40, 40); + + const createdElement = h.elements.find( + (element) => element.id !== frame.id && element.id !== cover.id, + ); + + expect(createdElement?.frameId).toBe(null); + expect(h.elements.map((element) => element.id)).toEqual([ + frame.id, + cover.id, + createdElement?.id, + ]); + }); + + it("should add a newly created element to a frame over a non-frame element", () => { + const cover = API.createElement({ + id: "cover", + type: "rectangle", + x: 10, + y: 10, + width: 80, + height: 80, + backgroundColor: "#ffc9c9", + }); + + API.setElements([cover, frame]); + + UI.clickTool("rectangle"); + mouse.downAt(20, 20); + mouse.moveTo(40, 40); + mouse.upAt(40, 40); + + const createdElement = h.elements.find( + (element) => element.id !== frame.id && element.id !== cover.id, + ); + + expect(createdElement?.frameId).toBe(frame.id); + }); + + it("should highlight the target frame while creating a new element", () => { + API.setElements([frame]); + + UI.clickTool("rectangle"); + mouse.downAt(20, 20); + mouse.moveTo(40, 40); + + expect(h.state.frameToHighlight?.id).toBe(frame.id); + + mouse.upAt(40, 40); + + expect(h.state.frameToHighlight).toBe(null); + }); + + it("should highlight the target frame while hovering with a creation tool", () => { + API.setElements([frame]); + + UI.clickTool("rectangle"); + mouse.moveTo(20, 20); + + expect(h.state.frameToHighlight?.id).toBe(frame.id); + + mouse.moveTo(200, 200); + + expect(h.state.frameToHighlight).toBe(null); + }); + + it("should not add grid-snapped text outside the frame to the clicked frame", async () => { + const offsetFrame = API.createElement({ + id: "offsetFrame", + type: "frame", + x: 10, + y: 0, + width: 150, + height: 150, + }); + + API.setElements([offsetFrame]); + API.setAppState({ + gridModeEnabled: true, + }); + + UI.clickTool("text"); + mouse.clickAt(12, 0); + + await getTextEditor(); + + const createdText = h.elements.find( + (element) => element.id !== offsetFrame.id, + ); + + expect(createdText?.x).toBe(0); + expect(createdText?.y).toBe(0); + expect(createdText?.frameId).toBe(null); + }); + + it("should add a newly created element to a frame behind another frame", () => { + const lockedFrame = API.createElement({ + id: "lockedFrame", + type: "frame", + x: 10, + y: 10, + width: 80, + height: 80, + locked: true, + }); + + API.setElements([frame, lockedFrame]); + + UI.clickTool("rectangle"); + mouse.downAt(20, 20); + mouse.moveTo(40, 40); + mouse.upAt(40, 40); + + const createdElement = h.elements.find( + (element) => element.id !== frame.id && element.id !== lockedFrame.id, + ); + + expect(createdElement?.frameId).toBe(frame.id); + }); + + it("should insert a newly created frame child just below its frame", () => { + const frameChildUnderCursor = API.createElement({ + id: "frameChildUnderCursor", + type: "rectangle", + x: 10, + y: 10, + width: 80, + height: 80, + backgroundColor: "#ffc9c9", + frameId: frame.id, + }); + const otherFrameChild = API.createElement({ + id: "otherFrameChild", + type: "rectangle", + x: 100, + y: 20, + width: 20, + height: 20, + frameId: frame.id, + }); + + API.setElements([frameChildUnderCursor, otherFrameChild, frame]); + + UI.clickTool("rectangle"); + mouse.downAt(20, 20); + mouse.moveTo(40, 40); + mouse.upAt(40, 40); + + const createdElement = h.elements.find( + (element) => + element.id !== frame.id && + element.id !== frameChildUnderCursor.id && + element.id !== otherFrameChild.id, + ); + + expect(createdElement?.frameId).toBe(frame.id); + expect(h.elements.map((element) => element.id)).toEqual([ + frameChildUnderCursor.id, + otherFrameChild.id, + createdElement?.id, + frame.id, + ]); + }); + + it("should insert a newly created frame child above the highest frame child", () => { + const frameChildUnderCursor = API.createElement({ + id: "frameChildUnderCursor", + type: "rectangle", + x: 10, + y: 10, + width: 80, + height: 80, + backgroundColor: "#ffc9c9", + frameId: frame.id, + }); + const otherFrameChild = API.createElement({ + id: "otherFrameChild", + type: "rectangle", + x: 100, + y: 20, + width: 20, + height: 20, + frameId: frame.id, + }); + + API.setElements([frame, frameChildUnderCursor, otherFrameChild]); + + UI.clickTool("rectangle"); + mouse.downAt(20, 20); + mouse.moveTo(40, 40); + mouse.upAt(40, 40); + + const createdElement = h.elements.find( + (element) => + element.id !== frame.id && + element.id !== frameChildUnderCursor.id && + element.id !== otherFrameChild.id, + ); + + expect(createdElement?.frameId).toBe(frame.id); + expect(h.elements.map((element) => element.id)).toEqual([ + frame.id, + frameChildUnderCursor.id, + otherFrameChild.id, + createdElement?.id, + ]); + }); + const commonTestCases = async ( func: typeof resizeFrameOverElement | typeof dragElementIntoFrame, ) => { @@ -415,6 +668,345 @@ describe("adding elements to frames", () => { describe("dragging elements into the frame", async () => { await commonTestCases(dragElementIntoFrame); + it("should add a dragged element fully containing the frame", () => { + const containingRect = API.createElement({ + type: "rectangle", + x: 220, + y: 20, + width: 300, + height: 300, + }); + + API.setElements([frame, containingRect]); + + dragElementIntoFrame(frame, containingRect); + + expect(API.getElement(containingRect).frameId).toBe(frame.id); + }); + + it("should drag an element into a frame", () => { + API.setElements([rect2, frame]); + + dragElementIntoFrame(frame, rect2); + + expect(rect2.frameId).toBe(frame.id); + }); + + it("should layer a dragged element above the highest frame child", () => { + const frameChild = API.createElement({ + id: "frameChild", + type: "rectangle", + x: 10, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + + API.setElements([frame, frameChild, rect2]); + + dragElementIntoFrame(frame, rect2); + + expect(rect2.frameId).toBe(frame.id); + expect(h.elements.map((element) => element.id)).toEqual([ + frame.id, + frameChild.id, + rect2.id, + ]); + expect(rect2.index! > frameChild.index!).toBe(true); + expect(rect2.index! > frame.index!).toBe(true); + }); + + it("should preview a dragged element above the highest frame child before pointerup", () => { + const frameChild = API.createElement({ + id: "frameChild", + type: "rectangle", + x: 10, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + + API.setElements([rect2, frame, frameChild]); + API.setSelectedElements([rect2]); + API.updateElement(rect2, { + x: 10, + y: 10, + }); + + const getRenderableElementIds = ( + selectedElementsAreBeingDragged: boolean, + ) => { + return h.app.renderer + .getRenderableElements({ + zoom: h.state.zoom, + offsetLeft: 0, + offsetTop: 0, + scrollX: 0, + scrollY: 0, + height: 1000, + width: 1000, + editingTextElement: h.state.editingTextElement, + newElement: h.state.newElement, + selectedElements: getSelectedElements(h.elements, h.state), + selectedElementsAreBeingDragged, + frameToHighlight: frame as ExcalidrawFrameLikeElement, + }) + .visibleElements.map((element) => element.id); + }; + + expect(h.elements.map((element) => element.id)).toEqual([ + rect2.id, + frame.id, + frameChild.id, + ]); + expect(getRenderableElementIds(false)).toEqual([ + rect2.id, + frame.id, + frameChild.id, + ]); + expect(getRenderableElementIds(true)).toEqual([ + frame.id, + frameChild.id, + rect2.id, + ]); + expect(h.elements.map((element) => element.id)).toEqual([ + rect2.id, + frame.id, + frameChild.id, + ]); + expect(rect2.frameId).toBe(null); + }); + + it("should not preview reorder dragged elements already in the highlighted frame", () => { + const frameChild = API.createElement({ + id: "frameChild", + type: "rectangle", + x: 10, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + const otherFrameChild = API.createElement({ + id: "otherFrameChild", + type: "rectangle", + x: 40, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + + API.setElements([frameChild, frame, otherFrameChild]); + API.setSelectedElements([frameChild]); + + const renderableElementIds = h.app.renderer + .getRenderableElements({ + zoom: h.state.zoom, + offsetLeft: 0, + offsetTop: 0, + scrollX: 0, + scrollY: 0, + height: 1000, + width: 1000, + editingTextElement: h.state.editingTextElement, + newElement: h.state.newElement, + selectedElements: getSelectedElements(h.elements, h.state), + selectedElementsAreBeingDragged: true, + frameToHighlight: frame as ExcalidrawFrameLikeElement, + }) + .visibleElements.map((element) => element.id); + + expect(renderableElementIds).toEqual([ + frameChild.id, + frame.id, + otherFrameChild.id, + ]); + }); + + it("should put a dragged mixed selection above the highest frame child", () => { + const frameChild = API.createElement({ + id: "frameChild", + type: "rectangle", + x: 50, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + boundElements: [{ id: "boundText", type: "text" }], + }); + const boundText = API.createElement({ + id: "boundText", + type: "text", + x: 50, + y: 10, + width: 20, + height: 20, + containerId: frameChild.id, + frameId: frame.id, + }); + const otherFrameChild = API.createElement({ + id: "otherFrameChild", + type: "rectangle", + x: 80, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + const nonFrameElement = API.createElement({ + id: "nonFrameElement", + type: "rectangle", + x: 155, + y: 10, + width: 20, + height: 20, + }); + + API.setElements([ + frame, + frameChild, + boundText, + otherFrameChild, + nonFrameElement, + ]); + API.setSelectedElements([frameChild, nonFrameElement]); + + mouse.downAt( + nonFrameElement.x + nonFrameElement.width / 2, + nonFrameElement.y + nonFrameElement.height / 2, + ); + mouse.moveTo(frame.x + frame.width - 5, nonFrameElement.y + 10); + mouse.up(); + + expect(frameChild.frameId).toBe(frame.id); + expect(boundText.frameId).toBe(frame.id); + expect(nonFrameElement.frameId).toBe(frame.id); + expect(h.elements.map((element) => element.id)).toEqual([ + frame.id, + otherFrameChild.id, + frameChild.id, + boundText.id, + nonFrameElement.id, + ]); + }); + + it("should not reorder dragged elements already in the highlighted frame", () => { + const frameChild = API.createElement({ + id: "frameChild", + type: "rectangle", + x: 50, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + const otherFrameChild = API.createElement({ + id: "otherFrameChild", + type: "rectangle", + x: 80, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + + API.setElements([frame, frameChild, otherFrameChild]); + API.setSelectedElements([frameChild]); + + mouse.downAt( + frameChild.x + frameChild.width / 2, + frameChild.y + frameChild.height / 2, + ); + mouse.moveTo(frameChild.x + frameChild.width / 2 + 5, frameChild.y + 10); + mouse.up(); + + expect(frameChild.frameId).toBe(frame.id); + expect(h.elements.map((element) => element.id)).toEqual([ + frame.id, + frameChild.id, + otherFrameChild.id, + ]); + }); + + it("should not drag an element into a frame behind a non-frame element", () => { + const cover = API.createElement({ + id: "cover", + type: "rectangle", + x: 10, + y: 10, + width: 80, + height: 80, + backgroundColor: "#ffc9c9", + }); + API.setElements([frame, cover, rect2]); + + mouse.clickAt(rect2.x, rect2.y); + mouse.downAt(rect2.x + rect2.width / 2, rect2.y + rect2.height / 2); + mouse.moveTo(20, 20); + mouse.upAt(20, 20); + + expect(rect2.frameId).toBe(null); + }); + + it("should drag an element into a frame over a non-frame element", () => { + const cover = API.createElement({ + id: "cover", + type: "rectangle", + x: 10, + y: 10, + width: 80, + height: 80, + backgroundColor: "#ffc9c9", + }); + API.setElements([cover, rect2, frame]); + + mouse.clickAt(rect2.x, rect2.y); + mouse.downAt(rect2.x + rect2.width / 2, rect2.y + rect2.height / 2); + mouse.moveTo(20, 20); + mouse.upAt(20, 20); + + expect(rect2.frameId).toBe(frame.id); + }); + + it("should keep dragging a frame child over a non-frame element above its frame", () => { + const cover = API.createElement({ + id: "cover", + type: "rectangle", + x: 10, + y: 10, + width: 80, + height: 80, + backgroundColor: "#ffc9c9", + }); + const frameChild = API.createElement({ + id: "frameChild", + type: "rectangle", + x: 100, + y: 20, + width: 20, + height: 20, + frameId: frame.id, + }); + + API.setElements([frameChild, frame, cover]); + API.setSelectedElements([frameChild]); + + mouse.downAt( + frameChild.x + frameChild.width / 2, + frameChild.y + frameChild.height / 2, + ); + mouse.moveTo(20, 20); + + expect(h.state.frameToHighlight?.id).toBe(frame.id); + + mouse.upAt(20, 20); + + expect(frameChild.frameId).toBe(frame.id); + }); + it.skip("should drag element inside, duplicate it and keep it in frame", () => { API.setElements([frame, rect2]); diff --git a/packages/element/tests/sortElements.test.ts b/packages/element/tests/sortElements.test.ts index 0928b84f29..0554d38e35 100644 --- a/packages/element/tests/sortElements.test.ts +++ b/packages/element/tests/sortElements.test.ts @@ -326,19 +326,59 @@ describe("normalizeElementsOrder", () => { ]), [ "BA_rect1", + "CBA_rect3", + "CBA_rect7", "BA_rect5", "BA_rect6", "A_rect2", "A_rect5", - "CBA_rect3", - "CBA_rect7", "rect4", "X_rect8", - "X_rect11", "YX_rect10", + "X_rect11", "rect9", ], ); + assertOrder( + normalizeElementOrder([ + API.createElement({ + id: "A_rect1", + type: "rectangle", + groupIds: ["A"], + }), + API.createElement({ + id: "CBA_rect2", + type: "rectangle", + groupIds: ["C", "B", "A"], + }), + API.createElement({ + id: "A_rect3", + type: "rectangle", + groupIds: ["A"], + }), + ]), + ["A_rect1", "CBA_rect2", "A_rect3"], + ); + assertOrder( + normalizeElementOrder([ + API.createElement({ + id: "abcT_rect1", + type: "rectangle", + groupIds: ["ab", "c", "T"], + }), + API.createElement({ + id: "abcT_rect2", + type: "rectangle", + groupIds: ["a", "bc", "T"], + }), + API.createElement({ + id: "abcT_rect3", + type: "rectangle", + groupIds: ["ab", "c", "T"], + }), + ]), + ["abcT_rect1", "abcT_rect3", "abcT_rect2"], + ); }); // TODO diff --git a/packages/excalidraw/actions/actionDeselect.ts b/packages/excalidraw/actions/actionDeselect.ts index 2b48eb825e..c9a92c7138 100644 --- a/packages/excalidraw/actions/actionDeselect.ts +++ b/packages/excalidraw/actions/actionDeselect.ts @@ -101,6 +101,7 @@ export const actionDeselect = register({ selectionElement: null, showHyperlinkPopup: false, suggestedBinding: null, + frameToHighlight: null, }, captureUpdate: CaptureUpdateAction.IMMEDIATELY, }; @@ -118,6 +119,7 @@ export const actionDeselect = register({ selectionElement: null, showHyperlinkPopup: false, suggestedBinding: null, + frameToHighlight: null, }, captureUpdate: CaptureUpdateAction.IMMEDIATELY, }; diff --git a/packages/excalidraw/actions/actionFinalize.tsx b/packages/excalidraw/actions/actionFinalize.tsx index 6b7cdf8edd..6ecdc797ba 100644 --- a/packages/excalidraw/actions/actionFinalize.tsx +++ b/packages/excalidraw/actions/actionFinalize.tsx @@ -335,8 +335,8 @@ export const actionFinalize = register({ selectionElement: null, multiElement: null, editingTextElement: null, - startBoundElement: null, suggestedBinding: null, + frameToHighlight: null, selectedElementIds: element && !appState.activeTool.locked && diff --git a/packages/excalidraw/actions/actionFrame.ts b/packages/excalidraw/actions/actionFrame.ts index 3a1b3635d3..81a43e243e 100644 --- a/packages/excalidraw/actions/actionFrame.ts +++ b/packages/excalidraw/actions/actionFrame.ts @@ -205,7 +205,6 @@ export const actionWrapSelectionInFrame = register({ [...app.scene.getElementsIncludingDeleted(), frame], selectedElements, frame, - appState, ); return { diff --git a/packages/excalidraw/actions/actionGroup.tsx b/packages/excalidraw/actions/actionGroup.tsx index c72216b761..c9bb8cf21d 100644 --- a/packages/excalidraw/actions/actionGroup.tsx +++ b/packages/excalidraw/actions/actionGroup.tsx @@ -277,7 +277,6 @@ export const actionUngroup = register({ elementsMap, ), frame, - app, ); } }); diff --git a/packages/excalidraw/actions/actionHistory.tsx b/packages/excalidraw/actions/actionHistory.tsx index 1b530adeaf..5db66670b7 100644 --- a/packages/excalidraw/actions/actionHistory.tsx +++ b/packages/excalidraw/actions/actionHistory.tsx @@ -1,5 +1,4 @@ import { - isWindows, KEYS, matchKey, arrayToMap, @@ -114,7 +113,7 @@ export const createRedoAction: ActionCreator = (history) => ({ ), keyTest: (event) => (event[KEYS.CTRL_OR_CMD] && event.shiftKey && matchKey(event, KEYS.Z)) || - (isWindows && event.ctrlKey && !event.shiftKey && matchKey(event, KEYS.Y)), + (event[KEYS.CTRL_OR_CMD] && !event.shiftKey && matchKey(event, KEYS.Y)), PanelComponent: ({ appState, updateData, data, app }) => { const { isRedoStackEmpty } = useEmitter( history.onHistoryChangedEmitter, diff --git a/packages/excalidraw/appState.ts b/packages/excalidraw/appState.ts index b047fcdb60..4b2c45bd6e 100644 --- a/packages/excalidraw/appState.ts +++ b/packages/excalidraw/appState.ts @@ -100,7 +100,6 @@ export const getDefaultAppState = (): Omit< open: false, panels: STATS_PANELS.generalStats | STATS_PANELS.elementProperties, }, - startBoundElement: null, suggestedBinding: null, frameRendering: { enabled: true, clip: true, name: true, outline: true }, frameToHighlight: null, @@ -237,7 +236,6 @@ const APP_STATE_STORAGE_CONF = (< selectionElement: { browser: false, export: false, server: false }, shouldCacheIgnoreZoom: { browser: true, export: false, server: false }, stats: { browser: true, export: false, server: false }, - startBoundElement: { browser: false, export: false, server: false }, suggestedBinding: { browser: false, export: false, server: false }, frameRendering: { browser: false, export: false, server: false }, frameToHighlight: { browser: false, export: false, server: false }, diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index c77a7ebd0a..7982584110 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -28,7 +28,6 @@ import { APP_NAME, CURSOR_TYPE, DEFAULT_TRANSFORM_HANDLE_SPACING, - DEFAULT_MAX_IMAGE_WIDTH_OR_HEIGHT, DEFAULT_VERTICAL_ALIGN, DRAGGING_THRESHOLD, ELEMENT_SHIFT_TRANSLATE_AMOUNT, @@ -38,7 +37,6 @@ import { IMAGE_MIME_TYPES, IMAGE_RENDER_TIMEOUT, LINE_CONFIRM_THRESHOLD, - MAX_ALLOWED_FILE_BYTES, MIME_TYPES, MQ_RIGHT_SIDEBAR_MIN_WIDTH, POINTER_BUTTON, @@ -176,7 +174,9 @@ import { isValidTextContainer, redrawTextBoundingBox, hasBoundingBox, + getCommonFrameId, getFrameChildren, + getFrameChildrenInsertionIndex, isCursorInFrame, addElementsToFrame, replaceAllElementsInFrame, @@ -259,6 +259,7 @@ import { maybeHandleArrowPointlikeDrag, getUncroppedWidthAndHeight, getActiveTextElement, + isEligibleFrameChildType, } from "@excalidraw/element"; import type { GlobalPoint, LocalPoint, Radians } from "@excalidraw/math"; @@ -603,6 +604,8 @@ const YOUTUBE_VIDEO_STATES = new Map< ValueOf >(); +const MAX_EMBEDDABLE_VIEWPORT_SCALE = 4; + let IS_PLAIN_PASTE = false; let IS_PLAIN_PASTE_TIMER = 0; let PLAIN_PASTE_TOAST_SHOWN = false; @@ -1735,6 +1738,18 @@ class App extends React.Component { this.state.activeEmbeddable?.element === el && this.state.activeEmbeddable?.state === "hover"; + // scale video embeds based on zoom (capped) so that smaller embeds + // on canvas when zoomed are still of legible quality + // (note: for some embed types like gdrive, the quality is poor when + // scaling mid playback and works only when you initially start the + // playback at the higher zoom level) + const shouldScaleEmbeddableViewport = src?.type === "video"; + const embeddableViewportScale = clamp( + shouldScaleEmbeddableViewport ? scale : 1, + 0.75, + MAX_EMBEDDABLE_VIEWPORT_SCALE, + ); + return (
{ padding: `${el.strokeWidth}px`, }} > - {(isEmbeddableElement(el) - ? this.props.renderEmbeddable?.(el, this.state) - : null) ?? ( -