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

FocusManager refactor #8051

Closed
wants to merge 23 commits into from
Closed

FocusManager refactor #8051

wants to merge 23 commits into from

Conversation

amwx
Copy link
Contributor

@amwx amwx commented Apr 27, 2022

What does the pull request do?

As in #7607, this PR is a major rework of the FocusManager to follow the UWP/WinUI API. This is a significant breaking change, but should modernize the API, make the logic less scattered, fix issues like preserving focus states when switching into and out of a popup (as one example), and making in-app navigation easier for users. This first PR focuses solely on the FocusManager API changes and tab navigation. Since the WinUI code isn't open source, this take the UWP design but recycles the existing WPF navigation logic for its implementation. From my early tests, this seems to work fine. Once the API here is approved and I get everything to a good state, I'll start another PR to implement XYFocus for directional navigation. Both are big changes (though this is the breaking change PR), so I figured its best to separate them

Right now, this is more a prototype for API approval, so I've only committed the bare bones so far (enums, EventArgs, and the empty FocusManager implementation). There are some things related to XYFocus that I've included now, but won't do anything until later. If there are concerns or issues with the proposed API, I'm not opposed to making changes and deviating somewhat.

As I said this is a big breaking change so to make review easier, these are the major changes:

  • The following were removed or marked as obsolete to be removed with this PR:
    IFocusScope, GotFocusEventArgs, IKeyboardNavigationHandler, KeyboardNavigationHandler, NavigationMode

KeyboardNavigationHandler was marked to be removed as new Focus API's should handle this. I haven't marked ICustomKeyboardNavigation anything yet, but my intention is that should no longer be needed. If there is a desire to keep it though, then I'm open to that consideration. (The same probably applies to INavigableContainer)
Most of the Focus logic in KeyboardDevice could probably be merged with the FocusManager too, but I haven't touched that yet.

  • An instance of FocusManager now exists on each TopLevel, as from what I can tell, is the base View for every platform. However, the public facing FocusManager exists as a static entity, thus FocusManager is no longer registered in AvaloniaLocator and Focus can be changed either by calling InputElement.Focus() or by using the static FocusManager methods as in UWP.

  • FocusManager was also moved to the Avalonia.Controls project. For some logic, we need reference to some Controls - mostly related to popups. Popups are treated in the same scope as the window/TopLevel they belong to, and for Overlay popups in particular we need to make sure focus never leaves the overlay layer if that's where current focus is.
    Windowed popups are treated the same. My intention for them is to make Windowed popups register with the OverlayLayer when they're opened - similar to UWP's PopupRoot layer. The reason - if FocusManager.FindFirstFocusableElement() is called, popups get precedence (particularly light dismiss or flyouts) and should be searched first before moving the window.

  • GettingFocus and LosingFocus events were added with corresponding EventArgs classes. GotFocus event was changed to just use standard RoutedEventArgs (hence the removal of GotFocusEventArgs). These args no longer carry the KeyModifiers that GotFocusEventArgs did, so if that's an issue let me know. Though IMO, focus shouldn't need info about key modifiers - that should be handled with the pointer or key event directly. FocusManager also has static events to match InputElement.
    Typical focus event order
    InputElement.LosingFocus -> FocusManager.LosingFocus -> InputElement.GettingFocus -> FocusManager.GettingFocus
    While a focus change is in progress, focused element cannot change or be cancelled except through these events
    Then the following are fired asynchronously via the dispatcher
    InputElement.LostFocus -> FocusManager.LostFocus -> InputElement.GotFocus -> FocusManager.GotFocus

  • NavigationMethod was changed to FocusState to match UWP. FocusState has the following values:
    Unfocused, Pointer, Keyboard, Programmatic
    The biggest change here is probably there's no longer separation between directional and tab - both are treated as just keyboard

  • InputManager keeps track of the last input type. This is so that if FocusState.Programmatic is used, we use the last device to send input to determine what the actual focus state is. For example, if you have button with a flyout that was previously focused via the pointer, and you invoke the button via the enter key to open the flyout, the flyout content will automatically focus with keyboard focus since the last input device was the keyboard

Assuming this is approved, I will also write a docs page on the new APIs so it is documented

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

/// <summary>
/// Specifies input device types from which input events are received
/// </summary>
public enum FocusInputDeviceKind
Copy link
Contributor

Choose a reason for hiding this comment

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

you could add software or automation

Copy link
Member

Choose a reason for hiding this comment

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

For "software" "Programmatic" term is used usually.

Copy link
Member

Choose a reason for hiding this comment

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

UWP uses "None" for that:

No input. Used only when the focus is moved programmatically.

https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.input.focusinputdevicekind?view=winrt-22621

@grokys
Copy link
Member

grokys commented Apr 27, 2022

Thanks for this @amwx - it's much needed! I will try to review as soon as possible, but most of us are at NDC this week so it may be next week before we can give you any detailed feedback. From your description though, it all sounds good (except maybe we need to find a way to not need to move FocusManager to Avalonia.Controls).

/// <summary>
/// Game controller/remote input device
/// </summary>
GameController
Copy link
Member

@maxkatz6 maxkatz6 Apr 27, 2022

Choose a reason for hiding this comment

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

Is this ported from UWP? Because otherwise I don't really like "GameController" term, because controller by itself isn't really limited to games. And also same means "remote controller" for TV-like devices, basically just a variation of console-controllers.

On other hand we don't support controllers yet, so it can be added later. Personally I want to see it in Avalonia in the future.

P.S. I just had a small look. Will review properly on next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the enum names are from UWP. 'GameController' term comes from the Xbox device family. Since Avalonia isn't tied to any platform, if you have a better name suggestion, I don't think it would hurt to change here. This is also unused at the moment, I left it in for future support.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about just Controller ?

