Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XY focus #13947

Merged
merged 34 commits into from
Feb 6, 2024
Merged

XY focus #13947

merged 34 commits into from
Feb 6, 2024

Conversation

maxkatz6
Copy link
Member

@maxkatz6 maxkatz6 commented Dec 14, 2023

What it is about:

XY navigation is essential for devices with DPad-like controllers, where we don't have classic Tab navigation nor we don't have pointer helping with focusing elements. For example: TVs and consoles, or desktop controlled with a gamepad. It also is often useful with a normal keyboard depending on the layout of elements, where tabbing is not optimal.

Code is translated and adapted from WinUI 3 source code - https://github.com/microsoft/microsoft-ui-xaml/tree/winui3/release/1.5-stable/dxaml/xcp/components/focus/XYFocus.

API of this PR
Everything is implemented as attached properties:

public class XYFocus
{
     // property definitions are simplified, all properties are mutable
    public static XYFocusNavigationModes NavigationModes (InputElement obj);
    public static XYFocusNavigationStrategy UpNavigationStrategy (InputElement obj);
    public static XYFocusNavigationStrategy DownNavigationStrategy (InputElement obj);
    public static XYFocusNavigationStrategy LeftNavigationStrategy (InputElement obj);
    public static XYFocusNavigationStrategy RightNavigationStrategy (InputElement obj);

    public static InputElement Up (InputElement obj);
    public static InputElement Down (InputElement obj);
    public static InputElement Left (InputElement obj);
    public static InputElement Right (InputElement obj);
}

public enum XYFocusNavigationModes
{
    Disabled = 0,
    Keyboard = 1,
    Gamepad = 2,
    Remote = 4,
    Enabled = Gamepad | Remote | Keyboard
}
public enum XYFocusNavigationStrategy
{
    Auto,
    Projection = 1,
    NavigationDirectionDistance = 2,
    RectilinearDistance = 3
}

See https://learn.microsoft.com/en-us/uwp/api/windows.ui.xaml.input.xyfocusnavigationstrategy?view=winrt-22621 for visual understanding of the XYFocusNavigationStrategy.

Differences from UWP:

  1. UWP has these properties on each Control directly, when we have it as attached properties. (debatable)

Temporary differences from UWP (we planning to implemented)

  1. UWP supports Gamepad input handling, we don't yet.
  2. Focus engagement is not yet implemented. I need to do more changes in FocusManager code. In this PR I planned to make less invasive changes.
  3. UWP provides FocusManager APIs to get next focusable element in a direction, without actually focusing it. As well as it accepts some other options like hint rectangle. In this PR the only focus was to handle keyboard input without refactoring our FocusManager.
  4. We do not have ability to focus non-visual elements like TextElement (text block inlines) yet. It will likely be done only in 12.0 as I expect bigger breaking changes along the way.

Possible bug that I am leaving outside of the scope of this PR:

  1. RectilinearDistance and NavigationDirectionDistance behavior is a bit different from UWP. But for some reason in actual UWP app I am getting wrong navigation results comparing to the official documentation. This PR follows official documentation. And also WinUI source code...I can't debug UWP source code to know the reason of these differences.
  2. In some cases Projection navigation gives non-optimal results, possibly because of different manifolds handling.
  3. In UWP, you can't leave "scope" of the KeyboardNavigationMode=Enabled tree. Couldn't find out why in time.

Animation

@avaloniaui-bot
Copy link

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

@maxkatz6 maxkatz6 marked this pull request as ready for review December 15, 2023 08:08
@maxkatz6 maxkatz6 requested a review from grokys December 15, 2023 08:17
@avaloniaui-bot
Copy link

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

@grokys
Copy link
Member

grokys commented Dec 15, 2023

UWP has these properties on each Control directly, when we have it as attached properties. (debatable)

I think it makes sense to have them as attached properties, personally.

A few issues I've noticed:

  • Seems to have broken menu arrow key handling (caught by integration tests)
  • When using the "Focus" page in ControlCatalog pressing the left key often results in another page being selected

Other than that the code looks good to me from what I've seen except a few nits below.

@avaloniaui-bot
Copy link

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

@avaloniaui-bot
Copy link

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

@avaloniaui-bot
Copy link

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

@maxkatz6 maxkatz6 requested a review from grokys January 11, 2024 07:40
@maxkatz6
Copy link
Member Author

When using the "Focus" page in ControlCatalog pressing the left key often results in another page being selected

Fixed that, so only subtrees with enabled XY navigation are selectable.

Also made a change from UWP by replacing XYFocusKeyboardNavigationMode with XYFocusNavigationModes.
In UWP it was only possible to toggle XY for Keyboard input. When it was always enabled for gamepad. With my changes it's a flags enum now which can enable/disable any key device type.

@avaloniaui-bot
Copy link

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

@avaloniaui-bot
Copy link

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

@avaloniaui-bot
Copy link

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

Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and seems to work well!

@grokys grokys added this pull request to the merge queue Feb 6, 2024
Merged via the queue into master with commit a4086c1 Feb 6, 2024
7 checks passed
@grokys grokys deleted the xy-focus branch February 6, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants