Skip to content

Commit 8de2b9c

Browse files
committed
fix(dropdown): align overflow and shortcut behavior with the public contract
1 parent 46559e8 commit 8de2b9c

File tree

16 files changed

+426
-15
lines changed

16 files changed

+426
-15
lines changed

docs/widgets/dropdown.md

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,12 @@ ui.dropdown({
3636

3737
- **Arrow keys** navigate items. **Enter** selects the highlighted item.
3838
- The current selection is visually highlighted.
39+
- Long menus render a deterministic visible window and keep the highlighted item in view as you navigate.
3940
- **Mouse click** on an item selects it and fires the `onSelect` callback.
4041
- **Clicking outside** the dropdown closes it (calls `onClose`).
4142
- Dropdown overlays register in the shared `LayerRegistry`, so z-order and
4243
hit-testing behavior is consistent with modal/layer overlays.
43-
- Item `shortcut` text is a display hint. Register actual key combos with
44-
`app.keys()` and route to the same action handlers.
44+
- Item `shortcut` bindings are active for the topmost open dropdown and trigger the same selection/close path as keyboard or mouse activation.
4545

4646
## Notes
4747

@@ -62,11 +62,13 @@ ui.dropdown({
6262
})
6363
```
6464

65-
Pair the labels with explicit bindings:
65+
Optional app-level bindings for the same actions:
6666

6767
```ts
6868
app.keys({
6969
"ctrl+s": () => runCommand("save"),
7070
"ctrl+q": () => runCommand("quit"),
7171
})
7272
```
73+
74+
Use `app.keys()` for app-level shortcuts that should work even when the dropdown is closed.

packages/core/src/app/widgetRenderer.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -980,6 +980,7 @@ export class WidgetRenderer<S> {
980980
private onCloseByLayerId: ReadonlyMap<string, () => void> = new Map<string, () => void>();
981981
private dropdownStack: readonly string[] = Object.freeze([]);
982982
private readonly dropdownSelectedIndexById = new Map<string, number>();
983+
private readonly dropdownWindowStartById = new Map<string, number>();
983984
private overlayShortcutOwners: readonly OverlayShortcutOwner[] = Object.freeze([]);
984985
private readonly overlayShortcutBySequence = new Map<string, OverlayShortcutBinding>();
985986
private overlayShortcutTrie = buildTrie<OverlayShortcutContext>(Object.freeze([]));
@@ -1493,6 +1494,7 @@ export class WidgetRenderer<S> {
14931494
this.commandPaletteLoadingById,
14941495
this.toolApprovalFocusedActionById,
14951496
this.dropdownSelectedIndexById,
1497+
this.dropdownWindowStartById,
14961498
this.diffViewerFocusedHunkById,
14971499
this.diffViewerExpandedHunksById,
14981500
this.tableRenderCacheById,
@@ -1754,6 +1756,7 @@ export class WidgetRenderer<S> {
17541756
toastContainers: this.toastContainers,
17551757
toastActionByFocusId: this.toastActionByFocusId,
17561758
dropdownSelectedIndexById: this.dropdownSelectedIndexById,
1759+
dropdownWindowStartById: this.dropdownWindowStartById,
17571760
toolApprovalFocusedActionById: this.toolApprovalFocusedActionById,
17581761
diffViewerFocusedHunkById: this.diffViewerFocusedHunkById,
17591762
diffViewerExpandedHunksById: this.diffViewerExpandedHunksById,
@@ -3328,6 +3331,7 @@ export class WidgetRenderer<S> {
33283331
loadedTreeChildrenByTreeId: this.loadedTreeChildrenByTreeId,
33293332
treeLoadTokenByTreeAndKey: this.treeLoadTokenByTreeAndKey,
33303333
dropdownSelectedIndexById: this.dropdownSelectedIndexById,
3334+
dropdownWindowStartById: this.dropdownWindowStartById,
33313335
pressedVirtualList: this.pressedVirtualList,
33323336
pressedFileTree: this.pressedFileTree,
33333337
lastFileTreeClick: this.lastFileTreeClick,
@@ -3569,6 +3573,7 @@ export class WidgetRenderer<S> {
35693573
this.commandPaletteLoadingById,
35703574
this.toolApprovalFocusedActionById,
35713575
this.dropdownSelectedIndexById,
3576+
this.dropdownWindowStartById,
35723577
this.diffViewerFocusedHunkById,
35733578
this.diffViewerExpandedHunksById,
35743579
this.tableRenderCacheById,
@@ -3608,6 +3613,7 @@ export class WidgetRenderer<S> {
36083613
commandPaletteLoadingById: this.commandPaletteLoadingById,
36093614
toolApprovalFocusedActionById: this.toolApprovalFocusedActionById,
36103615
dropdownSelectedIndexById: this.dropdownSelectedIndexById,
3616+
dropdownWindowStartById: this.dropdownWindowStartById,
36113617
diffViewerFocusedHunkById: this.diffViewerFocusedHunkById,
36123618
diffViewerExpandedHunksById: this.diffViewerExpandedHunksById,
36133619
focusAnnouncement,

packages/core/src/app/widgetRenderer/mouseRouting.ts

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import type { ZrevEvent } from "../../events.js";
22
import { ZR_MOD_CTRL, ZR_MOD_SHIFT } from "../../keybindings/keyCodes.js";
3+
import { computeDropdownWindow } from "../../layout/dropdownGeometry.js";
34
import { measureTextCells } from "../../layout/textMeasure.js";
45
import type { Rect } from "../../layout/types.js";
56
import type { FocusManagerState } from "../../runtime/focus.js";
@@ -88,6 +89,7 @@ type RouteDropdownMouseContext = Readonly<{
8889
dropdownStack: readonly string[];
8990
dropdownById: ReadonlyMap<string, DropdownProps>;
9091
dropdownSelectedIndexById: Map<string, number>;
92+
dropdownWindowStartById: Map<string, number>;
9193
pressedDropdown: Readonly<{ id: string; itemId: string }> | null;
9294
setPressedDropdown: (next: Readonly<{ id: string; itemId: string }> | null) => void;
9395
computeDropdownRect: (props: DropdownProps) => Rect | null;
@@ -325,12 +327,19 @@ export function routeDropdownMouse(
325327
const contentY = dropdownRect.y + 1;
326328
const contentW = Math.max(0, dropdownRect.w - 2);
327329
const contentH = Math.max(0, dropdownRect.h - 2);
330+
const window = computeDropdownWindow(
331+
dropdown.items.length,
332+
ctx.dropdownSelectedIndexById.get(topDropdownId) ?? 0,
333+
contentH,
334+
ctx.dropdownWindowStartById.get(topDropdownId) ?? 0,
335+
);
336+
const itemAreaW = window.overflow ? Math.max(0, contentW - 1) : contentW;
328337
const inContent =
329338
event.x >= contentX &&
330-
event.x < contentX + contentW &&
339+
event.x < contentX + itemAreaW &&
331340
event.y >= contentY &&
332341
event.y < contentY + contentH;
333-
const itemIndex = inContent ? event.y - contentY : null;
342+
const itemIndex = inContent ? window.startIndex + (event.y - contentY) : null;
334343

335344
const MOUSE_KIND_DOWN = 3;
336345
const MOUSE_KIND_UP = 4;
@@ -355,6 +364,15 @@ export function routeDropdownMouse(
355364
if (item && !item.divider && item.disabled !== true) {
356365
const prevSelected = ctx.dropdownSelectedIndexById.get(topDropdownId) ?? 0;
357366
ctx.dropdownSelectedIndexById.set(topDropdownId, itemIndex);
367+
ctx.dropdownWindowStartById.set(
368+
topDropdownId,
369+
computeDropdownWindow(
370+
dropdown.items.length,
371+
itemIndex,
372+
contentH,
373+
ctx.dropdownWindowStartById.get(topDropdownId) ?? 0,
374+
).startIndex,
375+
);
358376
ctx.setPressedDropdown(Object.freeze({ id: topDropdownId, itemId: item.id }));
359377
return Object.freeze({ needsRender: itemIndex !== prevSelected });
360378
}

packages/core/src/app/widgetRenderer/overlayShortcuts.ts

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,15 @@ const EMPTY_COMMAND_ITEMS: readonly CommandItem[] = Object.freeze([]);
7979

8080
function noopOverlayShortcutHandler(_ctx: OverlayShortcutContext): void {}
8181

82+
function describeThrown(value: unknown): string {
83+
if (value instanceof Error) return `${value.name}: ${value.message}`;
84+
try {
85+
return String(value);
86+
} catch {
87+
return "[unstringifiable thrown value]";
88+
}
89+
}
90+
8291
export function selectDropdownShortcutItem(
8392
ctx: SelectDropdownShortcutContext,
8493
dropdownId: string,
@@ -99,14 +108,14 @@ export function selectDropdownShortcutItem(
99108
try {
100109
dropdown.onSelect(item);
101110
} catch (e) {
102-
warnDev(`[rezi] onSelect callback threw: ${e instanceof Error ? e.message : String(e)}`);
111+
warnDev(`[rezi] onSelect callback threw: ${describeThrown(e)}`);
103112
}
104113
}
105114
if (dropdown.onClose) {
106115
try {
107116
dropdown.onClose();
108117
} catch (e) {
109-
warnDev(`[rezi] onClose callback threw: ${e instanceof Error ? e.message : String(e)}`);
118+
warnDev(`[rezi] onClose callback threw: ${describeThrown(e)}`);
110119
}
111120
}
112121
return true;
@@ -127,12 +136,12 @@ export function selectCommandPaletteShortcutItem(
127136
try {
128137
palette.onSelect(item);
129138
} catch (e) {
130-
warnDev(`[rezi] onSelect callback threw: ${e instanceof Error ? e.message : String(e)}`);
139+
warnDev(`[rezi] onSelect callback threw: ${describeThrown(e)}`);
131140
}
132141
try {
133142
palette.onClose();
134143
} catch (e) {
135-
warnDev(`[rezi] onClose callback threw: ${e instanceof Error ? e.message : String(e)}`);
144+
warnDev(`[rezi] onClose callback threw: ${describeThrown(e)}`);
136145
}
137146
return true;
138147
}

packages/core/src/app/widgetRenderer/overlayState.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,7 @@ type CleanupRoutingStateParams = RoutingWidgetMaps &
167167
loadedTreeChildrenByTreeId: Map<string, ReadonlyMap<string, readonly unknown[]>>;
168168
treeLoadTokenByTreeAndKey: Map<string, number>;
169169
dropdownSelectedIndexById: Map<string, number>;
170+
dropdownWindowStartById: Map<string, number>;
170171
pressedVirtualList: Readonly<{ id: string; index: number }> | null;
171172
pressedFileTree: Readonly<{ id: string; nodeIndex: number; nodeKey: string }> | null;
172173
lastFileTreeClick: Readonly<{
@@ -813,6 +814,9 @@ export function cleanupRoutingStateAfterRebuild(
813814
for (const dropdownId of params.dropdownSelectedIndexById.keys()) {
814815
if (!params.dropdownById.has(dropdownId)) params.dropdownSelectedIndexById.delete(dropdownId);
815816
}
817+
for (const dropdownId of params.dropdownWindowStartById.keys()) {
818+
if (!params.dropdownById.has(dropdownId)) params.dropdownWindowStartById.delete(dropdownId);
819+
}
816820

817821
for (const virtualListId of params.virtualListStore.keys()) {
818822
if (!params.virtualListById.has(virtualListId)) params.virtualListStore.delete(virtualListId);

packages/core/src/app/widgetRenderer/routeEngineEvent.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type { ZrevEvent } from "../../events.js";
22
import { ZR_MOD_CTRL } from "../../keybindings/keyCodes.js";
33
import type { LayoutOverflowMetadata } from "../../layout/constraints.js";
4+
import { computeDropdownWindow } from "../../layout/dropdownGeometry.js";
45
import { hitTestAnyId, hitTestFocusable } from "../../layout/hitTest.js";
56
import type { LayoutTree } from "../../layout/layout.js";
67
import type { Rect } from "../../layout/types.js";
@@ -221,6 +222,7 @@ export type RouteEngineEventContext = Readonly<{
221222
toastContainers: readonly Readonly<{ rect: Rect; props: ToastContainerProps }>[];
222223
toastActionByFocusId: ReadonlyMap<string, () => void>;
223224
dropdownSelectedIndexById: Map<string, number>;
225+
dropdownWindowStartById: Map<string, number>;
224226
toolApprovalFocusedActionById: Map<string, "allow" | "deny" | "allowSession">;
225227
diffViewerFocusedHunkById: Map<string, number>;
226228
diffViewerExpandedHunksById: Map<string, ReadonlySet<number>>;
@@ -310,6 +312,17 @@ export function routeEngineEventImpl(
310312
});
311313
if (dropdownResult.nextSelectedIndex !== undefined) {
312314
ctx.dropdownSelectedIndexById.set(topDropdownId, dropdownResult.nextSelectedIndex);
315+
const dropdownRect = ctx.computeDropdownRect(dropdown);
316+
const visibleRows = Math.max(0, (dropdownRect?.h ?? 0) - 2);
317+
ctx.dropdownWindowStartById.set(
318+
topDropdownId,
319+
computeDropdownWindow(
320+
dropdown.items.length,
321+
dropdownResult.nextSelectedIndex,
322+
visibleRows,
323+
ctx.dropdownWindowStartById.get(topDropdownId) ?? 0,
324+
).startIndex,
325+
);
313326
}
314327
if (dropdownResult.consumed) return ROUTE_RENDER;
315328
}
@@ -329,6 +342,7 @@ export function routeEngineEventImpl(
329342
dropdownStack: ctx.dropdownStack,
330343
dropdownById: ctx.dropdownById,
331344
dropdownSelectedIndexById: ctx.dropdownSelectedIndexById,
345+
dropdownWindowStartById: ctx.dropdownWindowStartById,
332346
pressedDropdown: state.pressedDropdown,
333347
setPressedDropdown: (next) => {
334348
state.pressedDropdown = next;

packages/core/src/layout/__tests__/dropdownGeometry.test.ts

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { assert, describe, test } from "@rezi-ui/testkit";
22
import type { DropdownProps } from "../../widgets/types.js";
3-
import { computeDropdownGeometry } from "../dropdownGeometry.js";
3+
import { computeDropdownGeometry, computeDropdownWindow } from "../dropdownGeometry.js";
44
import type { Rect } from "../types.js";
55

66
function dropdownProps(items: DropdownProps["items"]): DropdownProps {
@@ -80,4 +80,24 @@ describe("dropdownGeometry", () => {
8080
assert.equal(rect.w, 3);
8181
assert.equal(rect.h, 2);
8282
});
83+
84+
test("computeDropdownWindow preserves the previous window while selection stays visible", () => {
85+
assert.deepEqual(computeDropdownWindow(12, 6, 4, 4), {
86+
startIndex: 4,
87+
endIndex: 8,
88+
visibleRows: 4,
89+
selectedIndex: 6,
90+
overflow: true,
91+
});
92+
});
93+
94+
test("computeDropdownWindow advances only when selection leaves the visible window", () => {
95+
assert.deepEqual(computeDropdownWindow(12, 8, 4, 4), {
96+
startIndex: 5,
97+
endIndex: 9,
98+
visibleRows: 4,
99+
selectedIndex: 8,
100+
overflow: true,
101+
});
102+
});
83103
});

packages/core/src/layout/dropdownGeometry.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,60 @@ import { calculateAnchorPosition } from "./positioning.js";
33
import { measureTextCells } from "./textMeasure.js";
44
import type { Rect } from "./types.js";
55

6+
export type DropdownWindow = Readonly<{
7+
startIndex: number;
8+
endIndex: number;
9+
visibleRows: number;
10+
selectedIndex: number;
11+
overflow: boolean;
12+
}>;
13+
14+
function clampIndex(value: number, min: number, max: number): number {
15+
if (!Number.isFinite(value)) return min;
16+
const truncated = Math.trunc(value);
17+
if (truncated <= min) return min;
18+
if (truncated >= max) return max;
19+
return truncated;
20+
}
21+
22+
export function computeDropdownWindow(
23+
itemCount: number,
24+
selectedIndex: number,
25+
maxVisibleRows: number,
26+
previousStartIndex = 0,
27+
): DropdownWindow {
28+
if (itemCount <= 0 || maxVisibleRows <= 0) {
29+
return Object.freeze({
30+
startIndex: 0,
31+
endIndex: 0,
32+
visibleRows: 0,
33+
selectedIndex: 0,
34+
overflow: false,
35+
});
36+
}
37+
38+
const visibleRows = Math.min(itemCount, Math.max(0, Math.trunc(maxVisibleRows)));
39+
const normalizedSelectedIndex = clampIndex(selectedIndex, 0, itemCount - 1);
40+
const maxStartIndex = Math.max(0, itemCount - visibleRows);
41+
let startIndex = clampIndex(previousStartIndex, 0, maxStartIndex);
42+
43+
if (normalizedSelectedIndex < startIndex) {
44+
startIndex = normalizedSelectedIndex;
45+
} else if (normalizedSelectedIndex >= startIndex + visibleRows) {
46+
startIndex = normalizedSelectedIndex - visibleRows + 1;
47+
}
48+
49+
if (startIndex > maxStartIndex) startIndex = maxStartIndex;
50+
51+
return Object.freeze({
52+
startIndex,
53+
endIndex: startIndex + visibleRows,
54+
visibleRows,
55+
selectedIndex: normalizedSelectedIndex,
56+
overflow: itemCount > visibleRows,
57+
});
58+
}
59+
660
export function computeDropdownGeometry(
761
props: DropdownProps,
862
anchorRect: Rect | null,

packages/core/src/renderer/__tests__/overlay.edge.test.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,16 @@ import { type VNode, createDrawlistBuilder } from "../../index.js";
44
import { layout } from "../../layout/layout.js";
55
import { commitVNodeTree } from "../../runtime/commit.js";
66
import { createInstanceIdAllocator } from "../../runtime/instance.js";
7+
import { ui } from "../../widgets/ui.js";
78
import { renderToDrawlist } from "../renderToDrawlist.js";
89

910
function renderStrings(
1011
vnode: VNode,
1112
viewport: Readonly<{ cols: number; rows: number }>,
13+
opts: Readonly<{
14+
dropdownSelectedIndexById?: ReadonlyMap<string, number>;
15+
dropdownWindowStartById?: ReadonlyMap<string, number>;
16+
}> = {},
1217
): readonly string[] {
1318
const allocator = createInstanceIdAllocator(1);
1419
const commitRes = commitVNodeTree(null, vnode, { allocator });
@@ -33,6 +38,12 @@ function renderStrings(
3338
viewport,
3439
focusState: Object.freeze({ focusedId: null }),
3540
builder,
41+
...(opts.dropdownSelectedIndexById
42+
? { dropdownSelectedIndexById: opts.dropdownSelectedIndexById }
43+
: {}),
44+
...(opts.dropdownWindowStartById
45+
? { dropdownWindowStartById: opts.dropdownWindowStartById }
46+
: {}),
3647
});
3748
const built = builder.build();
3849
assert.equal(built.ok, true, "drawlist build should succeed");
@@ -101,4 +112,32 @@ describe("overlay rendering edge cases", () => {
101112
const strings = renderStrings(malformed, { cols: 10, rows: 3 });
102113
assert.ok(Array.isArray(strings));
103114
});
115+
116+
test("overflowing dropdown renders only the active visible window", () => {
117+
const vnode = ui.layers([
118+
ui.button({ id: "anchor", label: "Anchor" }),
119+
ui.dropdown({
120+
id: "menu",
121+
anchorId: "anchor",
122+
items: Array.from({ length: 8 }, (_, index) => ({
123+
id: `item-${String(index)}`,
124+
label: `Item ${String(index)}`,
125+
})),
126+
}),
127+
]);
128+
129+
const strings = renderStrings(
130+
vnode,
131+
{ cols: 14, rows: 5 },
132+
{
133+
dropdownSelectedIndexById: new Map([["menu", 6]]),
134+
dropdownWindowStartById: new Map([["menu", 4]]),
135+
},
136+
);
137+
138+
assert.equal(strings.includes("Item 0"), false);
139+
assert.equal(strings.includes("Item 4"), true);
140+
assert.equal(strings.includes("Item 6"), true);
141+
assert.equal(strings.includes("Item 7"), false);
142+
});
104143
});

0 commit comments

Comments
 (0)