-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fix TopMost handling for Popups on Windows #17841
base: master
Are you sure you want to change the base?
Fix TopMost handling for Popups on Windows #17841
Conversation
We have integration tests for Windows and macOS. But it might be challenging to setup. |
public void SetChild(Control? control) => Content = control; | ||
public void SetChild(Control? control) | ||
{ | ||
PlatformImpl?.SetTopmost(Topmost); |
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.
Feels a bit wrong to do anything unrelated to SetChild
in this method. Also, wouldn't it break if Topmost changed in runtime?
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.
Yeah, I wasn't sure about that one. The TopMost property is set in the line above in Popup.cs, so I thought setting the TopMost here would be least intrusive. Changing and affecting the property at runtime would need to handle it in OnPropertyChanged
of the PopupRoot? Would that approach be better?
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.
Check the last commit: Seems to work and doesn't feel "wrong" anymore? 😅
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.
Popup already has OnPropertyChanged for Topmost:
_openState.PopupHost.Topmost = change.GetNewValue<bool>(); |
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.
As for PopupRoot, probably mirror behavior of WindowShadowHint property?
https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Controls/Primitives/PopupRoot.cs#L63
https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Controls/Primitives/PopupRoot.cs#L224-L227
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.
@StefanKoell just noticed newer commit. Yes, that's much better.
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.
Got it! Should now work pretty much the same as with WindowShadowHint.
We can go safe and merge it into 11.3.0, but not backport into 11.2.x. |
This fixes a problem we discovered recently (did not raise an issue here yet, since we were unsure if we were using it wrongly). I would be really happy if this could find its way in the 11.2.X versions, though. |
<TextBlock Text="'Light Dismiss' Popup" /> | ||
<Button Content="Show Popup" Click="ButtonLightDismiss_OnClick" /> | ||
|
||
<TextBlock Text="Popup that stays open" /> | ||
<Button Content="Show Popup" Click="ButtonPopupStaysOpen_OnClick" /> | ||
|
||
<TextBlock Text="TopMost popup that stays open" /> | ||
<Button Content="Show Popup" Click="ButtonTopMostPopupStaysOpen" /> |
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.
These testing buttons can be moved to the IntegrationTestApp. I can try to add integration test on top of them later.
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.
I could use some guidance on this one. Not sure what I should do 😳
You can test this PR using the following package version. |
… activate the app.
@@ -84,6 +83,10 @@ protected override IntPtr WndProc(IntPtr hWnd, uint msg, IntPtr wParam, IntPtr l | |||
_maxAutoSize = null; | |||
goto default; | |||
case UnmanagedMethods.WindowsMessage.WM_MOUSEACTIVATE: | |||
if (_parent?.Handle is not null) | |||
{ | |||
UnmanagedMethods.SetFocus(_parent.Handle.Handle); |
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.
@maxkatz6 found an issue with focus management. I'm not sure if this is the correct way to handle it but if I don't set the focus to the parent when the popup is clicked, I'm not able to type in the popup if there's a textbox, for example.
You can test this PR using the following package version. |
What does the pull request do?
This PR fixes how the Popup handles the TopMost property on Windows.
What is the current behavior?
All popups on Windows are created with the
WS_EX_TOPMOST
style. In most scenarios (withIsLightDismissEnabled = true
) this is not a problem because the popup is hidden as soon as another window receives the focus.If
IsLightDismissEnabled
is set tofalse
(the popup needs to be closed using a user interaction on the popup), the behavior is not correct. Other windows will not cover that popup:What is the updated/expected behavior with this PR?
The Popup class has actually a property
TopMost
which should be used to control the behavior. This PR fixes the Popup implementation on Windows to honor the value of the property:I also created a Popups page in the Control Catalog which can be used to test the change in this PR.
How was the solution implemented (if it's not obvious)?
I think the (small) changes needed to fix the behavior are self-explanatory. If not, I'm happy to elaborate.
Checklist
Not sure how this can be unit tested. Also no public APIs have been altered.
Breaking changes
The previous behavior didn't honor the
TopMost
property at all - now it does. Since the property default value isfalse
, this PR changes the default behavior and make all Popups NOT top most anymore. I can't really tell if this breaking change has an impact on built-in popups. The usual flyout/popups/tooltips I tested are working fine as far as I can tell.