From b5fd40e5046805c1fa072ae702a2a5ef87c3ee64 Mon Sep 17 00:00:00 2001 From: Julien Lebosquain Date: Thu, 24 Oct 2024 09:31:52 +0200 Subject: [PATCH] Fix overlay popup focus issues (#17326) * Add failing focus tests for flyouts inside overlay popups * Implement IKeyboardNavigationHandler on OverlayPopupHost * Layout OverlayPopupHost content for focus to work --- src/Avalonia.Base/Input/IInputRoot.cs | 2 +- .../Primitives/OverlayPopupHost.cs | 51 +++++++++++++++++-- src/Avalonia.Controls/TopLevel.cs | 4 +- .../FlyoutTests.cs | 19 ++++--- .../Primitives/PopupTests.cs | 43 +++++++--------- 5 files changed, 81 insertions(+), 38 deletions(-) diff --git a/src/Avalonia.Base/Input/IInputRoot.cs b/src/Avalonia.Base/Input/IInputRoot.cs index 2be58472207..c1c5968ebe3 100644 --- a/src/Avalonia.Base/Input/IInputRoot.cs +++ b/src/Avalonia.Base/Input/IInputRoot.cs @@ -12,7 +12,7 @@ public interface IInputRoot : IInputElement /// /// Gets or sets the keyboard navigation handler. /// - IKeyboardNavigationHandler KeyboardNavigationHandler { get; } + IKeyboardNavigationHandler? KeyboardNavigationHandler { get; } /// /// Gets focus manager of the root. diff --git a/src/Avalonia.Controls/Primitives/OverlayPopupHost.cs b/src/Avalonia.Controls/Primitives/OverlayPopupHost.cs index 2e14b54716c..a7c7a17e6a3 100644 --- a/src/Avalonia.Controls/Primitives/OverlayPopupHost.cs +++ b/src/Avalonia.Controls/Primitives/OverlayPopupHost.cs @@ -2,14 +2,15 @@ using System.Collections.Generic; using Avalonia.Controls.Primitives.PopupPositioning; using Avalonia.Diagnostics; +using Avalonia.Input; using Avalonia.Interactivity; using Avalonia.Media; using Avalonia.Metadata; -using Avalonia.VisualTree; +using Avalonia.Platform; namespace Avalonia.Controls.Primitives { - public class OverlayPopupHost : ContentControl, IPopupHost, IManagedPopupPositionerPopup + public class OverlayPopupHost : ContentControl, IPopupHost, IManagedPopupPositionerPopup, IInputRoot { /// /// Defines the property. @@ -19,15 +20,21 @@ public class OverlayPopupHost : ContentControl, IPopupHost, IManagedPopupPositio private readonly OverlayLayer _overlayLayer; private readonly ManagedPopupPositioner _positioner; + private readonly IKeyboardNavigationHandler? _keyboardNavigationHandler; private Point _lastRequestedPosition; private PopupPositionRequest? _popupPositionRequest; private Size _popupSize; private bool _needsUpdate; + static OverlayPopupHost() + => KeyboardNavigation.TabNavigationProperty.OverrideDefaultValue(KeyboardNavigationMode.Cycle); + public OverlayPopupHost(OverlayLayer overlayLayer) { _overlayLayer = overlayLayer; _positioner = new ManagedPopupPositioner(this); + _keyboardNavigationHandler = AvaloniaLocator.Current.GetService(); + _keyboardNavigationHandler?.SetOwner(this); } /// @@ -53,6 +60,38 @@ bool IPopupHost.Topmost set { /* Not currently supported in overlay popups */ } } + private IInputRoot? InputRoot + => TopLevel.GetTopLevel(this); + + IKeyboardNavigationHandler? IInputRoot.KeyboardNavigationHandler + => _keyboardNavigationHandler; + + IFocusManager? IInputRoot.FocusManager + => InputRoot?.FocusManager; + + IPlatformSettings? IInputRoot.PlatformSettings + => InputRoot?.PlatformSettings; + + IInputElement? IInputRoot.PointerOverElement + { + get => InputRoot?.PointerOverElement; + set + { + if (InputRoot is { } inputRoot) + inputRoot.PointerOverElement = value; + } + } + + bool IInputRoot.ShowAccessKeys + { + get => InputRoot?.ShowAccessKeys ?? false; + set + { + if (InputRoot is { } inputRoot) + inputRoot.ShowAccessKeys = value; + } + } + /// internal override Interactive? InteractiveParent => Parent as Interactive; @@ -63,6 +102,12 @@ bool IPopupHost.Topmost public void Show() { _overlayLayer.Children.Add(this); + + if (Content is Visual { IsAttachedToVisualTree: false }) + { + // We need to force a measure pass so any descendants are built, for focus to work. + UpdateLayout(); + } } /// @@ -147,7 +192,7 @@ void IManagedPopupPositionerPopup.MoveAndResize(Point devicePoint, Size virtualS double IManagedPopupPositionerPopup.Scaling => 1; // TODO12: mark PrivateAPI or internal. - [Unstable("PopupHost is consireded an internal API. Use Popup or any Popup-based controls (Flyout, Tooltip) instead.")] + [Unstable("PopupHost is considered an internal API. Use Popup or any Popup-based controls (Flyout, Tooltip) instead.")] public static IPopupHost CreatePopupHost(Visual target, IAvaloniaDependencyResolver? dependencyResolver) { if (TopLevel.GetTopLevel(target) is { } topLevel && topLevel.PlatformImpl?.CreatePopup() is { } popupImpl) diff --git a/src/Avalonia.Controls/TopLevel.cs b/src/Avalonia.Controls/TopLevel.cs index 3d1c0b5b5f9..62c860c7a98 100644 --- a/src/Avalonia.Controls/TopLevel.cs +++ b/src/Avalonia.Controls/TopLevel.cs @@ -471,12 +471,12 @@ void UpdatePlatformImpl() /// /// Gets the access key handler for the window. /// - internal IAccessKeyHandler AccessKeyHandler => _accessKeyHandler!; + internal IAccessKeyHandler? AccessKeyHandler => _accessKeyHandler; /// /// Gets or sets the keyboard navigation handler for the window. /// - IKeyboardNavigationHandler IInputRoot.KeyboardNavigationHandler => _keyboardNavigationHandler!; + IKeyboardNavigationHandler? IInputRoot.KeyboardNavigationHandler => _keyboardNavigationHandler; /// IInputElement? IInputRoot.PointerOverElement diff --git a/tests/Avalonia.Controls.UnitTests/FlyoutTests.cs b/tests/Avalonia.Controls.UnitTests/FlyoutTests.cs index dad8c3b78e7..d748a709a56 100644 --- a/tests/Avalonia.Controls.UnitTests/FlyoutTests.cs +++ b/tests/Avalonia.Controls.UnitTests/FlyoutTests.cs @@ -20,6 +20,8 @@ namespace Avalonia.Controls.UnitTests { public class FlyoutTests { + protected bool UseOverlayPopups { get; set; } + [Fact] public void Opening_Raises_Single_Opening_Event() { @@ -373,10 +375,10 @@ public void ShowMode_Standard_Attemps_Focus_Flyout_Content() window.Show(); button.Focus(); - Assert.True(window.FocusManager.GetFocusedElement() == button); + Assert.Same(button, window.FocusManager!.GetFocusedElement()); button.Flyout.ShowAt(button); Assert.False(button.IsFocused); - Assert.True(window.FocusManager.GetFocusedElement() == flyoutTextBox); + Assert.Same(flyoutTextBox, window.FocusManager!.GetFocusedElement()); } } @@ -640,14 +642,11 @@ public void Setting_FlyoutPresenterClasses_Sets_Classes_On_FlyoutPresenter() } } - private static IDisposable CreateServicesWithFocus() + private IDisposable CreateServicesWithFocus() { return UnitTestApplication.Start(TestServices.StyledWindow.With(windowingPlatform: new MockWindowingPlatform(null, - x => - { - return MockWindowingPlatform.CreatePopupMock(x).Object; - }), + x => UseOverlayPopups ? null : MockWindowingPlatform.CreatePopupMock(x).Object), focusManager: new FocusManager(), keyboardDevice: () => new KeyboardDevice())); } @@ -681,4 +680,10 @@ public class TestFlyout : Flyout public new Popup Popup => base.Popup; } } + + public class OverlayPopupFlyoutTests : FlyoutTests + { + public OverlayPopupFlyoutTests() + => UseOverlayPopups = true; + } } diff --git a/tests/Avalonia.Controls.UnitTests/Primitives/PopupTests.cs b/tests/Avalonia.Controls.UnitTests/Primitives/PopupTests.cs index da37c9ad487..29671f07665 100644 --- a/tests/Avalonia.Controls.UnitTests/Primitives/PopupTests.cs +++ b/tests/Avalonia.Controls.UnitTests/Primitives/PopupTests.cs @@ -633,46 +633,39 @@ public void Focusable_Controls_In_Popup_Should_Get_Focus() { using (CreateServicesWithFocus()) { - var window = PreparedWindow(); + var window = PreparedWindow(new Panel { Children = { new Slider() }}); - var tb = new TextBox(); - var b = new Button(); - var p = new Popup + var textBox = new TextBox(); + var button = new Button(); + var popup = new Popup { PlacementTarget = window, Child = new StackPanel { Children = { - tb, - b + textBox, + button } } }; - ((ISetLogicalParent)p).SetParent(p.PlacementTarget); - window.Show(); - p.Open(); + ((ISetLogicalParent)popup).SetParent(popup.PlacementTarget); + window.Show(); + popup.Open(); - if(p.Host is OverlayPopupHost host) - { - //Need to measure/arrange for visual children to show up - //in OverlayPopupHost - host.Measure(Size.Infinity); - host.Arrange(new Rect(host.DesiredSize)); - } + button.Focus(); - tb.Focus(); + var inputRoot = Assert.IsAssignableFrom(popup.Host); - var focusManager = TopLevel.GetTopLevel(tb)!.FocusManager; - tb = Assert.IsType(focusManager.GetFocusedElement()); + var focusManager = inputRoot.FocusManager!; + Assert.Same(button, focusManager.GetFocusedElement()); //Ensure focus remains in the popup - var nextFocus = KeyboardNavigationHandler.GetNext(tb, NavigationDirection.Next); - - Assert.True(nextFocus == b); + inputRoot.KeyboardNavigationHandler!.Move(focusManager.GetFocusedElement()!, NavigationDirection.Next); + Assert.Same(textBox, focusManager.GetFocusedElement()); - p.Close(); + popup.Close(); } } @@ -1248,7 +1241,6 @@ public void Custom_Placement_Callback_Is_Executed() } } - private static PopupRoot CreateRoot(TopLevel popupParent, IPopupImpl impl = null) { impl ??= popupParent.PlatformImpl.CreatePopup(); @@ -1279,7 +1271,8 @@ private IDisposable CreateServicesWithFocus() return UnitTestApplication.Start(TestServices.StyledWindow.With( windowingPlatform: CreateMockWindowingPlatform(), focusManager: new FocusManager(), - keyboardDevice: () => new KeyboardDevice())); + keyboardDevice: () => new KeyboardDevice(), + keyboardNavigation: () => new KeyboardNavigationHandler())); }