-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix(systray): highlight status item and ensure popup appears above other windows on macOS #4913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v3-alpha
Are you sure you want to change the base?
Changes from all commits
3127826
e2788e1
3481956
0d8d462
cb38f07
6380b7c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,12 +93,48 @@ func systrayClickCallback(id C.long, buttonID C.int) { | |
| systemTray.processClick(button(buttonID)) | ||
| } | ||
|
|
||
| // systrayPreClickCallback is called from the NSEvent local monitor BEFORE the | ||
| // button processes the mouse-down. It returns 1 when the framework should | ||
| // show the menu via native tracking (proper highlight, no app activation), | ||
| // or 0 to let the action handler fire for custom click/window behaviour. | ||
| // | ||
| //export systrayPreClickCallback | ||
| func systrayPreClickCallback(id C.long, buttonID C.int) C.int { | ||
| systemTray := systemTrayMap[uint(id)] | ||
| if systemTray == nil || systemTray.nsMenu == nil { | ||
| return 0 | ||
| } | ||
| b := button(buttonID) | ||
| switch b { | ||
| case leftButtonDown: | ||
| if systemTray.parent.clickHandler == nil && | ||
| systemTray.parent.attachedWindow.Window == nil { | ||
| return 1 | ||
| } | ||
| case rightButtonDown: | ||
| if systemTray.parent.rightClickHandler == nil { | ||
| // Hide the attached window before the menu appears. | ||
| if systemTray.parent.attachedWindow.Window != nil && | ||
| systemTray.parent.attachedWindow.Window.IsVisible() { | ||
| systemTray.parent.attachedWindow.Window.Hide() | ||
| } | ||
| return 1 | ||
| } | ||
| } | ||
| return 0 | ||
| } | ||
|
|
||
| func (s *macosSystemTray) setIconPosition(position IconPosition) { | ||
| s.iconPosition = position | ||
| } | ||
|
|
||
| func (s *macosSystemTray) setMenu(menu *Menu) { | ||
| s.menu = menu | ||
| if s.nsStatusItem != nil && menu != nil { | ||
| menu.Update() | ||
| s.nsMenu = (menu.impl).(*macosMenu).nsMenu | ||
| C.systemTraySetCachedMenu(s.nsStatusItem, s.nsMenu) | ||
| } | ||
| } | ||
|
Comment on lines
131
to
138
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The same concern applies to the existing Suggested fix: dispatch the cache update to the main threadIn void systemTraySetCachedMenu(void* nsStatusItem, void *nsMenu) {
- NSStatusItem *statusItem = (NSStatusItem *)nsStatusItem;
- StatusItemController *controller = (StatusItemController *)[statusItem target];
- controller.cachedMenu = (NSMenu *)nsMenu;
+ dispatch_async(dispatch_get_main_queue(), ^{
+ NSStatusItem *statusItem = (NSStatusItem *)nsStatusItem;
+ StatusItemController *controller = (StatusItemController *)[statusItem target];
+ controller.cachedMenu = (NSMenu *)nsMenu;
+ });
}🤖 Prompt for AI Agents |
||
|
|
||
| func (s *macosSystemTray) positionWindow(window Window, offset int) error { | ||
|
|
@@ -167,6 +203,8 @@ func (s *macosSystemTray) run() { | |
| s.menu.Update() | ||
| // Convert impl to macosMenu object | ||
| s.nsMenu = (s.menu.impl).(*macosMenu).nsMenu | ||
| // Cache on the ObjC controller for the event monitor. | ||
| C.systemTraySetCachedMenu(s.nsStatusItem, s.nsMenu) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |||||||||||||||
| #include "systemtray_darwin.h" | ||||||||||||||||
|
|
||||||||||||||||
| extern void systrayClickCallback(long, int); | ||||||||||||||||
| extern int systrayPreClickCallback(long, int); | ||||||||||||||||
|
|
||||||||||||||||
| // StatusItemController.m | ||||||||||||||||
| @implementation StatusItemController | ||||||||||||||||
|
|
@@ -14,17 +15,43 @@ - (void)statusItemClicked:(id)sender { | |||||||||||||||
| systrayClickCallback(self.id, event.type); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| - (void)menuDidClose:(NSMenu *)menu { | ||||||||||||||||
| // Remove the menu from the status item so future clicks invoke the | ||||||||||||||||
| // action handler instead of re-showing the menu. | ||||||||||||||||
| self.statusItem.menu = nil; | ||||||||||||||||
| menu.delegate = nil; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| @end | ||||||||||||||||
|
|
||||||||||||||||
| // Create a new system tray | ||||||||||||||||
| void* systemTrayNew(long id) { | ||||||||||||||||
| StatusItemController *controller = [[StatusItemController alloc] init]; | ||||||||||||||||
| controller.id = id; | ||||||||||||||||
| NSStatusItem *statusItem = [[[NSStatusBar systemStatusBar] statusItemWithLength:NSVariableStatusItemLength] retain]; | ||||||||||||||||
| controller.statusItem = statusItem; | ||||||||||||||||
| [statusItem setTarget:controller]; | ||||||||||||||||
| [statusItem setAction:@selector(statusItemClicked:)]; | ||||||||||||||||
| NSButton *button = statusItem.button; | ||||||||||||||||
| [button sendActionOn:(NSEventMaskLeftMouseDown|NSEventMaskRightMouseDown)]; | ||||||||||||||||
|
|
||||||||||||||||
| // Install a local event monitor that fires BEFORE the button processes | ||||||||||||||||
| // the mouse-down. When the pre-click callback says "show menu", we | ||||||||||||||||
| // temporarily set statusItem.menu so the button enters native menu | ||||||||||||||||
| // tracking — this gives proper highlight and does not activate the app. | ||||||||||||||||
| controller.eventMonitor = [NSEvent addLocalMonitorForEventsMatchingMask: | ||||||||||||||||
| (NSEventMaskLeftMouseDown|NSEventMaskRightMouseDown) | ||||||||||||||||
| handler:^NSEvent *(NSEvent *event) { | ||||||||||||||||
| if (event.window != button.window) return event; | ||||||||||||||||
|
|
||||||||||||||||
| int action = systrayPreClickCallback((long)controller.id, (int)event.type); | ||||||||||||||||
| if (action == 1 && controller.cachedMenu != nil) { | ||||||||||||||||
| controller.cachedMenu.delegate = controller; | ||||||||||||||||
| statusItem.menu = controller.cachedMenu; | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+42
to
+51
|
||||||||||||||||
| return event; | ||||||||||||||||
| }]; | ||||||||||||||||
|
|
||||||||||||||||
| return (void*)statusItem; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -135,31 +162,56 @@ void systemTrayDestroy(void* nsStatusItem) { | |||||||||||||||
| // Remove the status item from the status bar and its associated menu | ||||||||||||||||
| dispatch_async(dispatch_get_main_queue(), ^{ | ||||||||||||||||
| NSStatusItem *statusItem = (NSStatusItem *)nsStatusItem; | ||||||||||||||||
| StatusItemController *controller = (StatusItemController *)[statusItem target]; | ||||||||||||||||
| if (controller.eventMonitor) { | ||||||||||||||||
| [NSEvent removeMonitor:controller.eventMonitor]; | ||||||||||||||||
| controller.eventMonitor = nil; | ||||||||||||||||
| } | ||||||||||||||||
| [[NSStatusBar systemStatusBar] removeStatusItem:statusItem]; | ||||||||||||||||
| [controller release]; | ||||||||||||||||
| [statusItem release]; | ||||||||||||||||
| }); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // showMenu is used for programmatic OpenMenu() calls. Click-triggered | ||||||||||||||||
| // menus are handled by the event monitor installed in systemTrayNew. | ||||||||||||||||
|
Comment on lines
+176
to
+177
|
||||||||||||||||
| // showMenu is used for programmatic OpenMenu() calls. Click-triggered | |
| // menus are handled by the event monitor installed in systemTrayNew. | |
| // showMenu is used for programmatic OpenMenu() calls and for menus opened | |
| // from Go click handlers (e.g., defaultClickHandler -> OpenMenu()). Some | |
| // click-triggered menus may also be initiated by the event monitor installed | |
| // in systemTrayNew, but they still ultimately flow through this function. |
Copilot
AI
Feb 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
systemTrayPositionWindow() now calls orderFrontRegardless, which has visible side effects (it can show/raise the window) even when the caller only intends to reposition it. Since SystemTray.PositionWindow() is a public API, this changes behavior beyond the attached-window use case. Consider removing orderFrontRegardless from positioning, or at least gating it on [(NSWindow*)nsWindow isVisible] / moving the bring-to-front behavior to the code path that actually shows the attached window.
| // Bring window to front | |
| [(NSWindow*)nsWindow orderFrontRegardless]; | |
| // Bring window to front only if it is already visible, to avoid changing | |
| // behavior of callers that only intend to reposition the window. | |
| if ([(NSWindow*)nsWindow isVisible]) { | |
| [(NSWindow*)nsWindow orderFrontRegardless]; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NSPopUpMenuWindowLevel likely causes the greyed-out controls reported by the reviewer.
NSPopUpMenuWindowLevel (101) combined with orderFrontRegardless (no app activation) puts the window above everything but leaves the previously focused app in an "inactive" state — hence greyed-out close/minimize/maximize buttons in apps like Chrome.
Native macOS status-item popups (Wi-Fi, Bluetooth, etc.) typically use an NSPanel with NSWindowStyleMaskNonactivatingPanel so they can overlay other windows without deactivating them. Consider:
- Using
NSStatusWindowLevel(25) instead — still above normal windows but less aggressive. - Or configuring the attached window as a non-activating panel, which is the standard macOS pattern for status-item popups.
Additionally, this level is set every time systemTrayPositionWindow is called but never reset, so the window permanently stays at popup-menu level even if it's later repositioned or used differently.
🤖 Prompt for AI Agents
In `@v3/pkg/application/systemtray_darwin.m` around lines 296 - 301, The code in
systemTrayPositionWindow sets the window level to NSPopUpMenuWindowLevel and
calls orderFrontRegardless, which can leave other apps inactive and produce
greyed-out controls; change this to use NSStatusWindowLevel or convert the
window to a non-activating panel (NSPanel with
NSWindowStyleMaskNonactivatingPanel) so the popup overlays without deactivating
the app, and replace orderFrontRegardless with a fronting call appropriate for
non-activating panels; also ensure the level change is applied only when showing
the tray popup (and restore or avoid persisting NSPopUpMenuWindowLevel) so the
window does not remain at popup level after repositioning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
systemTraySetCachedMenuis only called whenmenu != nil, so callingSetMenu(nil)leavess.nsMenuand the ObjCcachedMenupointing at the previous menu. BecausesystrayPreClickCallbackgates onsystemTray.nsMenu != nil, right-click can still show the stale menu after it has been cleared. Whenmenu == nil, explicitly sets.nsMenu = niland clear the cached menu on the ObjC controller as well.