fix(studio): persist canvas z-order actions correctly for static elements

An adversarial review of the canvas context-menu z-order pipeline (Bring to Front / Forward / Backward / Send to Back) found the resolver math sound but the glue between the menu and the commit hook broken: - The menu optimistically wrote style.zIndex AND position: relative to the live elements BEFORE the commit hook ran. The hook decides whether to persist position by checking getComputedStyle(el).position === 'static' — always false after the pre-apply — so the position patch was never persisted on the menu path and the reorder silently reverted at the post-commit reload for any nested/static element (root clips survive only because the runtime forces position:absolute). The same pre-apply made the failure rollback capture the already-mutated values, restoring the broken state on persist errors. The menu no longer pre-applies; the hook owns the live writes (it already applied both synchronously) and now sees true priors. Siblings without a persistable identity still get their z applied live-only so a renumber stays visually coherent. - The commit hook's entry.key store-sync plumbing had zero production callers; the store zIndex went stale until full reload. All three callers (canvas menu via PreviewOverlays, timeline lane z-sync, LayersPanel) now derive and pass the timeline store key (new deriveTimelineStoreKey helper). - patchElementBatch discarded the server's per-patch matched[]; unresolvable siblings persisted partially and silently. Unmatched targets now warn and report save-failure telemetry (z-reorder-unmatched) without rolling back the matched subset. - template/noscript elements counted as painting siblings, so renumber fallbacks wrote z-index/position into <template> tags in the source file. Excluded from the sibling family. - The default undo coalesce key merged DISTINCT z actions within 300ms into one undo entry; the action kind is now part of the key (LayersPanel drags keep coalescing within a drag; explicit lane-move gesture keys untouched). - rectsIntersect comment claimed touching rects intersect; the strict inequalities say otherwise — comment fixed.
2026-09-03 23:03:09 +00:00 · 2026-07-13 16:48:52 -07:00
parent 19139b91ed
commit 84963ea8ba
14 changed files with 491 additions and 47 deletions
@@ -3,7 +3,11 @@ import React, { act } from "react";
 import { createRoot, type Root } from "react-dom/client";
 import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 import { installReactActEnvironment, makeSelection } from "../../hooks/domSelectionTestHarness";
+import { resolveZIndexEntries } from "../nle/PreviewOverlays";
+import { useElementLifecycleOps } from "../../hooks/useElementLifecycleOps";
+import type { DomEditPatchBatch } from "../../hooks/domEditCommitTypes";
 import { CanvasContextMenu } from "./CanvasContextMenu";
+import type { ZOrderAction, ZOrderPatch } from "./canvasContextMenuZOrder";
 import type { DomEditSelection } from "./domEditing";

 installReactActEnvironment();
