Merge pull request #3492 from MarchingCube/fix-popup-close

Fix ComboBox causing keyboard focus loss when detached

src/Avalonia.Controls/Primitives/Popup.cs

@@ -2,12 +2,10 @@
 // Licensed under the MIT license. See licence.md file in the project root for full license information.
 
 using System;
-using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
 using System.Reactive.Disposables;
 using Avalonia.Controls.Presenters;
-using Avalonia.Data;
 using Avalonia.Input;
 using Avalonia.Input.Raw;
 using Avalonia.Interactivity;
@@ -15,6 +13,8 @@ using Avalonia.LogicalTree;
 using Avalonia.Metadata;
 using Avalonia.VisualTree;
 
+#nullable enable
+
 namespace Avalonia.Controls.Primitives
 {
     /// <summary>
@@ -25,8 +25,8 @@ namespace Avalonia.Controls.Primitives
         /// <summary>
         /// Defines the <see cref="Child"/> property.
         /// </summary>
-        public static readonly StyledProperty<Control> ChildProperty =
-            AvaloniaProperty.Register<Popup, Control>(nameof(Child));
+        public static readonly StyledProperty<Control?> ChildProperty =
+            AvaloniaProperty.Register<Popup, Control?>(nameof(Child));
 
         /// <summary>
         /// Defines the <see cref="IsOpen"/> property.
@@ -43,11 +43,13 @@ namespace Avalonia.Controls.Primitives
         public static readonly StyledProperty<PlacementMode> PlacementModeProperty =
             AvaloniaProperty.Register<Popup, PlacementMode>(nameof(PlacementMode), defaultValue: PlacementMode.Bottom);
 
+#pragma warning disable 618
         /// <summary>
         /// Defines the <see cref="ObeyScreenEdges"/> property.
         /// </summary>
         public static readonly StyledProperty<bool> ObeyScreenEdgesProperty =
             AvaloniaProperty.Register<Popup, bool>(nameof(ObeyScreenEdges), true);
+#pragma warning restore 618
 
         /// <summary>
         /// Defines the <see cref="HorizontalOffset"/> property.
@@ -64,8 +66,8 @@ namespace Avalonia.Controls.Primitives
         /// <summary>
         /// Defines the <see cref="PlacementTarget"/> property.
         /// </summary>
-        public static readonly StyledProperty<Control> PlacementTargetProperty =
-            AvaloniaProperty.Register<Popup, Control>(nameof(PlacementTarget));
+        public static readonly StyledProperty<Control?> PlacementTargetProperty =
+            AvaloniaProperty.Register<Popup, Control?>(nameof(PlacementTarget));
 
         /// <summary>
         /// Defines the <see cref="StaysOpen"/> property.
@@ -80,12 +82,8 @@ namespace Avalonia.Controls.Primitives
             AvaloniaProperty.Register<Popup, bool>(nameof(Topmost));
 
         private bool _isOpen;
-        private IPopupHost _popupHost;
-        private TopLevel _topLevel;
-        private IDisposable _nonClientListener;
-        private IDisposable _presenterSubscription;
-        bool _ignoreIsOpenChanged = false;
-        private List<IDisposable> _bindings = new List<IDisposable>();
+        private bool _ignoreIsOpenChanged;
+        private PopupOpenState? _openState;
 
         /// <summary>
         /// Initializes static members of the <see cref="Popup"/> class.
@@ -94,31 +92,26 @@ namespace Avalonia.Controls.Primitives
         {
             IsHitTestVisibleProperty.OverrideDefaultValue<Popup>(false);
             ChildProperty.Changed.AddClassHandler<Popup>((x, e) => x.ChildChanged(e));
-            IsOpenProperty.Changed.AddClassHandler<Popup>((x, e) => x.IsOpenChanged(e));
-        }
-
-        public Popup()
-        {
-            
+            IsOpenProperty.Changed.AddClassHandler<Popup>((x, e) => x.IsOpenChanged((AvaloniaPropertyChangedEventArgs<bool>)e));
         }
 
         /// <summary>
         /// Raised when the popup closes.
         /// </summary>
-        public event EventHandler Closed;
+        public event EventHandler? Closed;
 
         /// <summary>
         /// Raised when the popup opens.
         /// </summary>
-        public event EventHandler Opened;
+        public event EventHandler? Opened;
 
-        public IPopupHost Host => _popupHost;
+        public IPopupHost? Host => _openState?.PopupHost;
 
         /// <summary>
         /// Gets or sets the control to display in the popup.
         /// </summary>
         [Content]
-        public Control Child
+        public Control? Child
         {
             get { return GetValue(ChildProperty); }
             set { SetValue(ChildProperty, value); }
@@ -131,7 +124,7 @@ namespace Avalonia.Controls.Primitives
         /// This property allows a client to customize the behaviour of the popup by injecting
         /// a specialized dependency resolver into the <see cref="PopupRoot"/>'s constructor.
         /// </remarks>
-        public IAvaloniaDependencyResolver DependencyResolver
+        public IAvaloniaDependencyResolver? DependencyResolver
         {
             get;
             set;
@@ -183,7 +176,7 @@ namespace Avalonia.Controls.Primitives
         /// <summary>
         /// Gets or sets the control that is used to determine the popup's position.
         /// </summary>
-        public Control PlacementTarget
+        public Control? PlacementTarget
         {
             get { return GetValue(PlacementTargetProperty); }
             set { SetValue(PlacementTargetProperty, value); }
@@ -211,7 +204,7 @@ namespace Avalonia.Controls.Primitives
         /// <summary>
         /// Gets the root of the popup window.
         /// </summary>
-        IVisual IVisualTreeHost.Root => _popupHost?.HostedVisualTreeRoot;
+        IVisual? IVisualTreeHost.Root => _openState?.PopupHost.HostedVisualTreeRoot;
 
         /// <summary>
         /// Opens the popup.
@@ -219,50 +212,91 @@ namespace Avalonia.Controls.Primitives
         public void Open()
         {
             // Popup is currently open
-            if (_topLevel != null)
+            if (_openState != null)
+            {
                 return;
-            CloseCurrent();
+            }
+
             var placementTarget = PlacementTarget ?? this.GetLogicalAncestors().OfType<IVisual>().FirstOrDefault();
+
             if (placementTarget == null)
+            {
                 throw new InvalidOperationException("Popup has no logical parent and PlacementTarget is null");
+            }
             
-            _topLevel = placementTarget.GetVisualRoot() as TopLevel;
+            var topLevel = placementTarget.VisualRoot as TopLevel;
 
-            if (_topLevel == null)
+            if (topLevel == null)
             {
                 throw new InvalidOperationException(
                     "Attempted to open a popup not attached to a TopLevel");
             }
 
-            _popupHost = OverlayPopupHost.CreatePopupHost(placementTarget, DependencyResolver);
+            var popupHost = OverlayPopupHost.CreatePopupHost(placementTarget, DependencyResolver);
+
+            var handlerCleanup = new CompositeDisposable(5);
 
-            _bindings.Add(_popupHost.BindConstraints(this, WidthProperty, MinWidthProperty, MaxWidthProperty,
+            void DeferCleanup(IDisposable? disposable)
+            {
+                if (disposable is null)
+                {
+                    return;
+                }
+
+                handlerCleanup.Add(disposable);
+            }
+
+            DeferCleanup(popupHost.BindConstraints(this, WidthProperty, MinWidthProperty, MaxWidthProperty,
                 HeightProperty, MinHeightProperty, MaxHeightProperty, TopmostProperty));
 
-            _popupHost.SetChild(Child);
-            ((ISetLogicalParent)_popupHost).SetParent(this);
-            _popupHost.ConfigurePosition(placementTarget,
-                PlacementMode, new Point(HorizontalOffset, VerticalOffset));
-            _popupHost.TemplateApplied += RootTemplateApplied;
-            
-            var window = _topLevel as Window;
-            if (window != null)
+            popupHost.SetChild(Child);
+            ((ISetLogicalParent)popupHost).SetParent(this);
+
+            popupHost.ConfigurePosition(
+                placementTarget,
+                PlacementMode, 
+                new Point(HorizontalOffset, VerticalOffset));
+
+            DeferCleanup(SubscribeToEventHandler<IPopupHost, EventHandler<TemplateAppliedEventArgs>>(popupHost, RootTemplateApplied,
+                (x, handler) => x.TemplateApplied += handler,
+                (x, handler) => x.TemplateApplied -= handler));
+
+            if (topLevel is Window window)
             {
-                window.Deactivated += WindowDeactivated;
+                DeferCleanup(SubscribeToEventHandler<Window, EventHandler>(window, WindowDeactivated,
+                    (x, handler) => x.Deactivated += handler,
+                    (x, handler) => x.Deactivated -= handler));
             }
             else
             {
-                var parentPopuproot = _topLevel as PopupRoot;
-                if (parentPopuproot?.Parent is Popup popup)
+                var parentPopupRoot = topLevel as PopupRoot;
+
+                if (parentPopupRoot?.Parent is Popup popup)
                 {
-                    popup.Closed += ParentClosed;
+                    DeferCleanup(SubscribeToEventHandler<Popup, EventHandler>(popup, ParentClosed,
+                        (x, handler) => x.Closed += handler,
+                        (x, handler) => x.Closed -= handler));
                 }
             }
-            _topLevel.AddHandler(PointerPressedEvent, PointerPressedOutside, RoutingStrategies.Tunnel);
-            _nonClientListener = InputManager.Instance?.Process.Subscribe(ListenForNonClientClick);
-        
 
-            _popupHost.Show();
+            DeferCleanup(topLevel.AddHandler(PointerPressedEvent, PointerPressedOutside, RoutingStrategies.Tunnel));
+
+            DeferCleanup(InputManager.Instance?.Process.Subscribe(ListenForNonClientClick));
+
+            var cleanupPopup = Disposable.Create((popupHost, handlerCleanup), state =>
+            {
+                state.handlerCleanup.Dispose();
+
+                state.popupHost.SetChild(null);
+                state.popupHost.Hide();
+
+                ((ISetLogicalParent)state.popupHost).SetParent(null);
+                state.popupHost.Dispose();
+            });
+
+            _openState = new PopupOpenState(topLevel, popupHost, cleanupPopup);
+
+            popupHost.Show();
 
             using (BeginIgnoringIsOpen())
             {
@@ -277,14 +311,19 @@ namespace Avalonia.Controls.Primitives
         /// </summary>
         public void Close()
         {
-            if (_popupHost != null)
+            if (_openState is null)
             {
-                _popupHost.TemplateApplied -= RootTemplateApplied;
+                using (BeginIgnoringIsOpen())
+                {
+                    IsOpen = false;
+                }
+
+                return;
             }
 
-            _presenterSubscription?.Dispose();
+            _openState.Dispose();
+            _openState = null;
 
-            CloseCurrent();
             using (BeginIgnoringIsOpen())
             {
                 IsOpen = false;
@@ -293,41 +332,6 @@ namespace Avalonia.Controls.Primitives
             Closed?.Invoke(this, EventArgs.Empty);
         }
 
-        void CloseCurrent()
-        {
-            if (_topLevel != null)
-            {
-                _topLevel.RemoveHandler(PointerPressedEvent, PointerPressedOutside);
-                var window = _topLevel as Window;
-                if (window != null)
-                    window.Deactivated -= WindowDeactivated;
-                else
-                {
-                    var parentPopuproot = _topLevel as PopupRoot;
-                    if (parentPopuproot?.Parent is Popup popup)
-                    {
-                        popup.Closed -= ParentClosed;
-                    }
-                }
-                _nonClientListener?.Dispose();
-                _nonClientListener = null;
-                
-                _topLevel = null;
-            }
-            if (_popupHost != null)
-            {
-                foreach(var b in _bindings)
-                    b.Dispose();
-                _bindings.Clear();
-                _popupHost.SetChild(null);
-                _popupHost.Hide();
-                ((ISetLogicalParent)_popupHost).SetParent(null);
-                _popupHost.Dispose();
-                _popupHost = null;
-            }
-
-        }
-
         /// <summary>
         /// Measures the control.
         /// </summary>
@@ -345,16 +349,22 @@ namespace Avalonia.Controls.Primitives
             Close();
         }
 
+        private static IDisposable SubscribeToEventHandler<T, TEventHandler>(T target, TEventHandler handler, Action<T, TEventHandler> subscribe, Action<T, TEventHandler> unsubscribe)
+        {
+            subscribe(target, handler);
+
+            return Disposable.Create((unsubscribe, target, handler), state => state.unsubscribe(state.target, state.handler));
+        }
 
         /// <summary>
         /// Called when the <see cref="IsOpen"/> property changes.
         /// </summary>
         /// <param name="e">The event args.</param>
-        private void IsOpenChanged(AvaloniaPropertyChangedEventArgs e)
+        private void IsOpenChanged(AvaloniaPropertyChangedEventArgs<bool> e)
         {
             if (!_ignoreIsOpenChanged)
             {
-                if ((bool)e.NewValue)
+                if (e.NewValue.Value)
                 {
                     Open();
                 }
@@ -373,7 +383,7 @@ namespace Avalonia.Controls.Primitives
         {
             LogicalChildren.Clear();
 
-            ((ISetLogicalParent)e.OldValue)?.SetParent(null);
+            ((ISetLogicalParent?)e.OldValue)?.SetParent(null);
 
             if (e.NewValue != null)
             {
@@ -394,34 +404,37 @@ namespace Avalonia.Controls.Primitives
 
         private void PointerPressedOutside(object sender, PointerPressedEventArgs e)
         {
-            if (!StaysOpen)
+            if (!StaysOpen && !IsChildOrThis((IVisual)e.Source))
             {
-                if (!IsChildOrThis((IVisual)e.Source))
-                {
-                    Close();
-                    e.Handled = true;
-                }
+                Close();
+                e.Handled = true;
             }
         }
 
         private void RootTemplateApplied(object sender, TemplateAppliedEventArgs e)
         {
-            _popupHost.TemplateApplied -= RootTemplateApplied;
-
-            if (_presenterSubscription != null)
+            if (_openState is null)
             {
-                _presenterSubscription.Dispose();
-                _presenterSubscription = null;
+                return;
             }
 
+            var popupHost = _openState.PopupHost;
+
+            popupHost.TemplateApplied -= RootTemplateApplied;
+
+            _openState.SetPresenterSubscription(null);
+
             // If the Popup appears in a control template, then the child controls
             // that appear in the popup host need to have their TemplatedParent
             // properties set.
-            if (TemplatedParent != null)
+            if (TemplatedParent != null && popupHost.Presenter != null)
             {
-                _popupHost.Presenter?.ApplyTemplate();
-                _popupHost.Presenter?.GetObservable(ContentPresenter.ChildProperty)
+                popupHost.Presenter.ApplyTemplate();
+
+                var presenterSubscription = popupHost.Presenter.GetObservable(ContentPresenter.ChildProperty)
                     .Subscribe(SetTemplatedParentAndApplyChildTemplates);
+
+                _openState.SetPresenterSubscription(presenterSubscription);
             }
         }
 
@@ -440,7 +453,7 @@ namespace Avalonia.Controls.Primitives
 
                 if (!(control is IPresenter) && control.TemplatedParent == templatedParent)
                 {
-                    foreach (IControl child in control.GetVisualChildren())
+                    foreach (IControl child in control.VisualChildren)
                     {
                         SetTemplatedParentAndApplyChildTemplates(child);
                     }
@@ -450,22 +463,41 @@ namespace Avalonia.Controls.Primitives
 
         private bool IsChildOrThis(IVisual child)
         {
-            IVisual root = child.GetVisualRoot();
-            while (root is IHostedVisualTreeRoot hostedRoot )
+            if (_openState is null)
             {
-                if (root == this._popupHost)
+                return false;
+            }
+
+            var popupHost = _openState.PopupHost;
+
+            IVisual? root = child.VisualRoot;
+            
+            while (root is IHostedVisualTreeRoot hostedRoot)
+            {
+                if (root == popupHost)
+                {
                     return true;
-                root = hostedRoot.Host?.GetVisualRoot();
+                }
+
+                root = hostedRoot.Host?.VisualRoot;
             }
+
             return false;
         }
         
         public bool IsInsidePopup(IVisual visual)
         {
-            return _popupHost != null && ((IVisual)_popupHost)?.IsVisualAncestorOf(visual) == true;
+            if (_openState is null)
+            {
+                return false;
+            }
+
+            var popupHost = _openState.PopupHost;
+
+            return popupHost != null && ((IVisual)popupHost).IsVisualAncestorOf(visual);
         }
 
-        public bool IsPointerOverPopup => ((IInputElement)_popupHost).IsPointerOver;
+        public bool IsPointerOverPopup => ((IInputElement?)_openState?.PopupHost)?.IsPointerOver ?? false;
 
         private void WindowDeactivated(object sender, EventArgs e)
         {
@@ -503,5 +535,36 @@ namespace Avalonia.Controls.Primitives
                 _owner._ignoreIsOpenChanged = false;
             }
         }
+
+        private class PopupOpenState : IDisposable
+        {
+            private readonly IDisposable _cleanup;
+            private IDisposable? _presenterCleanup;
+
+            public PopupOpenState(TopLevel topLevel, IPopupHost popupHost, IDisposable cleanup)
+            {
+                TopLevel = topLevel;
+                PopupHost = popupHost;
+                _cleanup = cleanup;
+            }
+
+            public TopLevel TopLevel { get; }
+
+            public IPopupHost PopupHost { get; }
+
+            public void SetPresenterSubscription(IDisposable? presenterCleanup)
+            {
+                _presenterCleanup?.Dispose();
+
+                _presenterCleanup = presenterCleanup;
+            }
+
+            public void Dispose()
+            {
+                _presenterCleanup?.Dispose();
+
+                _cleanup.Dispose();
+            }
+        }
     }
 }

tests/Avalonia.Controls.UnitTests/ComboBoxTests.cs

@@ -108,5 +108,36 @@ namespace Avalonia.Controls.UnitTests
                 };
             });
         }
+
+        [Fact]
+        public void Detaching_Closed_ComboBox_Keeps_Current_Focus()
+        {
+            using (UnitTestApplication.Start(TestServices.RealFocus))
+            {
+                var target = new ComboBox
+                {
+                    Items = new[] { new Canvas() },
+                    SelectedIndex = 0,
+                    Template = GetTemplate(),
+                };
+
+                var other = new Control { Focusable = true };
+
+                StackPanel panel;
+
+                var root = new TestRoot { Child = panel = new StackPanel { Children = { target, other } } };
+
+                target.ApplyTemplate();
+                target.Presenter.ApplyTemplate();
+
+                other.Focus();
+
+                Assert.True(other.IsFocused);
+
+                panel.Children.Remove(target);
+
+                Assert.True(other.IsFocused);
+            }
+        }
     }
 }

tests/Avalonia.Controls.UnitTests/Primitives/PopupTests.cs

@@ -223,7 +223,33 @@ namespace Avalonia.Controls.UnitTests.Primitives
             }
         }
 
-        
+        [Fact]
+        public void Popup_Close_On_Closed_Popup_Should_Not_Raise_Closed_Event()
+        {
+            using (CreateServices())
+            {
+                var window = PreparedWindow();
+                var target = new Popup() { PlacementMode = PlacementMode.Pointer };
+
+                window.Content = target;
+                window.ApplyTemplate();
+                
+                int closedCount = 0;
+
+                target.Closed += (sender, args) =>
+                {
+                    closedCount++;
+                };
+
+                target.Close();
+                target.Close();
+                target.Close();
+                target.Close();
+
+                Assert.Equal(0, closedCount);
+            }
+        }
+
         [Fact]
         public void Templated_Control_With_Popup_In_Template_Should_Set_TemplatedParent()
         {