Skip to content

Commit 589e3f0

Browse files
vanceingallsclaude
andcommitted
fix(studio): slide the FX popover in-bounds instead of hanging it off the edge
Review found the minimum defeating the viewport cap: `Math.max(MIN_POPOVER_HEIGHT, available)` kept the box 160px tall even when the chosen gap was smaller, so the box extended past the edge it opened away from. At 200px of viewport with the anchor at 100..120 it flipped up to `bottom: 104px` and spanned y = -64..96 — every preset still reachable, but through a ~57px window with the top third of the dialog off-screen. Reachable at high browser zoom, not only in a synthetic short window: `available` drops under 160 once the gap is under ~172px, which 400% zoom on a 1080p display produces on both sides. Shrinking to the gap would undo the floor on purpose (a 20px gap gives a 20px popover — the vanishing popover in a new costume), so honour the floor and clamp the resulting box into the viewport the way `left` already is. Two parts: - Cap the floor by the window itself (`innerHeight - 2 * VIEWPORT_MARGIN`). The minimum is a floor against a tight gap, not against a tight window; below 176px of viewport, physical space has to win. - Inset the `top` / `bottom` offset to `innerHeight - height - VIEWPORT_MARGIN`, so a floor larger than the gap slides the box back in rather than off the top. The tight case now lands at `bottom: 32px` with `maxHeight: 160px` — the box at y = 8..40, one margin on each side. The two ordinary cases are unchanged (34/726 down, 72/688 up), which the existing tests pin. Tests: two added — both edges in-bounds when the minimum exceeds the gap, and the floor yielding when the whole window is shorter than it. Both fail on the previous arithmetic (104px vs 32px, 160px vs 104px). The three pre-existing geometry tests now pin `window.innerHeight` through one shared helper instead of inheriting happy-dom's 768 default, so their expected numbers are derivable from the test and immune to a dependency bump. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent f3ccd7f commit 589e3f0

2 files changed

Lines changed: 69 additions & 17 deletions

File tree

packages/studio/src/components/editor/TimelineFxPopover.test.tsx

Lines changed: 49 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,18 @@ function rect(top: number, bottom: number): DOMRect {
1414
return { left: 0, top, right: 0, bottom } as DOMRect;
1515
}
1616

17+
/** Pin the viewport height: every expected number below is derived from it, and
18+
* happy-dom's 768 default is not something this file should silently inherit. */
19+
function withViewportHeight<T>(value: number, run: () => T): T {
20+
const previous = window.innerHeight;
21+
Object.defineProperty(window, "innerHeight", { value, configurable: true });
22+
try {
23+
return run();
24+
} finally {
25+
Object.defineProperty(window, "innerHeight", { value: previous, configurable: true });
26+
}
27+
}
28+
1729
function dialogOf(host: HTMLElement): HTMLElement {
1830
const el = host.querySelector('[role="dialog"]');
1931
if (!el) throw new Error("no dialog");
@@ -112,35 +124,58 @@ describe("TimelineFxPopover", () => {
112124
expect(onClose).not.toHaveBeenCalled();
113125
});
114126

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

126139
it("caps its height to the space above when it flips upward", () => {
127140
// spaceBelow 48 < 260 and spaceAbove 700 is larger, so it flips up.
128-
const { host } = mount({ anchorRect: rect(700, 720) });
129-
const dialog = dialogOf(host);
141+
const dialog = withViewportHeight(768, () =>
142+
dialogOf(mount({ anchorRect: rect(700, 720) }).host),
143+
);
130144
expect(dialog.style.bottom).toBe("72px");
131145
expect(dialog.style.maxHeight).toBe("688px");
132146
});
133147

134148
it("never caps below a usable minimum, however tight the gap", () => {
135-
const innerHeight = window.innerHeight;
136-
Object.defineProperty(window, "innerHeight", { value: 200, configurable: true });
137-
try {
138-
// Both gaps are tiny (80 below / 100 above); the cap must not collapse.
139-
const { host } = mount({ anchorRect: rect(100, 120) });
140-
expect(dialogOf(host).style.maxHeight).toBe("160px");
141-
} finally {
142-
Object.defineProperty(window, "innerHeight", { value: innerHeight, configurable: true });
143-
}
149+
// Both gaps are tiny (80 below / 100 above); the cap must not collapse.
150+
const dialog = withViewportHeight(200, () =>
151+
dialogOf(mount({ anchorRect: rect(100, 120) }).host),
152+
);
153+
expect(dialog.style.maxHeight).toBe("160px");
154+
});
155+
156+
it("keeps both edges in the viewport when the minimum exceeds the gap", () => {
157+
// The case the minimum used to lose: at 200px of viewport (roughly 400% zoom
158+
// on a laptop) neither gap can hold 160px, so honouring the floor has to
159+
// slide the box in rather than hang its top edge off-screen.
160+
const dialog = withViewportHeight(200, () =>
161+
dialogOf(mount({ anchorRect: rect(100, 120) }).host),
162+
);
163+
// bottom:32 + maxHeight:160 puts the box at y = 8..40 — a margin on each side.
164+
expect(dialog.style.bottom).toBe("32px");
165+
const bottom = Number.parseFloat(dialog.style.bottom);
166+
const height = Number.parseFloat(dialog.style.maxHeight);
167+
expect(bottom).toBeGreaterThanOrEqual(8);
168+
expect(200 - bottom - height).toBeGreaterThanOrEqual(8);
169+
});
170+
171+
it("shrinks below the minimum only when the whole window is shorter", () => {
172+
// A floor against a tight gap is not a floor against a tight window: 120px of
173+
// viewport cannot hold 160px, and hanging off the edge is worse than short.
174+
const dialog = withViewportHeight(120, () =>
175+
dialogOf(mount({ anchorRect: rect(60, 80) }).host),
176+
);
177+
expect(dialog.style.maxHeight).toBe("104px");
178+
expect(dialog.style.bottom).toBe("8px");
144179
});
145180

146181
it("scrolls the preset list and leaves the footer outside the scroller", () => {

packages/studio/src/components/editor/TimelineFxPopover.tsx

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,14 +36,31 @@ function clampedStyle(anchorRect: DOMRect): CSSProperties {
3636
// footer ("+ effect" / "Open rack") went with it. Cap to whatever the chosen
3737
// side actually has and let the list scroll inside that.
3838
const available = (openUpward ? spaceAbove : spaceBelow) - VIEWPORT_MARGIN - 4;
39+
// The minimum is a floor against a tight GAP, not against a tight window: keep
40+
// a usable list when the gap is smaller than 160px, but never ask for more
41+
// height than the viewport itself can hold.
42+
const height = Math.min(
43+
Math.max(MIN_POPOVER_HEIGHT, available),
44+
window.innerHeight - VIEWPORT_MARGIN * 2,
45+
);
46+
// A floor larger than the gap would hang the box off the edge it opened away
47+
// from — reachable at high browser zoom, where both gaps fall under ~172px.
48+
// Slide it back in-bounds the way `left` is already clamped, rather than
49+
// shrinking below the floor: the offset that keeps BOTH edges inside is
50+
// `innerHeight - height - VIEWPORT_MARGIN`, from either side.
51+
const inset = (desired: number) =>
52+
Math.max(
53+
VIEWPORT_MARGIN,
54+
Math.min(desired, Math.max(VIEWPORT_MARGIN, window.innerHeight - height - VIEWPORT_MARGIN)),
55+
);
3956
return {
4057
position: "fixed",
4158
left,
4259
width: POPOVER_WIDTH,
43-
maxHeight: Math.max(MIN_POPOVER_HEIGHT, available),
60+
maxHeight: height,
4461
...(openUpward
45-
? { bottom: window.innerHeight - anchorRect.top + 4 }
46-
: { top: anchorRect.bottom + 4 }),
62+
? { bottom: inset(window.innerHeight - anchorRect.top + 4) }
63+
: { top: inset(anchorRect.bottom + 4) }),
4764
};
4865
}
4966

0 commit comments

Comments
 (0)