Skip to content

table, menu: Show keyboard focus on DataTable and keep menu highlights on key presses - #3307

Merged
madcodelife merged 1 commit into
mainfrom
list-menu-focus
Sep 29, 2026
Merged

madcodelife merged 1 commit into
mainfrom
list-menu-focus

Conversation

@madcodelife

@madcodelife madcodelife commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

Follow-up to #3299 / #3300, which noted that List / ListItem, Table and the menus are focusable or keyboard-navigable but don't go through focus_style. This PR checks each one and fixes the two that were actually broken.

Audit

With focus_ring = false and focus_ring = true (same result in both unless stated):

Component Tab stop Arrow keys Keyboard focus / navigation (before) Mouse hover Selection Verdict
DataTable (TableState) Yes (tab_stop(true)) Up/Down/Left/Right, Home/End, PgUp/PgDn, Tab moves cells Nothing when tabbed in. Rows only show once a key selects one. table_hover row bg, hidden while typing (GPUI) accent / table_active row or cell Fixed: focus ring on keyboard focus
PopupMenu / ContextMenu / DropdownMenu items No: the menu holds focus while open Up/Down/Left/Right The accent highlight is the cursor. Bug 1: hover item A, press down → A and B both lit. Bug 2: hover A, press a key with no binding → highlight goes out, next down restarts at the top. Hovering moves the highlight, the native menu convention same highlight Fixed both
AppMenuBar Titles are ghost Buttons, so Tab stops with the #3299 edge line Left/Right between menus Open title shows the ghost open bg, and the popup is a PopupMenu (above) ghost hover — Covered by Button + the PopupMenu fix
List / ListItem No. ListState's handle isn't a tab stop. A searchable list focuses its query Input. Up/Down Selected row (list_active / accent) is the cursor, and it looks different from hover list_hover, hidden while typing (GPUI) same as keyboard cursor No change needed, covered by a new test

Not changed, worth a separate decision: a List's selected row looks the same whether or not the list has focus, and ListState isn't a Tab stop. Native table views grey out the selection when unfocused. Doing that would change selection styling, which is out of scope here.

Changes

  • DataTable: calls focus_ring_style when its handle is focused and window.last_input_was_keyboard(). That is GPUI's focus_visible rule; it matters here because clicking a row also focuses the table.
  • MenuItemElement: group_hover on its own group → hover.
    • GPUI computes an element's style before it pushes the element's group hitbox. A group_hover on the element's own group therefore takes the hover_state.group fallback, which ignores keyboard modality, so the pointer's item stayed lit next to the keyboard cursor.
    • hover uses the hitbox, which GPUI already suppresses after a key press. Mouse-hover look is unchanged.
    • The now-unused group_name is removed.
  • PopupMenu: the on_hover(false) that clears the highlight is skipped when the last input was a key press. GPUI ends hover under a still pointer on any key press, so the highlight used to vanish and the next arrow key restarted from the top. A real pointer exit still clears it.

Tests

crates/kit/tests/rendering.rs (main thread, harness = false, Metal). Capture now takes its scale from the window, so it works for any window size.

  • menu_highlight_is_the_keyboard_cursor: exactly one highlighted row after each step.
    • Hover Beta → [1].
    • down → [2]. Fails with [1, 2] when group_hover is restored.
    • Hover Beta again → [1].
    • Unbound x → [1]. Fails with [] without the on_hover guard.
    • down → [2].
  • list_selection_is_the_keyboard_cursor:
    • Hover row 1 → only row 1 is lit.
    • down → only row 0 is lit, and its fill differs from the hover fill.
    • Fails with [] when the selection isn't forwarded to the item. Nothing needed fixing for List, so this one locks existing behaviour.
  • table_shows_keyboard_focus_only, for focus_ring = false and true:
    • Tab from a button into the table → ring pixels on its top border. With true, the outer band changes too.
    • In a fresh window, hover then click a row → the table is focused, but the border and outer band are identical to the hovered capture.
    • Fails without the fix, and also fails when the keyboard gate is dropped.
cargo test -p gpui-kit --features test-support,component,assets --test rendering   # 10 passed (Metal)
cargo test -p gpui-kit --features test-support,component,assets                    # 278 passed
cargo test -p gpui-component                                                        # 575 passed
cargo clippy -p gpui-component -p gpui-kit --features gpui-kit/test-support,gpui-kit/component,gpui-kit/assets --all-targets -- -D warnings

Note: last_input_was_keyboard flips back when the mouse moves, so the table's ring disappears on mouse movement, the same as GPUI's own focus_visible style. Switching modality already triggers a single window.refresh() in GPUI, so this adds no idle redraws.

…hts on key presses

- `DataTable` is a Tab stop but drew nothing when focused. It now calls
  `focus_ring_style` on keyboard focus only (`last_input_was_keyboard`),
  since clicking a row focuses it too.
- `MenuItemElement` styled hover with `group_hover` on its own group. GPUI
  computes an element's style before it registers the element's group, so
  that fallback ignores keyboard modality: after hovering an item and
  pressing `down`, both items were lit. Use `hover` instead.
- `PopupMenu` cleared its highlight when a key press ended hover under a
  still pointer, so the next arrow key restarted from the top. Keep it.
- Main-thread pixel tests for a menu, a list and a table.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@madcodelife
madcodelife merged commit cc83991 into main Sep 29, 2026
11 checks passed
@madcodelife
madcodelife deleted the list-menu-focus branch September 29, 2026 02:45
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