-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix dialog losing focus when changing WindowState #3923
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: master
Are you sure you want to change the base?
Conversation
|
It seems like there is a problem with running the tests via github actions. This PR just had another failing test ( I had the same issue in #3915 |
LGTM, thank you for the review! Co-authored-by: Kevin B <[email protected]>
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.
Discussion on how to get the last focused element
|
I also adjusted the test to test against both DialogHost styles. I would say this problem is worth testing for both styles. |
|
So the issue with the test should be resolved if you rebase on the latest main. I believe I fixed that test last week. On the note of focus, I think we want to use the Keyboard Focused element rather than the logically focused element. The problem with the current events, is that Windows will have removed the keyboard focused element by the time those events are raised. After a little testing I think that we can watch one of the WND_PROC messages to know when a minimize is about to occur, and capture the element with keyboard focus then. private void ListenForWindowStateChanged(Window? window)
{
if (window is not null)
{
if (PresentationSource.FromVisual(window) is HwndSource source)
{
HwndSourceHook hook = Hook;
source.AddHook(hook);
//TODO: Need to unregister hooks as well, and ensure we don't double register
}
IntPtr Hook(IntPtr hwnd, int msg, IntPtr wParam, IntPtr lParam, ref bool handled)
{
//https://learn.microsoft.com/en-us/windows/win32/menurc/wm-syscommand
const int WM_SYSCOMMAND = 0x0112;
const int SC_MINIMIZE = 0xf020;
switch (msg)
{
case WM_SYSCOMMAND:
if (wParam.ToInt64() == SC_MINIMIZE && //Minimize
_popupContentControl?.IsKeyboardFocusWithin == true) //Only persistent the one with keyboard focus
{
var element = Keyboard.FocusedElement;
//TODO: Perist keyboard focused element
}
break;
}
return IntPtr.Zero;
}
}
}This code still has the TODOs that would need to be addressed, but this way we can rely on looking at keyboard focus to see which element we care about. |
Replaced the Window.StateChanged event-based approach with an HwndSourceHook-based implementation to handle window state changes (minimize/restore) more effectively. Removed `_previousWindowState` and introduced `_hook` for managing system commands (`WM_SYSCOMMAND`). Added a `Hook` method to handle `SC_MINIMIZE` and `SC_RESTORE` commands, ensuring proper focus management. Refactored focus restoration logic to use asynchronous behavior (`Task.Delay` and `Dispatcher.BeginInvoke`) for improved reliability. Removed redundant state-checking methods and improved code clarity with constants for system commands.
|
I implemented the changes as you suggested. From my manual testing it works fine, however the UI test I added is failing. Edit: I'm starting to think you can't properly debug the tests. If I place a breakpoint in the switch statement inside of the |

fixes #3434
The fix is basically to listen for the
StateChangedevent of the parentWindow.If the window is restored from the minimized state, we focus the previously focused element, which got stored in a field
_lastFocusedDialogElementwhen the window got minimized.I want to point out the artificial delay I added, that we have in other places in the DialogHost class too. I just couldn't get it to work without the (50ms) delay, as described in my comment above the "hacky" code.