Skip to content

Commit 7600d74

Browse files
RtlZeroMemoryRtlZeroMemory
committed
fix(core): address review feedback for tabs and pagination routing
1 parent 413702d commit 7600d74

6 files changed

Lines changed: 210 additions & 10 deletions

File tree

‎packages/core/src/runtime/focus.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,7 @@ export type FocusZone = Readonly<{
159159
columns: number;
160160
wrapAround: boolean;
161161
focusableIds: readonly string[];
162+
parentZoneId?: string;
162163
lastFocusedId: string | null;
163164
}>;
164165

@@ -575,6 +576,7 @@ export function finalizeFocusWithPreCollectedMetadata(
575576
columns: collected.columns,
576577
wrapAround: collected.wrapAround,
577578
focusableIds: collected.focusableIds,
579+
...(collected.parentZoneId ? { parentZoneId: collected.parentZoneId } : {}),
578580
lastFocusedId,
579581
}),
580582
);

‎packages/core/src/runtime/router/pagination.ts‎

Lines changed: 58 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import type { ZrevEvent } from "../../events.js";
22
import {
33
getPaginationControlId,
4+
getPaginationPageId,
45
getPaginationZoneId,
6+
movePaginationPage,
57
parsePaginationId,
68
} from "../../widgets/pagination.js";
79
import { computeZoneMovement } from "../focus.js";
@@ -17,6 +19,25 @@ function isEnabled(enabledById: EnabledById, id: string): boolean {
1719
return enabledById.get(id) === true;
1820
}
1921

