fix(studio): settle boundary retimes, delete every keyframed tween, tighten the test locks

Review follow-ups on the expanded keyframe lanes. Writer: - `onMoveKeyframe`'s flat-tween boundary branch answered `true` the moment it dispatched update-meta, so a rejected write left the diamond parked at its drop position. `observeGsapMutation` now resolves to whether the mutation landed and the boundary branch returns it, matching the other branches. - "Delete All Keyframes" cleared only the first keyframed tween on the layer, so a layer with position AND opacity keyframes kept half of them. It now walks every keyframed tween, serially, through the clicked element's selection. - The post-convert lookup in `commitFlatViaKeyframes` matched by target selector, which picks an arbitrary tween when a target carries several. Match by id first. Interaction and a11y: - A rejected retime whose commit settled after a newer drag reverted the selection to its own source keyframe, undoing a retime the user could see. The revert now only runs while it is still the lane's latest gesture. - Diamonds key on the authored identity instead of index plus rendered clip-%, so a neighbour's retime no longer remounts the button mid-drag. - The disclosure caret gets `aria-controls` on an always-mounted lanes container, and both it and the property-group toggle grow to the 24x24 WCAG 2.2 minimum. - `LayerDisclosureRow` takes the same adaptive `columnWidth` as its sibling lane rows instead of hardcoding LABEL_COL_W over the canvas. Test locks: - The timeline callbacks harness resolves a DISTINCT selection per element, so the clicked-element writes are actually pinned; three assertions that passed either way now name the clicked element's selection. - New: null-selection aborts every mutation, delete-all covers both tweens, a rejected boundary retime reports `false`, and a stale revert leaves selection. - The playhead-percentage assertion checks 25, not `expect.any(Number)` (which also accepts NaN); ease segments assert their label ORDER, not just that the three curves differ; the collapsed-diamond callback asserts the whole target. - Dropped a duplicate `selection override` describe left by a rebase.
2026-09-11 23:00:03 +00:00 · 2026-07-28 01:19:05 +02:00
parent c20c5366da
commit 8470b88aa1
10 changed files with 325 additions and 74 deletions
@@ -1,6 +1,6 @@
 import { CaretRight } from "@phosphor-icons/react";
 import type { TimelineElement } from "../store/playerStore";
-import { LABEL_COL_W, TRACK_H } from "./timelineLayout";
+import { TRACK_H } from "./timelineLayout";
 import { TrackClipCount } from "./TrackClipCount";

 // Layer row (Figma order: disclosure ▸/▾, diamond, name) — the disclosure lives
@@ -10,6 +10,8 @@ export function LayerDisclosureRow({
  clipCount,
  isExpanded,
  gutterBackground,
+  columnWidth,
+  lanesId,
  onToggleClipExpanded,
  children,
 }: {
@@ -17,6 +19,11 @@ export function LayerDisclosureRow({
  clipCount: number;
  isExpanded: boolean;
  gutterBackground: string;
+  /** Same adaptive width the lane rows use: a narrowed header column must not
+   *  leave this row hanging over the clips it labels. */
+  columnWidth: number;
+  /** Id of the element holding the lanes this row's caret expands. */
+  lanesId: string;
  onToggleClipExpanded: () => void;
  /** Trailing controls that act on the LAYER (the visibility eye), not on a lane. */
  children?: React.ReactNode;
@@ -26,7 +33,7 @@ export function LayerDisclosureRow({
    <div
      className="absolute left-0 top-0 flex items-center gap-1.5 overflow-hidden px-1.5 text-[11px]"
      style={{
-        width: LABEL_COL_W,
+        width: columnWidth,
        height: TRACK_H,
        color: "#ffffff",
        background: gutterBackground,
@@ -35,9 +42,12 @@ export function LayerDisclosureRow({
      <button
        type="button"
        aria-expanded={isExpanded}
+        aria-controls={lanesId}
        aria-label={`${isExpanded ? "Collapse" : "Expand"} ${name} keyframes`}
        title={`${isExpanded ? "Collapse" : "Expand"} keyframe lanes`}
-        className="flex h-5 w-4 shrink-0 items-center justify-center rounded border-0 bg-transparent p-0 text-white/55 hover:text-white focus-visible:outline focus-visible:outline-1 focus-visible:outline-[#3CE6AC]"
+        // h-6 w-6 = the 24x24 WCAG 2.2 minimum target. The caret glyph stays 11px;
+        // only the hit box grows.
+        className="flex h-6 w-6 shrink-0 items-center justify-center rounded border-0 bg-transparent p-0 text-white/55 hover:text-white focus-visible:outline focus-visible:outline-1 focus-visible:outline-[#3CE6AC]"
        onPointerDown={(event) => event.stopPropagation()}
        onClick={(event) => {
          event.stopPropagation();
@@ -310,6 +310,89 @@ describe("TimelineClipDiamonds", () => {
    act(() => root.unmount());
  });

+  it("leaves the selection alone when a stale retime fails after a newer drag", async () => {
+    const onClickKeyframe = vi.fn();
+    let failFirstDrag: (() => void) | undefined;
+    const onMoveKeyframe = vi
+      .fn()
+      .mockImplementationOnce(
+        () =>
+          new Promise<boolean>((resolve) => {
+            failFirstDrag = () => resolve(false);
+          }),
+      )
+      .mockResolvedValue(true);
+    const host = document.createElement("div");
+    document.body.append(host);
+    const root = createRoot(host);
+    act(() => {
+      root.render(
+        <TimelineDiamondLane
+          keyframesData={{
+            format: "percentage",
+            keyframes: [
+              {
+                percentage: 0,
+                tweenPercentage: 0,
+                propertyGroup: "position",
+                animationId: "anim-1",
+                properties: { x: 0 },
+              },
+              {
+                percentage: 50,
+                tweenPercentage: 50,
+                propertyGroup: "position",
+                animationId: "anim-1",
+                properties: { x: 100 },
+              },
+              {
+                percentage: 100,
+                tweenPercentage: 100,
+                propertyGroup: "position",
+                animationId: "anim-1",
+                properties: { x: 200 },
+              },
+            ],
+          }}
+          clipWidthPx={200}
+          clipHeightPx={48}
+          accentColor="#4ba3d2"
+          isSelected
+          currentPercentage={0}
+          elementId="clip-1"
+          selectedKeyframes={new Set()}
+          onClickKeyframe={onClickKeyframe}
+          onMoveKeyframe={onMoveKeyframe}
+          groupAware
+        />,
+      );
+    });
+    const diamond = host.querySelector<HTMLButtonElement>('button[title="50%"]');
+
+    // Two drags back to back; the first one's commit is still in flight.
+    act(() => {
+      diamond!.dispatchEvent(
+        pointerEvent("pointerdown", { bubbles: true, button: 0, clientX: 100 }),
+      );
+      diamond!.dispatchEvent(pointerEvent("pointerup", { bubbles: true, button: 0, clientX: 150 }));
+      diamond!.dispatchEvent(
+        pointerEvent("pointerdown", { bubbles: true, button: 0, clientX: 150 }),
+      );
+      diamond!.dispatchEvent(pointerEvent("pointerup", { bubbles: true, button: 0, clientX: 170 }));
+    });
+    onClickKeyframe.mockClear();
+
+    await act(async () => {
+      failFirstDrag?.();
+      await Promise.resolve();
+    });
+
+    // The stale failure must not drag the selection back to the first drag's
+    // source: the second retime, which the user can see, owns it now.
+    expect(onClickKeyframe).not.toHaveBeenCalled();
+    act(() => root.unmount());
+  });
+
  it("composes a rapid second retime from the pending position", () => {
    const onMoveKeyframe = vi.fn().mockResolvedValue(true);
    const host = document.createElement("div");
@@ -63,6 +63,10 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
  // the first away, once per mounted lane.
  pendingRetimeRef.current ??= new Map();
  const pendingRetimes = pendingRetimeRef.current;
+  // The most recent retime dispatched from this lane, whichever diamond it came
+  // from. Selection is lane-wide, so "is my revert still relevant" is a lane-wide
+  // question, not a per-keyframe one.
+  const latestRetimeRef = useRef<{ clipPct: number; tweenPct: number } | null>(null);
  useEffect(() => {
    // Clear a pending entry once the authoritative cache reflects THAT keyframe
    // at ~its destination. Match by tolerance, not equality: cache writers round
@@ -354,6 +358,7 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
              : target;
            const pending = { clipPct: res.toClipPct, tweenPct: newTweenPct };
            pendingRetimes.set(kfKey, pending);
+            latestRetimeRef.current = pending;
            const clearPending = () => {
              if (pendingRetimes.get(kfKey) === pending) {
                pendingRetimes.delete(kfKey);
@@ -365,8 +370,13 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
            // position strands the playhead + selection on a keyframe that does
            // not exist there.
            const revertRetime = () => {
+              // Only the newest gesture owns the selection. A rejected first drag
+              // whose commit settles after a second one started would otherwise
+              // park the selection back on ITS source keyframe, undoing a retime
+              // the user has already made and moving the playhead with it.
+              const isLatest = latestRetimeRef.current === pending;
              clearPending();
-              onClickKeyframe?.(fromTarget);
+              if (isLatest) onClickKeyframe?.(fromTarget);
            };
            void onMoveKeyframe?.(fromTarget, res.toClipPct).then((committed) => {
              if (!committed) revertRetime();
@@ -386,7 +396,11 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({

        return (
          <button
-            key={`${i}-${kf.percentage}`}
+            // Key on the authored identity (tween-%), not the rendered clip-% or
+            // the row index: a clip resize, a neighbour's retime, or a re-sort
+            // changes both of those without changing WHICH keyframe this is, and
+            // a key change remounts the button mid-drag (losing pointer capture).
+            key={`${kf.animationId ?? i}:${kf.propertyGroup ?? ""}:${kf.tweenPercentage ?? kf.percentage}`}
            type="button"
            className="absolute"
            data-keyframe-group={groupAware ? kf.propertyGroup : undefined}
@@ -346,6 +346,16 @@ describe("TimelinePropertyLanes", () => {
    );
    expect(paths).toHaveLength(3);
    expect(new Set(paths).size).toBe(3);
+    // Uniqueness alone passes even when the curves are swapped between segments.
+    // Each segment is labelled with the ease it draws, so pin the ORDER: a
+    // segment carries the ease of the keyframe it arrives at.
+    expect(
+      segments.map((segment) => revealEaseButton(segment)?.getAttribute("aria-label")),
+    ).toEqual([
+      "Edit none easing",
+      "Edit power2.out easing",
+      "Edit custom(M0,0 C0.1,0.2 0.3,0.9 1,1) easing",
+    ]);
    act(() => root.unmount());
  });

@@ -391,6 +401,8 @@ describe("TimelinePropertyLanes", () => {

  it("keeps the collapsed TimelineClipDiamonds positions and callback contract unchanged", () => {
    const onClickKeyframe = vi.fn();
+    const COLLAPSED_IDENTITY = { animationId: "position-tween", propertyGroup: "position" };
+    const COLLAPSED_TARGET = { ...COLLAPSED_IDENTITY, percentage: 50, tweenPercentage: 50 };
    const host = document.createElement("div");
    document.body.append(host);
    const root = createRoot(host);
@@ -400,8 +412,13 @@ describe("TimelinePropertyLanes", () => {
          keyframesData={{
            format: "percentage",
            keyframes: [
-              { percentage: 0, properties: { x: 0 } },
-              { percentage: 50, properties: { x: 100 } },
+              { percentage: 0, ...COLLAPSED_IDENTITY, tweenPercentage: 0, properties: { x: 0 } },
+              {
+                percentage: 50,
+                ...COLLAPSED_IDENTITY,
+                tweenPercentage: 50,
+                properties: { x: 100 },
+              },
            ],
          }}
          clipWidthPx={200}
@@ -410,7 +427,7 @@ describe("TimelinePropertyLanes", () => {
          isSelected
          currentPercentage={-10}
          elementId="clip-1"
-          selectedKeyframes={new Set(["clip-1:50"])}
+          selectedKeyframes={new Set([timelineKeyframeSelectionKey("clip-1", COLLAPSED_TARGET)])}
          onClickKeyframe={onClickKeyframe}
        />,
      );
@@ -424,10 +441,10 @@ describe("TimelinePropertyLanes", () => {
    act(() => {
      diamonds[1]?.dispatchEvent(new MouseEvent("pointerup", { bubbles: true, button: 0 }));
    });
-    expect(onClickKeyframe).toHaveBeenCalledWith(
-      "clip-1",
-      expect.objectContaining({ percentage: 50 }),
-    );
+    // The whole identity, not just the percentage: objectContaining on the one
+    // field passes even when the animation id / property group / tween-% the
+    // diamond-identity refactor added are dropped on the way out.
+    expect(onClickKeyframe).toHaveBeenCalledWith("clip-1", COLLAPSED_TARGET);
    act(() => root.unmount());
  });
 });
@@ -236,7 +236,8 @@ function PropertyGroupHeaderRow({
          aria-pressed={!!navigation.currentKeyframe}
          aria-label={`${navigation.currentKeyframe ? "Remove" : "Add"} ${label} keyframe`}
          title={`${navigation.currentKeyframe ? "Remove" : "Add"} ${label} keyframe`}
-          className="flex h-5 w-4 shrink-0 items-center justify-center border-0 bg-transparent p-0 text-[11px] text-[#3CE6AC] focus-visible:outline focus-visible:outline-1 focus-visible:outline-[#3CE6AC]"
+          // h-6 w-6 = the 24x24 WCAG 2.2 minimum target; the ◆ glyph stays 11px.
+          className="flex h-6 w-6 shrink-0 items-center justify-center border-0 bg-transparent p-0 text-[11px] text-[#3CE6AC] focus-visible:outline focus-visible:outline-1 focus-visible:outline-[#3CE6AC]"
          onClick={(event) => {
            // Same as the disclosure caret and the eye: a control in the label
            // column owns its click, it does not also hit the track row behind it.
@@ -286,6 +287,7 @@ export function TimelineTrackHeader({
  // owns the gutter past it, so a 0% diamond isn't clipped by this panel).
  const showTrackLabel = contentOrigin >= LABEL_COL_W;
  const isKeyframeLayer = !!keyframeClip && lanes.length > 0;
+  const lanesId = `timeline-lanes-track-${trackNumber}`;

  return (
    <div
@@ -319,6 +321,8 @@ export function TimelineTrackHeader({
            clipCount={clipCount}
            isExpanded={isExpanded}
            gutterBackground={theme.gutterBackground}
+            columnWidth={showTrackLabel ? LABEL_COL_W : contentOrigin}
+            lanesId={lanesId}
            onToggleClipExpanded={onToggleClipExpanded}
          >
            {/* The eye belongs to the LAYER, so it lives on the always-mounted
@@ -333,22 +337,25 @@ export function TimelineTrackHeader({
              onToggle={onToggleTrackHidden}
            />
          </LayerDisclosureRow>
-          {isExpanded &&
-            lanes.map((lane, laneIndex) => (
-              <PropertyGroupHeaderRow
-                key={lane.group}
-                lane={lane}
-                laneIndex={laneIndex}
-                isLastLane={laneIndex === lanes.length - 1}
-                expandedElement={keyframeClip}
-                currentTime={currentTime}
-                clipPercentage={clipPercentage}
-                gutterBackground={theme.gutterBackground}
-                columnWidth={showTrackLabel ? LABEL_COL_W : contentOrigin}
-                onTogglePropertyGroupKeyframe={onTogglePropertyGroupKeyframe}
-                onSeek={onSeek}
-              />
-            ))}
+          {/* Always mounted so the caret's aria-controls resolves in both states. */}
+          <div id={lanesId}>
+            {isExpanded &&
+              lanes.map((lane, laneIndex) => (
+                <PropertyGroupHeaderRow
+                  key={lane.group}
+                  lane={lane}
+                  laneIndex={laneIndex}
+                  isLastLane={laneIndex === lanes.length - 1}
+                  expandedElement={keyframeClip}
+                  currentTime={currentTime}
+                  clipPercentage={clipPercentage}
+                  gutterBackground={theme.gutterBackground}
+                  columnWidth={showTrackLabel ? LABEL_COL_W : contentOrigin}
+                  onTogglePropertyGroupKeyframe={onTogglePropertyGroupKeyframe}
+                  onSeek={onSeek}
+                />
+              ))}
+          </div>
        </>
      )}
    </div>