Repository navigation
fix(macos): prevent exiting Show Desktop mode during Option+Drag - #1155
KnifeLemon merged 5 commits into
Conversation
d62daf9 to
0611844
Compare
KnifeLemon
left a comment
There was a problem hiding this comment.
Thanks, @AzrielTheHellrazor. This is a real problem and your cause analysis is clear. Leaving Show Desktop because of the donut is annoying, and the fix for it does need to avoid the whole-app activation. It can't go in as it is, though, mainly because TakeKeyboard exists for something this changes. We don't have a Mac, so all of this comes from reading the code: please tell me if your machine shows otherwise.
Must fix
1. The keyboard never goes back
ReturnFocus (MacOSPlatformServices.cs:159) only acts when Filee is frontmost, and now it isn't, so it does nothing. After a drop (RadialController.cs:451) the ring can stay up for up to 5 s as the key window, so typing goes to the donut instead of the previous app. Please release key status explicitly, and test it.
2. Secure Input hasn't been measured
#855 measured keys reaching the app in front, which is why TakeKeyboard activates at all. A key non-activating panel may still get them, but nobody has tested that. Avalonia raises Activated from becomeKeyWindow, so IsActive is true and HookFlipsKey (:742) ignores the hook. If keys miss the panel, Tab stops flipping on every macOS drag, not just under Secure Input.
Could you test Terminal with Secure Keyboard Entry on, and Tab during a normal drag, and report both? The comment at :751, docs/ARCHITECTURE.md:146-152 and DonutMenu.Flip.cs:9-11 also describe the old behaviour.
3. The swizzle isn't needed (NativeMethods.cs:515-575)
Avalonia 12.1.4's Activate() is makeKeyAndOrderFront: plus activateIgnoringOtherApps:. So a small platform method that sends makeKeyAndOrderFront: to the window handle gets the same result. As written, it patches a system class for the whole process, and if Avalonia moves to [NSApp activate] the fix stops working while the tests stay green.
4. Please remove setBecomesKeyOnlyIfNeeded: (:174-176)
The non-activating mask already prevents activation, and this window needs the keyboard. AvnView doesn't override needsPanelToBecomeKey, so clicks would stop making the donut key, in click mode too.
Smaller
SuppressAppActivation_toggles_safely(MacOSWindowTests.cs:150) asserts nothing, so it only proves there's no crash.RadialControllerTests.cs:78checks the mock's call order only, and:80returns where it should useAssert.Skip.
What's fine
stationary only reaches the donut, since RaiseTopmost has no other app caller. The tool, order and save windows still activate themselves, so KeepKeyboard holds, and other platforms are unchanged by the no-op default. I've approved CI for this PR.
0611844 to
7292c33
Compare
|
Hi @KnifeLemon, Thank you so much for the thoughtful, detailed code review! Your points on the keyboard lifecycle and keeping the native interaction clean were spot on. I actually just downloaded Filee today, and I'm really grateful you built it. I've been needing a tool like this for a long time and had even been thinking about building something similar myself. Since you mentioned not having a Mac, I'll gladly keep testing on macOS and send over PRs whenever I spot any other quirks! Here are the updates and measurements based on your feedback:
Also, if you'd like to visually see how Show Desktop + Option+Drag + Tab flip looks in action on macOS, I'd be happy to record a short screen video and upload it to YouTube or share it here. Let me know if there's anything else you'd like adjusted! |
Option+dragging files onto the radial menu prematurely deactivated macOS "Show Desktop" (Desktop Reveal) mode and forced background application windows back onto the screen. Root cause: RadialController.TakeKeyboard() called window.Activate(), which in Avalonia's macOS backend invokes [NSApp activateIgnoringOtherApps:YES]. Activating the entire application causes WindowServer to exit Desktop Reveal and drop pushed-aside application windows back onto the screen. Additionally, collectionBehavior in RaiseTopmost lacked NSWindowCollectionBehaviorStationary (1 << 4). Resolution: 1. Replaced whole-app activation with targeted IPlatformServices.TakeKeyWindow(handle): Sends makeKeyAndOrderFront: directly to the floating AvnPanel handle, making it key so it receives keyboard events (e.g. Tab flip key) without calling activateIgnoringOtherApps: or touching NSApp. 2. Explicitly release key window status on drop or close via IPlatformServices.ReleaseKeyWindow(handle) and ReturnFocus(): Sends resignKeyWindow to the floating panel so typing immediately returns to the previously frontmost application while the progress ring completes. 3. Added NSWindowCollectionBehaviorStationary (1 << 4) to collectionBehavior in RaiseTopmost so Exposé and Desktop Reveal leave the panel undisturbed. 4. Avoided swizzling and removed setBecomesKeyOnlyIfNeeded: to preserve standard panel keyboard event delivery across both click and drag modes. 5. Added unit tests for TakeKeyWindow and ReleaseKeyWindow lifecycle in MacOSWindowTests and RadialControllerTests, and updated architecture documentation.
7292c33 to
ebadfc7
Compare
|
@AzrielTheHellrazor Thanks, this is a big step, and real measurements on a Mac are what we were missing. Yes please to the video, and we'd gladly take your help testing on macOS. One thing left: Could you review this on your Mac? Drop files, then type while the ring is still showing (TextEdit in front, for example), and tell us where the letters go. If they don't reach TextEdit, ending key status needs another way: ordering the panel out, or giving focus back to the previous app. |
…t app - Replace direct resignKeyWindow invocation with orderOut: followed by orderFrontRegardless (when visible), adhering to AppKit guidance and ensuring WindowServer drops key window status while keeping the ring visible. - In ReturnFocus, activate the previous frontmost process to route typing back to it once the drop finishes. - Update MacOSWindowTests to verify orderOut: and orderFrontRegardless sequencing and assert resignKeyWindow is not invoked directly.
|
@KnifeLemon Here is the screen recording showing desktop reveal, Option+Drag, and keystroke routing. Regarding
Important: The red portable jobs fail in MergeImagesTests(temp path expectation), which also fails on current main, so it is unrelated to this PR. Here is the video: https://youtu.be/q6Ap0oDIgws |
KnifeLemon
left a comment
There was a problem hiding this comment.
Thanks, @AzrielTheHellrazor, and thanks for the video. orderOut: is a much better way to end key status than resignKeyWindow. I went through it against Avalonia's window code: level, collection behaviour and panel class all stay, and Deactivated fires as HookFlipsKey needs. It's also safe if the window closes meanwhile, and other platforms are unchanged. We still can't run any of this ourselves, so tell me if your Mac shows otherwise.
Must fix
ReturnFocus lost its "still in front" check (MacOSPlatformServices.cs:172)
It used to activate the previous app only when Filee was in front. Now it always does, and that causes two things:
- It takes the user away from where they went. Option+Drag from Finder until the files are on the donut, Cmd+Tab to another app, and drop there. The donut closes 250 ms after the release (
RadialController.cs:194-202), andActivateProcessasks for Finder to come forward.IPlatformServices.cs:173-174still promises not to do this. - It puts the fix itself at risk. The donut no longer activates Filee, so in the normal path the previous app is still frontmost, and every drop and close now activates an app that's already active. Apple doesn't document that as harmless, and an activation is what this PR set out to avoid.
Could you put the check back (from != 0 && FrontmostProcessId() == Environment.ProcessId) and try again on your Mac? orderOut: may already be enough, since the previous app never stopped being active. If typing doesn't reach TextEdit without the activation, activate only when Filee or from is in front, and leave it alone when a third app is.
Please report these three cases:
- Show Desktop: drop on a slice, with the windows staying aside. Please also try it with TextEdit focused before you enter Show Desktop.
- Typing during the ring: with TextEdit in front, once with Filee's main window open behind it and once without.
- Cmd+Tab: the case above.
Smaller
MacOSWindowTests.cs:161: the fake clearsisKeyby itself, so this test shows the calls and their order, not that AppKit moves the keyboard. That's fine for what it guards, as your manual test is the real evidence. Nothing coversActivateProcess, though, because_focusFromstays 0 there.RadialControllerTests.cs:165and:228:if (index >= 0)passes silently if no such slice exists. Please assert instead.- PR description: point 2 still says
resignKeyWindow. ARCHITECTURE.md: if the check comes back, "unless the user clicked elsewhere meanwhile" should come back too.
CI
The red jobs aren't yours: it's only MergeImagesTests, which is fixed on main now. Please merge main in. The job stops at the first failing project, so the Platform and App tests never ran on macOS or Linux on this commit, and we need that run before merging.
…p-drag-activation
…xistence in tests
|
Hi @KnifeLemon, Thank you for the thorough follow-up review! I have addressed all the feedback and pushed the updates: 1.
|
KnifeLemon
left a comment
There was a problem hiding this comment.
Thanks @AzrielTheHellrazor, the focus handling looks right now and your Mac runs cover what we can't test here. All checks are green, so this is going in.
Summary
Fixes the issue on macOS where initiating Filee's radial menu via Option+Drag while macOS is in "Show Desktop" (Desktop Reveal) mode caused pushed-aside application windows to drop back down onto the screen.
Root Cause
When files are dragged onto the radial menu,
RadialController.TakeKeyboard()previously called Avalonia'swindow.Activate().In Avalonia's macOS backend (
WindowBaseImpl),Activate()sendsmakeKeyAndOrderFront:and unconditionally calls[NSApp activateIgnoringOtherApps:YES].Activating the entire application commands WindowServer to exit Desktop Reveal and Exposé modes, immediately restoring background windows.
Additionally, the window's
collectionBehaviorlackedNSWindowCollectionBehaviorStationary(1 << 4), which tells Mission Control and Exposé to keep the window in place without disturbing the current space reveal state.Resolution
TakeKeyWindow): Replaced whole-appwindow.Activate()with a clean platform service callIPlatformServices.TakeKeyWindow(nint handle)that sendsmakeKeyAndOrderFront:directly to the nativeAvnPanelwindow handle without touchingNSApp.ReleaseKeyWindow): AddedIPlatformServices.ReleaseKeyWindow(nint handle)which callsorderOut:(followed immediately byorderFrontRegardlesswhen visible) upon file drop or donut close, ensuring typing focus immediately returns to the previous frontmost application while the conversion progress ring finishes.NSWindowCollectionBehaviorStationary(1 << 4) tocollectionBehaviorinRaiseTopmostso Exposé and Desktop Reveal modes remain uninterrupted.setBecomesKeyOnlyIfNeeded: 1so that click and drag interactions continue to deliver keyboard events without relying on custom view hooks.