Skip to content

feat(core): Implement Scoped Keyboard Focus Trap & Modal Management Subsystem (#3346) #3360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
knoxiboy wants to merge 1 commit into
base: main
Choose a base branch
from
+114 −0
Open

feat(core): Implement Scoped Keyboard Focus Trap & Modal Management Subsystem (#3346) #3360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
61 changes: 61 additions & 0 deletions packages/core/src/input/focusLock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// ─────────────────────────────────────────────────────
// @termuijs/core — Scoped Keyboard Focus Lock & Modal Manager
// ─────────────────────────────────────────────────────

import type { FocusManager, Focusable } from '../events/FocusManager.js';

export interface TrapOptions {
containerId: string;
}

export class FocusLockManager {
private _focusManager: FocusManager;
private _trapStack: string[] = [];

constructor(focusManager: FocusManager) {
this._focusManager = focusManager;
}
Comment on lines +11 to +17

get isLocked(): boolean {
return this._trapStack.length > 0;
}

get activeTrapContainerId(): string | null {
if (this._trapStack.length === 0) return null;
return this._trapStack[this._trapStack.length - 1];
}

pushTrap(options: TrapOptions, focusables: Focusable[] = []): void {
this._trapStack.push(options.containerId);
this._focusManager.trap(options.containerId, focusables);
}

popTrap(): string | null {
if (this._trapStack.length === 0) return null;

const popped = this._trapStack.pop()!;
this._focusManager.release();

return popped;
}
Comment on lines +28 to +40
Comment on lines +33 to +40

@coderabbitai coderabbitai Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add a comment justifying the non-null assertion.

Line 36 uses this._trapStack.pop()!. This is a non-null assertion, which TypeScript tooling treats as a type assertion that bypasses the type checker. The coding guidelines require an inline comment explaining why any type assertion is safe.

Add a short comment noting that the preceding length check on line 34 guarantees pop() returns a value.

🔧 Proposed fix 
   popTrap(): string | null {
     if (this._trapStack.length === 0) return null;

-    const popped = this._trapStack.pop()!;
+    // Non-null: the length check above guarantees an element exists.
+    const popped = this._trapStack.pop()!;
     this._focusManager.release();

     return popped;
   }

As per coding guidelines, "No type assertions without an inline comment explaining why."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
popTrap(): string | null {
if (this._trapStack.length === 0) return null;
const popped = this._trapStack.pop()!;
this._focusManager.release();
return popped;
}
popTrap(): string | null {
if (this._trapStack.length === 0) return null;
// Non-null: the length check above guarantees an element exists.
const popped = this._trapStack.pop()!;
this._focusManager.release();
return popped;
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/input/focusLock.ts` around lines 33 - 40, Add a concise
inline comment immediately before the non-null assertion in popTrap, explaining
that the preceding _trapStack.length check guarantees pop() returns a value.
Leave the existing stack-pop and focus-release behavior unchanged.

Source: Coding guidelines


handleKeyDown(keyName: string, isShift: boolean = false): boolean {
if (!this.isLocked) return false;

if (keyName === 'escape') {
this.popTrap();
return true;
}

if (keyName === 'tab') {
if (isShift) {
this._focusManager.focusPrev();
} else {
this._focusManager.focusNext();
}
return true;
}

return false;
}
}
53 changes: 53 additions & 0 deletions packages/core/test/focusLock.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { describe, expect, test, beforeEach } from 'bun:test';

@coderabbitai coderabbitai Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Determine the project's test runner and compare import conventions across core test files.
set -euo pipefail

echo "--- package.json test scripts ---"
fd -H package.json -x cat -n {} \; 2>/dev/null | rg -n -B2 -A2 '"test"'

echo "--- import styles used across core test files ---"
rg -n "from 'bun:test'|from 'vitest'" packages/core/test packages/widgets 2>/dev/null

echo "--- vitest / bun config files ---"
fd -i 'vitest.config|bunfig' -H

Repository: Karanjot786/TermUI

Length of output: 17315

🏁 Script executed:

#!/bin/bash
# Inspect the exact core package test script/config and any Bun/Vitest environment setup.
set -euo pipefail

echo "--- root package manager files ---"
fd -H 'package(-lock)?\.json|bun.lockb?|pnpm-lock.yaml|yarn.lock|vitest.config.ts|bunfig' . -t f | sed 's#^\./##' | sort

echo "--- root package.json relevant fields ---"
python3 - <<'PY'
import json, pathlib
p=pathlib.Path('package.json')
data=json.loads(p.read_text())
print("type:", data.get("type"))
print("scripts:", data.get("scripts", {}))
print("devDependencies.test:", data.get("devDependencies",{}).get("vitest"))
print("dependencies.test:", data.get("dependencies",{}).get("vitest"))
PY

echo "--- packages/core/package.json relevant fields ---"
python3 - <<'PY'
import json
data=json.loads(open('packages/core/package.json').read())
print("scripts:", data.get("scripts"))
print("devDependencies:", data.get("devDependences"))
for k,v in (data.get("devDependencies",{}) or {}).items():
    if k in ("vitest","bun","typescript"):
        print(k, v)
PY

echo "--- vitest.config.ts ---"
cat -n vitest.config.ts

echo "--- bunfig.toml ---"
cat -n bunfig.toml

echo "--- any bun:test imports elsewhere ---"
rg -n "from 'bun:test'|from \"bun:test\"|import 'bun:test'|import \"bun:test\"" . --glob '!node_modules' --glob '!dist' --glob '!build'

Repository: Karanjot786/TermUI

Length of output: 4255

🌐 Web query:

Vitest environment node global test aliases describe expect vi import from vitest official docs

💡 Result:

In Vitest, environment, globals, and API usage are configured as follows: Environment: The default environment in Vitest is 'node' [1][2]. You can change this to 'jsdom', 'happy-dom', or 'edge-runtime' via the 'test.environment' option in your configuration file [1][3]. For file-specific requirements, you can use the @vitest-environment control comment at the top of the file [1][2]: // @vitest-environment jsdom Globals: By default, Vitest does not inject globals for explicitness [4][5]. To use APIs like 'describe', 'test', 'expect', and 'vi' globally, you must enable them in your configuration [4][6]: // vitest.config.ts export default defineConfig({ test: { globals: true, }, }) If using TypeScript, you must also add 'vitest/globals' to your 'tsconfig.json' to ensure proper type resolution [4][5]: { "compilerOptions": { "types": ["vitest/globals"] } } Imports and Aliases: If 'globals' are disabled (the recommended default), you must import the necessary APIs directly from the 'vitest' package [7][8]: import { describe, expect, test, vi } from 'vitest' Regarding aliases, Vitest uses your Vite configuration [9][10]. You can define path aliases in your 'vite.config.ts' (or 'vitest.config.ts') using standard Vite 'resolve.alias' options [9][10]. Note that for 'vi.mock', Vitest performs static analysis; if you use 'vi' without importing it directly from 'vitest', it may not be able to hoist the mock correctly unless 'globals' is enabled [7][11].

Citations:

🌐 Web query:

Vitest supports Bun test imports from 'bun:test' environment node default config

💡 Result:

Vitest does not natively support or execute tests using the 'bun:test' module directly. 'bun:test' is a built-in test runner specific to the Bun runtime [1], whereas Vitest is a separate test runner that is designed primarily to run in Node.js environments [2]. While recent versions of Vitest (starting from 4.0.7+) have improved compatibility for running within the Bun runtime, this refers to executing Vitest's own test suite—using Vitest's APIs (e.g., vitest/dist/index.js)—inside the Bun environment [3]. It does not enable the use of Bun's native 'bun:test' API [1]. Attempting to import 'bun:test' while running Vitest can cause issues, such as build-time errors where Vite (Vitest's underlying bundler) attempts to process the import [4] or run-time errors where the Vitest module runner incorrectly modifies module specifiers [5]. Developers needing to run code that uses 'bun:test' must use the native 'bun test' command [1]. If you are using Vitest to test code meant for both environments, you must use Vitest's own mocking and assertion APIs rather than importing 'bun:test' directly [1]. If you encounter issues with static analysis, a common workaround is to use dynamic imports with a non-literal specifier (e.g., import('bun' + ':test')) to prevent the bundler from attempting to resolve the module [4].

Citations:

Use the project test runner’s APIs in packages/core/test/focusLock.test.ts.

The core package test script runs Vitest against packages/core/src/**/*.test.*, and Vitest uses import { ... } from 'vitest' when globals are configured per-project. This file only imports from bun:test, which Vitest does not run and blocks bun test from discovering the suite consistently. Move the tests/fixture into the configured core test path, or import describe, expect, test, beforeEach from vitest.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/test/focusLock.test.ts` at line 1, Update focusLock.test.ts to
use the project’s Vitest APIs by importing describe, expect, test, and
beforeEach from vitest instead of bun:test, while preserving the existing test
suite behavior.

Source: Learnings

import { FocusManager } from '../src/events/FocusManager.js';
import { FocusLockManager } from '../src/input/focusLock.js';

describe('FocusLockManager', () => {
let focusManager: FocusManager;
let focusLock: FocusLockManager;

beforeEach(() => {
focusManager = new FocusManager();
focusLock = new FocusLockManager(focusManager);

focusManager.register({ id: 'bg-btn', tabIndex: 1, focusable: true });
focusManager.register({ id: 'modal-btn1', tabIndex: 2, focusable: true });
focusManager.register({ id: 'modal-btn2', tabIndex: 3, focusable: true });
focusManager.start();
});

test('pushes trap and locks focus within container', () => {
expect(focusLock.isLocked).toBe(false);

focusLock.pushTrap(
{ containerId: 'modal-1' },
[
{ id: 'modal-btn1', tabIndex: 1, focusable: true },
{ id: 'modal-btn2', tabIndex: 2, focusable: true },
]
);

expect(focusLock.isLocked).toBe(true);
expect(focusLock.activeTrapContainerId).toBe('modal-1');
});

test('handles tab navigation cycling inside trap', () => {
focusLock.pushTrap({ containerId: 'modal-1' }, [
{ id: 'modal-btn1', tabIndex: 1, focusable: true },
{ id: 'modal-btn2', tabIndex: 2, focusable: true },
]);

const handled = focusLock.handleKeyDown('tab');
expect(handled).toBe(true);
});
Comment on lines +34 to +42
Comment on lines +34 to +42

@coderabbitai coderabbitai Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Strengthen the "tab navigation cycling" test to assert observable focus movement.

This test only checks handled === true. It does not verify that focus actually moved to 'modal-btn2', that a second Tab wraps back to 'modal-btn1', or that 'bg-btn' is never reachable while the trap is active. The test name promises "cycling inside trap," but nothing here confirms cycling or isolation actually occurred.

Add assertions on focusManager.currentId after each handleKeyDown('tab') call, and confirm 'bg-btn' is skipped.

✅ Proposed strengthening 
   test('handles tab navigation cycling inside trap', () => {
     focusLock.pushTrap({ containerId: 'modal-1' }, [
       { id: 'modal-btn1', tabIndex: 1, focusable: true },
       { id: 'modal-btn2', tabIndex: 2, focusable: true },
     ]);

     const handled = focusLock.handleKeyDown('tab');
     expect(handled).toBe(true);
+    expect(focusManager.currentId).toBe('modal-btn2');
+
+    focusLock.handleKeyDown('tab');
+    expect(focusManager.currentId).toBe('modal-btn1');
   });

As per coding guidelines, "Tests must be real. ... Assert observable behavior or rendered output."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test('handles tab navigation cycling inside trap', () => {
focusLock.pushTrap({ containerId: 'modal-1' }, [
{ id: 'modal-btn1', tabIndex: 1, focusable: true },
{ id: 'modal-btn2', tabIndex: 2, focusable: true },
]);
const handled = focusLock.handleKeyDown('tab');
expect(handled).toBe(true);
});
test('handles tab navigation cycling inside trap', () => {
focusLock.pushTrap({ containerId: 'modal-1' }, [
{ id: 'modal-btn1', tabIndex: 1, focusable: true },
{ id: 'modal-btn2', tabIndex: 2, focusable: true },
]);
const handled = focusLock.handleKeyDown('tab');
expect(handled).toBe(true);
expect(focusManager.currentId).toBe('modal-btn2');
focusLock.handleKeyDown('tab');
expect(focusManager.currentId).toBe('modal-btn1');
});
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/test/focusLock.test.ts` around lines 34 - 42, Strengthen the
`handles tab navigation cycling inside trap` test by asserting
`focusManager.currentId` becomes `modal-btn2` after the first
`handleKeyDown('tab')`, then becomes `modal-btn1` after a second Tab to verify
wrapping. Also assert the background target `bg-btn` is never selected while the
trap remains active, while preserving the existing handled-result assertion.

Source: Coding guidelines


test('pops trap on escape key', () => {
focusLock.pushTrap({ containerId: 'modal-1' }, [
{ id: 'modal-btn1', tabIndex: 1, focusable: true },
]);

const handled = focusLock.handleKeyDown('escape');
expect(handled).toBe(true);
expect(focusLock.isLocked).toBe(false);
});
Comment on lines +44 to +52
Comment on lines +44 to +52

@coderabbitai coderabbitai Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Assert focus restoration after popTrap, and add a dedicated isolation test.

This test verifies isLocked becomes false after Escape, but does not verify that focus is restored to the widget that was focused before the trap (focusManager.currentId), which is an explicit PR objective ("Restore the previous focus highlight state").

Separately, no test in this file verifies that background widgets ('bg-btn') are unreachable via Tab while a trap is active — the core "isolate key event propagation" objective from the linked issue is untested.

✅ Proposed additions 
   test('pops trap on escape key', () => {
     focusLock.pushTrap({ containerId: 'modal-1' }, [
       { id: 'modal-btn1', tabIndex: 1, focusable: true },
     ]);

     const handled = focusLock.handleKeyDown('escape');
     expect(handled).toBe(true);
     expect(focusLock.isLocked).toBe(false);
+    expect(focusManager.currentId).toBe('bg-btn');
   });
+
+  test('isolates focus from background widgets while trapped', () => {
+    focusLock.pushTrap({ containerId: 'modal-1' }, [
+      { id: 'modal-btn1', tabIndex: 1, focusable: true },
+      { id: 'modal-btn2', tabIndex: 2, focusable: true },
+    ]);
+
+    for (let i = 0; i < 5; i++) {
+      focusLock.handleKeyDown('tab');
+      expect(focusManager.currentId).not.toBe('bg-btn');
+    }
+  });

As per PR objectives, "Restore the previous focus highlight state" and "Include unit tests covering focus isolation and related behavior."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test('pops trap on escape key', () => {
focusLock.pushTrap({ containerId: 'modal-1' }, [
{ id: 'modal-btn1', tabIndex: 1, focusable: true },
]);
const handled = focusLock.handleKeyDown('escape');
expect(handled).toBe(true);
expect(focusLock.isLocked).toBe(false);
});
test('pops trap on escape key', () => {
focusLock.pushTrap({ containerId: 'modal-1' }, [
{ id: 'modal-btn1', tabIndex: 1, focusable: true },
]);
const handled = focusLock.handleKeyDown('escape');
expect(handled).toBe(true);
expect(focusLock.isLocked).toBe(false);
expect(focusManager.currentId).toBe('bg-btn');
});
test('isolates focus from background widgets while trapped', () => {
focusLock.pushTrap({ containerId: 'modal-1' }, [
{ id: 'modal-btn1', tabIndex: 1, focusable: true },
{ id: 'modal-btn2', tabIndex: 2, focusable: true },
]);
for (let i = 0; i < 5; i++) {
focusLock.handleKeyDown('tab');
expect(focusManager.currentId).not.toBe('bg-btn');
}
});
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/test/focusLock.test.ts` around lines 44 - 52, Extend the Escape
test around focusLock.handleKeyDown to assert focusManager.currentId is restored
to the widget focused before pushTrap, not only that isLocked becomes false. Add
a dedicated test that activates a trap, attempts Tab navigation toward the
background widget 'bg-btn', and verifies that widget remains unreachable while
the trap is active.

});
Loading