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
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions samples/ControlCatalog/MainView.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@
<TabItem Header="Pointers">
<pages:PointersPage />
</TabItem>
<TabItem Header="Popups">
<pages:PopupsPage />
</TabItem>
<TabItem Header="ProgressBar">
<pages:ProgressBarPage />
</TabItem>
Expand Down
18 changes: 18 additions & 0 deletions samples/ControlCatalog/Pages/PopupsPage.xaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<UserControl x:Class="ControlCatalog.Pages.PopupsPage"
xmlns="https://github.com/avaloniaui"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml">

<StackPanel Orientation="Vertical" Spacing="10">

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


</StackPanel>

</UserControl>
88 changes: 88 additions & 0 deletions samples/ControlCatalog/Pages/PopupsPage.xaml.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
using Avalonia;
using Avalonia.Controls;
using Avalonia.Controls.Primitives;
using Avalonia.Interactivity;
using Avalonia.Markup.Xaml;

namespace ControlCatalog.Pages;

public class PopupsPage : UserControl
{
private Popup? _popup;
private Popup? _topMostPopup;
public PopupsPage()
{
InitializeComponent();

}

private void InitializeComponent()
{
AvaloniaXamlLoader.Load(this);
}

private void ButtonLightDismiss_OnClick(object sender, RoutedEventArgs e)
{
new Popup
{
Placement = PlacementMode.Bottom,
PlacementTarget = sender as Button,
Child = new Panel { Margin = new Thickness(10) ,Children = { new TextBlock { Text = "Popup Content" } }},
IsLightDismissEnabled = true,
IsOpen = true
};
}

private void ButtonPopupStaysOpen_OnClick(object sender, RoutedEventArgs e)
{
if (_popup is not null)
return;

var closeButton = new Button { Content = "Click to Close" };
closeButton.Click += (o, args) =>
{
if (_popup is null)
return;
_popup.IsOpen = false;
_popup = null;
};
_popup = new Popup
{
Placement = PlacementMode.Bottom,
PlacementTarget = sender as Button,
Child = new Panel
{
Margin = new Thickness(10), Children = { closeButton }
},
IsLightDismissEnabled = false,
IsOpen = true
};
}

private void ButtonTopMostPopupStaysOpen(object sender, RoutedEventArgs e)
{
if (_topMostPopup is not null)
return;

var closeButton = new Button { Content = "Click to Close" };
closeButton.Click += (o, args) =>
{
if (_topMostPopup is null)
return;
_topMostPopup.IsOpen = false;
_topMostPopup = null;
};
_topMostPopup = new Popup
{
Placement = PlacementMode.Bottom,
PlacementTarget = sender as Button,
Child = new Panel
{
Margin = new Thickness(10), Children = { closeButton }
},
IsLightDismissEnabled = false,
Topmost = true,
IsOpen = true
};
}
}
6 changes: 5 additions & 1 deletion src/Avalonia.Controls/Primitives/PopupRoot.cs
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,11 @@ void IPopupHost.ConfigurePosition(PopupPositionRequest request)
UpdatePosition();
}

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.

Content = control;
}

public void TakeFocus() => PlatformImpl?.TakeFocus();

Expand Down
3 changes: 1 addition & 2 deletions src/Windows/Avalonia.Win32/PopupImpl.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,7 @@ protected override IntPtr CreateWindowOverride(ushort atom)
UnmanagedMethods.WindowStyles.WS_CLIPCHILDREN;

UnmanagedMethods.WindowStyles exStyle =
UnmanagedMethods.WindowStyles.WS_EX_TOOLWINDOW |
UnmanagedMethods.WindowStyles.WS_EX_TOPMOST;
UnmanagedMethods.WindowStyles.WS_EX_TOOLWINDOW;

var result = UnmanagedMethods.CreateWindowEx(
(int)exStyle,
Expand Down
Loading