22+
function canPressId(
23+
enabledById: EnabledById,
24+
id: string,
25+
pressableIds?: ReadonlySet<string>,
26+
): boolean {
27+
return isEnabled(enabledById, id) && (pressableIds === undefined || pressableIds.has(id));
28+
}
29+
30+
function inferTotalPagesFromZone(zone: FocusZone): number | null {
31+
let maxPage = 0;
32+
for (const id of zone.focusableIds) {
33+
const parsed = parsePaginationId(id);
34+
if (parsed?.kind === "page" && parsed.page > maxPage) {
35+
maxPage = parsed.page;
36+
}
37+
}
38+
return maxPage > 0 ? maxPage : null;
39+
}
40+
2041
function findZoneForId(zones: ReadonlyMap<string, FocusZone>, id: string): string | null {
2142
for (const [zoneId, zone] of zones) {
2243
if (zone.focusableIds.includes(id)) return zoneId;
@@ -54,6 +75,42 @@ export function routePaginationKey(
5475
if (!zone) return Object.freeze({});
5576

5677
if (event.key === ZR_KEY_LEFT || event.key === ZR_KEY_RIGHT) {
78+
const direction = event.key === ZR_KEY_LEFT ? "prev" : "next";
79+
80+
if (parsed.kind === "page") {
81+
const totalPages = inferTotalPagesFromZone(zone) ?? parsed.page;
82+
const targetPage = movePaginationPage(parsed.page, totalPages, direction);
83+
if (targetPage === parsed.page) return Object.freeze({});
84+
85+
const targetPageId = getPaginationPageId(parsed.paginationId, targetPage);
86+
if (
87+
zone.focusableIds.includes(targetPageId) &&
88+
canPressId(ctx.enabledById, targetPageId, ctx.pressableIds)
89+
) {
90+
const action: RoutedAction = Object.freeze({ id: targetPageId, action: "press" });
91+
return Object.freeze({
92+
nextFocusedId: targetPageId,
93+
nextZoneId: zoneId,
94+
action,
95+
});
96+
}
97+
98+
const stepControlId = getPaginationControlId(parsed.paginationId, direction);
99+
if (
100+
zone.focusableIds.includes(stepControlId) &&
101+
canPressId(ctx.enabledById, stepControlId, ctx.pressableIds)
102+
) {
103+
const action: RoutedAction = Object.freeze({ id: stepControlId, action: "press" });
104+
return Object.freeze({
105+
nextFocusedId: ctx.focusedId,
106+
nextZoneId: zoneId,
107+
action,
108+
});
109+
}
110+
111+
return Object.freeze({});
112+
}
113+
57114
const nextFocusedId = computeZoneMovement(
58115
zone,
59116
ctx.focusedId,
@@ -65,9 +122,7 @@ export function routePaginationKey(
65122
return Object.freeze({ nextFocusedId, nextZoneId: zoneId });
66123
}
67124

68-
const canPress =
69-
isEnabled(ctx.enabledById, nextFocusedId) &&
70-
(ctx.pressableIds === undefined || ctx.pressableIds.has(nextFocusedId));
125+
const canPress = canPressId(ctx.enabledById, nextFocusedId, ctx.pressableIds);
71126

72127
const action: RoutedAction | undefined = canPress
73128
? Object.freeze({ id: nextFocusedId, action: "press" })

‎packages/core/src/runtime/router/tabs.ts‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,27 @@ function findZoneForId(zones: ReadonlyMap<string, FocusZone>, id: string): strin
2323
return null;
2424
}
2525

26+
function resolveTabsIdFromZone(
27+
zones: ReadonlyMap<string, FocusZone>,
28+
startZoneId: string,
29+
): string | null {
30+
const visited = new Set<string>();
31+
let cursor: string | null = startZoneId;
32+
33+
while (cursor !== null) {
34+
if (visited.has(cursor)) return null;
35+
visited.add(cursor);
36+
37+
const tabsId = parseTabsContentZoneId(cursor);
38+
if (tabsId !== null) return tabsId;
39+
40+
const parentId: string | null = zones.get(cursor)?.parentZoneId ?? null;
41+
cursor = parentId;
42+
}
43+
44+
return null;
45+
}
46+
2647
export type TabsRoutingCtx = Readonly<{
2748
focusedId: string | null;
2849
activeZoneId: string | null;
@@ -81,7 +102,7 @@ export function routeTabsKey(event: ZrevEvent, ctx: TabsRoutingCtx): RoutingResu
81102
const zoneId = ctx.activeZoneId ?? findZoneForId(ctx.zones, focusedId);
82103
if (zoneId === null) return null;
83104

84-
const tabsId = parseTabsContentZoneId(zoneId);
105+
const tabsId = resolveTabsIdFromZone(ctx.zones, zoneId);
85106
if (tabsId === null) return null;
86107

87108
const barZoneId = getTabsBarZoneId(tabsId);

‎packages/core/src/runtime/widgetMeta.ts‎

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,7 @@ export type CollectedZone = Readonly<{
273273
columns: number;
274274
wrapAround: boolean;
275275
focusableIds: readonly string[];
276+
parentZoneId?: string;
276277
onEnter?: () => void;
277278
onExit?: () => void;
278279
}>;
@@ -323,10 +324,13 @@ function collectFocusableIdsInSubtree(node: RuntimeInstance): readonly string[]
323324
export function collectFocusZones(tree: RuntimeInstance): ReadonlyMap<string, CollectedZone> {
324325
const m = new Map<string, CollectedZone>();
325326

326-
const stack: RuntimeInstance[] = [tree];
327+
const stack: Array<{ node: RuntimeInstance; parentZoneId: string | null }> = [
328+
{ node: tree, parentZoneId: null },
329+
];
327330
while (stack.length > 0) {
328-
const node = stack.pop();
329-
if (!node) continue;
331+
const item = stack.pop();
332+
if (!item) continue;
333+
const node = item.node;
330334

331335
if (node.vnode.kind === "focusZone") {
332336
const props = node.vnode.props as {
@@ -373,6 +377,9 @@ export function collectFocusZones(tree: RuntimeInstance): ReadonlyMap<string, Co
373377
wrapAround,
374378
focusableIds: Object.freeze(focusableIds),
375379
};
380+
if (item.parentZoneId !== null) {
381+
(zone as { parentZoneId?: string }).parentZoneId = item.parentZoneId;
382+
}
376383
if (onEnter !== undefined) {
377384
(zone as { onEnter?: () => void }).onEnter = onEnter;
378385
}
@@ -384,9 +391,16 @@ export function collectFocusZones(tree: RuntimeInstance): ReadonlyMap<string, Co
384391
}
385392

386393
// Continue traversing children for nested zones
394+
let childParentZoneId = item.parentZoneId;
395+
if (node.vnode.kind === "focusZone") {
396+
const zoneId = (node.vnode.props as { id?: unknown }).id;
397+
if (typeof zoneId === "string" && zoneId.length > 0) {
398+
childParentZoneId = zoneId;
399+
}
400+
}
387401
for (let i = node.children.length - 1; i >= 0; i--) {
388402
const c = node.children[i];
389-
if (c) stack.push(c);
403+
if (c) stack.push({ node: c, parentZoneId: childParentZoneId ?? null });
390404
}
391405
}
392406

@@ -534,7 +548,11 @@ export class WidgetMetadataCollector {
534548
// Zone/trap intermediate data (reused)
535549
private readonly _zoneDataById = new Map<
536550
string,
537-
Omit<CollectedZone, "focusableIds"> & { onEnter?: () => void; onExit?: () => void }
551+
Omit<CollectedZone, "focusableIds"> & {
552+
parentZoneId?: string;
553+
onEnter?: () => void;
554+
onExit?: () => void;
555+
}
538556
>();
539557
private readonly _trapDataById = new Map<string, Omit<CollectedTrap, "focusableIds">>();
540558
private readonly _zoneFocusables = new Map<string, string[]>();
@@ -698,10 +716,23 @@ export class WidgetMetadataCollector {
698716
const onExit =
699717
typeof props.onExit === "function" ? (props.onExit as () => void) : undefined;
700718

719+
let parentZoneId: string | null = null;
720+
for (let i = this._containerStack.length - 1; i >= 0; i--) {
721+
const container = this._containerStack[i];
722+
if (container?.kind === "zone") {
723+
parentZoneId = container.id;
724+
break;
725+
}
726+
}
727+
701728
const zoneData: Omit<CollectedZone, "focusableIds"> & {
729+
parentZoneId?: string;
702730
onEnter?: () => void;
703731
onExit?: () => void;
704732
} = { id, tabIndex, navigation, columns, wrapAround };
733+
if (parentZoneId !== null) {
734+
zoneData.parentZoneId = parentZoneId;
735+
}
705736
if (onEnter !== undefined) {
706737
zoneData.onEnter = onEnter;
707738
}

‎packages/core/src/widgets/__tests__/pagination.test.ts‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,12 @@ describe("pagination keyboard routing", () => {
151151
const page1 = getPaginationPageId("pages", 1);
152152
const page2 = getPaginationPageId("pages", 2);
153153
const page3 = getPaginationPageId("pages", 3);
154+
const page9 = getPaginationPageId("pages", 9);
155+
const page10 = getPaginationPageId("pages", 10);
156+
const page11 = getPaginationPageId("pages", 11);
157+
const page20 = getPaginationPageId("pages", 20);
158+
const prev = getPaginationControlId("pages", "prev");
159+
const next = getPaginationControlId("pages", "next");
154160
const first = getPaginationControlId("pages", "first");
155161
const last = getPaginationControlId("pages", "last");
156162

@@ -242,4 +248,56 @@ describe("pagination keyboard routing", () => {
242248
});
243249
assert.deepEqual(res, {});
244250
});
251+
252+
test("right arrow on sparse page buttons uses next control instead of jumping pages", () => {
253+
const res = routePaginationKey(keyDown(23), {
254+
focusedId: page1,
255+
activeZoneId: zoneId,
256+
zones: new Map([
257+
[zoneId, zone(zoneId, [prev, page1, page9, page10, page11, page20, next], false)],
258+
]),
259+
enabledById: new Map([
260+
[prev, true],
261+
[page1, true],
262+
[page9, true],
263+
[page10, true],
264+
[page11, true],
265+
[page20, true],
266+
[next, true],
267+
]),
268+
pressableIds: new Set([prev, page1, page9, page10, page11, page20, next]),
269+
});
270+
271+
assert.deepEqual(res, {
272+
nextFocusedId: page1,
273+
nextZoneId: zoneId,
274+
action: { id: next, action: "press" },
275+
});
276+
});
277+
278+
test("left arrow on sparse page buttons uses prev control instead of jumping pages", () => {
279+
const res = routePaginationKey(keyDown(22), {
280+
focusedId: page20,
281+
activeZoneId: zoneId,
282+
zones: new Map([
283+
[zoneId, zone(zoneId, [prev, page1, page9, page10, page11, page20, next], false)],
284+
]),
285+
enabledById: new Map([
286+
[prev, true],
287+
[page1, true],
288+
[page9, true],
289+
[page10, true],
290+
[page11, true],
291+
[page20, true],
292+
[next, true],
293+
]),
294+
pressableIds: new Set([prev, page1, page9, page10, page11, page20, next]),
295+
});
296+
297+
assert.deepEqual(res, {
298+
nextFocusedId: page20,
299+
nextZoneId: zoneId,
300+
action: { id: prev, action: "press" },
301+
});
302+
});
245303
});

‎packages/core/src/widgets/__tests__/tabs.test.ts‎

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,20 @@ function keyUp(key: number): ZrevEvent {
2424
return { kind: "key", timeMs: 0, key, mods: 0, action: "up" };
2525
}
2626

27-
function zone(id: string, focusableIds: readonly string[], wrapAround: boolean): FocusZone {
27+
function zone(
28+
id: string,
29+
focusableIds: readonly string[],
30+
wrapAround: boolean,
31+
parentZoneId?: string,
32+
): FocusZone {
2833
return {
2934
id,
3035
tabIndex: 0,
3136
navigation: "linear",
3237
columns: 1,
3338
wrapAround,
3439
focusableIds,
40+
...(parentZoneId ? { parentZoneId } : {}),
3541
lastFocusedId: null,
3642
};
3743
}
@@ -326,4 +332,31 @@ describe("tabs keyboard routing", () => {
326332

327333
assert.equal(res, null);
328334
});
335+
336+
test("escape from nested zone inside tab content returns focus to tab bar", () => {
337+
const nestedId = "nested-zone";
338+
const res = routeTabsKey(keyDown(1), {
339+
focusedId: "nested-btn",
340+
activeZoneId: nestedId,
341+
zones: new Map([
342+
[barId, zone(barId, [t0, t1, t2], true)],
343+
[contentId, zone(contentId, ["content-btn"], false)],
344+
[nestedId, zone(nestedId, ["nested-btn"], false, contentId)],
345+
]),
346+
lastFocusedByZone: new Map([[barId, t1]]),
347+
enabledById: new Map([
348+
[t0, true],
349+
[t1, true],
350+
[t2, true],
351+
["content-btn", true],
352+
["nested-btn", true],
353+
]),
354+
pressableIds: new Set([t0, t1, t2, "content-btn", "nested-btn"]),
355+
});
356+
357+
assert.deepEqual(res, {
358+
nextFocusedId: t1,
359+
nextZoneId: barId,
360+
});
361+
});
329362
});

0 commit comments

Comments
 (0)