@@ -24,7 +28,7 @@ afterEach(() => {

 function renderMenu(props: {
  selection: DomEditSelection;
-  onApplyZIndex?: () => void;
+  onApplyZIndex?: (patches: ZOrderPatch[], action: ZOrderAction) => void;
  onDelete?: (selection: DomEditSelection) => void;
 }) {
  root = createRoot(host);
@@ -113,3 +117,110 @@ describe("CanvasContextMenu — handler gating", () => {
    expect(document.body.querySelector(".border-t")).toBeNull();
  });
 });
+
+// ── Menu z-action → commit path (wired the way PreviewOverlays wires the app) ──
+
+function pressMenuItem(label: string) {
+  const button = zOrderButtons().find((b) => b.textContent === label);
+  expect(button).toBeDefined();
+  act(() => {
+    button!.dispatchEvent(
+      new PointerEvent("pointerdown", { bubbles: true, cancelable: true, button: 0 }),
+    );
+  });
+}
+
+/** Target (static, earlier in DOM) below an equal-z sibling — z action must renumber. */
+function makeStaticFamily() {
+  const parent = document.createElement("div");
+  const target = document.createElement("div");
+  target.id = "target";
+  // In happy-dom an unset computed position is "" (not "static"), which would
+  // skip the commit hook's static-position injection; declare it explicitly so
+  // the test exercises the browser default.
+  target.style.position = "static";
+  const other = document.createElement("div");
+  other.id = "other";
+  parent.append(target, other);
+  document.body.append(parent);
+  return { parent, target, other };
+}
+
+interface CapturedBatchCall {
+  batches: DomEditPatchBatch[];
+  options: { label: string; coalesceKey: string };
+}
+
+/** Mount the REAL commit hook (persist layer mocked at commitDomEditPatchBatches). */
+function renderCommitHook(captured: CapturedBatchCall[]) {
+  type Commit = ReturnType<typeof useElementLifecycleOps>["handleDomZIndexReorderCommit"];
+  let commit: Commit | undefined;
+  function Harness() {
+    ({ handleDomZIndexReorderCommit: commit } = useElementLifecycleOps({
+      activeCompPath: "index.html",
+      showToast: vi.fn(),
+      writeProjectFile: vi.fn(async () => {}),
+      domEditSaveTimestampRef: { current: 0 },
+      editHistory: { recordEdit: vi.fn(async () => {}) },
+      projectIdRef: { current: null },
+      reloadPreview: vi.fn(),
+      clearDomSelection: vi.fn(),
+      commitDomEditPatchBatches: async (batches, options) => {
+        captured.push({ batches, options });
+      },
+    }));
+    return null;
+  }
+  const hookHost = document.createElement("div");
+  document.body.append(hookHost);
+  const hookRoot = createRoot(hookHost);
+  act(() => hookRoot.render(<Harness />));
+  return { commit: commit!, cleanup: () => act(() => hookRoot.unmount()) };
+}
+
+describe("CanvasContextMenu — z-action commit path", () => {
+  it("never mutates live styles itself and persists the position patch for a static element", async () => {
+    const { target } = makeStaticFamily();
+    const selection = makeSelection("Target", target);
+    const captured: CapturedBatchCall[] = [];
+    const { commit, cleanup } = renderCommitHook(captured);
+
+    // Wire onApplyZIndex the way the app does (PreviewOverlays → the commit
+    // hook), asserting the menu has NOT touched the DOM when it fires — the
+    // hook must capture true pre-change styles for its rollback.
+    const stylesAtApply: Array<{ zIndex: string; position: string }> = [];
+    renderMenu({
+      selection,
+      onApplyZIndex: (patches, action) => {
+        stylesAtApply.push({ zIndex: target.style.zIndex, position: target.style.position });
+        const { entries } = resolveZIndexEntries(selection, patches);
+        void commit(entries, undefined, action);
+      },
+    });
+
+    await act(async () => pressMenuItem("Bring forward"));
+
+    // The menu left the element pristine; only the commit hook wrote styles.
+    expect(stylesAtApply).toEqual([{ zIndex: "", position: "static" }]);
+    expect(target.style.zIndex).toBe("1");
+    expect(target.style.position).toBe("relative");
+
+    // The persisted payload carries BOTH the z-index and the injected position,
+    // so the reorder survives the post-commit reloadPreview().
+    expect(captured).toHaveLength(1);
+    const targetPatch = captured[0]?.batches
+      .flatMap((batch) => batch.patches)
+      .find((patch) => patch.target.id === "target");
+    expect(targetPatch?.operations).toEqual(
+      expect.arrayContaining([
+        { type: "inline-style", property: "z-index", value: "1" },
+        { type: "inline-style", property: "position", value: "relative" },
+      ]),
+    );
+    // F7: the action kind is part of the default undo coalesce key, so two
+    // different menu actions never merge into one undo step.
+    expect(captured[0]?.options.coalesceKey).toContain("bring-forward");
+
+    cleanup();
+  });
+});
@@ -7,10 +7,13 @@
 * useContextMenuDismiss.
 *
 * ── Wiring (z-order persistence) ─────────────────────────────────────────────
- * Z-index changes are applied optimistically to the live iframe element(s) via
+ * Z-index changes are resolved against the live iframe DOM via
 * `resolveZOrderChange`, which returns a MULTI-element patch list (tie-aware:
 * moving a target past an equal-z sibling can require renumbering the affected
- * set). The patches are surfaced through the `onApplyZIndex` prop.
+ * set). The patches are surfaced through the `onApplyZIndex` prop; the menu
+ * itself never mutates element styles — handleDomZIndexReorderCommit applies
+ * the live z-index (and injects position when needed) in the same synchronous
+ * flow, and captures the TRUE prior styles for its failure rollback.
 *
 * The prop MUST be wired at the call site to route through the full persist
 * path. PreviewOverlays.tsx builds the per-patch PatchTargets (the selected
@@ -27,6 +30,7 @@ import { useContextMenuDismiss } from "../../hooks/useContextMenuDismiss";
 import {
  isZOrderActionEnabled,
  resolveZOrderChange,
+  type ZOrderAction,
  type ZOrderPatch,
 } from "./canvasContextMenuZOrder";

@@ -38,12 +42,15 @@ interface CanvasContextMenuProps {
  selection: DomEditSelection;
  onClose: () => void;
  /**
-   * Called with the resolved z-order patch list after an optimistic DOM update.
-   * Each patch is an { element, zIndex } pair (the target and, when a renumber
-   * is needed, affected siblings). Wire to handleDomZIndexReorderCommit (see
-   * module-level wiring comment).
+   * Called with the resolved z-order patch list and the menu action that
+   * produced it (the action feeds the undo coalesce key, so two DIFFERENT
+   * actions never merge into one undo step). Each patch is an
+   * { element, zIndex } pair (the target and, when a renumber is needed,
+   * affected siblings). The menu does NOT touch the live DOM — wire to
+   * handleDomZIndexReorderCommit, which applies the live styles itself
+   * (see module-level wiring comment).
   */
-  onApplyZIndex?: (patches: ZOrderPatch[]) => void;
+  onApplyZIndex?: (patches: ZOrderPatch[], action: ZOrderAction) => void;
  /**
   * Delete the selected element. Wire to handleDomEditElementDelete from
   * useDomEditActionsContext — same path as the Delete/Backspace hotkey.
@@ -93,20 +100,15 @@ export const CanvasContextMenu = memo(function CanvasContextMenu({
  const el = selection.element;

  function handleZAction(action: ZAction) {
-    // No persist handler → do NOT touch the live iframe DOM. An optimistic
-    // write with nothing to persist just reverts on the next reload.
    if (!onApplyZIndex) return;
    const patches = resolveZOrderChange(el, action);
    if (patches === null) return;
-    // Optimistic update — visible immediately even before persist completes.
-    for (const patch of patches) {
-      patch.element.style.zIndex = String(patch.zIndex);
-      const view = patch.element.ownerDocument?.defaultView;
-      if (view && view.getComputedStyle(patch.element).position === "static") {
-        patch.element.style.position = "relative";
-      }
-    }
-    onApplyZIndex(patches);
+    // Do NOT pre-apply styles here: handleDomZIndexReorderCommit writes the
+    // live z-index (and injects position:relative for static elements) in the
+    // same synchronous flow, so feedback is still instant — and it must read
+    // the PRE-change styles itself, both to capture true rollback values and
+    // to detect a static position that needs persisting.
+    onApplyZIndex(patches, action);
    onClose();
  }

@@ -27,7 +27,7 @@ import { useDomEditCompositionRect } from "./useDomEditCompositionRect";
 import { useMountEffect } from "../../hooks/useMountEffect";
 import { startOffCanvasIndicatorRefresh } from "./offCanvasIndicatorRefresh";
 import { CanvasContextMenu } from "./CanvasContextMenu";
-import type { ZOrderPatch } from "./canvasContextMenuZOrder";
+import type { ZOrderAction, ZOrderPatch } from "./canvasContextMenuZOrder";
 import { getPreviewTargetFromPointer } from "../../utils/studioPreviewHelpers";

 // Re-exports for external consumers — preserving existing import paths.
@@ -91,12 +91,17 @@ interface DomEditOverlayProps {
   */
  onDeleteSelection?: (selection: DomEditSelection) => void;
  /**
-   * Called with the resolved z-order patch list after an optimistic DOM update.
-   * The patch list is tie-aware and may include sibling elements (see
-   * canvasContextMenuZOrder). Wire to handleDomZIndexReorderCommit from
+   * Called with the resolved z-order patch list and the menu action that
+   * produced it (feeds the undo coalesce key). The patch list is tie-aware and
+   * may include sibling elements (see canvasContextMenuZOrder); the live DOM is
+   * NOT yet mutated. Wire to handleDomZIndexReorderCommit from
   * useDomEditActionsContext. See CanvasContextMenu.tsx module comment.
   */
-  onApplyZIndex?: (selection: DomEditSelection, patches: ZOrderPatch[]) => void;
+  onApplyZIndex?: (
+    selection: DomEditSelection,
+    patches: ZOrderPatch[],
+    action: ZOrderAction,
+  ) => void;
 }

 // fallow-ignore-next-line complexity
@@ -562,8 +567,8 @@ export const DomEditOverlay = memo(function DomEditOverlay({
          }
          onApplyZIndex={
            onApplyZIndex
-              ? (patches) => {
-                  onApplyZIndex(contextMenu.sel, patches);
+              ? (patches, action) => {
+                  onApplyZIndex(contextMenu.sel, patches, action);
                }
              : undefined
          }
@@ -15,6 +15,7 @@ import {
 import { Layers } from "../../icons/SystemIcons";
 import { useLayerDrag, isLayerDraggable, type LayerReorderEvent } from "./useLayerDrag";
 import { computeReorderZValues, getElementZIndex } from "../../player/lib/layerOrdering";
+import { deriveTimelineStoreKey } from "../../player/lib/timelineElementHelpers";

 const TAG_ICONS: Record<string, string> = {
  video: "Vi",
@@ -280,9 +281,17 @@ export const LayersPanel = memo(function LayersPanel() {
        selector: layer.selector,
        selectorIndex: layer.selectorIndex,
        sourceFile: layer.sourceFile,
+        key: deriveTimelineStoreKey({
+          domId: layer.id,
+          selector: layer.selector,
+          selectorIndex: layer.selectorIndex,
+          sourceFile: layer.sourceFile,
+        }),
      }));

-      handleDomZIndexReorderCommit(entries);
+      // "layer-drag" keeps consecutive drops of the same sibling set coalescing
+      // into one undo step, without merging with a context-menu z action.
+      handleDomZIndexReorderCommit(entries, undefined, "layer-drag");
    },
    [handleDomZIndexReorderCommit],
  );
@@ -375,6 +375,27 @@ describe("resolveZOrderChange – excludes non-painting siblings", () => {
    expect(order.indexOf("target")).toBeLessThan(order.indexOf("a"));
  });

+  it("ignores <template>/<noscript> siblings in the family", () => {
+    // A renumber fallback once wrote z-index/position into <template> source
+    // markup because templates entered the sibling family. They never paint —
+    // exclude them like audio/script/style.
+    const parent = document.createElement("div");
+    const a = makeEl("a", "0");
+    const template = document.createElement("template");
+    template.style.zIndex = "2";
+    const noscript = document.createElement("noscript");
+    const target = makeEl("target", "0");
+    parent.append(a, template, noscript, target);
+
+    const patches = resolveZOrderPatches(target, "send-to-back");
+    for (const p of patches) {
+      expect(p.element).not.toBe(template);
+      expect(p.element).not.toBe(noscript);
+    }
+    const order = renderOrderIds(parent, { a, target }, patches);
+    expect(order.indexOf("target")).toBeLessThan(order.indexOf("a"));
+  });
+
  it("a lone painting element beside only non-painting siblings has no family → null", () => {
    const parent = document.createElement("div");
    const target = makeEl("target", "1");
@@ -81,8 +81,18 @@ function isElementNode(node: Node): node is HTMLElement {
 * z-index onto the qa-clean audio element, and counting it as a sibling skews the
 * renumber for the visible elements. `<script>/<style>/<link>/<meta>` are also
 * non-painting and could otherwise pad the family / eat a z slot.
+ * `<template>/<noscript>` never paint either — letting them in meant renumber
+ * fallbacks wrote z-index/position into template source markup.
 */
-const NON_PAINTING_TAGS = new Set(["AUDIO", "SCRIPT", "STYLE", "LINK", "META"]);
+const NON_PAINTING_TAGS = new Set([
+  "AUDIO",
+  "SCRIPT",
+  "STYLE",
+  "LINK",
+  "META",
+  "TEMPLATE",
+  "NOSCRIPT",
+]);

 /** A painting element: an element node whose tag actually renders pixels. */
 function isPaintingElement(node: Node): node is HTMLElement {
@@ -112,7 +122,7 @@ function getFamily(target: HTMLElement): { entries: RenderEntry[]; targetIndex:
  return { entries, targetIndex };
 }

-/** True if two DOM bounding rects intersect (even if touching). */
+/** True if two DOM bounding rects strictly overlap (rects that merely touch do NOT intersect). */
 function rectsIntersect(
  a: { left: number; top: number; right: number; bottom: number },
  b: { left: number; top: number; right: number; bottom: number },
@@ -18,6 +18,7 @@ import {
 import { readStudioUiPreferences } from "../../utils/studioUiPreferences";
 import { readHfId, type DomEditSelection } from "../editor/domEditing";
 import { buildStableSelector } from "../editor/domEditingDom";
+import { deriveTimelineStoreKey } from "../../player/lib/timelineElementHelpers";
 import type { BlockPreviewInfo } from "../sidebar/BlocksTab";
 import type { GestureRecordingState } from "../editor/GestureRecordControl";
 import type { ReactNode } from "react";
@@ -38,6 +39,8 @@ type ZIndexReorderEntry = {
  selector?: string;
  selectorIndex?: number;
  sourceFile: string;
+  /** Timeline store key — lets the commit update the store zIndex synchronously. */
+  key?: string;
 };

 /** Can this element be robustly re-targeted for a persisted z change? */
@@ -58,6 +61,12 @@ function selectedZIndexEntry(sel: DomEditSelection, zIndex: number): ZIndexReord
    selector: sel.selector,
    selectorIndex: sel.selectorIndex,
    sourceFile: sel.sourceFile,
+    key: deriveTimelineStoreKey({
+      domId: sel.id ?? undefined,
+      selector: sel.selector,
+      selectorIndex: sel.selectorIndex,
+      sourceFile: sel.sourceFile,
+    }),
  };
 }

@@ -75,7 +84,15 @@ function siblingZIndexEntry(
  const id = element.id || undefined;
  const selector = buildStableSelector(element);
  if (!canTargetZIndexElement(element, id, selector)) return null;
-  return { element, zIndex, id, selector, selectorIndex: undefined, sourceFile };
+  return {
+    element,
+    zIndex,
+    id,
+    selector,
+    selectorIndex: undefined,
+    sourceFile,
+    key: deriveTimelineStoreKey({ domId: id, selector, sourceFile }),
+  };
 }

 /** Short human-readable label for a dropped sibling, for the console warning below. */
@@ -88,14 +105,16 @@ function describeZIndexElement(element: HTMLElement): string {
 }

 // Resolve z-index patches into commit entries; a sibling with no stable
-// id/selector can't be written to source, so it is collected as a label for the
-// revert-on-reload warning instead.
-function resolveZIndexEntries(
+// id/selector can't be written to source, so it is returned as `dropped` for
+// the revert-on-reload warning (and a live-only style write, so the resolved
+// stacking order still renders coherently). Exported so tests can drive the
+// menu → commit path through the same wiring the app uses.
+export function resolveZIndexEntries(
  sel: DomEditSelection,
  patches: ReadonlyArray<{ element: HTMLElement; zIndex: number }>,
-): { entries: ZIndexReorderEntry[]; droppedLabels: string[] } {
+): { entries: ZIndexReorderEntry[]; dropped: Array<{ element: HTMLElement; zIndex: number }> } {
  const entries: ZIndexReorderEntry[] = [];
-  const droppedLabels: string[] = [];
+  const dropped: Array<{ element: HTMLElement; zIndex: number }> = [];
  for (const patch of patches) {
    if (patch.element === sel.element) {
      entries.push(selectedZIndexEntry(sel, patch.zIndex));
@@ -103,9 +122,9 @@ function resolveZIndexEntries(
    }
    const entry = siblingZIndexEntry(patch.element, patch.zIndex, sel.sourceFile);
    if (entry) entries.push(entry);
-    else droppedLabels.push(describeZIndexElement(patch.element));
+    else dropped.push(patch);
  }
-  return { entries, droppedLabels };
+  return { entries, dropped };
 }

 // fallow-ignore-next-line complexity
@@ -205,19 +224,20 @@ export function PreviewOverlays({
        onRotationCommit={handleDomRotationCommit}
        onStyleCommit={handleDomStyleCommit}
        onDeleteSelection={handleDomEditElementDelete}
-        onApplyZIndex={(sel, patches) => {
-          const { entries, droppedLabels } = resolveZIndexEntries(sel, patches);
-          if (droppedLabels.length > 0) {
-            // The optimistic z-index has already been applied live at this point —
-            // these siblings just won't be written to source, so they'll revert to
-            // their prior stacking order on the next reload with no other signal.
+        onApplyZIndex={(sel, patches, action) => {
+          const { entries, dropped } = resolveZIndexEntries(sel, patches);
+          if (dropped.length > 0) {
+            // These siblings can't be written to source. Apply their live z
+            // anyway so the resolved stacking order renders coherently — it
+            // just reverts to the prior order on the next reload.
+            for (const patch of dropped) patch.element.style.zIndex = String(patch.zIndex);
            console.warn(
              "[studio] z-index reorder: dropping sibling(s) with no stable id/selector " +
                "(will revert on reload):",
-              droppedLabels.join(", "),
+              dropped.map((patch) => describeZIndexElement(patch.element)).join(", "),
            );
          }
-          if (entries.length > 0) handleDomZIndexReorderCommit(entries);
+          if (entries.length > 0) handleDomZIndexReorderCommit(entries, undefined, action);
        }}
        gridVisible={snapPrefs.gridVisible}
        gridSpacing={snapPrefs.gridSpacing}