Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

StefanKoell
Copy link
Contributor

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 (with IsLightDismissEnabled = true) this is not a problem because the popup is hidden as soon as another window receives the focus.

If IsLightDismissEnabled is set to false (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:
CleanShot 2024-12-28 at 12 37 07

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:
CleanShot 2024-12-28 at 13 04 22

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 is false, 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.

@maxkatz6
Copy link
Member

Not sure how this can be unit tested.

We have integration tests for Windows and macOS. But it might be challenging to setup.
Test App: https://github.com/AvaloniaUI/Avalonia/tree/master/samples/IntegrationTestApp
Test Methods: https://github.com/AvaloniaUI/Avalonia/tree/master/tests/Avalonia.IntegrationTests.Appium

@maxkatz6
Copy link
Member

This PR fixes #4630 and probably (?) #4227

public void SetChild(Control? control) => Content = control;
public void SetChild(Control? control)
{
PlatformImpl?.SetTopmost(Topmost);
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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? 😅

Copy link
Member

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>();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

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.

Copy link
Contributor Author

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.

@maxkatz6
Copy link
Member

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.

We can go safe and merge it into 11.3.0, but not backport into 11.2.x.

@Firedragonweb
Copy link

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.

Comment on lines +7 to +14
<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" />
Copy link
Member

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.

Copy link
Contributor Author

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 😳

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054093-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@@ -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);
Copy link
Contributor Author

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.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054095-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants