Unify selection event handling for all SelectingItemsControl types plus TreeView

[TomEdwardsEnscape]

This PR substantially refactors selection logic to achieve a consistent experience which adapts to user expectations on different input devices. Behaviour is unchanged for standard mouse input, but touch and pen input have both been altered to fix bugs and bring Avalonia into line with native UI behaviours on relevant devices.

What is the current behavior?

The current state of selection logic is outlined in #18902.

What is the updated/expected behavior with this PR?

User facing:

• ListBox no longer cares if you swipe before releasing touch, unless a gesture is triggered or you swipe off the item entirely
• TreeViewItem is now selected on touch/pen release
• If Gestures.IsHoldWithMouseEnabled is true for a container, then mouse input selects on release instead of press

API:

• Container types now always handle selection input, instead of letting the event bubble up to their ItemsControl. Previously some did this and others did not. This is of particular relevance to TreeViewItem, which can be nested.
• However, when handling the events containers privately call UpdateSelectionFromEvent on their ItemsControl, a virtual method which contains shared selection logic for all containers of that control. This is where behaviour is customised.
• The default implementation of UpdateSelectionFromEvent calls EventSelectionTrigger, an overloaded virtual method which determines whether the state of the device calls for selection on press or release. Tweaking the return value is enough to implement correct behaviour for all SelectingItemsControl types defined in Avalonia.
• TopLevel.PlatformSettings.HotkeyConfiguration is now used, with a fallback to Application.Current.PlatformSettings.HotkeyConfiguration if null. This resolves a TODO comment I found in the codebase.

How was the solution implemented (if it's not obvious)?

Because TreeView does not inherit from SelectingItemsControl, I defined the standard selection logic in public static methods which both of these types call in their virtual methods.

I moved event handling to container types because this is the sane way to ensure that the event actually came from a container. Previously all events that reached the SelectingItemsControl/TreeView were handled, and the visual tree needed to be searched or hit tested to tell which container each one came from, if it came from one at all. This is no longer necessary.

I originally aimed to have selection behaviour customisable via properties, but this proved impractical. It's instead achieved with method overrides.

Breaking changes

An ItemsControl no longer sees the original events which cause its selection to change. This could disrupt existing custom event processing. To adapt to the new system, relevant code must be moved to an override of the new UpdateSelectionFromEvent method(s). An example of this happening can be seen in the changes to ComboBox in this PR.

The switch from selecting on touch press to touch release on some controls may break existing event handling.

Obsoletions / Deprecations

• SelectingItemsControl.UpdateSelection
• SelectingItemsControl.UpdateSelectionFromEventSource

Calls to either of these methods should be replaced with a call to SelectingItemsControl.UpdateSelectionFromEvent.

Fixed issues

Fixes #18902

Future work

• Add a TreeViewItem:pressed style to the Avalonia.Themes.Simple
• Fix bug I noticed where TreeViewItem:pressed style from Avalonia.Themes.Fluent doesn't activate on iOS, but does on Mac and Windows
• Integrate selection changes into Calendar

[avaloniaui-bot]

You can test this PR using the following package version. 12.0.999-cibuild0058124-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[TomEdwardsEnscape]

@MrJul this PR has had the "Needs API Review" label for some time now, and several more recent PRs have been API-approved in the meantime. Is there any ETA for when this one might come up for review?

[MrJul]

@TomEdwardsEnscape Sorry, we don't have any set order for the API review items. Sometimes simple ones that would take a few minutes of discussion take priority.

For this one, I personally wanted to have a good grasp of it before doing any API review. The API changes, while important, are secondary in this specific case. Understanding and discussing the behavioral changes should be done first.

Could you please update your branch to the latest master?

[avaloniaui-bot]

You can test this PR using the following package version. 12.0.999-cibuild0058900-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[TomEdwardsEnscape]

@MrJul the branch has been updated.

[avaloniaui-bot]

You can test this PR using the following package version. 12.0.999-cibuild0059209-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[MrJul]

API diff for review:

Avalonia.Base (net6.0, net8.0, netstandard2.0)

  namespace Avalonia.Input
  {
+     public interface IKeyModifiersEventArgs
+     {
+         KeyModifiers KeyModifiers { get; }
+     }
  }

Avalonia.Controls (net6.0, net8.0, netstandard2.0)

  namespace Avalonia.Controls
  {
      public class ComboBox : SelectingItemsControl
      {
+         protected override InputSelectionTrigger EventSelectionTrigger(InputElement selectable, PointerEventArgs eventArgs);
+         public override bool UpdateSelectionFromEvent(Control container, RoutedEventArgs eventArgs);
      }
      public class ListBoxItem : ContentControl, ISelectable
      {
+         protected override void OnKeyDown(KeyEventArgs e);
+         protected bool UpdateSelectionFromEvent(RoutedEventArgs e);
      }
      public class TabControl : SelectingItemsControl
      {
-         protected override void OnGotFocus(GotFocusEventArgs e);
-         protected override void OnPointerPressed(PointerPressedEventArgs e);
-         protected override void OnPointerReleased(PointerReleasedEventArgs e);
+         protected override InputSelectionTrigger EventSelectionTrigger(InputElement selectable, PointerEventArgs eventArgs);
+         public override bool UpdateSelectionFromEvent(Control container, RoutedEventArgs eventArgs);
      }
      public class TabItem : HeaderedContentControl, ISelectable
      {
+         protected override void OnGotFocus(GotFocusEventArgs e);
+         protected override void OnPointerPressed(PointerPressedEventArgs e);
+         protected override void OnPointerReleased(PointerReleasedEventArgs e);
+         protected bool UpdateSelectionFromEvent(RoutedEventArgs e);
      }
      public class TreeView : ItemsControl, ICustomKeyboardNavigation
      {
-         protected override void OnPointerPressed(PointerPressedEventArgs e);
+         protected virtual InputSelectionTrigger EventSelectionTrigger(InputElement selectable, KeyEventArgs eventArgs);
+         protected virtual InputSelectionTrigger EventSelectionTrigger(InputElement selectable, PointerEventArgs eventArgs);
+         public virtual bool UpdateSelectionFromEvent(Control container, RoutedEventArgs eventArgs);
      }
      public class TreeViewItem : HeaderedItemsControl, ISelectable
      {
+         protected override void OnPointerPressed(PointerPressedEventArgs e);
+         protected override void OnPointerReleased(PointerReleasedEventArgs e);
      }
  }
  namespace Avalonia.Controls.Primitives
  {
      public class SelectingItemsControl : ItemsControl
      {
+         protected virtual InputSelectionTrigger EventSelectionTrigger(InputElement selectable, KeyEventArgs eventArgs);
+         protected virtual InputSelectionTrigger EventSelectionTrigger(InputElement selectable, PointerEventArgs eventArgs);
+         public new static SelectingItemsControl? ItemsControlFromItemContainer(Control container);
+         public virtual bool UpdateSelectionFromEvent(Control container, RoutedEventArgs eventArgs);
      }
      public class TabStrip : SelectingItemsControl
      {
-         protected override void OnGotFocus(GotFocusEventArgs e);
-         protected override void OnPointerPressed(PointerPressedEventArgs e);
+         protected override InputSelectionTrigger EventSelectionTrigger(InputElement selectable, PointerEventArgs e);
+         public override bool UpdateSelectionFromEvent(Control container, RoutedEventArgs eventArgs);
      }
      public class TabStripItem : ListBoxItem
      {
+         protected override void OnGotFocus(GotFocusEventArgs e);
      }
+     public enum InputSelectionTrigger
+     {
+         None = 0,
+         Press = 1,
+         Release = 2
+     }
+     public static class SelectionEventLogic
+     {
+         public static InputSelectionTrigger EventSelectionTrigger(InputElement selectable, KeyEventArgs eventArgs);
+         public static InputSelectionTrigger EventSelectionTrigger(InputElement selectable, PointerEventArgs eventArgs);
+         public static bool HasRangeSelectionModifier(InputElement selectable, RoutedEventArgs eventArgs);
+         public static bool HasToggleSelectionModifier(InputElement selectable, RoutedEventArgs eventArgs);
+     }
  }

[MrJul]

During the API review meeting, we finally got to this one!

While we like the overall idea of unifying touch behavior for v12, we feel like the API shape could be improved. InputSelectionTrigger isn't quite necessary. The various EventSelectionTrigger methods could probably return some bool instead to determine if an event is going to trigger selection or not. It might complexify the implementation a bit, but gives a cleaner API surface.

SelectionEventLogic feels like a bit too broad of a name; ItemSelectionEventTriggers was proposed instead. EventSelectionTrigger should probably have a verb, but no final name was really defined since changing the logic to return a bool will impact the names.

IKeyModifiersEventArgs should be marked as NotClientImplementable.

@TomEdwardsEnscape Do you think the InputSelectionTrigger removal is feasible?

We're going to revisit this later.

[TomEdwardsEnscape]

I made the requested change to return boolean values instead of the InputSelectionTrigger enum. This method is where it gets ugly.

On mouse up we now only get "false" back from the base method, and we don't know why. To keep the control working I had to expose a new method, IsPointerEventWithinBounds, and then write entirely new logic here instead of calling the shared method and tweaking the return value.

[avaloniaui-bot]

You can test this PR using the following package version. 12.0.999-cibuild0060220-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[avaloniaui-bot]

You can test this PR using the following package version. 12.0.999-cibuild0060251-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]