button: Keep the focus line off text and legible on fills by madcodelife · Pull Request #3300 · longbridge/gpui-kit

madcodelife · 2026-09-28T14:21:42Z

Follow-up to #3299 (#3298). When Theme::focus_ring is off, #3299 draws a 1px ring line on a borderless button's edge. That has two visual problems:

Text / Link: these variants have no padding, so the line sits on the label and its underline.
Filled variants (Primary, Secondary, Danger, …): a ring-coloured line on the edge barely shows against dark fills. In the default light theme it is neutral-400 on neutral-900.

Change

Each borderless button now picks where its focus line goes, through a crate-private FocusLine passed to styled::focus_style:

Variant	Line	Colour
Ghost	on the edge (unchanged)	`ring`
Text, Link	2px (0.125rem) outside the edge, radius = element radius + 2px	`ring`
Primary / Secondary / Danger / Info / Success / Warning / Custom	2px inside the fill, radius = max(element radius − 2px, 0)	the variant's own foreground at 60%
Default, any `.outline()`	tinted border (unchanged)	`ring`

Filled variants use their own foreground. The theme already keeps a button's foreground legible against its fill, in every variant and in both modes. The ring token is tuned for the page background, not for a primary fill.
Why not a 2px ring line? It is still low-contrast on fills.
Why not an outside offset ring (background-coloured gap plus a ring line)? It is exactly what focus_ring = false exists to avoid: containers clip it.
Text/Link outside is safe in practice. The outset is only 3px including antialiasing. Callers place these buttons with padding around them; the ai-chat HITL card's "other option" text button renders unclipped. A clipping ancestor flush with such a button would cut the line.
Layout is unchanged in every case. The line is an absolutely positioned child and no border width is added.
Nothing else changes. focus_ring = true, bordered variants, and other callers of focus_ring_style behave as before.

Tests

crates/kit/tests/rendering.rs. These run on the main thread (harness = false, Metal), with Ghost / Text / Link / Primary buttons and focus_ring = false.

focus_lines_stay_off_content_and_contrast_with_fills runs in light and dark. Focus moves with Tab through Root. For each button:

Every button: no pixel changes outside the button (outside the button plus 3px for Text/Link).
Ghost: ring-coloured pixels on the top edge.
Text / Link: no pixel changes inside the button's bounds, which is where the label is (the four corner squares crossed by the line's arc are excluded). There are ring-coloured pixels 2px above the edge.
Primary: the line row 2px inside differs from the fill by ≥ 96/255 in some channel.

clicking_a_button_draws_no_focus_line: clicking a hovered button leaves focus empty and the capture identical to the hovered one.

These tests fail on origin/main before #3299 at ghost, and on #3299 alone at text (288 device pixels over the label).

cargo test -p gpui-kit --features test-support,component,assets --test rendering
cargo test -p gpui-component
cargo clippy -p gpui-component -p gpui-kit --features gpui-kit/test-support,gpui-kit/component,gpui-kit/assets --all-targets -- -D warnings

Text and Link buttons have no padding, so a line on their edge sat on the label; draw it 2px outside instead. On filled variants a `ring` line on the edge barely showed against dark fills; draw it 2px inside in the button's foreground at 60%, which the theme keeps legible against the fill. Ghost keeps the edge line, bordered variants keep the tinted border. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…hts on key presses (#3307) 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 `Button`s, 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. - Bordered (default): the border is tinted `ring`, plus the outer ring when `focus_ring = true`. - `bordered(false)`: the #3299 1px edge line. - No layout change. The line is appended after the table content, so rows don't paint over it. - **`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. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

madcodelife merged commit eb6c13a into main Sep 28, 2026
11 checks passed

madcodelife deleted the button-focus-line-placement branch September 28, 2026 14:50

madcodelife mentioned this pull request Sep 29, 2026

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

Merged

Navigation Menu

Uh oh!

button: Keep the focus line off text and legible on fills - #3300

button: Keep the focus line off text and legible on fills#3300
madcodelife merged 1 commit into
mainfrom
button-focus-line-placement

madcodelife commented Sep 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

madcodelife commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change

Tests

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

madcodelife commented Sep 28, 2026 •

edited

Loading