mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-03 04:38:33 +00:00
fix(studio): scope lane ids per timeline and stop inventing a track row
Two latent defects in the announcement path this branch adds. - lanesId was keyed by render row alone, so a second TimelineLanes on the page (a mini-timeline beside the main one) would mint the same timeline-lanes-track-0 and every caret's aria-controls would resolve to whichever instance mounted first. The prefix now comes from useId, with the colons stripped so the id stays a legal CSS selector. - trackDisplayNumber returned trackOrder.length + 1 for a key it could not find, which is indistinguishable from a real row: the label announced a row the user can see is wrong and nothing upstream could tell it had guessed. It returns null now, and trackDisplaySuffix drops the number from the label rather than inventing one.
This commit is contained in:
@@ -1,7 +1,11 @@
|
||||
import { useCallback } from "react";
|
||||
import { usePlayerStore, type TimelineElement } from "../player";
|
||||
import { useExpandedTimelineElements } from "../player/hooks/useExpandedTimelineElements";
|
||||
import { timelineTrackOrder, trackDisplayNumber } from "../player/components/timelineTrackDisplay";
|
||||
import {
|
||||
timelineTrackOrder,
|
||||
trackDisplayNumber,
|
||||
trackDisplaySuffix,
|
||||
} from "../player/components/timelineTrackDisplay";
|
||||
import { saveProjectFilesWithHistory } from "../utils/studioFileHistory";
|
||||
import { readTagSnippetByTarget, type PatchOperation } from "../utils/sourcePatcher";
|
||||
import {
|
||||
@@ -211,13 +215,15 @@ export async function toggleTimelineTrackHidden({
|
||||
}: ToggleTimelineTrackHiddenInput): Promise<string[]> {
|
||||
// `track` is the fractional sort key the callback needs; the history entry is
|
||||
// read by a human, so it gets the display row instead.
|
||||
const displayNumber = trackDisplayNumber(timelineTrackOrder(timelineElements), track);
|
||||
const suffix = trackDisplaySuffix(
|
||||
trackDisplayNumber(timelineTrackOrder(timelineElements), track),
|
||||
);
|
||||
return setElementsHidden({
|
||||
projectId,
|
||||
activeCompPath,
|
||||
elements: timelineElements.filter((element) => element.track === track),
|
||||
hidden,
|
||||
label: hidden ? `Hide track ${displayNumber}` : `Show track ${displayNumber}`,
|
||||
label: hidden ? `Hide track${suffix}` : `Show track${suffix}`,
|
||||
previewIframe,
|
||||
writeProjectFile,
|
||||
recordEdit,
|
||||
|
||||
@@ -215,6 +215,30 @@ describe("TimelineLanes disclosure target", () => {
|
||||
act(() => view.root.unmount());
|
||||
});
|
||||
|
||||
// Two timelines on one page (a mini-timeline in a modal beside the main one)
|
||||
// both minted `timeline-lanes-track-0`, so every caret's aria-controls
|
||||
// resolved to whichever instance mounted first.
|
||||
it("mints lane ids that do not collide with a second TimelineLanes on the page", () => {
|
||||
const first = renderLanes({ animations: ANIMATIONS, expandedClipIds: ["clip-a"] });
|
||||
const second = renderLanes({ animations: ANIMATIONS, expandedClipIds: ["clip-a"] });
|
||||
|
||||
const idsFor = (host: HTMLElement) =>
|
||||
Array.from(host.querySelectorAll("button[aria-controls]")).map((caret) =>
|
||||
caret.getAttribute("aria-controls"),
|
||||
);
|
||||
const firstIds = idsFor(first.host);
|
||||
const secondIds = idsFor(second.host);
|
||||
|
||||
expect(firstIds.length).toBeGreaterThan(0);
|
||||
expect(firstIds.some((id) => secondIds.includes(id))).toBe(false);
|
||||
// Still a legal CSS id selector: the aria-controls lookups above use `#id`.
|
||||
for (const id of [...firstIds, ...secondIds]) {
|
||||
expect(id).toMatch(/^[A-Za-z][\w-]*$/);
|
||||
}
|
||||
act(() => first.root.unmount());
|
||||
act(() => second.root.unmount());
|
||||
});
|
||||
|
||||
// The passenger branch wraps [clip, lanes] in a transformed div that re-renders
|
||||
// on every pointer move. An unstable key there remounts the lanes and drops the
|
||||
// in-flight drag.
|
||||
|
||||
@@ -1,10 +1,11 @@
|
||||
import { useId } from "react";
|
||||
import { BeatStrip, BeatBackgroundLines } from "./BeatStrip";
|
||||
import { TimelineClip } from "./TimelineClip";
|
||||
import { TimelineClipDiamonds } from "./TimelineClipDiamonds";
|
||||
import { TimelinePropertyLanes } from "./TimelinePropertyLanes";
|
||||
import { TimelineTrackHeader } from "./TimelineTrackHeader";
|
||||
import { resolveTrackKeyframeClip } from "./useTimelineTrackLayout";
|
||||
import { trackDisplayNumber } from "./timelineTrackDisplay";
|
||||
import { trackDisplayNumber, trackDisplaySuffix } from "./timelineTrackDisplay";
|
||||
import { clipTimingStart } from "../../hooks/gsapShared";
|
||||
import { getTimelineEditCapabilities, resolveBlockedTimelineEditIntent } from "./timelineEditing";
|
||||
import { CLIP_Y, CLIP_HANDLE_W, TRACK_H, getTimelineRowHeight } from "./timelineLayout";
|
||||
@@ -89,6 +90,12 @@ export function TimelineLanes({
|
||||
onRazorSplit,
|
||||
onRazorSplitAll,
|
||||
}: TimelineLanesProps) {
|
||||
// Per-INSTANCE, so two timelines on one page (a mini-timeline in a modal
|
||||
// beside the main one) cannot both mint `...-track-0` and have every caret's
|
||||
// aria-controls resolve to whichever mounted first. React's useId embeds
|
||||
// colons, which are legal in an id and in aria-controls but need escaping in
|
||||
// a CSS `#id` selector, so they come out here and the prefix stays plain.
|
||||
const lanesIdPrefix = `timeline-lanes${useId().replaceAll(":", "")}`;
|
||||
const expandedClipIds = usePlayerStore((s) => s.expandedClipIds);
|
||||
const toggleClipExpanded = usePlayerStore((s) => s.toggleClipExpanded);
|
||||
const toggleClipExpandedTracked = (key: string) => {
|
||||
@@ -137,7 +144,7 @@ export function TimelineLanes({
|
||||
// the disclosure: the caret in the sticky header and the diamond lanes
|
||||
// on the canvas. Keyed by display row, not by `trackNum`, which is a
|
||||
// fractional sort key and would mint ids like `...-0.16666666666666666`.
|
||||
const lanesId = `timeline-lanes-track-${row}`;
|
||||
const lanesId = `${lanesIdPrefix}-track-${row}`;
|
||||
return (
|
||||
<div
|
||||
key={trackNum}
|
||||
@@ -154,7 +161,10 @@ export function TimelineLanes({
|
||||
// key, so it stays out of every label and in every callback.
|
||||
trackDisplayNumber={displayNumber}
|
||||
trackLabel={
|
||||
els[0]?.label ?? els[0]?.domId ?? els[0]?.id ?? `Track ${displayNumber}`
|
||||
els[0]?.label ??
|
||||
els[0]?.domId ??
|
||||
els[0]?.id ??
|
||||
`Track${trackDisplaySuffix(displayNumber)}`
|
||||
}
|
||||
lanesId={lanesId}
|
||||
contentOrigin={contentOrigin}
|
||||
|
||||
@@ -15,6 +15,7 @@ import {
|
||||
type TimelinePropertyLane,
|
||||
} from "./trackHeaderLaneState";
|
||||
import { valueReadout } from "./trackHeaderLaneValues";
|
||||
import { trackDisplaySuffix } from "./timelineTrackDisplay";
|
||||
|
||||
interface TimelineTrackHeaderProps {
|
||||
/** The track's real key: a FRACTIONAL z-order sort value. Routes callbacks;
|
||||
@@ -22,8 +23,9 @@ interface TimelineTrackHeaderProps {
|
||||
trackNumber: number;
|
||||
/** The track's 1-based position in the rendered order: the only number safe
|
||||
* to put in a label. Announcing `trackNumber` read out "track
|
||||
* 0.16666666666666666". */
|
||||
trackDisplayNumber: number;
|
||||
* 0.16666666666666666". Null when the key has no row, which drops the number
|
||||
* from the label rather than inventing one (see trackDisplayNumber). */
|
||||
trackDisplayNumber: number | null;
|
||||
trackLabel: string;
|
||||
/** Id of the canvas-side lanes element the disclosure caret expands. Minted by
|
||||
* TimelineLanes, which is the one place that sees both subtrees. */
|
||||
@@ -55,14 +57,15 @@ function VisibilityButton({
|
||||
}: {
|
||||
hidden: boolean;
|
||||
trackNumber: number;
|
||||
trackDisplayNumber: number;
|
||||
trackDisplayNumber: number | null;
|
||||
visible: boolean;
|
||||
onToggle: TimelineEditCallbacks["onToggleTrackHidden"];
|
||||
}) {
|
||||
if (!visible) return <span aria-hidden="true" className="h-6 w-6 shrink-0" />;
|
||||
// Display number in the text, real key in the callback. The two must not be
|
||||
// conflated in either direction.
|
||||
const label = hidden ? `Show track ${trackDisplayNumber}` : `Hide track ${trackDisplayNumber}`;
|
||||
const suffix = trackDisplaySuffix(trackDisplayNumber);
|
||||
const label = hidden ? `Show track${suffix}` : `Hide track${suffix}`;
|
||||
return (
|
||||
<button
|
||||
type="button"
|
||||
|
||||
@@ -0,0 +1,35 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { timelineTrackOrder, trackDisplayNumber, trackDisplaySuffix } from "./timelineTrackDisplay";
|
||||
|
||||
describe("timelineTrackOrder", () => {
|
||||
it("is the ascending distinct key order, fractional sub-comp keys included", () => {
|
||||
const elements = [{ track: 1 }, { track: 0 }, { track: 1 / 6 }, { track: 1 }];
|
||||
|
||||
expect(timelineTrackOrder(elements)).toEqual([0, 1 / 6, 1]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("trackDisplayNumber", () => {
|
||||
it("is the key's 1-based row, not the fractional key itself", () => {
|
||||
const order = timelineTrackOrder([{ track: 0 }, { track: 1 / 6 }, { track: 1 }]);
|
||||
|
||||
expect(trackDisplayNumber(order, 1 / 6)).toBe(2);
|
||||
expect(trackDisplayNumber(order, 1)).toBe(3);
|
||||
});
|
||||
|
||||
it("is null for a key with no row rather than a plausible-looking one", () => {
|
||||
// The old end-row fallback announced "track 4" for a track the user cannot
|
||||
// see at row 4, and nothing upstream could tell that apart from a real row.
|
||||
expect(trackDisplayNumber([0, 1, 2], 7)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe("trackDisplaySuffix", () => {
|
||||
it("names the row when there is one", () => {
|
||||
expect(`Hide track${trackDisplaySuffix(3)}`).toBe("Hide track 3");
|
||||
});
|
||||
|
||||
it("drops the number entirely when there is no row", () => {
|
||||
expect(`Hide track${trackDisplaySuffix(null)}`).toBe("Hide track");
|
||||
});
|
||||
});
|
||||
@@ -15,10 +15,24 @@ export function timelineTrackOrder(elements: readonly { track: number }[]): numb
|
||||
}
|
||||
|
||||
/**
|
||||
* A track key's 1-based display row. A key not in `trackOrder` (a drag preview
|
||||
* onto a brand-new track, say) reads as the row it would land on at the end.
|
||||
* A track key's 1-based display row, or null when the key is not in
|
||||
* `trackOrder` at all.
|
||||
*
|
||||
* Both callers build `trackOrder` from the same elements the key came from, so
|
||||
* null is unreachable by construction today. It is null rather than a number
|
||||
* because the only numbers available to return (the end row, the last row) are
|
||||
* indistinguishable from a real answer: a label would announce a row the user
|
||||
* can see is wrong, and nothing upstream would ever learn it had guessed.
|
||||
*/
|
||||
export function trackDisplayNumber(trackOrder: readonly number[], track: number): number {
|
||||
export function trackDisplayNumber(trackOrder: readonly number[], track: number): number | null {
|
||||
const row = trackOrder.indexOf(track);
|
||||
return row < 0 ? trackOrder.length + 1 : row + 1;
|
||||
return row < 0 ? null : row + 1;
|
||||
}
|
||||
|
||||
/**
|
||||
* The `" 3"` in `Hide track 3`, empty when there is no display row to name.
|
||||
* Announcing "Hide track" is thin; announcing an invented row is wrong.
|
||||
*/
|
||||
export function trackDisplaySuffix(displayNumber: number | null): string {
|
||||
return displayNumber === null ? "" : ` ${displayNumber}`;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user