diff --git a/packages/element/src/flowchart.ts b/packages/element/src/flowchart.ts index 0b781e69..b2b32807 100644 --- a/packages/element/src/flowchart.ts +++ b/packages/element/src/flowchart.ts @@ -1,6 +1,11 @@ -import { KEYS, invariant, toBrandedType } from "@excalidraw/common"; +import { invariant, toBrandedType, type Bounds } from "@excalidraw/common"; -import { type GlobalPoint, pointFrom, type LocalPoint } from "@excalidraw/math"; +import { + clamp, + type GlobalPoint, + pointFrom, + type LocalPoint, +} from "@excalidraw/math"; import type { AppState, @@ -43,318 +48,236 @@ import { import type { Scene } from "./Scene"; -type LinkDirection = "up" | "right" | "down" | "left"; +export type LinkDirection = "up" | "right" | "down" | "left"; const VERTICAL_OFFSET = 100; const HORIZONTAL_OFFSET = 100; -export const getLinkDirectionFromKey = (key: string): LinkDirection => { - switch (key) { - case KEYS.ARROW_UP: - return "up"; - case KEYS.ARROW_DOWN: - return "down"; - case KEYS.ARROW_RIGHT: - return "right"; - case KEYS.ARROW_LEFT: - return "left"; - default: - return "right"; - } -}; - -const getNodeRelatives = ( - type: "predecessors" | "successors", - node: ExcalidrawBindableElement, - elementsMap: ElementsMap, - direction: LinkDirection, -) => { - const items = [...elementsMap.values()].reduce( - (acc: { relative: ExcalidrawBindableElement; heading: Heading }[], el) => { - let oppositeBinding; - if ( - isElbowArrow(el) && - // we want check existence of the opposite binding, in the direction - // we're interested in - (oppositeBinding = - el[type === "predecessors" ? "startBinding" : "endBinding"]) && - // similarly, we need to filter only arrows bound to target node - el[type === "predecessors" ? "endBinding" : "startBinding"] - ?.elementId === node.id - ) { - const relative = elementsMap.get(oppositeBinding.elementId); +type Interval = { start: number; end: number }; - if (!relative) { - return acc; - } +const mergeIntervals = (intervals: Interval[]): Interval[] => { + const sorted = [...intervals].sort((a, b) => a.start - b.start); + const merged: Interval[] = []; - invariant( - isBindableElement(relative), - "not an ExcalidrawBindableElement", - ); + for (const interval of sorted) { + const last = merged[merged.length - 1]; + if (last && interval.start <= last.end) { + last.end = Math.max(last.end, interval.end); + } else { + merged.push({ ...interval }); + } + } - const edgePoint = ( - type === "predecessors" ? el.points[el.points.length - 1] : [0, 0] - ) as Readonly; + return merged; +}; - const heading = headingForPointFromElement( - node, - aabbForElement(node, elementsMap), - [edgePoint[0] + el.x, edgePoint[1] + el.y] as Readonly, - ); +const intervalIsFree = (start: number, size: number, occupied: Interval[]) => + occupied.every((o) => start + size <= o.start || start >= o.end); + +// Nearest `start` for a segment of `size` avoiding every occupied interval, +// searching both sides of `ideal`; a tie resolves toward the positive side. +const findNearestFreeSlot = ( + ideal: number, + size: number, + occupied: Interval[], +): number => { + if (intervalIsFree(ideal, size, occupied)) { + return ideal; + } - acc.push({ - relative, - heading, - }); - } - return acc; - }, - [], - ); + const gapStarts = [-Infinity, ...occupied.map((o) => o.end)]; + const gapEnds = [...occupied.map((o) => o.start), Infinity]; - switch (direction) { - case "up": - return items - .filter((item) => compareHeading(item.heading, HEADING_UP)) - .map((item) => item.relative); - case "down": - return items - .filter((item) => compareHeading(item.heading, HEADING_DOWN)) - .map((item) => item.relative); - case "right": - return items - .filter((item) => compareHeading(item.heading, HEADING_RIGHT)) - .map((item) => item.relative); - case "left": - return items - .filter((item) => compareHeading(item.heading, HEADING_LEFT)) - .map((item) => item.relative); + let best = ideal; + let bestDistance = Infinity; + for (let i = 0; i < gapStarts.length; i++) { + if (gapEnds[i] - gapStarts[i] < size) { + continue; + } + const start = clamp(ideal, gapStarts[i], gapEnds[i] - size); + const distance = Math.abs(start - ideal); + if (distance <= bestDistance) { + best = start; + bestDistance = distance; + } } -}; -const getSuccessors = ( - node: ExcalidrawBindableElement, - elementsMap: ElementsMap, - direction: LinkDirection, -) => { - return getNodeRelatives("successors", node, elementsMap, direction); + return best; }; -export const getPredecessors = ( +// Walk the arrow bindings to collect every node that belongs to the same +// flowchart as `node` — the whole connected component acts as the obstacle +// set during placement (#8518). +const getConnectedFlowchartNodes = ( node: ExcalidrawBindableElement, elementsMap: ElementsMap, - direction: LinkDirection, -) => { - return getNodeRelatives("predecessors", node, elementsMap, direction); -}; - -const getOffsets = ( - element: ExcalidrawFlowchartNodeElement, - linkedNodes: ExcalidrawElement[], - direction: LinkDirection, -) => { - const _HORIZONTAL_OFFSET = HORIZONTAL_OFFSET + element.width; - - // check if vertical space or horizontal space is available first - if (direction === "up" || direction === "down") { - const _VERTICAL_OFFSET = VERTICAL_OFFSET + element.height; - // check vertical space - const minX = element.x; - const maxX = element.x + element.width; - - // vertical space is available - if ( - linkedNodes.every( - (linkedNode) => - linkedNode.x + linkedNode.width < minX || linkedNode.x > maxX, - ) - ) { - return { - x: 0, - y: _VERTICAL_OFFSET * (direction === "up" ? -1 : 1), - }; - } - } else if (direction === "right" || direction === "left") { - const minY = element.y; - const maxY = element.y + element.height; +): ExcalidrawBindableElement[] => { + const arrows = [...elementsMap.values()].filter(isElbowArrow); + const visited = new Set([node.id]); + const queue: string[] = [node.id]; + const connected: ExcalidrawBindableElement[] = []; + + while (queue.length > 0) { + const currentId = queue.shift()!; + for (const arrow of arrows) { + const startId = arrow.startBinding?.elementId; + const endId = arrow.endBinding?.elementId; + + let neighborId: string | undefined; + if (startId === currentId) { + neighborId = endId; + } else if (endId === currentId) { + neighborId = startId; + } - if ( - linkedNodes.every( - (linkedNode) => - linkedNode.y + linkedNode.height < minY || linkedNode.y > maxY, - ) - ) { - return { - x: - (HORIZONTAL_OFFSET + element.width) * (direction === "left" ? -1 : 1), - y: 0, - }; - } - } + if (!neighborId || visited.has(neighborId)) { + continue; + } - if (direction === "up" || direction === "down") { - const _VERTICAL_OFFSET = VERTICAL_OFFSET + element.height; - const y = linkedNodes.length === 0 ? _VERTICAL_OFFSET : _VERTICAL_OFFSET; - const x = - linkedNodes.length === 0 - ? 0 - : (linkedNodes.length + 1) % 2 === 0 - ? ((linkedNodes.length + 1) / 2) * _HORIZONTAL_OFFSET - : (linkedNodes.length / 2) * _HORIZONTAL_OFFSET * -1; - - if (direction === "up") { - return { - x, - y: y * -1, - }; + visited.add(neighborId); + const neighbor = elementsMap.get(neighborId); + if (neighbor && isBindableElement(neighbor)) { + connected.push(neighbor); + queue.push(neighborId); + } } - - return { - x, - y, - }; } - const _VERTICAL_OFFSET = VERTICAL_OFFSET + element.height; - const x = - (linkedNodes.length === 0 ? HORIZONTAL_OFFSET : HORIZONTAL_OFFSET) + - element.width; - const y = - linkedNodes.length === 0 - ? 0 - : (linkedNodes.length + 1) % 2 === 0 - ? ((linkedNodes.length + 1) / 2) * _VERTICAL_OFFSET - : (linkedNodes.length / 2) * _VERTICAL_OFFSET * -1; - - if (direction === "left") { - return { - x: x * -1, - y, - }; - } - return { - x, - y, - }; + return connected; }; -const addNewNode = ( - element: NonDeleted, - appState: AppState, +// Place a cluster of `count` equally-sized nodes next to `parent`: +// - the primary axis (the creation direction) is fixed at exactly one gap +// away from the parent, forming a search band the cluster will occupy +// - along the cross axis the cluster slides into the free slot nearest the +// parent-centered ideal, treating band obstacles as immovable +// - `stickyCrossStart` anchors an already-visible pending cluster: growing it +// extends it at either end so the existing pending nodes keep their +// positions, unless the grown cluster no longer fits there +const placeCluster = ( + parent: ExcalidrawFlowchartNodeElement, direction: LinkDirection, - scene: Scene, -) => { - const elementsMap = scene.getNonDeletedElementsMap(); - const successors = getSuccessors(element, elementsMap, direction); - const predeccessors = getPredecessors(element, elementsMap, direction); - - const offsets = getOffsets( - element, - [...successors, ...predeccessors], - direction, + count: number, + obstacles: readonly Bounds[], + stickyCrossStart: number | null, +): { positions: { x: number; y: number }[]; crossStart: number } => { + const horizontal = direction === "left" || direction === "right"; + // INSIGHT: new nodes copy the parent's dimensions + const nodePrimarySize = horizontal ? parent.width : parent.height; + const nodeCrossSize = horizontal ? parent.height : parent.width; + const primaryGap = horizontal ? HORIZONTAL_OFFSET : VERTICAL_OFFSET; + const crossGap = horizontal ? VERTICAL_OFFSET : HORIZONTAL_OFFSET; + + const parentPrimaryStart = horizontal ? parent.x : parent.y; + const parentCrossCenter = horizontal + ? parent.y + parent.height / 2 + : parent.x + parent.width / 2; + + const primaryStart = + direction === "right" || direction === "down" + ? parentPrimaryStart + nodePrimarySize + primaryGap + : parentPrimaryStart - primaryGap - nodePrimarySize; + + // cross-axis intervals of the obstacles sharing the band, inflated so the + // cluster keeps at least one gap of clearance + const occupied = mergeIntervals( + obstacles + .filter((bounds) => { + const start = horizontal ? bounds[0] : bounds[1]; + const end = horizontal ? bounds[2] : bounds[3]; + return start < primaryStart + nodePrimarySize && end > primaryStart; + }) + .map((bounds) => ({ + start: (horizontal ? bounds[1] : bounds[0]) - crossGap, + end: (horizontal ? bounds[3] : bounds[2]) + crossGap, + })), ); - const nextNode = newElement({ - type: element.type, - x: element.x + offsets.x, - y: element.y + offsets.y, - // TODO: extract this to a util - width: element.width, - height: element.height, - roundness: element.roundness, - roughness: element.roughness, - backgroundColor: element.backgroundColor, - strokeColor: element.strokeColor, - strokeWidth: element.strokeWidth, - opacity: element.opacity, - fillStyle: element.fillStyle, - strokeStyle: element.strokeStyle, + const step = nodeCrossSize + crossGap; + const clusterCrossSize = count * nodeCrossSize + (count - 1) * crossGap; + + const anchoredStart = + stickyCrossStart === null + ? null + : [stickyCrossStart, stickyCrossStart - step] + .filter((start) => intervalIsFree(start, clusterCrossSize, occupied)) + .sort( + (a, b) => + Math.abs(a + clusterCrossSize / 2 - parentCrossCenter) - + Math.abs(b + clusterCrossSize / 2 - parentCrossCenter), + )[0] ?? null; + + const crossStart = + anchoredStart ?? + findNearestFreeSlot( + parentCrossCenter - clusterCrossSize / 2, + clusterCrossSize, + occupied, + ); + + const positions = Array.from({ length: count }, (_, index) => { + const cross = crossStart + index * step; + return horizontal + ? { x: primaryStart, y: cross } + : { x: cross, y: primaryStart }; + }); + + return { positions, crossStart }; +}; + +const cloneFlowchartNode = ( + template: ExcalidrawFlowchartNodeElement, + x: number, + y: number, +) => { + const node = newElement({ + type: template.type, + x, + y, + width: template.width, + height: template.height, + roundness: template.roundness, + roughness: template.roughness, + backgroundColor: template.backgroundColor, + strokeColor: template.strokeColor, + strokeWidth: template.strokeWidth, + opacity: template.opacity, + fillStyle: template.fillStyle, + strokeStyle: template.strokeStyle, }); invariant( - isFlowchartNodeElement(nextNode), + isFlowchartNodeElement(node), "not an ExcalidrawFlowchartNodeElement", ); - const bindingArrow = createBindingArrow( - element, - nextNode, - direction, - appState, - scene, - ); - - return { - nextNode, - bindingArrow, - }; + return node; }; -export const addNewNodes = ( +const addNewNodes = ( startNode: NonDeleted, appState: AppState, direction: LinkDirection, scene: Scene, numberOfNodes: number, + stickyCrossStart: number | null = null, ) => { - // always start from 0 and distribute evenly - const newNodes: NonDeletedExcalidrawElement[] = []; - - for (let i = 0; i < numberOfNodes; i++) { - let nextX: number; - let nextY: number; - if (direction === "left" || direction === "right") { - const totalHeight = - VERTICAL_OFFSET * (numberOfNodes - 1) + - numberOfNodes * startNode.height; - - const startY = startNode.y + startNode.height / 2 - totalHeight / 2; - - let offsetX = HORIZONTAL_OFFSET + startNode.width; - if (direction === "left") { - offsetX *= -1; - } - nextX = startNode.x + offsetX; - const offsetY = (VERTICAL_OFFSET + startNode.height) * i; - nextY = startY + offsetY; - } else { - const totalWidth = - HORIZONTAL_OFFSET * (numberOfNodes - 1) + - numberOfNodes * startNode.width; - const startX = startNode.x + startNode.width / 2 - totalWidth / 2; - let offsetY = VERTICAL_OFFSET + startNode.height; - - if (direction === "up") { - offsetY *= -1; - } - nextY = startNode.y + offsetY; - const offsetX = (HORIZONTAL_OFFSET + startNode.width) * i; - nextX = startX + offsetX; - } + const elementsMap = scene.getNonDeletedElementsMap(); + const obstacles = getConnectedFlowchartNodes(startNode, elementsMap).map( + (node) => aabbForElement(node, elementsMap), + ); - const nextNode = newElement({ - type: startNode.type, - x: nextX, - y: nextY, - // TODO: extract this to a util - width: startNode.width, - height: startNode.height, - roundness: startNode.roundness, - roughness: startNode.roughness, - backgroundColor: startNode.backgroundColor, - strokeColor: startNode.strokeColor, - strokeWidth: startNode.strokeWidth, - opacity: startNode.opacity, - fillStyle: startNode.fillStyle, - strokeStyle: startNode.strokeStyle, - }); - - invariant( - isFlowchartNodeElement(nextNode), - "not an ExcalidrawFlowchartNodeElement", - ); + const { positions, crossStart } = placeCluster( + startNode, + direction, + numberOfNodes, + obstacles, + stickyCrossStart, + ); + const nodes: NonDeletedExcalidrawElement[] = []; + for (const position of positions) { + const nextNode = cloneFlowchartNode(startNode, position.x, position.y); const bindingArrow = createBindingArrow( startNode, nextNode, @@ -363,11 +286,10 @@ export const addNewNodes = ( scene, ); - newNodes.push(nextNode); - newNodes.push(bindingArrow); + nodes.push(nextNode, bindingArrow); } - return newNodes; + return { nodes, crossStart }; }; const createBindingArrow = ( @@ -565,8 +487,8 @@ export class FlowChartNavigator { } const nodes = [ - ...getSuccessors(element, elementsMap, direction), - ...getPredecessors(element, elementsMap, direction), + ...FlowChartNavigator.getSuccessors(element, elementsMap, direction), + ...FlowChartNavigator.getPredecessors(element, elementsMap, direction), ]; /** @@ -615,8 +537,8 @@ export class FlowChartNavigator { const otherLinkedNodes = otherDirections .map((dir) => [ - ...getSuccessors(element, elementsMap, dir), - ...getPredecessors(element, elementsMap, dir), + ...FlowChartNavigator.getSuccessors(element, elementsMap, dir), + ...FlowChartNavigator.getPredecessors(element, elementsMap, dir), ]) .flat() .filter((linkedNode) => !this.visitedNodes.has(linkedNode.id)); @@ -633,12 +555,114 @@ export class FlowChartNavigator { return null; } + + private static getNodeRelatives( + type: "predecessors" | "successors", + node: ExcalidrawBindableElement, + elementsMap: ElementsMap, + direction: LinkDirection, + ) { + const items = [...elementsMap.values()].reduce( + ( + acc: { relative: ExcalidrawBindableElement; heading: Heading }[], + el, + ) => { + let oppositeBinding; + if ( + isElbowArrow(el) && + // we want check existence of the opposite binding, in the direction + // we're interested in + (oppositeBinding = + el[type === "predecessors" ? "startBinding" : "endBinding"]) && + // similarly, we need to filter only arrows bound to target node + el[type === "predecessors" ? "endBinding" : "startBinding"] + ?.elementId === node.id + ) { + const relative = elementsMap.get(oppositeBinding.elementId); + + if (!relative) { + return acc; + } + + invariant( + isBindableElement(relative), + "not an ExcalidrawBindableElement", + ); + + const edgePoint = ( + type === "predecessors" ? el.points[el.points.length - 1] : [0, 0] + ) as Readonly; + + const heading = headingForPointFromElement( + node, + aabbForElement(node, elementsMap), + [edgePoint[0] + el.x, edgePoint[1] + el.y] as Readonly, + ); + + acc.push({ + relative, + heading, + }); + } + return acc; + }, + [], + ); + + switch (direction) { + case "up": + return items + .filter((item) => compareHeading(item.heading, HEADING_UP)) + .map((item) => item.relative); + case "down": + return items + .filter((item) => compareHeading(item.heading, HEADING_DOWN)) + .map((item) => item.relative); + case "right": + return items + .filter((item) => compareHeading(item.heading, HEADING_RIGHT)) + .map((item) => item.relative); + case "left": + return items + .filter((item) => compareHeading(item.heading, HEADING_LEFT)) + .map((item) => item.relative); + } + } + + private static getSuccessors( + node: ExcalidrawBindableElement, + elementsMap: ElementsMap, + direction: LinkDirection, + ) { + return FlowChartNavigator.getNodeRelatives( + "successors", + node, + elementsMap, + direction, + ); + } + + private static getPredecessors( + node: ExcalidrawBindableElement, + elementsMap: ElementsMap, + direction: LinkDirection, + ) { + return FlowChartNavigator.getNodeRelatives( + "predecessors", + node, + elementsMap, + direction, + ); + } } export class FlowChartCreator { isCreatingChart: boolean = false; private numberOfNodes: number = 0; - private direction: LinkDirection | null = "right"; + private direction: LinkDirection | null = null; + // cross-axis anchor of the pending cluster, so growing it keeps the + // already-visible pending nodes in place + private clusterCrossStart: number | null = null; pendingNodes: PendingExcalidrawElements | null = null; createNodes( @@ -648,33 +672,28 @@ export class FlowChartCreator { scene: Scene, ) { const elementsMap = scene.getNonDeletedElementsMap(); - if (direction !== this.direction) { - const { nextNode, bindingArrow } = addNewNode( - startNode, - appState, - direction, - scene, - ); + if (direction !== this.direction) { this.numberOfNodes = 1; - this.isCreatingChart = true; - this.direction = direction; - this.pendingNodes = [nextNode, bindingArrow]; + this.clusterCrossStart = null; } else { this.numberOfNodes += 1; - const newNodes = addNewNodes( - startNode, - appState, - direction, - scene, - this.numberOfNodes, - ); - - this.isCreatingChart = true; - this.direction = direction; - this.pendingNodes = newNodes; } + const { nodes, crossStart } = addNewNodes( + startNode, + appState, + direction, + scene, + this.numberOfNodes, + this.clusterCrossStart, + ); + + this.isCreatingChart = true; + this.direction = direction; + this.clusterCrossStart = crossStart; + this.pendingNodes = nodes; + // add pending nodes to the same frame as the start node // if every pending node is at least intersecting with the frame if (startNode.frameId) { @@ -707,6 +726,7 @@ export class FlowChartCreator { this.pendingNodes = null; this.direction = null; this.numberOfNodes = 0; + this.clusterCrossStart = null; } } diff --git a/packages/element/tests/flowchart.test.tsx b/packages/element/tests/flowchart.test.tsx index 505cdd11..b3c36489 100644 --- a/packages/element/tests/flowchart.test.tsx +++ b/packages/element/tests/flowchart.test.tsx @@ -156,6 +156,280 @@ describe("flow chart creation", () => { expect(firstChildNode.x).toBe(secondChildNode.x); expect(secondChildNode.x).toBe(thirdChildNode.x); }); + + // regression for #8518: additional siblings must not overlap existing ones + it("does not overlap existing siblings when adding more children (down)", () => { + API.clearSelection(); + const parent = API.createElement({ + type: "rectangle", + width: 400, + height: 300, + }); + API.setElements([parent]); + API.setSelectedElements([parent]); + + for (let i = 0; i < 4; i++) { + API.setSelectedElements([parent]); + Keyboard.withModifierKeys({ ctrl: true }, () => { + Keyboard.keyPress(KEYS.ARROW_DOWN); + }); + Keyboard.keyUp(KEYS.CTRL_OR_CMD); + } + + const children = h.elements.filter( + (el) => el.type === "rectangle" && el.id !== parent.id, + ); + expect(children.length).toBe(4); + + // all siblings should sit on the same row (no vertical misalignment) + const ys = new Set(children.map((c) => c.y)); + expect(ys.size).toBe(1); + + // no two siblings should overlap horizontally + const sorted = [...children].sort((a, b) => a.x - b.x); + for (let i = 1; i < sorted.length; i++) { + expect(sorted[i].x).toBeGreaterThanOrEqual( + sorted[i - 1].x + sorted[i - 1].width, + ); + } + }); + + // regression for #8518: a second batch of children (added by holding the + // modifier and pressing the arrow several times) must clear the first batch + it("does not overlap a previous batch of children (down)", () => { + API.clearSelection(); + const parent = API.createElement({ + type: "rectangle", + width: 400, + height: 300, + }); + API.setElements([parent]); + + // hold the modifier and press down N times to create a batch at once + const addBatch = (count: number) => { + API.setSelectedElements([parent]); + Keyboard.withModifierKeys({ ctrl: true }, () => { + for (let i = 0; i < count; i++) { + Keyboard.keyPress(KEYS.ARROW_DOWN); + } + }); + Keyboard.keyUp(KEYS.CTRL_OR_CMD); + }; + + addBatch(3); + addBatch(2); + + const children = h.elements.filter( + (el) => el.type === "rectangle" && el.id !== parent.id, + ); + expect(children.length).toBe(5); + + const overlaps = (a: typeof children[0], b: typeof children[0]) => + a.x < b.x + b.width && + a.x + a.width > b.x && + a.y < b.y + b.height && + a.y + a.height > b.y; + + for (let i = 0; i < children.length; i++) { + for (let j = i + 1; j < children.length; j++) { + expect(overlaps(children[i], children[j])).toBe(false); + } + } + }); + + // regression for #8518: a new child must also clear nodes that aren't the + // start node's direct siblings but sit where it would land — e.g. a sibling + // reached through a shared parent + it("does not overlap a sibling reached through a shared parent", () => { + API.clearSelection(); + const parent = API.createElement({ + type: "rectangle", + width: 200, + height: 100, + }); + API.setElements([parent]); + + // two right-children stack into a column to the right of the parent + API.setSelectedElements([parent]); + Keyboard.withModifierKeys({ ctrl: true }, () => { + Keyboard.keyPress(KEYS.ARROW_RIGHT); + Keyboard.keyPress(KEYS.ARROW_RIGHT); + }); + Keyboard.keyUp(KEYS.CTRL_OR_CMD); + + const rightChildren = h.elements + .filter((el) => el.type === "rectangle" && el.id !== parent.id) + .sort((a, b) => a.y - b.y); + expect(rightChildren.length).toBe(2); + const [upper, lower] = rightChildren; + + // add a child below the upper sibling; it must not land on the lower one + // that sits directly beneath it + API.setSelectedElements([upper] as NonDeletedExcalidrawElement[]); + Keyboard.withModifierKeys({ ctrl: true }, () => { + Keyboard.keyPress(KEYS.ARROW_DOWN); + }); + Keyboard.keyUp(KEYS.CTRL_OR_CMD); + + const newChild = h.elements.filter( + (el) => + el.type === "rectangle" && + el.id !== parent.id && + el.id !== upper.id && + el.id !== lower.id, + )[0]; + expect(newChild).toBeTruthy(); + + const overlaps = (a: typeof newChild, b: typeof lower) => + a.x < b.x + b.width && + a.x + a.width > b.x && + a.y < b.y + b.height && + a.y + a.height > b.y; + + expect(overlaps(newChild, lower)).toBe(false); + }); +}); + +describe("flow chart band-search placement", () => { + const addChild = (parent: NonDeletedExcalidrawElement, key: string) => { + API.setSelectedElements([parent]); + Keyboard.withModifierKeys({ ctrl: true }, () => { + Keyboard.keyPress(key); + }); + Keyboard.keyUp(KEYS.CTRL_OR_CMD); + }; + + const children = (parent: NonDeletedExcalidrawElement) => + h.elements.filter((el) => el.type === "rectangle" && el.id !== parent.id); + + const pendingRects = () => + (h.app.flowchart.pendingNodes ?? []) + .filter((el) => el.type === "rectangle") + .map((el) => ({ x: el.x, y: el.y })); + + it("places the first child exactly one offset away in every direction", () => { + const cases = [ + { key: KEYS.ARROW_RIGHT, x: 300, y: 0 }, + { key: KEYS.ARROW_LEFT, x: -300, y: 0 }, + { key: KEYS.ARROW_DOWN, x: 0, y: 200 }, + { key: KEYS.ARROW_UP, x: 0, y: -200 }, + ]; + + for (const { key, x, y } of cases) { + API.clearSelection(); + const parent = API.createElement({ + type: "rectangle", + width: 200, + height: 100, + }); + API.setElements([parent]); + + addChild(parent, key); + + const [child] = children(parent); + expect(child).toBeTruthy(); + expect(child.x).toBe(x); + expect(child.y).toBe(y); + } + }); + + it("slides into the nearest free gap between obstacles without moving them", () => { + API.clearSelection(); + const parent = API.createElement({ + type: "rectangle", + width: 200, + height: 100, + }); + API.setElements([parent]); + + addChild(parent, KEYS.ARROW_DOWN); + addChild(parent, KEYS.ARROW_DOWN); + + const [c1, c2] = children(parent).sort((a, b) => a.x - b.x); + expect([c1.x, c2.x]).toEqual([0, 300]); + + // widen the gap between the siblings so a third child fits in between + API.updateElement(c2, { x: 600 }); + + addChild(parent, KEYS.ARROW_DOWN); + + const c3 = children(parent).find( + (el) => el.id !== c1.id && el.id !== c2.id, + )!; + expect({ x: c3.x, y: c3.y }).toEqual({ x: 300, y: 200 }); + + // the obstacles were not moved + expect({ x: c1.x, y: c1.y }).toEqual({ x: 0, y: 200 }); + expect({ x: c2.x, y: c2.y }).toEqual({ x: 600, y: 200 }); + }); + + it("keeps pending nodes in place while the cluster grows", () => { + API.clearSelection(); + const parent = API.createElement({ + type: "rectangle", + width: 200, + height: 100, + }); + API.setElements([parent]); + API.setSelectedElements([parent]); + + Keyboard.withModifierKeys({ ctrl: true }, () => { + Keyboard.keyPress(KEYS.ARROW_RIGHT); + expect(pendingRects()).toEqual([{ x: 300, y: 0 }]); + + Keyboard.keyPress(KEYS.ARROW_RIGHT); + expect(pendingRects()).toEqual([ + { x: 300, y: 0 }, + { x: 300, y: 200 }, + ]); + + Keyboard.keyPress(KEYS.ARROW_RIGHT); + expect(pendingRects()).toEqual([ + { x: 300, y: -200 }, + { x: 300, y: 0 }, + { x: 300, y: 200 }, + ]); + }); + Keyboard.keyUp(KEYS.CTRL_OR_CMD); + }); + + it("repositions the whole pending cluster when it no longer fits", () => { + API.clearSelection(); + const parent = API.createElement({ + type: "rectangle", + width: 200, + height: 100, + }); + API.setElements([parent]); + + // box in the space right of the parent, leaving room for a single node + addChild(parent, KEYS.ARROW_RIGHT); + const [c1] = children(parent); + API.updateElement(c1, { y: -300 }); + + addChild(parent, KEYS.ARROW_RIGHT); + const c2 = children(parent).find((el) => el.id !== c1.id)!; + expect({ x: c2.x, y: c2.y }).toEqual({ x: 300, y: 0 }); + API.updateElement(c2, { y: 300 }); + + API.setSelectedElements([parent]); + Keyboard.withModifierKeys({ ctrl: true }, () => { + Keyboard.keyPress(KEYS.ARROW_RIGHT); + expect(pendingRects()).toEqual([{ x: 300, y: 0 }]); + + // growing cannot extend at either end, so the cluster moves as a whole + Keyboard.keyPress(KEYS.ARROW_RIGHT); + expect(pendingRects()).toEqual([ + { x: 300, y: -100 }, + { x: 300, y: 100 }, + ]); + }); + Keyboard.keyUp(KEYS.CTRL_OR_CMD); + + // the obstacles were not moved + expect({ x: c1.x, y: c1.y }).toEqual({ x: 300, y: -300 }); + expect({ x: c2.x, y: c2.y }).toEqual({ x: 300, y: 300 }); + }); }); describe("flow chart navigation", () => { diff --git a/packages/excalidraw/actions/actionHistory.tsx b/packages/excalidraw/actions/actionHistory.tsx index 03c56db6..dfc2046b 100644 --- a/packages/excalidraw/actions/actionHistory.tsx +++ b/packages/excalidraw/actions/actionHistory.tsx @@ -35,7 +35,7 @@ const executeHistoryAction = ( !appState.newElement && !appState.selectedElementsAreBeingDragged && !appState.selectionElement && - !app.flowChartCreator.isCreatingChart + !app.flowchart.isCreatingChart ) { const result = updater(); diff --git a/packages/excalidraw/components/App.flowchart.ts b/packages/excalidraw/components/App.flowchart.ts new file mode 100644 index 00000000..1c9b33de --- /dev/null +++ b/packages/excalidraw/components/App.flowchart.ts @@ -0,0 +1,192 @@ +import { isArrowKey, KEYS } from "@excalidraw/common"; + +import { + makeNextSelectedElementIds, + CaptureUpdateAction, + FlowChartCreator, + FlowChartNavigator, + getSelectedElements, + isFlowchartNodeElement, + type LinkDirection, +} from "@excalidraw/element"; + +import type { + ExcalidrawElement, + NonDeletedExcalidrawElement, +} from "@excalidraw/element/types"; + +import type React from "react"; +import type App from "./App"; +import type { PendingExcalidrawElements } from "../types"; + +type FlowchartOperation = + | { type: "none" } + | { type: "canceled" } + | { type: "creating"; pending: PendingExcalidrawElements } + | { type: "navigating"; nodeId: ExcalidrawElement["id"] | null } + | { type: "committed"; nodes: PendingExcalidrawElements } + | { type: "navigationEnded" }; + +/** + * Captures the App state management for the flowchart functionality. + */ +export class AppFlowchart { + private creator = new FlowChartCreator(); + private navigator = new FlowChartNavigator(); + + constructor(private app: App) {} + + get pendingNodes() { + return this.creator.pendingNodes; + } + + get isCreatingChart() { + return this.creator.isCreatingChart; + } + + handleKeyEvent = (event: React.KeyboardEvent | KeyboardEvent): boolean => { + const operation = this.resolveKeyboardEventToOperation(event); + + switch (operation.type) { + case "none": + return false; + case "canceled": + this.app.triggerRender(true); + return true; + case "creating": + event.preventDefault(); + if (operation.pending.length) { + this.app.revealIfHidden(operation.pending); + } + return true; + case "navigating": { + event.preventDefault(); + const node = + operation.nodeId && + this.app.scene.getNonDeletedElementsMap().get(operation.nodeId); + if (node) { + this.selectAndReveal(node); + } + return true; + } + case "committed": { + if (operation.nodes.length) { + this.app.insertNewElements(operation.nodes); + } + + const firstNode = operation.nodes[0]; + if (firstNode) { + this.selectAndReveal(firstNode); + } + + this.captureUpdate(); + return true; + } + case "navigationEnded": + this.captureUpdate(); + return true; + } + }; + + private resolveKeyboardEventToOperation( + event: React.KeyboardEvent | KeyboardEvent, + ): FlowchartOperation { + const { creator, navigator, app } = this; + + if (event.type === "keydown") { + if (event.key === KEYS.ESCAPE && creator.isCreatingChart) { + creator.clear(); + return { type: "canceled" }; + } + + if (!isArrowKey(event.key)) { + return { type: "none" }; + } + + if (event[KEYS.CTRL_OR_CMD] && !event.shiftKey) { + const selectedElements = getSelectedElements( + app.scene.getNonDeletedElementsMap(), + app.state, + ); + + if ( + selectedElements.length === 1 && + isFlowchartNodeElement(selectedElements[0]) + ) { + creator.createNodes( + selectedElements[0], + app.state, + AppFlowchart.getLinkDirectionFromKey(event.key), + app.scene, + ); + } + + return { type: "creating", pending: creator.pendingNodes ?? [] }; + } + + if (event.altKey) { + const elementsMap = app.scene.getNonDeletedElementsMap(); + const selectedElements = getSelectedElements(elementsMap, app.state); + + if (selectedElements.length === 1) { + return { + type: "navigating", + nodeId: navigator.exploreByDirection( + selectedElements[0], + elementsMap, + AppFlowchart.getLinkDirectionFromKey(event.key), + ), + }; + } + } + + return { type: "none" }; + } + + // keyup: releasing a modifier finalizes the workflow it was driving; + // both can finalize on the same event + const navigationEnded = !event.altKey && navigator.isExploring; + if (navigationEnded) { + navigator.clear(); + } + + if (!event[KEYS.CTRL_OR_CMD] && creator.isCreatingChart) { + const nodes = creator.pendingNodes ?? []; + creator.clear(); + return { type: "committed", nodes }; + } + + return navigationEnded ? { type: "navigationEnded" } : { type: "none" }; + } + + private selectAndReveal(node: NonDeletedExcalidrawElement) { + this.app.setState((prevState) => ({ + selectedElementIds: makeNextSelectedElementIds( + { [node.id]: true }, + prevState, + ), + })); + this.app.revealIfHidden([node]); + } + + private captureUpdate() { + this.app.syncActionResult({ + captureUpdate: CaptureUpdateAction.IMMEDIATELY, + }); + } + + private static getLinkDirectionFromKey(key: string): LinkDirection { + switch (key) { + case KEYS.ARROW_UP: + return "up"; + case KEYS.ARROW_DOWN: + return "down"; + case KEYS.ARROW_RIGHT: + return "right"; + case KEYS.ARROW_LEFT: + return "left"; + default: + return "right"; + } + } +} diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 4f54c0fa..1432552d 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -154,7 +154,6 @@ import { isMagicFrameElement, isTextBindableContainer, isElbowArrow, - isFlowchartNodeElement, isBindableElement, isTextElement, getNormalizedDimensions, @@ -196,9 +195,6 @@ import { hitElementBoundingBoxOnly, hitElementItself, getVisibleSceneBounds, - FlowChartCreator, - FlowChartNavigator, - getLinkDirectionFromKey, cropElement, wrapText, isElementLink, @@ -448,6 +444,7 @@ import ConvertElementTypePopup, { } from "./ConvertElementTypePopup"; import { activeConfirmDialogAtom } from "./ActiveConfirmDialog"; +import { AppFlowchart } from "./App.flowchart"; import BraveMeasureTextError from "./BraveMeasureTextError"; import { ContextMenu, CONTEXT_MENU_SEPARATOR } from "./ContextMenu"; import { activeEyeDropperAtom } from "./EyeDropper"; @@ -723,8 +720,7 @@ class App extends React.Component { public onStateChange: OnStateChange = this.appStateObserver.onStateChange; - public flowChartCreator: FlowChartCreator = new FlowChartCreator(); - private flowChartNavigator: FlowChartNavigator = new FlowChartNavigator(); + public flowchart: AppFlowchart = new AppFlowchart(this); bindModeHandler: ReturnType | null = null; @@ -2399,7 +2395,7 @@ class App extends React.Component { elementsPendingErasure: this.elementsPendingErasure, pendingFlowchartNodes: - this.flowChartCreator.pendingNodes, + this.flowchart.pendingNodes, theme: this.state.theme, }} /> @@ -4556,6 +4552,38 @@ class App extends React.Component { ); }; + // scroll `elements` into view only if they aren't already fully visible. + // Targets their bounds rather than the elements so it also works for + // elements not yet committed to the canvas. + revealIfHidden = (elements: NonDeletedExcalidrawElement[]) => { + if ( + !elements.length || + isElementCompletelyInViewport( + elements, + this.canvas.width / window.devicePixelRatio, + this.canvas.height / window.devicePixelRatio, + { + offsetLeft: this.state.offsetLeft, + offsetTop: this.state.offsetTop, + scrollX: this.state.scrollX, + scrollY: this.state.scrollY, + zoom: this.state.zoom, + }, + this.scene.getNonDeletedElementsMap(), + this.getViewportOffsets(), + ) + ) { + return; + } + + this.setViewport({ + target: getCommonBounds(elements), + fit: "scale-down", + animation: { duration: 300 }, + offsets: { ui: true }, + }); + }; + private maybeUnfollowRemoteUser = () => { if (this.state.userToFollow) { this.setState({ userToFollow: null }); @@ -4794,7 +4822,7 @@ class App extends React.Component { }); }; - private triggerRender = ( + public triggerRender = ( /** force always re-renders canvas even if no change */ force?: boolean, ) => { @@ -5065,122 +5093,9 @@ class App extends React.Component { } } - if ( - event.key === KEYS.ESCAPE && - this.flowChartCreator.isCreatingChart - ) { - this.flowChartCreator.clear(); - this.triggerRender(true); - return; - } - - const arrowKeyPressed = isArrowKey(event.key); - - if (event[KEYS.CTRL_OR_CMD] && arrowKeyPressed && !event.shiftKey) { - event.preventDefault(); - - const selectedElements = getSelectedElements( - this.scene.getNonDeletedElementsMap(), - this.state, - ); - - if ( - selectedElements.length === 1 && - isFlowchartNodeElement(selectedElements[0]) - ) { - this.flowChartCreator.createNodes( - selectedElements[0], - this.state, - getLinkDirectionFromKey(event.key), - this.scene, - ); - } - - if ( - this.flowChartCreator.pendingNodes?.length && - !isElementCompletelyInViewport( - this.flowChartCreator.pendingNodes, - this.canvas.width / window.devicePixelRatio, - this.canvas.height / window.devicePixelRatio, - { - offsetLeft: this.state.offsetLeft, - offsetTop: this.state.offsetTop, - scrollX: this.state.scrollX, - scrollY: this.state.scrollY, - zoom: this.state.zoom, - }, - this.scene.getNonDeletedElementsMap(), - this.getViewportOffsets(), - ) - ) { - this.setViewport({ - target: getCommonBounds(this.flowChartCreator.pendingNodes), - fit: "scale-down", - animation: { duration: 300 }, - offsets: { ui: true }, - }); - } - + if (this.flowchart.handleKeyEvent(event)) { return; } - - if (event.altKey) { - const selectedElements = getSelectedElements( - this.scene.getNonDeletedElementsMap(), - this.state, - ); - - if (selectedElements.length === 1 && arrowKeyPressed) { - event.preventDefault(); - - const nextId = this.flowChartNavigator.exploreByDirection( - selectedElements[0], - this.scene.getNonDeletedElementsMap(), - getLinkDirectionFromKey(event.key), - ); - - if (nextId) { - this.setState((prevState) => ({ - selectedElementIds: makeNextSelectedElementIds( - { - [nextId]: true, - }, - prevState, - ), - })); - - const nextNode = this.scene - .getNonDeletedElementsMap() - .get(nextId); - - if ( - nextNode && - !isElementCompletelyInViewport( - [nextNode], - this.canvas.width / window.devicePixelRatio, - this.canvas.height / window.devicePixelRatio, - { - offsetLeft: this.state.offsetLeft, - offsetTop: this.state.offsetTop, - scrollX: this.state.scrollX, - scrollY: this.state.scrollY, - zoom: this.state.zoom, - }, - this.scene.getNonDeletedElementsMap(), - this.getViewportOffsets(), - ) - ) { - this.setViewport({ - target: nextNode, - fit: "scale-down", - animation: { duration: 300 }, - offsets: { ui: true }, - }); - } - } - return; - } - } } if ( @@ -5703,64 +5618,7 @@ class App extends React.Component { this.setState({ suggestedBinding: null }); } - if (!event.altKey) { - if (this.flowChartNavigator.isExploring) { - this.flowChartNavigator.clear(); - this.syncActionResult({ - captureUpdate: CaptureUpdateAction.IMMEDIATELY, - }); - } - } - - if (!event[KEYS.CTRL_OR_CMD]) { - if (this.flowChartCreator.isCreatingChart) { - if (this.flowChartCreator.pendingNodes?.length) { - this.insertNewElements(this.flowChartCreator.pendingNodes); - } - - const firstNode = this.flowChartCreator.pendingNodes?.[0]; - - if (firstNode) { - this.setState((prevState) => ({ - selectedElementIds: makeNextSelectedElementIds( - { - [firstNode.id]: true, - }, - prevState, - ), - })); - - if ( - !isElementCompletelyInViewport( - [firstNode], - this.canvas.width / window.devicePixelRatio, - this.canvas.height / window.devicePixelRatio, - { - offsetLeft: this.state.offsetLeft, - offsetTop: this.state.offsetTop, - scrollX: this.state.scrollX, - scrollY: this.state.scrollY, - zoom: this.state.zoom, - }, - this.scene.getNonDeletedElementsMap(), - this.getViewportOffsets(), - ) - ) { - this.setViewport({ - target: firstNode, - fit: "scale-down", - animation: { duration: 300 }, - offsets: { ui: true }, - }); - } - } - - this.flowChartCreator.clear(); - this.syncActionResult({ - captureUpdate: CaptureUpdateAction.IMMEDIATELY, - }); - } - } + this.flowchart.handleKeyEvent(event); }); // We purposely widen the `tool` type so this helper can be called with @@ -7200,7 +7058,7 @@ class App extends React.Component { ); }; - private insertNewElements = (elements: readonly ExcalidrawElement[]) => { + public insertNewElements = (elements: readonly ExcalidrawElement[]) => { if (!elements.length) { return; } diff --git a/packages/excalidraw/types.ts b/packages/excalidraw/types.ts index 700f0698..0213780f 100644 --- a/packages/excalidraw/types.ts +++ b/packages/excalidraw/types.ts @@ -900,7 +900,7 @@ export type AppClassProperties = { onMagicframeToolSelect: App["onMagicframeToolSelect"]; getName: App["getName"]; dismissLinearEditor: App["dismissLinearEditor"]; - flowChartCreator: App["flowChartCreator"]; + flowchart: App["flowchart"]; getEffectiveGridSize: App["getEffectiveGridSize"]; setPlugins: App["setPlugins"]; plugins: App["plugins"];