Skip to content

Commit

Permalink
Fix overlay popup focus issues (#17326)
Browse files Browse the repository at this point in the history 
* Add failing focus tests for flyouts inside overlay popups

* Implement IKeyboardNavigationHandler on OverlayPopupHost

* Layout OverlayPopupHost content for focus to work
  • Loading branch information
@MrJul @maxkatz6
MrJul authored and maxkatz6 committed Oct 27, 2024
1 parent f70cf39 commit 3ebd774
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 38 deletions.
2 changes: 1 addition & 1 deletion src/Avalonia.Base/Input/IInputRoot.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ public interface IInputRoot : IInputElement
/// <summary>
/// Gets or sets the keyboard navigation handler.
/// </summary>
IKeyboardNavigationHandler KeyboardNavigationHandler { get; }
IKeyboardNavigationHandler? KeyboardNavigationHandler { get; }

/// <summary>
/// Gets focus manager of the root.
Expand Down
51 changes: 48 additions & 3 deletions src/Avalonia.Controls/Primitives/OverlayPopupHost.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
/// <summary>
/// Defines the <see cref="Transform"/> property.
Expand All @@ -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<OverlayPopupHost>(KeyboardNavigationMode.Cycle);

public OverlayPopupHost(OverlayLayer overlayLayer)
{
_overlayLayer = overlayLayer;
_positioner = new ManagedPopupPositioner(this);
_keyboardNavigationHandler = AvaloniaLocator.Current.GetService<IKeyboardNavigationHandler>();
_keyboardNavigationHandler?.SetOwner(this);
}

/// <inheritdoc />
Expand All @@ -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;
}
}

/// <inheritdoc />
internal override Interactive? InteractiveParent => Parent as Interactive;

Expand All @@ -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();
}
}

/// <inheritdoc />
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions src/Avalonia.Controls/TopLevel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -471,12 +471,12 @@ void UpdatePlatformImpl()
/// <summary>
/// Gets the access key handler for the window.
/// </summary>
internal IAccessKeyHandler AccessKeyHandler => _accessKeyHandler!;
internal IAccessKeyHandler? AccessKeyHandler => _accessKeyHandler;

/// <summary>
/// Gets or sets the keyboard navigation handler for the window.
/// </summary>
IKeyboardNavigationHandler IInputRoot.KeyboardNavigationHandler => _keyboardNavigationHandler!;
IKeyboardNavigationHandler? IInputRoot.KeyboardNavigationHandler => _keyboardNavigationHandler;

/// <inheritdoc/>
IInputElement? IInputRoot.PointerOverElement
Expand Down
19 changes: 12 additions & 7 deletions tests/Avalonia.Controls.UnitTests/FlyoutTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ namespace Avalonia.Controls.UnitTests
{
public class FlyoutTests
{
protected bool UseOverlayPopups { get; set; }

[Fact]
public void Opening_Raises_Single_Opening_Event()
{
Expand Down Expand Up @@ -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());
}
}

Expand Down Expand Up @@ -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()));
}
Expand Down Expand Up @@ -681,4 +680,10 @@ public class TestFlyout : Flyout
public new Popup Popup => base.Popup;
}
}

public class OverlayPopupFlyoutTests : FlyoutTests
{
public OverlayPopupFlyoutTests()
=> UseOverlayPopups = true;
}
}
43 changes: 18 additions & 25 deletions tests/Avalonia.Controls.UnitTests/Primitives/PopupTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<IInputRoot>(popup.Host);

var focusManager = TopLevel.GetTopLevel(tb)!.FocusManager;
tb = Assert.IsType<TextBox>(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();
}
}

Expand Down Expand Up @@ -1250,7 +1243,6 @@ public void Custom_Placement_Callback_Is_Executed()
}
}


private static PopupRoot CreateRoot(TopLevel popupParent, IPopupImpl impl = null)
{
impl ??= popupParent.PlatformImpl.CreatePopup();
Expand Down Expand Up @@ -1281,7 +1273,8 @@ private IDisposable CreateServicesWithFocus()
return UnitTestApplication.Start(TestServices.StyledWindow.With(
windowingPlatform: CreateMockWindowingPlatform(),
focusManager: new FocusManager(),
keyboardDevice: () => new KeyboardDevice()));
keyboardDevice: () => new KeyboardDevice(),
keyboardNavigation: () => new KeyboardNavigationHandler()));
}


Expand Down

0 comments on commit 3ebd774

Please sign in to comment.