Skip to content

fix(launcher): unify dialog keyboard/focus handling onto a native Modal component#458

Merged
Xoshbin merged 2 commits into
mainfrom
fix/unify-modal-keyboard-handling
Jul 8, 2026
Merged

fix(launcher): unify dialog keyboard/focus handling onto a native Modal component#458
Xoshbin merged 2 commits into
mainfrom
fix/unify-modal-keyboard-handling

Conversation

@Xoshbin

@Xoshbin Xoshbin commented Jul 8, 2026

Copy link
Copy Markdown
Owner

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the application's dialogs by replacing custom modal overlays with a unified, native <dialog>-backed Modal component, updating global keyboard handlers to prevent key leaks when modals are open. The review feedback suggests enhancing the isActionableElementFocused helper to correctly handle HTMLSelectElement, button-like HTMLInputElement types, and contenteditable elements, as well as refining getFirstFocusable to skip hidden inputs and adding corresponding unit tests.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread asyar-launcher/src/components/base/Modal.logic.ts
Comment on lines +28 to +37
export function getFirstFocusable(container: HTMLElement): HTMLElement | null {
const candidates = container.querySelectorAll<HTMLElement>(FOCUSABLE_SELECTOR);
for (let i = 0; i < candidates.length; i++) {
const el = candidates[i];
if (el.hasAttribute('disabled')) continue;
if (el.tabIndex < 0) continue;
return el;
}
return null;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The getFirstFocusable helper is used to find a fallback focus target inside the dialog. However, it currently does not filter out hidden inputs (<input type="hidden">), which are matched by FOCUSABLE_SELECTOR but are not focusable. Additionally, checking both the disabled attribute and the disabled property ensures maximum compatibility across different browsers and test environments (like jsdom).

We should update the loop to skip hidden inputs and check both the attribute and property for disabled states.

Suggested change
export function getFirstFocusable(container: HTMLElement): HTMLElement | null {
const candidates = container.querySelectorAll<HTMLElement>(FOCUSABLE_SELECTOR);
for (let i = 0; i < candidates.length; i++) {
const el = candidates[i];
if (el.hasAttribute('disabled')) continue;
if (el.tabIndex < 0) continue;
return el;
}
return null;
}
export function getFirstFocusable(container: HTMLElement): HTMLElement | null {
const candidates = container.querySelectorAll<HTMLElement>(FOCUSABLE_SELECTOR);
for (let i = 0; i < candidates.length; i++) {
const el = candidates[i];
if (el.hasAttribute('disabled') || (el as any).disabled) continue;
if (el instanceof HTMLInputElement && el.type === 'hidden') continue;
if (el.tabIndex < 0) continue;
return el;
}
return null;
}

Comment on lines +36 to +40
it('returns true for a textarea (Enter should insert a newline, not submit)', () => {
const textarea = document.createElement('textarea');
expect(isActionableElementFocused(textarea)).toBe(true);
});
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

To ensure the new actionable element checks are thoroughly verified and regression-proof, we should add unit tests for HTMLSelectElement, actionable HTMLInputElement types, and contenteditable elements.

  it('returns true for a textarea (Enter should insert a newline, not submit)', () => {
    const textarea = document.createElement('textarea');
    expect(isActionableElementFocused(textarea)).toBe(true);
  });

  it('returns true for a select element', () => {
    const select = document.createElement('select');
    expect(isActionableElementFocused(select)).toBe(true);
  });

  it('returns true for actionable input types', () => {
    const submit = document.createElement('input');
    submit.type = 'submit';
    expect(isActionableElementFocused(submit)).toBe(true);

    const button = document.createElement('input');
    button.type = 'button';
    expect(isActionableElementFocused(button)).toBe(true);
  });

  it('returns true for contenteditable elements', () => {
    const div = document.createElement('div');
    div.contentEditable = 'true';
    expect(isActionableElementFocused(div)).toBe(true);
  });
});

@Xoshbin Xoshbin merged commit fa40bb7 into main Jul 8, 2026
1 of 2 checks passed
@Xoshbin Xoshbin deleted the fix/unify-modal-keyboard-handling branch July 8, 2026 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant