Skip to content

Use one visible keyboard focus ring - #206

Merged
tebriz91 merged 2 commits into
mainfrom
windows/focus-ring
Oct 28, 2026
Merged

tebriz91 merged 2 commits into
mainfrom
windows/focus-ring

Conversation

@tebriz91

@tebriz91 tebriz91 commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Keyboard focus now adds one 2 px accent ring with a 2 px outside offset, remaining visible while toolbar panels are open (fixes #194).

  • Replace every app and PDF focus_visible style with the shared ring. Include sidebar and Settings rows, text inputs, Inspector tabs, and the newly merged shortcuts sheet.
  • Preserve hover, selection and toggle fills. Paint above sibling fills and deferred surfaces; reserve the full ring when scrolling a focused control into view.
  • Accept ADR 0074 in current-rules form, remove 0.3.0 item 5 from ROADMAP, and add changes/focus-ring.md.

Validation

  • Rebased on origin/main at b7c1d28, including merged feat(ui): keyboard shortcuts panel from one binding table (ADR 0073) #204.
  • just lint and just test passed: 822 tests passed, 20 intentionally ignored, plus release-script tests.
  • just fmt-check, just deny, and just lint-scripts passed.
  • just headless-renders target/focus-ring-renders-complete passed: 50 screen records and 10 focused/resting comparisons across light and dark. Pixel checks verify the stroke, unchanged fills/labels, and all sides, including Review beside selected Share.
  • Reproduced the Settings footer clipping with a failing regression; forward and backward traversal now keep the entire outside ring visible in both themes.

Review

Claude Haiku (claude-haiku-5-5, max) failed with Claude API rate limit reached. Try again later. Used the user-authorized fallback, Codex gpt-6.1-sol with high reasoning effort.

  1. P2: later siblings cover the ring — fixed. The ring now paints in a deferred layer above all app popovers/sheets, retaining its parent scroll mask. A both-theme pixel regression failed before the fix with one missing side. GPUI prompt buttons use the same painter directly in their already topmost layer, because GPUI prepaints prompts after its deferred pass; existing dialog tests pass.
  2. P2: scrolling clips the outside stroke — fixed. Focus reveal includes the 4 px ring outset and the host border. A both-theme regression checks forward and backward traversal in a 640 x 480 Settings window. Tab render captures now settle deferred scrolling before rasterization.
  3. P2: alias focus still adds a fill — fixed. Removed its focus-only fill and border. The shared input supplies the ring; the alias text colour, selection and hover decoration remain.

The follow-up Codex review confirmed those three fixes and found one further P2:

  1. P2: the Comments toggle clips its right-hand stroke — fixed. Reserve the ring outset on all sides of the toggle. A new paired-image regression uses the public comments.docx fixture in the actual Original pane; it failed before the fix with one missing side and now passes in both themes. The headless runner waits for DOCX temporary-file cleanup before writing its summary.

No other significant findings were reported in the follow-up review. All four findings have been addressed. Final head: 59f4ba42b846f55ae3245f0016b02bb4e404f395.

Native Windows verification remains pending: needs the user at the machine. sky.list_windows() returned Computer Use native pipe is unavailable: failed to connect native pipe: The system cannot find the file specified. (os error 2). No native app was started or captured. The headless DirectX WARP checks do not establish native keyboard, accessibility, IME or display-presentation acceptance.

Do not merge.

@tebriz91
tebriz91 marked this pull request as ready for review October 28, 2026 03:57
@tebriz91
tebriz91 merged commit ab20942 into main Oct 28, 2026
5 checks passed
@tebriz91
tebriz91 deleted the windows/focus-ring branch October 28, 2026 04:09
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.

Panel toggles in the toolbar show no focus ring while their panel is open (Windows)

1 participant