fix(menubar): suppress clicks when fullscreen menu bar items rest above the screen#626
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ChangesMenu bar visibility detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates NSScreen.isSystemMenuBarVisible() to better match perceived menu bar visibility in fullscreen/auto-hide scenarios by requiring at least one status item window to intersect the screen’s vertical bounds (not just be flagged .onScreen).
Changes:
- Expanded documentation explaining why
.onScreenis insufficient in fullscreen auto-hide states - Updated visibility logic to check each menu bar item window’s bounds against the current screen frame
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let windowIDs = Bridging.getMenuBarWindowList(option: [.onScreen, .activeSpace, .itemsOnly]) | ||
| let screenFrame = frame | ||
| return windowIDs.contains { windowID in | ||
| guard let bounds = Bridging.getWindowBounds(for: windowID) else { | ||
| return false | ||
| } | ||
| return bounds.intersects(screenFrame) | ||
| } |
| let windowIDs = Bridging.getMenuBarWindowList(option: [.onScreen, .activeSpace, .itemsOnly]) | ||
| let screenFrame = frame | ||
| return windowIDs.contains { windowID in | ||
| guard let bounds = Bridging.getWindowBounds(for: windowID) else { | ||
| return false | ||
| } | ||
| return bounds.intersects(screenFrame) | ||
| } |
|
Can you please detail the reproduction steps for this on the existing Having a video of you reproducing the suspected issue would also help me understand whether what you're experiencing is "as expected" or actually a bug. |
|
This seems to have been fixed in the latest beta release |
What does this PR do?
Tightens
NSScreen.isSystemMenuBarVisible()so it returnsfalsenot just when the Window Server reports zero on-screen menu bar items, but also when those items are flagged on-screen yet positioned above the screen's top edge (the auto-hide resting position used inside a fullscreen space). All three call sites —HIDEventManager.handleShowOnClick,HIDEventManager.handleSecondaryContextMenu, andControlItem.performAction— pick up the stricter check automatically.PR Type
Does this PR introduce a breaking change?
If yes, please describe the impact and migration path:
N/A
What is the current behavior?
While another app is in fullscreen with the system menu bar auto-hidden, clicking near the top of that app's window (e.g. the top edge of a Chrome tab) makes Thaw treat the click as a menu bar click and reveal the hidden section. The existing guard added in #574 only filters by the Window Server's
kCGWindowIsOnscreenflag, but macOS keepsNSStatusItemwindows flagged as on-screen even while they sit at the auto-hide resting position above the visible screen — so the guard lets the phantom click through.Issue Number: N/A
What is the new behavior?
isSystemMenuBarVisible()now additionally requires at least one item's reported bounds (Bridging.getWindowBounds) to intersect thisNSScreen's frame. While the menu bar is auto-hidden in fullscreen all items sit above the screen and the function returnsfalse, so show-on-click, show-on-double-click, the secondary context menu, andControlItem.performActionall stay quiet. Once the user hovers to reveal the menu bar the items slide into the screen frame and every existing behaviour resumes unchanged. The same bounds-vs-screen filter already lives inMenuBarItemImageCachefor the exact same macOS quirk, so this stays consistent with established patterns in the codebase.PR Checklist
Other information
Test plan (manual, since the affected paths depend on the live Window Server and aren't covered by
ThawTests):Ctrl+Cmd+F, and do not hover at the top edge.handleShowOnClick: suppressing, no menu bar items on-screen for active space.Notes for reviewers
Thaw/Utilities/Extensions.swift. The function signature is unchanged; only the body is stricter, so existing callers don't need updates.frame.intersects(bounds)check mirrorsMenuBarItemImageCache.swift(the comment there explicitly calls out the macOS bug where hidden items are reported as on-screen), so the coordinate-space approximation is the established pattern in this codebase rather than a new one.Summary by CodeRabbit