fix(win32): hide the tab-strip chrome when the tab bar is disabled (#244) - #245
Merged
Merged
Conversation
) `window-show-tab-bar = never` makes `Host.tabBarHeight()` return 0, but the tab strip's chrome are real child HWNDs and `layoutChromeForRect` laid every one of them out visible anyway. With a zero-height strip the tab buttons, the UIA Selection container and the [+] / [v] cluster collapsed to `button_height = @max(1, 0 - scaled(6))` -- one pixel -- at the top of the client area and owner-drew themselves there; the active tab's accent border is the blue line in the report. The host carries `WS_CLIPCHILDREN`, so the parent's own band paint can never erase a child rect, and the already-correct `tab_h > 0` gate in `paintChromeTabBar` only covers what the parent paints. It looks tab-dependent because of z-order: the first tab's terminal surface sits above all of the chrome and covers the residue, while every later tab's surface lands below it. Decide the strip's visibility once per layout and gate its children on it: tab buttons, the Selection container and the [+] / [v] buttons are hidden when the tab bar is off, which also drops them out of the F6 focus cycle (`focusableHwnd` keys off `IsWindowVisible`) and out of the UIA tree, and a hidden strip no longer retargets the focused-tab underline or arms its 16 ms slide heartbeat. `App.reconfigureTheme` relayouts on a tab-bar visibility change as well as on an integrated-titlebar frame-mode flip, so a config reload that turns the strip on or off applies on builds where the frame mode does not move.
![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 21 hours and 43 minutes by commenting @sourcery-ai review. Upgrade to get a review now.
|
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 (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Windows implementation tracks tab bar visibility, relayouts chrome after configuration changes, hides tab-strip child windows when disabled, updates UIA state, and adds Windows-only regression tests. ChangesTab bar visibility correction
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The tab-strip controls now follow the disabled tab-bar configuration, including runtime restoration, without an identified merge-blocking regression. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 GuideThe PR fixes Win32 tab-bar-off rendering and accessibility artifacts by explicitly hiding every tab-strip child HWND, updating host layout on runtime visibility changes, and adding pure and live-window regression tests for hide/show and focus behavior. Sequence diagram for tab-bar visibility during layoutsequenceDiagram
participant Config as Runtime config
participant App
participant Host
participant HWNDs as Tab-strip child HWNDs
Config->>App: reconfigureTheme()
App->>App: tabBarVisibleForConfig()
App->>Host: layout()
Host->>Host: shouldShowTabBar()
Host->>Host: tabStripChildVisible(tab_bar_visible, in_strip)
Host->>HWNDs: applyChildVisibility(visible)
alt tab bar disabled
Host->>Host: leave active_tab_left null
Host->>HWNDs: Hide tab buttons, Selection, [+], [▾]
else tab bar enabled
Host->>HWNDs: Position and show strip children
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
 |
Review follow-up to hiding the tab-strip chrome. The HWNDs are hidden but still alive, and three things kept treating them as live UI. `ChromeControlProvider` reported `IsKeyboardFocusable` from the role alone, so the hidden tab buttons and the [+] / [v] cluster still claimed to be keyboard focus targets while the host's focus-region cycle -- which filters candidates through `focusableHwnd`, i.e. `IsWindowVisible` -- would never land on them. It now requires the HWND to be visible, matching the reasoning already applied to the painted caption buttons. `notifyActiveTabUiaSelectionChanged` and the `NameChanged` raise in `syncTabButtons` fired on every tab switch and every retitle with the strip hidden: cross-process COM raises for elements that are not in the UIA tree. Both are gated on `shouldShowTabBar()`; the label and name caches are still committed, so the strip is correct the moment it comes back. `layoutChromeForRect` also keeps applying the [+] / [v] rect while the cluster is hidden -- the visibility gate alone fixes the residue, and `showOverflowMenu` anchors its popup to `GetWindowRect(overflow_hwnd)`, so the rect must not fall behind the window. Both buttons now go through `tabStripChildVisible` like the rest of the strip.
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
With
window-show-tab-bar = never, the tab strip's chrome children stayedvisible as 1 px windows at the top of the client area and painted themselves
over the terminal from the second tab onwards. This hides them, and keeps the
hide/re-show correct across a runtime config reload.
Fixes #244
Root cause
Host.tabBarHeight()returns 0 for.never, but the tab strip is made ofreal child HWNDs and
Host.layoutChromeForRectlaid every one of them outvisible regardless. With a zero-height strip that means
button_height = @max(1, tabBarHeight() - scaled(6))== 1 andaction_y = @max(0, ...)== 0, so each tab button, the UIA Selectioncontainer and the
[+]/[▾]cluster became a one-pixel-tall window aty = scaled(3)and owner-drew itself there — the active tab button's borderis
theme.accent, which is the blue line in the report. The host carriesWS_CLIPCHILDREN, so the parent's band paint can never erase a child rect,and the existing
tab_h > 0gate inpaintChromeTabBaronly covers what theparent paints.
It looks tab-dependent because of z-order. Measured live with
EnumChildWindows(topmost first): the first tab'snoctty.win32surface isat z=0, above all of the chrome, so it covers the residue; the second tab's
surface lands at z=6, below the chrome children at z=1..5, so from the second
tab on the residue shows.
Two consequences beyond the pixels:
focusableHwndkeys offIsWindowVisible, so the strip's tab buttons and[+]/[▾]were liveF6 focus targets, and UIA exposed them as
TabItem/Buttonelements with the tab bar off.
Fix
layoutChromeForRectdecides the strip's visibility once(
tab_strip_visible = shouldShowTabBar()) and gates every strip child onit through a new pure
tabStripChildVisible(tab_bar_visible, in_strip):tab buttons, the Selection container,
[+]and[▾]. A hidden strip alsoleaves
active_tab_leftnull, so a tab switch no longer retargets thefocused-tab underline or arms its 16 ms slide heartbeat for a line that is
never painted.
shouldShowTabBarnow sharestabBarVisibleForConfigwithApp.reconfigureTheme, which relayouts a host when the tab-bar visibilitychanges and not only when the integrated-titlebar frame mode flips. On
Win11 the two always coincide for this setting; below build 22000 the
frame mode never moves, so the reload path needed its own trigger.
ChromeControlProvidernow requiresIsWindowVisiblebefore claimingIsKeyboardFocusable(the painted caption buttons already reason thisway), and
notifyActiveTabUiaSelectionChangedplus theNameChangedraise in
syncTabButtonsare gated onshouldShowTabBar()rather thandoing cross-process COM raises for elements that are not in the UIA tree.
The
[+]/[▾]rect keeps being applied while hidden -- the visibilitygate alone fixes the residue, and
showOverflowMenuanchors its popup toGetWindowRect(overflow_hwnd)-- and both buttons go throughtabStripChildVisiblelike the rest of the strip.How verified
Built with
zig build -Demit-exe=trueand driven headlessly (no foregroundstealing) with a Python + ctypes harness: launch with
--config-fileholdingthe reporter's config,
WM_COMMAND1904 / 1907 / 1908 for new tab / previoustab / next tab,
EnumChildWindowsfor the objective state, PrintWindow and ascreen BitBlt for pixels.
Before (05a9c47),
never, two tabs, tab 2 active — every strip childvisible at 1 px:
The screen capture of that state shows the reported artifacts: a thin line
across the top-left of the terminal content and two short dashes at the top
right where
[+]and[▾]are.After, same steps (single tab, two tabs, both switch directions, and
maximized + switch): every one of ids 1000/1001/1904/1911/1912 reports
visible=0at every stage, the only visible children are the terminalsurfaces, and the screen capture of the top band is clean.
Tab bar still works (
window-show-tab-bar = auto, same harness): tabbuttons 330x51,
[+]/[▾]48x48, all visible, and the screenshot showsthe normal strip with the focused-tab underline.
Runtime config reload (
Reload Configfrom the command palette, configfile rewritten between reloads, two tabs open):
UI Automation (raw view walk of the host element, two tabs open): with
neverthe tree now holds only the terminalTextelement and the stockcaption; with
autoit holdsTab "Tabs",Button "New tab",Button "More tabs"and bothTabItems, all still reportingIsKeyboardFocusable = 1(theneverrun has no such elements left toreport on). (The before-fix UIA tree was not captured; that the hidden HWNDs
used to be exposed is inference from the
autocontrol plus the measuredvisible=1above.)All of the live runs above were repeated on the review follow-up commit with
the same results; the
[+]/[▾]rects now read as the current right-edgeposition while hidden instead of the creation rect.
Tests — two added to
src/apprt/win32.zig:win32 tabStripChildVisible hides strip chrome when the tab bar is off(pure predicate, including
tabBarVisibleForConfig).win32 layoutChromeForRect hides tab-strip chrome when the tab bar is off— a live-HWND regression test: real tab-button / container /
[+]/[▾]children under a real host window,
layoutChromeForRectrun with.neverthen
.always, assertingIsWindowVisible,tabStripFocusHwnd(), thelive
[▾]rect and the restored button height. Verified it fails on thepre-fix behaviour (
expected 0, found 1).ChromeControlProvider hidden chrome is not keyboard focusableinsrc/apprt/win32_uia/widgets.zig— a real hidden window, assertingIsKeyboardFocusablefalse while hidden and true once shown. Also verifiedto fail without the provider change.
zig build test -Dtest-filter=ConPTY -Dtest-filter=strip→ 92 passed, 4skipped, 0 failed;
-Dtest-filter=ChromeControlProvider→ 80 passed, 4skipped; wider sweeps
-Dtest-filter=win32→ 1485 passed, 5 skipped and-Dtest-filter=tab→ 364 passed, 4 skipped, all with 0 failures (79 passedwith the
ConPTYfilter alone).Not verified: anything on Windows 10 / builds below 22000, where the new
reconfigureThemerelayout trigger is the only thing that applies awindow-show-tab-barreload — this machine is Windows 11 26200, where theframe-mode flip already covered it.
AI assistance
Per
AI_POLICY.md: this change was authored by an AI agent (Claude Code)operating the maintainer's account with authorization. The agent reproduced
the bug on a build of
main, diagnosed it, wrote the fix and the tests, andcollected all of the evidence above on Windows 11 26200. A separate
adversarial review pass (also AI) was run against the diff independently of
the author. Maintainer review of the diff, the root-cause explanation and the
before/after evidence is still to come; nothing here should be merged on the
strength of the agent's own account of it.
Summary by CodeRabbit