Fix dialog losing focus when changing WindowState #3923
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]>
| var windowState = window.WindowState; | ||
| if (windowState == WindowState.Minimized) | ||
| { | ||
| _lastFocusedDialogElement = FocusManager.GetFocusedElement(window); |
There was a problem hiding this comment.
I saw your comment about this line in your past twitch broadcast (I think you forgot to submit it?):
I honestly didn't think of that edge case.
I guess instead of getting the focused element of the Window you'd rather get it from the current dialog instance like so?
_lastFocusedDialogElement = FocusManager.GetFocusedElement(this);FocusManager.GetFocusedElement(this) returns null if the default dialoghost style is used. This is some kind of problem with the PopupEx I assume. I can't think of an easy solution and I'm not sure if this is worth looking into.
I may be biased, but I don't like the current default style for the dialoghost anyway (that could be a me-problem though).
|
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. |
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.