Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions packages/studio/src/components/editor/TimelineFxPopover.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,16 @@ import { TimelineFxPopover } from "./TimelineFxPopover.js";
const EMPTY_CHAIN: HfAudioFxChain = { version: 1, nodes: [] };
const RECT = { left: 0, top: 0, right: 0, bottom: 0 } as DOMRect;

function rect(top: number, bottom: number): DOMRect {
return { left: 0, top, right: 0, bottom } as DOMRect;
}

function dialogOf(host: HTMLElement): HTMLElement {
const el = host.querySelector('[role="dialog"]');
if (!el) throw new Error("no dialog");
return el as HTMLElement;
}

function byTextButton(host: HTMLElement, text: string): HTMLButtonElement | undefined {
return Array.from(host.querySelectorAll("button")).find((b) => b.textContent?.includes(text));
}
Expand Down Expand Up @@ -102,6 +112,50 @@ describe("TimelineFxPopover", () => {
expect(onClose).not.toHaveBeenCalled();
});

// These four guard the regression that shipped once already: an uncapped
// popover grew past the gap it opened into, ran under the timeline chrome and
// took its footer with it, and nothing scrolled.
it("caps its height to the space below when it opens downward", () => {
// spaceBelow 738 > spaceAbove 10, so it opens down: 738 - margin(8) - gap(4).
const { host } = mount({ anchorRect: rect(10, 30) });
const dialog = dialogOf(host);
expect(dialog.style.top).toBe("34px");
expect(dialog.style.maxHeight).toBe("726px");
});

it("caps its height to the space above when it flips upward", () => {
// spaceBelow 48 < 260 and spaceAbove 700 is larger, so it flips up.
const { host } = mount({ anchorRect: rect(700, 720) });
const dialog = dialogOf(host);
expect(dialog.style.bottom).toBe("72px");
expect(dialog.style.maxHeight).toBe("688px");
});

it("never caps below a usable minimum, however tight the gap", () => {
const innerHeight = window.innerHeight;
Object.defineProperty(window, "innerHeight", { value: 200, configurable: true });
try {
// Both gaps are tiny (80 below / 100 above); the cap must not collapse.
const { host } = mount({ anchorRect: rect(100, 120) });
expect(dialogOf(host).style.maxHeight).toBe("160px");
} finally {
Object.defineProperty(window, "innerHeight", { value: innerHeight, configurable: true });
}
});

it("scrolls the preset list and leaves the footer outside the scroller", () => {
const { host } = mount();
const dialog = dialogOf(host);
const scroller = dialog.querySelector(".overflow-y-auto");
expect(scroller).toBeTruthy();
// The footer must be a SIBLING of the scroller, not inside it — otherwise it
// scrolls away instead of staying put, which is the original bug.
const footer = byTextButton(host, "Open rack");
expect(footer).toBeDefined();
expect(scroller?.contains(footer as Node)).toBe(false);
expect(dialog.contains(footer as Node)).toBe(true);
});

it("the footer opens the rack and closes the popover", () => {
const { host, onOpenRack, onClose } = mount();
const openRack = byTextButton(host, "Open rack");
Expand Down
43 changes: 30 additions & 13 deletions packages/studio/src/components/editor/TimelineFxPopover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,30 @@ import { useFxAudition } from "./useFxAudition.js";

const POPOVER_WIDTH = 260;
const VIEWPORT_MARGIN = 8;
/** Below this the popover is useless anyway; it scrolls instead of vanishing. */
const MIN_POPOVER_HEIGHT = 160;

function clampedStyle(anchorRect: DOMRect): CSSProperties {
const left = Math.min(
Math.max(anchorRect.left, VIEWPORT_MARGIN),
Math.max(VIEWPORT_MARGIN, window.innerWidth - POPOVER_WIDTH - VIEWPORT_MARGIN),
);
const spaceBelow = window.innerHeight - anchorRect.bottom;
const openUpward = spaceBelow < 260 && anchorRect.top > spaceBelow;
// Named, because the height cap below needs the same quantity the flip does.
// (`spaceAbove === anchorRect.top` for a viewport-relative rect, so the flip
// condition itself is unchanged — this is a rename, not a behaviour fix.)
const spaceAbove = anchorRect.top;
const openUpward = spaceBelow < 260 && spaceAbove > spaceBelow;
// Flipping direction alone is not enough: the preset list is taller than either
// gap on a short window, so the popover ran off the top or the bottom and its
// footer ("+ effect" / "Open rack") went with it. Cap to whatever the chosen
// side actually has and let the list scroll inside that.
const available = (openUpward ? spaceAbove : spaceBelow) - VIEWPORT_MARGIN - 4;
return {
position: "fixed",
left,
width: POPOVER_WIDTH,
maxHeight: Math.max(MIN_POPOVER_HEIGHT, available),
...(openUpward
? { bottom: window.innerHeight - anchorRect.top + 4 }
: { top: anchorRect.bottom + 4 }),
Expand Down Expand Up @@ -96,22 +108,27 @@ export function TimelineFxPopover({
ref={rootRef}
role="dialog"
aria-label="Effects"
className="z-50 rounded-md border border-white/10 bg-[#1b1b1f] p-2 shadow-xl"
className="z-50 flex flex-col overflow-hidden rounded-md border border-white/10 bg-[#1b1b1f] p-2 shadow-xl"
style={clampedStyle(anchorRect)}
onKeyDown={onKeyDown}
onPointerDown={(event) => event.stopPropagation()}
>
<FxPresetMenu
trackKind={trackKind}
onPick={applyPreset}
onAudition={
onChainPreview
? (id) =>
audition(id ? (base) => applyPresetToChain(base, id, trackKind) ?? base : null)
: undefined
}
/>
<div className="mt-2 flex items-center justify-between border-t border-white/10 pt-2 text-[10px] text-white/55">
{/* The list scrolls; the footer below stays put. `min-h-0` is load-bearing
— a flex child defaults to min-height:auto and would refuse to shrink,
pushing the footer out of the popover instead of scrolling. */}
<div className="min-h-0 flex-1 overflow-y-auto">
<FxPresetMenu
trackKind={trackKind}
onPick={applyPreset}
onAudition={
onChainPreview
? (id) =>
audition(id ? (base) => applyPresetToChain(base, id, trackKind) ?? base : null)
: undefined
}
/>
</div>
<div className="mt-2 flex shrink-0 items-center justify-between border-t border-white/10 pt-2 text-[10px] text-white/55">
<button
type="button"
className="hover:text-white"
Expand Down
Loading