fix z-index action to account for deleted elems and add tests (#1077)

src/actions/actionZindex.tsx

@@ -5,7 +5,6 @@ import {
   moveAllLeft,
   moveAllRight,
 } from "../zindex";
-import { getSelectedIndices } from "../scene";
 import { KEYS, isDarwin } from "../keys";
 import { t } from "../i18n";
 import { getShortcutKey } from "../utils";
@@ -16,15 +15,69 @@ import {
   sendToBack,
   bringForward,
 } from "../components/icons";
+import { ExcalidrawElement } from "../element/types";
+import { AppState } from "../types";
+
+function getElementIndices(
+  direction: "left" | "right",
+  elements: readonly ExcalidrawElement[],
+  appState: AppState,
+) {
+  const selectedIndices: number[] = [];
+  let deletedIndicesCache: number[] = [];
+
+  function cb(element: ExcalidrawElement, index: number) {
+    if (element.isDeleted) {
+      // we want to build an array of deleted elements that are preceeding
+      //  a selected element so that we move them together
+      deletedIndicesCache.push(index);
+    } else {
+      if (appState.selectedElementIds[element.id]) {
+        selectedIndices.push(...deletedIndicesCache, index);
+      }
+      // always empty cache of deleted elements after either pushing a group
+      //  of selected/deleted elements, of after encountering non-deleted elem
+      deletedIndicesCache = [];
+    }
+  }
+
+  // sending back → select contiguous deleted elements that are to the left of
+  //  selected element(s)
+  if (direction === "left") {
+    let i = -1;
+    const len = elements.length;
+    while (++i < len) {
+      cb(elements[i], i);
+    }
+    // moving to front → loop from right to left so that we don't need to
+    //  backtrack when gathering deleted elements
+  } else {
+    let i = elements.length;
+    while (--i > -1) {
+      cb(elements[i], i);
+    }
+  }
+  // sort in case we were gathering indexes from right to left
+  return selectedIndices.sort();
+}
+
+function moveElements(
+  func: typeof moveOneLeft,
+  elements: readonly ExcalidrawElement[],
+  appState: AppState,
+) {
+  const _elements = elements.slice();
+  const direction =
+    func === moveOneLeft || func === moveAllLeft ? "left" : "right";
+  const indices = getElementIndices(direction, _elements, appState);
+  return func(_elements, indices);
+}
 
 export const actionSendBackward = register({
   name: "sendBackward",
   perform: (elements, appState) => {
     return {
-      elements: moveOneLeft(
+      elements: moveElements(moveOneLeft, elements, appState),
-        [...elements],
-        getSelectedIndices(elements, appState),
-      ),
       appState,
       commitToHistory: true,
     };
@@ -49,10 +102,7 @@ export const actionBringForward = register({
   name: "bringForward",
   perform: (elements, appState) => {
     return {
-      elements: moveOneRight(
+      elements: moveElements(moveOneRight, elements, appState),
-        [...elements],
-        getSelectedIndices(elements, appState),
-      ),
       appState,
       commitToHistory: true,
     };
@@ -77,10 +127,7 @@ export const actionSendToBack = register({
   name: "sendToBack",
   perform: (elements, appState) => {
     return {
-      elements: moveAllLeft(
+      elements: moveElements(moveAllLeft, elements, appState),
-        [...elements],
-        getSelectedIndices(elements, appState),
-      ),
       appState,
       commitToHistory: true,
     };
@@ -113,10 +160,7 @@ export const actionBringToFront = register({
   name: "bringToFront",
   perform: (elements, appState) => {
     return {
-      elements: moveAllRight(
+      elements: moveElements(moveAllRight, elements, appState),
-        [...elements],
-        getSelectedIndices(elements, appState),
-      ),
       appState,
       commitToHistory: true,
     };

src/components/App.tsx

@@ -300,6 +300,10 @@ export class App extends React.Component<any, AppState> {
             return this.setState(...args);
           },
         },
+        app: {
+          configurable: true,
+          value: this,
+        },
       });
     }
 
@@ -2498,7 +2502,9 @@ declare global {
     h: {
       elements: readonly ExcalidrawElement[];
       state: AppState;
+      setState: React.Component<any, AppState>["setState"];
       history: SceneHistory;
+      app: InstanceType<typeof App>;
     };
   }
 }

src/scene/index.ts

@@ -1,6 +1,5 @@
 export { isOverScrollBars } from "./scrollbars";
 export {
-  getSelectedIndices,
   deleteSelectedElements,
   isSomeElementSelected,
   getElementsWithinSelection,

src/scene/selection.ts

@@ -49,19 +49,6 @@ export function deleteSelectedElements(
   };
 }
 
-export function getSelectedIndices(
-  elements: readonly ExcalidrawElement[],
-  appState: AppState,
-) {
-  const selectedIndices: number[] = [];
-  elements.forEach((element, index) => {
-    if (appState.selectedElementIds[element.id]) {
-      selectedIndices.push(index);
-    }
-  });
-  return selectedIndices;
-}
-
 export function isSomeElementSelected(
   elements: readonly ExcalidrawElement[],
   appState: AppState,

src/tests/zindex.test.tsx

@@ -0,0 +1,187 @@
+import React from "react";
+import ReactDOM from "react-dom";
+import { render } from "./test-utils";
+import { App } from "../components/App";
+import { reseed } from "../random";
+import { newElement } from "../element";
+import {
+  actionSendBackward,
+  actionBringForward,
+  actionBringToFront,
+  actionSendToBack,
+} from "../actions";
+
+// Unmount ReactDOM from root
+ReactDOM.unmountComponentAtNode(document.getElementById("root")!);
+
+beforeEach(() => {
+  localStorage.clear();
+  reseed(7);
+});
+
+const { h } = window;
+
+function populateElements(
+  elements: { id: string; isDeleted?: boolean; isSelected?: boolean }[],
+) {
+  const selectedElementIds: any = {};
+
+  h.elements = elements.map(({ id, isDeleted = false, isSelected = false }) => {
+    const element: Mutable<ReturnType<typeof newElement>> = newElement({
+      type: "rectangle",
+      x: 100,
+      y: 100,
+      strokeColor: h.state.currentItemStrokeColor,
+      backgroundColor: h.state.currentItemBackgroundColor,
+      fillStyle: h.state.currentItemFillStyle,
+      strokeWidth: h.state.currentItemStrokeWidth,
+      roughness: h.state.currentItemRoughness,
+      opacity: h.state.currentItemOpacity,
+    });
+    element.id = id;
+    element.isDeleted = isDeleted;
+    if (isSelected) {
+      selectedElementIds[element.id] = true;
+    }
+    return element;
+  });
+
+  h.setState({
+    ...h.state,
+    selectedElementIds,
+  });
+
+  return selectedElementIds;
+}
+
+type Actions =
+  | typeof actionBringForward
+  | typeof actionSendBackward
+  | typeof actionBringToFront
+  | typeof actionSendToBack;
+
+function assertZindex({
+  elements,
+  operations,
+}: {
+  elements: { id: string; isDeleted?: true; isSelected?: true }[];
+  operations: [Actions, string[]][];
+}) {
+  const selectedElementIds = populateElements(elements);
+  operations.forEach(([action, expected]) => {
+    h.app.actionManager.executeAction(action);
+    expect(h.elements.map((element) => element.id)).toEqual(expected);
+    expect(h.state.selectedElementIds).toEqual(selectedElementIds);
+  });
+}
+
+describe("z-index manipulation", () => {
+  it("send back", () => {
+    render(<App />);
+
+    assertZindex({
+      elements: [
+        { id: "A" },
+        { id: "B", isDeleted: true },
+        { id: "C", isDeleted: true },
+        { id: "D", isSelected: true },
+      ],
+      operations: [
+        [actionSendBackward, ["B", "C", "D", "A"]],
+        // noop
+        [actionSendBackward, ["B", "C", "D", "A"]],
+      ],
+    });
+
+    assertZindex({
+      elements: [
+        { id: "A", isDeleted: true },
+        { id: "B" },
+        { id: "C", isDeleted: true },
+        { id: "D", isSelected: true },
+      ],
+      operations: [[actionSendBackward, ["A", "C", "D", "B"]]],
+    });
+
+    assertZindex({
+      elements: [
+        { id: "A" },
+        { id: "B", isDeleted: true },
+        { id: "C", isDeleted: true },
+        { id: "D", isSelected: true },
+        { id: "E", isSelected: true },
+      ],
+      operations: [[actionSendBackward, ["B", "C", "D", "E", "A"]]],
+    });
+
+    assertZindex({
+      elements: [
+        { id: "A" },
+        { id: "B" },
+        { id: "C", isDeleted: true },
+        { id: "D", isDeleted: true },
+        { id: "E", isSelected: true },
+        { id: "F" },
+        { id: "G", isSelected: true },
+      ],
+      operations: [
+        [actionSendBackward, ["A", "C", "D", "E", "B", "G", "F"]],
+        [actionSendBackward, ["C", "D", "E", "A", "G", "B", "F"]],
+      ],
+    });
+  });
+
+  it("bring forward", () => {
+    render(<App />);
+
+    assertZindex({
+      elements: [
+        { id: "A", isSelected: true },
+        { id: "B", isDeleted: true },
+        { id: "C", isDeleted: true },
+        { id: "D" },
+        { id: "E", isSelected: true },
+        { id: "F", isDeleted: true },
+        { id: "G" },
+      ],
+      operations: [
+        [actionBringForward, ["D", "A", "B", "C", "G", "E", "F"]],
+        [actionBringForward, ["D", "G", "A", "B", "C", "E", "F"]],
+      ],
+    });
+  });
+
+  it("bring to front", () => {
+    render(<App />);
+
+    assertZindex({
+      elements: [
+        { id: "A", isSelected: true },
+        { id: "B", isDeleted: true },
+        { id: "C", isDeleted: true },
+        { id: "D" },
+        { id: "E", isSelected: true },
+        { id: "F", isDeleted: true },
+        { id: "G" },
+      ],
+      operations: [[actionBringToFront, ["D", "G", "A", "B", "C", "E", "F"]]],
+    });
+  });
+
+  it("send to back", () => {
+    render(<App />);
+
+    assertZindex({
+      elements: [
+        { id: "A" },
+        { id: "B", isDeleted: true },
+        { id: "C" },
+        { id: "D", isDeleted: true },
+        { id: "E", isSelected: true },
+        { id: "F", isDeleted: true },
+        { id: "G" },
+      ],
+      operations: [[actionSendToBack, ["D", "E", "A", "B", "C", "F", "G"]]],
+    });
+  });
+});