@amwx
Copy link
Contributor Author

amwx commented Apr 27, 2022

Thanks for this @amwx - it's much needed! I will try to review as soon as possible, but most of us are at NDC this week so it may be next week before we can give you any detailed feedback. From your description though, it all sounds good

Great! I'll keep working on some of this for now, so no worries on the time frame.

(except maybe we need to find a way to not need to move FocusManager to Avalonia.Controls).

I agree, I didn't really like moving it to the controls project. I think I have an idea, but it involves shuffling the visual tree hierarchy a little bit. I'll open a separate issue for that discussion (edit: issue created #8052)

Edit: I think moving it to Avalonia.Controls isn't going to work anyway. InputElement needs knowledge of the FocusManager to respond and move focus if a control is disabled while focused and Visual needs knowledge to respond if it's visibility changes, and to handle being detached from the tree.

Comment on lines +58 to +59
// RawPointerEventArgs -> Mouse or ???
// --> RawTouchEventArgs -> Touch
Copy link
Member

Choose a reason for hiding this comment

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

RawPointerEventArgs can be used for mouse, touch and pen. RawTouchEventArgs can be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But how does RawPointerEventArgs differentiate between touch, pen, and mouse? For focus purposes, we really only need keyboard vs. non-keyboard here, but since I made the property public (as it may be useful in other scenarios) it should properly reflect the last input type.
Though I think proper pen/touch support was still/is still being worked on when I did this so maybe this is improved and there's a better way now?

Copy link
Member

Choose a reason for hiding this comment

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

Depending on "Device" property type.

Copy link
Member

Choose a reason for hiding this comment

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

Touch support is supported for a while now. Pen support is still on PR blocked by unsolved yet issues.
But even after Pen/WM_POINTER PR merged RawPointerEventArgs.Device is only one place to check input type of RAW event. As for now.

@@ -1,5 +1,9 @@
namespace Avalonia.Input
{
// TODO_FOCUS: WinUI doesn't have First,Last,PgUp,PgDown directions
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember WinUI API, but these navigation directions are useful for lists.
And from what I remember, UWP might implement these key handlers on a control level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WinUI excluded those options in FocusNavigationDirection. IMO, these are better left to implement on individual controls where viewport and other necessary information is readily available. That being said, these come from WPF, same as the existing tab navigation logic so they should already work? I've never tested them. I didn't remove them from the enum as I wanted outside opinions.

https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.input.focusnavigationdirection?view=winrt-22621

@@ -1,6 +0,0 @@
namespace Avalonia.Input
{
public interface IFocusScope
Copy link
Member

Choose a reason for hiding this comment

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

I remember IFocusScope was added only to support Popups, and now same problem was resolved with internal Popup.ManagesFocus (as I understood). But do we have any focus scoping now in this PR, to isolate focus temporary and restore previous focus when scope removed? @grokys

/// In UWP, a separate enum 'XYFocusNavigationStrategyOverride' exists, but holds same
/// values as XYFocusNavigationStrategy - so just using that one
/// </remarks>
public XYFocusNavigationStrategy XYFocusNavigationStrategyOverride { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

This PR is going to be huge even without XYFocus. Wouldn't it be better to implement it in another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FindNextElementOptions is only included now so I could add the full FocusManager API. It isn't used until XYFocus is implemented, so it's more just a placeholder for now.

@amwx
Copy link
Contributor Author

amwx commented May 30, 2022

As a side note, I've thought of a use case where sending the KeyModifiers in the LosingFocus/GettingFocus event args is useful, so I will add them back. In WinUI you can check the keyboard state at any time (method differs between UWP & WinUI3), where as that's not the case with Avalonia.

@amwx
Copy link
Contributor Author

amwx commented Jun 4, 2022

I've returned the FocusManager back to Avalonia.Input as I think I found a way to handle the issues. I've added two interfaces, IOverlayHost and IOverlayVisual to Avalonia.VisualTree to indicate the necessary controls without need the references to Avalonia.Controls.

IOverlayHost is implemented by OverlayLayer and has one method GetTopmostLightDismissElement which returns the topmost popup/element registered so the focus manager knows if popups/overlays are active.

PopupRoot and OverlayPopupHost implements IOverlayVisual which allows the TabNavigation code to detect Popups without knowing they're popups, specifically to enforce a cycle behavior to keep focus navigation within the popup (in the case of using overlay popups).

@SuperJMN
Copy link
Contributor

I'd like to test this PR, but I have discovered that it doesn't build 😓

@amwx
Copy link
Contributor Author

amwx commented Jun 14, 2022

I'd like to test this PR, but I have discovered that it doesn't build 😓

This is still a WIP. I haven't finished or pushed out changes to some controls that still rely on the old focus manager which is why it doesn't build. I'm hoping to get all this finished up this week.

@amwx
Copy link
Contributor Author

amwx commented Jun 23, 2022

Just an update on this... Something broke in one of the last few commits or merges of master and I don't know where and walking back to figure it out proved too complicated. Instead, I decided to start over so I'm closing this PR and will open a new one with the new branch (didn't want to do this, but it was the easier solution). I've also made a couple design changes that actually seems to be working out better and requires less "hacks" to keep track of focus (will explain in new PR - original intent of following UWP API is still intact, though). I want to get most, if not all, controls working along with some basic samples for testing before I open the new PR. My goal is to have that PR ready for review and testing when I open it so this can move forward.

@amwx amwx closed this Jun 23, 2022
@SuperJMN
Copy link
Contributor

No rush. Thanks for the great effort you're doing!

@amwx amwx mentioned this pull request Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants