polish(win32): show focus rings only for keyboard focus and tidy the chrome - #247
Merged
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @amanthanvi, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 5 days and 14 hours by commenting @sourcery-ai review. Upgrade to get a review now.
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesThe Windows frontend now tracks keyboard and pointer input modes for focus-ring rendering. Settings and host chrome use the shared focus-cue policy, with high-contrast and modal overlay exceptions. The change also corrects overlay edit sizing, themed edit borders, integrated-titlebar accents, and command-palette punctuation. Win32 focus and chrome
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~30 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The Win32 focus and chrome updates have no concrete unresolved merge-blocking risk in the supplied evidence. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideThis polish pass centralizes keyboard-versus-pointer focus-cue decisions and wires them into both Win32 chrome paths, while correcting menu dismissal focus, tab rendering and repaint behavior, titlebar accents, palette EDIT geometry, Settings field borders, and hint punctuation. Unit coverage was updated for the policy and DPI-sensitive layout changes, with the implementation informed by Windows UI-state and pixel measurements. Sequence diagram for keyboard-aware Win32 focus ringssequenceDiagram
participant User
participant AppPump
participant HostOrSettings
participant focus_cue
participant OwnerDrawnControl
User->>AppPump: Tab, arrow, F6, or pointer input
alt Settings dialog navigation
AppPump->>HostOrSettings: noteInputMessage(hwnd, message, wParam)
AppPump->>AppPump: IsDialogMessageW
else Host chrome input
HostOrSettings->>HostOrSettings: noteChromeKeyInput or noteChromePointerInput
end
HostOrSettings->>focus_cue: keyRevealsRing(vk) or showRing(focused, disabled, mode, high_contrast)
focus_cue-->>HostOrSettings: keyboard mode, pointer mode, or High Contrast result
HostOrSettings->>OwnerDrawnControl: InvalidateRect on mode change
OwnerDrawnControl->>focus_cue: showRing(...)
focus_cue-->>OwnerDrawnControl: Paint ring only when policy allows
Flow diagram for dismissed chevron menu focus restorationflowchart LR
Click[Click chevron button] --> Menu[TrackPopupMenu]
Menu -->|command selected| Activation[Existing activation target]
Menu -->|dismissed with Escape or no command| Check{"GetFocus() == button"}
Check -->|yes| Restore[refocusHostAfterActivation]
Check -->|no| Continue[Continue normally]
Restore --> Shell[Terminal or activation target receives next key]
Activation --> Shell
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…chrome Every owner-drawn button in the host and in the settings window painted its focus ring straight off `GetFocus()`. Native controls do not: the OS keeps `UISF_HIDEFOCUS` set (measured `WM_QUERYUISTATE` = 0x3 on a fresh host and settings window) until the dialog manager sees Tab or an arrow key, and a mouse click sets it again. So a tab that had just been clicked, the `[▾]` whose menu had just been dismissed, and the section button that opens Settings all wore a blue ring nobody asked for. * `win32/focus_cue.zig` is the pure policy: rings show only in keyboard mode, High Contrast keeps them on pointer focus, and only navigation keys (Tab, Enter, Space, Escape, arrows, Home/End/PgUp/PgDn, F6) flip the mode back. The host tracks the mode from its chrome button procs, the overlay query edit and the docked search edit, and from the `cycle_focus_region` keybind, which calls `SetFocus` without any key reaching the control. The settings window tracks it from the App pump, ahead of `IsDialogMessageW`, because the dialog manager consumes the navigation keys and never dispatches them to the control that gains focus. * Dismissing the `[▾]` menu without choosing a command left keyboard focus on the chevron, so the next keystroke went to the button, not the shell. Focus now returns to the terminal (or the overlay) through the same activation-target rule the window already uses. * The tab label no longer carries the `* ` active marker. The raised button and the accent underline already say which tab is active, UIA says it through `SelectionItem.IsSelected`, and the marker cost every active title two cells and made labels jump on every switch. Tab activation now invalidates the two buttons directly, since the label text no longer changes. * The 2 px accent strip on row 0 of the integrated titlebar sat directly under DWM's own 1 px accent border and read as a blue rim around the whole window. It is now painted only for the separate tab row, where it sits below the system caption. * The command palette query EDIT was 12 px tall inside a 24 px row at 96 DPI, under a font whose GDI cell is 19 px, so the EDIT class clipped every glyph at the waist. The vertical inset is 2 px, which keeps the frame's stroke visible around the child at 96..288 DPI. * Settings edit fields used `WS_EX_CLIENTEDGE`, a hard-coded 3-D bevel that `DarkMode_Explorer` never themes, so every dark-theme field wore a near-white 2 px frame. A themed `WS_BORDER` EDIT draws the same 1 px frame the combo boxes beside it draw, in both themes. * The palette hint line rendered "Open the config file.." because a full stop was appended to descriptions that already end in one. Verified live at 150% DPI on Windows 11 26200 with a screen-BitBlt harness (child enumeration plus GetGUIThreadInfo focus): F6 into the strip paints the ring on the active tab; clicking a tab focuses it with no ring; Right arrow brings the ring back on the tab it lands on; the Settings rail shows no ring on open or after a click and shows it after a Down arrow; focus after dismissing the `[▾]` menu is the terminal surface; the query box shows whole glyphs; the dark settings fields have a 1 px (48,48,48) frame instead of (254,254,254).
Follow-ups from an independent review of the focus-cue change. * The `* ` active marker stays in the tab label. The UIA tree does say which tab is selected, but `docs/accessibility-matrix.md` records that NVDA reads the BUTTON-class tab items through MSAA window text and never speaks the selection state, so the asterisk is the only spoken cue for the active tab. The flagship contract suite also pins the test that proves it. Reverted to the label builder on main. * The confirm prompt's Allow / Cancel ring is not gated on the input mode. Focus lands on Allow programmatically and Enter presses whatever holds focus, so the ring is the only mark of what Enter will do; native dialogs keep their default button visibly marked under `UISF_HIDEFOCUS` for the same reason. * Escape on a `[▾]` menu that the keyboard opened (F6, Space) leaves focus on the chevron, as every menu does; only a pointer-opened menu hands focus back to the terminal. * The tab-overview banner's F6 and the strip's own Delete / F2 / Apps verbs count as navigation, so the control they leave focus on paints its ring. A pointer press on the terminal surface returns the mode to pointer. * The settings window resets its mode when its HWND is torn down, so a window reopened by the mouse does not inherit the keyboard mode from the previous one, and the `WM_SETTINGS_PROOF_LIVE_FOCUS_RING` proof reports whether a ring is painted rather than whether the button has focus. Verified live: F6 then Space on the chevron, Escape -> focus stays on `Button id=1911` with its ring; click then Escape -> focus on the terminal surface; a multi-line paste started from a palette row click -> Allow holds focus with its ring; Settings Down, close, reopen -> Terminal selected with no ring. The four `buildTabButtonLabel` contract names resolve to exactly one declaration each.
amanthanvi
force-pushed
the
polish/win32-chrome
branch
from
September 16, 2026 03:01
3991a14 to
bf4a230
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A polish pass over the Win32 chrome and the Settings window, driven by the
things that have been visibly janky for a while: the blue outline that hung
around tabs and buttons, the near-white bevels on dark Settings fields, the
clipped text in the command palette query box, and a few smaller rough edges
that turned up once the harness was pointed at them. No new features; every
change is a refinement of the incumbent look.
What changed and why
Focus rings follow the keyboard, not the pointer. Every owner-drawn
button in the host and in Settings painted its focus ring straight off
GetFocus(). Native Windows controls do not: the OS keepsUISF_HIDEFOCUSset until the dialog manager sees Tab or an arrow key, and a mouse click sets
it again (measured:
WM_QUERYUISTATEreturns0x3on a fresh host and afresh Settings window). So a tab you had just clicked, the
[▾]whose menuyou had just dismissed, and the section button that opens Settings all wore
a ring nobody asked for.
src/apprt/win32/focus_cue.zigis the pure policy(rings in keyboard mode only; High Contrast keeps them on pointer focus;
only navigation keys reveal them). The host tracks the mode from its chrome
button procs and from the
cycle_focus_regionkeybind, which callsSetFocuswithout any key reaching the control. Settings tracks it from theApp pump ahead of
IsDialogMessageW, because the dialog manager consumesTab and the arrows and never dispatches them to the control that gains
focus.
[▾]no longer keeps focus after its menu is dismissed. The BUTTON classtakes focus on the click that opens the menu, and nothing gave it back when
the menu closed without a command, so the next keystroke went to the
chevron rather than the shell. Focus now returns through the same
activation-target rule the window already uses.
Modal prompt buttons keep their ring. The confirm prompt's Allow /
Cancel are the one place the pointer gate does not apply: focus lands on
Allow programmatically and Enter presses whatever holds focus, so the ring
is the only mark of what Enter will do, the same reason native dialogs keep
their default button visibly marked.
The
*active-tab marker stays. An earlier revision of this branchdropped it as a third cue next to the raised button and the underline, but
docs/accessibility-matrix.mdrecords that NVDA reads the BUTTON-class tabitems through MSAA window text and never speaks the UIA selection state, so
the asterisk is the only spoken cue for the active tab. Reverted.
No accent strip under DWM's accent border. The 2 px strip painted on row
0 of the integrated titlebar sat directly under the 1 px accent border DWM
already draws and read as a blue rim around the whole window. It is now
painted only for the separate tab row (Win10, or decorations without the
integrated titlebar), where it sits below the system caption.
Command palette query text is no longer clipped. The EDIT child was
12 px tall inside a 24 px row at 96 DPI under a font whose GDI cell is
19 px, and the EDIT class clips glyphs to its client rect, so every letter
was cut at the waist. The vertical inset is now 2 px, which keeps the
frame's stroke visible around the child at 96, 144, 192 and 288 DPI.
Settings edit fields match the combo boxes. They used
WS_EX_CLIENTEDGE, a hard-coded 3-D bevel thatDarkMode_Explorerneverthemes, so every dark-theme field wore a near-white 2 px frame. A themed
WS_BORDEREDIT draws the same 1 px frame the combo boxes beside it draw,in both themes.
Palette hint copy. "Open the config file.." is now "Open the config
file." (a full stop was appended to descriptions that already end in one).
How verified
Built with
zig build -Demit-exe=trueand driven headlessly at 150% DPI onWindows 11 26200 with a Python + ctypes harness (
EnumChildWindows,GetGUIThreadInfofor focus, screen BitBlt for pixels,WM_COMMANDfortabs and the palette).
Before / after, same steps:
[▾]menu with EscapeButton id=1911(104,144,212)(32,32,32)(254,254,254)(48,48,48)[▾], then EscapeUnit tests added or updated:
focus_cuepolicy (3), overlay edit childheight at four DPIs, palette feedback full stop, Settings section ring with
input mode. Filters
run:
focus,label,tab,overlay,settings,palette(each pairedwith
ConPTY), all passing.Not covered here: the confirm preview EDIT and
UIA_LabeledByPropertyIdgaps from #242, and the Settings "Enable background blur" label from #239,
are still open follow-ups.
AI assistance
Per
AI_POLICY.md: this change was authored by an AI agent (Claude Code)operating the maintainer's account with the maintainer's authorization. The
agent measured the baseline, made the changes, ran the unit tests and the
live before/after captures quoted above, and obtained an independent
adversarial review before the PR was opened. The maintainer reviews the
diff and the captures before merge.
Summary by Sourcery
Polish the Win32 chrome and Settings presentation with input-aware focus cues, cleaner borders and accents, corrected palette text rendering, and improved focus behavior.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes