From b46126714b72635b6fb699e679a8f103d5449105 Mon Sep 17 00:00:00 2001 From: StefanKoell Date: Sun, 10 Nov 2024 11:27:07 +0100 Subject: [PATCH] Fixes and improves several access key (accelerator) related issues (#17292) * fix accelerator behavior for menu items and labels * add elements with matching accelerator to test cycling in sub menus * Add AccessKeyHandler tests for accelerators with more than one match * Implement accelerator behavior based on WPF handling * Remove commented code * Remove OnAccessKey override => handled by DefaultMenuInteractionHandler * remove obsolete test * handle OnAccessKeyPressed for selected tab item * fix unit tests * use AccessKeyEvent instead of AccessKeyPressedEvent in unit tests * navigate menu with and without ALT key * Revert formatting changes in Tests * Fix AccessKeyHandler comments * move private types to bottom * Remove lock statements, optimize removal of AccessKeyRegistrations * remove call to Dispatcher.UIThread.Post * simplifiy AccessKeyHandler.SortByHierarchy * remove unnecessary method AccessKeyHandler.GetTargetsForSender * regenerate API suppression file * revert unneeded changes in MenuPage.axaml * correct formatting changes * do not sort by hierarchy if too few targets * make AccessKeyEventArgs internal * make AccessKeyPressedEventArgs internal --------- Co-authored-by: Hans Docsek --- api/Avalonia.nupkg.xml | 6 + samples/ControlCatalog/MainView.xaml | 3 + .../ControlCatalog/Pages/AcceleratorPage.xaml | 115 +++++ .../Pages/AcceleratorPage.xaml.cs | 18 + src/Avalonia.Base/Input/AccessKeyHandler.cs | 408 ++++++++++++++++-- src/Avalonia.Base/Input/InputElement.cs | 31 +- src/Avalonia.Controls/Button.cs | 25 +- src/Avalonia.Controls/Label.cs | 31 +- src/Avalonia.Controls/Menu.cs | 12 +- src/Avalonia.Controls/MenuItem.cs | 12 +- .../MenuItemAccessKeyHandler.cs | 93 +--- .../Platform/DefaultMenuInteractionHandler.cs | 12 +- src/Avalonia.Controls/TabItem.cs | 26 +- .../Input/AccessKeyHandlerTests.cs | 123 +++--- .../Input/InputElement_Focus.cs | 6 +- .../ButtonTests.cs | 19 +- .../TabControlTests.cs | 10 +- .../Data/BindingTests_Method.cs | 2 +- 18 files changed, 735 insertions(+), 217 deletions(-) create mode 100644 samples/ControlCatalog/Pages/AcceleratorPage.xaml create mode 100644 samples/ControlCatalog/Pages/AcceleratorPage.xaml.cs diff --git a/api/Avalonia.nupkg.xml b/api/Avalonia.nupkg.xml index 1edc3b5ec2..f6797ada37 100644 --- a/api/Avalonia.nupkg.xml +++ b/api/Avalonia.nupkg.xml @@ -91,4 +91,10 @@ baseline/netstandard2.0/Avalonia.Controls.dll target/netstandard2.0/Avalonia.Controls.dll + + CP0012 + M:Avalonia.Controls.Button.OnAccessKey(Avalonia.Interactivity.RoutedEventArgs) + baseline/netstandard2.0/Avalonia.Controls.dll + target/netstandard2.0/Avalonia.Controls.dll + \ No newline at end of file diff --git a/samples/ControlCatalog/MainView.xaml b/samples/ControlCatalog/MainView.xaml index 6246c73ce2..e3eed5fb0e 100644 --- a/samples/ControlCatalog/MainView.xaml +++ b/samples/ControlCatalog/MainView.xaml @@ -18,6 +18,9 @@ + + + diff --git a/samples/ControlCatalog/Pages/AcceleratorPage.xaml b/samples/ControlCatalog/Pages/AcceleratorPage.xaml new file mode 100644 index 0000000000..18072ef1c3 --- /dev/null +++ b/samples/ControlCatalog/Pages/AcceleratorPage.xaml @@ -0,0 +1,115 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Accelerator Support + + + + + This is tab 1 content + + This is tab 1 content + + This is tab 1 content + + + + + This is tab 2 content + + + + + + This is tab 4 content + + + This is fab 5 content + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/samples/ControlCatalog/Pages/AcceleratorPage.xaml.cs b/samples/ControlCatalog/Pages/AcceleratorPage.xaml.cs new file mode 100644 index 0000000000..0d0d684700 --- /dev/null +++ b/samples/ControlCatalog/Pages/AcceleratorPage.xaml.cs @@ -0,0 +1,18 @@ +using Avalonia.Controls; +using Avalonia.Markup.Xaml; + +namespace ControlCatalog.Pages +{ + public class AcceleratorPage : UserControl + { + public AcceleratorPage() + { + this.InitializeComponent(); + } + + private void InitializeComponent() + { + AvaloniaXamlLoader.Load(this); + } + } +} diff --git a/src/Avalonia.Base/Input/AccessKeyHandler.cs b/src/Avalonia.Base/Input/AccessKeyHandler.cs index fe5e2c46a2..96a4847db8 100644 --- a/src/Avalonia.Base/Input/AccessKeyHandler.cs +++ b/src/Avalonia.Base/Input/AccessKeyHandler.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Linq; using Avalonia.Interactivity; using Avalonia.LogicalTree; +using Avalonia.VisualTree; namespace Avalonia.Input { @@ -11,11 +12,20 @@ namespace Avalonia.Input /// internal class AccessKeyHandler : IAccessKeyHandler { + /// + /// Defines the AccessKey attached event. + /// + public static readonly RoutedEvent AccessKeyEvent = + RoutedEvent.Register( + "AccessKey", + RoutingStrategies.Bubble, + typeof(AccessKeyHandler)); + /// /// Defines the AccessKeyPressed attached event. /// - public static readonly RoutedEvent AccessKeyPressedEvent = - RoutedEvent.Register( + public static readonly RoutedEvent AccessKeyPressedEvent = + RoutedEvent.Register( "AccessKeyPressed", RoutingStrategies.Bubble, typeof(AccessKeyHandler)); @@ -23,7 +33,9 @@ namespace Avalonia.Input /// /// The registered access keys. /// - private readonly List<(string AccessKey, IInputElement Element)> _registered = new(); + private readonly List _registrations = []; + + protected IReadOnlyList Registrations => _registrations; /// /// The window to which the handler belongs. @@ -48,7 +60,7 @@ namespace Avalonia.Input /// /// Element to restore following AltKey taking focus. /// - private IInputElement? _restoreFocusElement; + private WeakReference? _restoreFocusElementRef; /// /// The window's main menu. @@ -97,6 +109,12 @@ namespace Avalonia.Input _owner.AddHandler(InputElement.KeyDownEvent, OnKeyDown, RoutingStrategies.Bubble); _owner.AddHandler(InputElement.KeyUpEvent, OnPreviewKeyUp, RoutingStrategies.Tunnel); _owner.AddHandler(InputElement.PointerPressedEvent, OnPreviewPointerPressed, RoutingStrategies.Tunnel); + + OnSetOwner(owner); + } + + protected virtual void OnSetOwner(IInputRoot owner) + { } /// @@ -106,14 +124,19 @@ namespace Avalonia.Input /// The input element. public void Register(char accessKey, IInputElement element) { - var existing = _registered.FirstOrDefault(x => x.Item2 == element); - - if (existing != default) + var key = NormalizeKey(accessKey.ToString()); + + // remove dead elements with matching key + for (var i = _registrations.Count - 1; i >= 0; i--) { - _registered.Remove(existing); + var registration = _registrations[i]; + if (registration.Key == key && registration.GetInputElement() == null) + { + _registrations.RemoveAt(i); + } } - _registered.Add((accessKey.ToString().ToUpperInvariant(), element)); + _registrations.Add(new AccessKeyRegistration(key, new WeakReference(element))); } /// @@ -122,9 +145,15 @@ namespace Avalonia.Input /// The input element. public void Unregister(IInputElement element) { - foreach (var i in _registered.Where(x => x.Item2 == element).ToList()) + // remove element and all dead elements + for (var i = _registrations.Count - 1; i >= 0; i--) { - _registered.Remove(i); + var registration = _registrations[i]; + var inputElement = registration.GetInputElement(); + if (inputElement == null || inputElement == element) + { + _registrations.RemoveAt(i); + } } } @@ -135,21 +164,29 @@ namespace Avalonia.Input /// The event args. protected virtual void OnPreviewKeyDown(object? sender, KeyEventArgs e) { - if (e.Key == Key.LeftAlt || e.Key == Key.RightAlt) + // if the owner (IInputRoot) does not have the keyboard focus, ignore all keyboard events + // KeyboardDevice.IsKeyboardFocusWithin in case of a PopupRoot seems to only work once, so we created our own + var isFocusWithinOwner = IsFocusWithinOwner(_owner!); + if (!isFocusWithinOwner) + return; + + if (e.Key is Key.LeftAlt or Key.RightAlt) { _altIsDown = true; - if (MainMenu == null || !MainMenu.IsOpen) + if (MainMenu is not { IsOpen: true }) { var focusManager = FocusManager.GetFocusManager(e.Source as IInputElement); - + // TODO: Use FocusScopes to store the current element and restore it when context menu is closed. // Save currently focused input element. - _restoreFocusElement = focusManager?.GetFocusedElement(); + var focusedElement = focusManager?.GetFocusedElement(); + if (focusedElement is not null) + _restoreFocusElementRef = new WeakReference(focusedElement); // When Alt is pressed without a main menu, or with a closed main menu, show // access key markers in the window (i.e. "_File"). - _owner!.ShowAccessKeys = _showingAccessKeys = true; + _owner!.ShowAccessKeys = _showingAccessKeys = isFocusWithinOwner; } else { @@ -157,8 +194,11 @@ namespace Avalonia.Input CloseMenu(); _ignoreAltUp = true; - _restoreFocusElement?.Focus(); - _restoreFocusElement = null; + if (_restoreFocusElementRef?.TryGetTarget(out var restoreElement) ?? false) + { + restoreElement.Focus(); + } + _restoreFocusElementRef = null; } } else if (_altIsDown) @@ -174,35 +214,20 @@ namespace Avalonia.Input /// The event args. protected virtual void OnKeyDown(object? sender, KeyEventArgs e) { - bool menuIsOpen = MainMenu?.IsOpen == true; + // if the owner (IInputRoot) does not have the keyboard focus, ignore all keyboard events + // KeyboardDevice.IsKeyboardFocusWithin in case of a PopupRoot seems to only work once, so we created our own + var isFocusWithinOwner = IsFocusWithinOwner(_owner!); + if (!isFocusWithinOwner) + return; - if (e.KeyModifiers.HasAllFlags(KeyModifiers.Alt) && !e.KeyModifiers.HasAllFlags(KeyModifiers.Control) || menuIsOpen) - { - // If any other key is pressed with the Alt key held down, or the main menu is open, - // find all controls who have registered that access key. - var text = e.Key.ToString(); - var matches = _registered - .Where(x => string.Equals(x.AccessKey, text, StringComparison.OrdinalIgnoreCase) - && x.Element.IsEffectivelyVisible - && x.Element.IsEffectivelyEnabled) - .Select(x => x.Element); - - // If the menu is open, only match controls in the menu's visual tree. - if (menuIsOpen) - { - matches = matches.Where(x => x is not null && ((Visual)MainMenu!).IsLogicalAncestorOf((Visual)x)); - } - - var match = matches.FirstOrDefault(); + if ((!e.KeyModifiers.HasAllFlags(KeyModifiers.Alt) || e.KeyModifiers.HasAllFlags(KeyModifiers.Control)) && + MainMenu?.IsOpen != true) + return; - // If there was a match, raise the AccessKeyPressed event on it. - if (match is not null) - { - match.RaiseEvent(new RoutedEventArgs(AccessKeyPressedEvent)); - } - } + e.Handled = ProcessKey(e.Key.ToString(), e.Source as IInputElement); } + /// /// Handles the Alt/F10 keys being released in the window. /// @@ -255,5 +280,302 @@ namespace Avalonia.Input { _owner!.ShowAccessKeys = false; } + + /// + /// Processes the given key for the element's targets + /// + /// The access key to process. + /// The element to get the targets which are in scope. + /// If there matches true, otherwise false. + protected bool ProcessKey(string key, IInputElement? element) + { + key = NormalizeKey(key); + var senderInfo = GetTargetForElement(element, key); + // Find the possible targets matching the access key + var targets = SortByHierarchy(GetTargetsForKey(key, element, senderInfo)); + var result = ProcessKey(key, targets); + return result != ProcessKeyResult.NoMatch; + } + + private static string NormalizeKey(string key) => key.ToUpperInvariant(); + + private static ProcessKeyResult ProcessKey(string key, List targets) + { + if (!targets.Any()) + return ProcessKeyResult.NoMatch; + + var isSingleTarget = true; + var lastWasFocused = false; + + IInputElement? effectiveTarget = null; + + var chosenIndex = 0; + for (var i = 0; i < targets.Count; i++) + { + var target = targets[i]; + + if (!IsTargetable(target)) + continue; + + if (effectiveTarget == null) + { + effectiveTarget = target; + chosenIndex = i; + } + else + { + if (lastWasFocused) + { + effectiveTarget = target; + chosenIndex = i; + } + + isSingleTarget = false; + } + + lastWasFocused = target.IsFocused; + } + + if (effectiveTarget == null) + return ProcessKeyResult.NoMatch; + + var args = new AccessKeyEventArgs(key, isMultiple: !isSingleTarget); + effectiveTarget.RaiseEvent(args); + + return chosenIndex == targets.Count - 1 ? ProcessKeyResult.LastMatch : ProcessKeyResult.MoreMatches; + } + + private List GetTargetsForKey(string key, IInputElement? sender, + AccessKeyInformation senderInfo) + { + var possibleElements = CopyMatchingAndPurgeDead(key); + + if (!possibleElements.Any()) + return possibleElements; + + var finalTargets = new List(1); + + // Go through all the possible elements, find the interesting candidates + foreach (var element in possibleElements) + { + if (element != sender) + { + if (!IsTargetable(element)) + continue; + + var elementInfo = GetTargetForElement(element, key); + if (elementInfo.Target == null) + continue; + + finalTargets.Add(elementInfo.Target); + } + else + { + // This is the same element that sent the event so it must be in the same scope. + // Just add it to the final targets + if (senderInfo.Target == null) + continue; + + finalTargets.Add(senderInfo.Target); + } + } + + return finalTargets; + } + + private static bool IsTargetable(IInputElement element) => + element is { IsEffectivelyEnabled: true, IsEffectivelyVisible: true }; + + private List CopyMatchingAndPurgeDead(string key) + { + var matches = new List(_registrations.Count); + + // collect live elements with matching key and remove dead elements + for (var i = _registrations.Count - 1; i >= 0; i--) + { + var registration = _registrations[i]; + var inputElement = registration.GetInputElement(); + if (inputElement != null) + { + if (registration.Key == key) + { + matches.Add(inputElement); + } + } + else + { + _registrations.RemoveAt(i); + } + } + + // since we collected the elements when iterating from back to front + // we need to reverse them to ensure the original order + matches.Reverse(); + + return matches; + } + + /// + /// Returns targeting information for the given element. + /// + /// + /// + /// AccessKeyInformation with target for the access key. + private static AccessKeyInformation GetTargetForElement(IInputElement? element, string key) + { + var info = new AccessKeyInformation(); + if (element == null) + return info; + + var args = new AccessKeyPressedEventArgs(key); + element.RaiseEvent(args); + info.Target = args.Target; + + return info; + } + + /// + /// Checks if the focused element is a descendent of the owner. + /// + /// The owner to check. + /// If focused element is decendant of owner true, otherwise false. + private static bool IsFocusWithinOwner(IInputRoot owner) + { + var focusedElement = KeyboardDevice.Instance?.FocusedElement; + if (focusedElement is not InputElement inputElement) + return false; + + var isAncestorOf = owner is Visual root && root.IsVisualAncestorOf(inputElement); + return isAncestorOf; + } + + /// + /// Sorts the list of targets according to logical ancestors in the hierarchy + /// so that child elements, for example within in the content of a tab, + /// are processed before the next parent item i.e. the next tab item. + /// + private static List SortByHierarchy(List targets) + { + // bail out, if there are no targets to sort + if (targets.Count <= 1) + return targets; + + var sorted = new List(targets.Count); + var queue = new Queue(targets); + while (queue.Count > 0) + { + var element = queue.Dequeue(); + + // if the element was already added, do nothing + if (sorted.Contains(element)) + continue; + + // add the element itself + sorted.Add(element); + + // if the element is not a potential parent, do nothing + if (element is not ILogical parentElement) + continue; + + // add all descendants of the element + sorted.AddRange(queue + .Where(child => parentElement + .IsLogicalAncestorOf(child as ILogical))); + } + return sorted; + } + + private enum ProcessKeyResult + { + NoMatch, + MoreMatches, + LastMatch + } + + private struct AccessKeyInformation + { + public IInputElement? Target { get; set; } + } + } + + /// + /// The inputs to an AccessKeyPressedEventHandler + /// + internal class AccessKeyPressedEventArgs : RoutedEventArgs + { + /// + /// The constructor for AccessKeyPressed event args + /// + public AccessKeyPressedEventArgs() + { + RoutedEvent = AccessKeyHandler.AccessKeyPressedEvent; + Key = null; + } + + /// + /// Constructor for AccessKeyPressed event args + /// + /// + public AccessKeyPressedEventArgs(string key) : this() + { + RoutedEvent = AccessKeyHandler.AccessKeyPressedEvent; + Key = key; + } + + /// + /// Target element for the element that raised this event. + /// + /// + public IInputElement? Target { get; set; } + + /// + /// Key that was pressed + /// + /// + public string? Key { get; } + } + + /// + /// Information pertaining to when the access key associated with an element is pressed + /// + internal class AccessKeyEventArgs : RoutedEventArgs + { + /// + /// Constructor + /// + internal AccessKeyEventArgs(string key, bool isMultiple) + { + RoutedEvent = AccessKeyHandler.AccessKeyEvent; + + Key = key; + IsMultiple = isMultiple; + } + + /// + /// The key that was pressed which invoked this access key + /// + /// + public string Key { get; } + + /// + /// Were there other elements which are also invoked by this key + /// + /// + public bool IsMultiple { get; } + } + + internal class AccessKeyRegistration + { + private readonly WeakReference _target; + public string Key { get; } + + public AccessKeyRegistration(string key, WeakReference target) + { + _target = target; + Key = key; + } + + public IInputElement? GetInputElement() => + _target.TryGetTarget(out var target) ? target : null; } } diff --git a/src/Avalonia.Base/Input/InputElement.cs b/src/Avalonia.Base/Input/InputElement.cs index bdb6500168..5da27c6804 100644 --- a/src/Avalonia.Base/Input/InputElement.cs +++ b/src/Avalonia.Base/Input/InputElement.cs @@ -1,17 +1,15 @@ +#nullable enable + using System; using System.Collections.Generic; -using System.Linq; using Avalonia.Controls; using Avalonia.Controls.Metadata; -using Avalonia.Data; using Avalonia.Input.GestureRecognizers; using Avalonia.Input.TextInput; using Avalonia.Interactivity; using Avalonia.Reactive; using Avalonia.VisualTree; -#nullable enable - namespace Avalonia.Input { /// @@ -231,6 +229,10 @@ namespace Avalonia.Input PointerPressedEvent.AddClassHandler((x, e) => x.OnGesturePointerPressed(e), handledEventsToo: true); PointerReleasedEvent.AddClassHandler((x, e) => x.OnGesturePointerReleased(e), handledEventsToo: true); PointerCaptureLostEvent.AddClassHandler((x, e) => x.OnGesturePointerCaptureLost(e), handledEventsToo: true); + + + // Access Key Handling + AccessKeyHandler.AccessKeyEvent.AddClassHandler((x, e) => x.OnAccessKey(e)); } public InputElement() @@ -282,7 +284,7 @@ namespace Avalonia.Input add { AddHandler(TextInputEvent, value); } remove { RemoveHandler(TextInputEvent, value); } } - + /// /// Occurs when an input element gains input focus and input method is looking for the corresponding client /// @@ -346,7 +348,7 @@ namespace Avalonia.Input add => AddHandler(PointerCaptureLostEvent, value); remove => RemoveHandler(PointerCaptureLostEvent, value); } - + /// /// Occurs when the mouse is scrolled over the control. /// @@ -355,7 +357,7 @@ namespace Avalonia.Input add { AddHandler(PointerWheelChangedEvent, value); } remove { RemoveHandler(PointerWheelChangedEvent, value); } } - + /// /// Occurs when a tap gesture occurs on the control. /// @@ -364,7 +366,7 @@ namespace Avalonia.Input add { AddHandler(TappedEvent, value); } remove { RemoveHandler(TappedEvent, value); } } - + /// /// Occurs when a hold gesture occurs on the control. /// @@ -409,7 +411,7 @@ namespace Avalonia.Input get { return GetValue(CursorProperty); } set { SetValue(CursorProperty, value); } } - + /// /// Gets a value indicating whether keyboard focus is anywhere within the element or its visual tree child elements. /// @@ -515,6 +517,17 @@ namespace Avalonia.Input } } + /// + /// This method is used to execute the action on an effective IInputElement when a corresponding access key has been invoked. + /// By default, the Focus() method is invoked with the NavigationMethod.Tab to indicate a visual focus adorner. + /// Overwrite this method if other methods or additional functionality is needed when an item should receive the focus. + /// + /// AccessKeyEventArgs are passed on to indicate if there are multiple matches or not. + protected virtual void OnAccessKey(RoutedEventArgs e) + { + Focus(NavigationMethod.Tab); + } + /// protected override void OnAttachedToVisualTreeCore(VisualTreeAttachmentEventArgs e) { diff --git a/src/Avalonia.Controls/Button.cs b/src/Avalonia.Controls/Button.cs index 614847ee4e..9673eaadf7 100644 --- a/src/Avalonia.Controls/Button.cs +++ b/src/Avalonia.Controls/Button.cs @@ -104,7 +104,7 @@ namespace Avalonia.Controls static Button() { FocusableProperty.OverrideDefaultValue(typeof(Button), true); - AccessKeyHandler.AccessKeyPressedEvent.AddClassHandler