button: Show keyboard focus on borderless variants when `focus_ring` is off by madcodelife · Pull Request #3299 · longbridge/gpui-kit

madcodelife · 2026-09-28T13:55:52Z

Problem

With Theme::focus_ring = false, focus_ring_style only tinted the element's border with theme.ring. Button only has a border width on the Default variant and with .outline(), so Ghost / Text / Link and the filled variants (Primary, Secondary, Danger, …) showed no keyboard focus at all, even though they are Tab stops.

Fix

focus_ring_style, when the outer ring is off:

Element has a border (Default / outline buttons, Input, Checkbox, Radio, Switch track, Select, …): unchanged, the border is tinted.
Element has no border: an absolute child at inset 0 draws a 1px ring-coloured border, with the element's own corner radii.
- No layout change: no border width is added and the child is absolutely positioned.
- It can't be clipped: it sits inside the element's bounds.
- Radius: with no border or padding between them, the element's radius is already concentric with its edge. Theme radius 0 gives a square ring.
- Only on keyboard focus: Button still calls prevent_default on mouse down, so a click never focuses it and never shows the ring.

1px matches the width of the tinted border the bordered variants already get, so every variant reads the same. Bordered elements keep the tinted border rather than switching to the inner ring: the tinted border already sits at the edge, and an extra inner line would double it.

focus_ring = true is unchanged (outer 3px ring).

The helper change also covers the other borderless callers of focus_ring_style (questionnaire options, carousel, time field). They now show the inner ring too when focus_ring is off.

Refactor: the border-width / corner-radius reads in focus_ring were moved into small private helpers so the inner ring reuses them. No behaviour change there.

Tests

Two new Metal pixel tests in crates/kit/tests/rendering.rs (main thread, harness = false). They render Ghost (icon), Text, Link and Primary buttons with focus_ring = false:

borderless_buttons_show_keyboard_focus_inside_when_focus_ring_is_off: moves focus with Tab through Root (focus_next for the first stop, since Root's binding needs something inside it focused). For each button it asserts: no ring pixels while unfocused, ring pixels along the top edge inside its bounds when focused, and zero changed pixels outside its bounds.
clicking_a_button_draws_no_focus_ring: clicking each button leaves focus empty and draws no ring pixels.

Without the fix, the first test fails on ghost ("a keyboard-focused ghost button must draw a ring inside its bounds").

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 --all-targets -- -D warnings

…is off With `Theme::focus_ring` off, `focus_ring_style` only tinted the border, so Ghost / Text / Link and filled buttons without `.outline()` showed no focus at all. Elements without a border now get a 1px ring inside their edge instead: no layout change, and nothing an ancestor can clip. Closes #3298 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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 ``` 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 db8c62b into main Sep 28, 2026
11 checks passed

madcodelife deleted the button-focus-inner-ring branch September 28, 2026 14:09

madcodelife mentioned this pull request Sep 28, 2026

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

Merged

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: Show keyboard focus on borderless variants when `focus_ring` is off - #3299

button: Show keyboard focus on borderless variants when `focus_ring` is off#3299
madcodelife merged 1 commit into
mainfrom
button-focus-inner-ring

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!

Problem

Fix

Tests

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

madcodelife commented Sep 28, 2026 •

edited

Loading