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

Limit IFocusManager API, extend IInputElement API, remove focus static members from public API #11407

Merged
merged 7 commits into from
May 19, 2023

Conversation

maxkatz6
Copy link
Member

@maxkatz6 maxkatz6 commented May 17, 2023

What does the pull request do?

  • Clean-ups inconsistence between focus and keyboard focus APIs.
  • Replaces public static API with TopLevel API
  • Removes unstable IFocusScope API after discussion

Keeping in mind that we have a very long list of refactorings related to the focus system in Avalonia, we really need to limit it now with something that won't be broken mid 11.x.

// Limited APIs:
public interface IFocusManager
{
-    Avalonia.Input.IInputElement? Current { get; }
-    Avalonia.Input.IFocusScope? Scope { get; }
-    void Focus(Avalonia.Input.IInputElement? control, Avalonia.Input.NavigationMethod method = Avalonia.Input.NavigationMethod.Unspecified, Avalonia.Input.KeyModifiers keyModifiers = Avalonia.Input.KeyModifiers.None);
-    void RemoveFocusScope(Avalonia.Input.IFocusScope scope);
-    void SetFocusScope(Avalonia.Input.IFocusScope scope);
+    Avalonia.Input.IInputElement? GetFocusedElement();
+    [Avalonia.Metadata.UnstableAttribute("This API might be removed in 11.x minor updates. Please consider focusing another element instead of removing focus at all for better UX.")]
+    void ClearFocus();
}

- public interface IKeyboardDevice : Avalonia.Input.IInputDevice, System.ComponentModel.INotifyPropertyChanged
+ public interface IKeyboardDevice : Avalonia.Input.IInputDevice
{
-    Avalonia.Input.IInputElement? FocusedElement { get; }
-    void SetFocusedElement(Avalonia.Input.IInputElement? element, Avalonia.Input.NavigationMethod method, Avalonia.Input.KeyModifiers modifiers);
}

// Hidden APIs:
- [Avalonia.Metadata.PrivateApi]
- public class FocusManager : Avalonia.Input.IFocusManager
- { }
 
- [Avalonia.Metadata.PrivateApi]
- public class KeyboardDevice : Avalonia.Input.IKeyboardDevice
- { }
- [Avalonia.Metadata.PrivateApi]
- public class MouseDevice : Avalonia.Input.IMouseDevice
- { }
- [Avalonia.Metadata.PrivateApi]
- public class PenDevice : Avalonia.Input.IPointerDevice
- { }
- [Avalonia.Metadata.PrivateApi]
- public class TouchDevice : Avalonia.Input.IPointerDevice
- { }

// Not implementable by user code anymore. But intended to be returned in 11.x.
+ [Avalonia.Metadata.NotClientImplementableAttribute]
public interface IFocusScope
{
}

// Extended APIs:
public class InputElement
{
...
-   void Focus();
+  bool Focus(NavigationMethod method = Unspecified, KeyModifiers keyModifiers = None);
...
}

public class TopLevel
{
...
+   IFocusManager FocusManager { get; }
...
}

Breaking changes

Yes.

  • Static FocusManager.Instance replaced with TopLevel.FocusManager.
  • FocusManager.Focus is replaced with InputElement.Focus
  • Instead of FocusManager.Focus(null) there is FocusManager.ClearFocus() now
  • InputScope functionality is hidden from the API. It might or might not be restored in the future.
  • IKeyboardDevice doesn't have methods duplicating FocusManager anymore
  • KeyboardDevice and FocusManager implementations are internal now
  • Less related, but MouseDevice, TouchDevice and PenDevice are internal now as well (they didn't had any public APIs anyway).

Fixed issues

Fixes partially #11381
Prepares for #7607

@maxkatz6 maxkatz6 requested review from kekekeks and grokys May 17, 2023 01:46
@maxkatz6
Copy link
Member Author

cc @amwx

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0034881-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@grokys
Copy link
Member

grokys commented May 18, 2023

I think the only thing missing from the API is a way to track focus changes globally: previously one could do that by subscribing to IKeyboardDevice.PropertyChanged (which wasn't an ideal API). I think I've only needed this for diagnostic purposes though (at least I can't remember any other uses right now), so not sure how important it is.

@maxkatz6 maxkatz6 enabled auto-merge May 18, 2023 22:48
@maxkatz6 maxkatz6 added this pull request to the merge queue May 18, 2023
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0035030-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

Merged via the queue into master with commit ad7b69d May 19, 2023
@maxkatz6 maxkatz6 deleted the focus-manager-per-top-level branch May 19, 2023 00:20
@MrJul
Copy link
Member

MrJul commented May 20, 2023

Small feedback on ClearFocus being potentially removed later: if Avalonia is embedded inside any other UI system, you need to be able to clear the focus as you don't want an Avalonia control to stay visually focused when the focus has moved to a non-Avalonia UI element. Probably a niche interop scenario again, I know, but still a real one :)

The whole API will probably change with #7607 anyway? If I'm interpreting "preparing" correctly, that PR is on the table again, which is great, as focus needs a good overhaul!

@kekekeks
Copy link
Member

kekekeks commented May 20, 2023

you need to be able to clear the focus as you don't want an Avalonia control to stay visually focused

That should be handled by ITopLevelImpl.Deactivated event handler

@MrJul
Copy link
Member

MrJul commented May 20, 2023

That should be handled by ITopLevelImpl.Deactivated event handler

Indeed! Thanks for the pointer, it turns out that indeed that extra ClearFocus call wasn't needed with ITopLevelImpl.LostFocus. Please disregard my previous comment.

@stoorps
Copy link

stoorps commented Jun 6, 2023

Hi, I may be missing something here, but I feel like this regression is now fully blocking our project from upgrading to 0.11.
Can we no longer simulate keyboard key input (not just text) across the entire application? If so this is a huge problem for us.

If this isn't the case there's no reason to read on, and I'd really appreciate an explanation as to how this is achieved now that InputManager is no longer accessible.

Otherwise...

We have an in-app keyboard control for a touchscreen "POS-like" application which needs to be able to simulate key-presses, and not just text input. We can't use an OS-provided onscreen keyboard as it takes up way too much screen real estate on our product, and show/hiding the OS keyboard is also not an ideal solution for many reasons, the slowness on Windows being one of them.

We could previously set our keyboard control (and the buttons that comprise it) to Focusable="False" which preserves keyboard focus on the input element selected by the user. This meant we could throw keyboard events from the top and let Avalonia (or the target control) handle the routing and handling. As far as I can tell, this is now no longer possible, and we're going to have to have handlers for each InputElement implementation somewhere, and track which element currently has focus.

The second part of this issue is that (as far as I can tell) we can no simulate special keys/combos, or directly invoke some of the behaviours attributed to them. That means that actions like copy, paste, return/enter (not newline), etc, and the functionality they invoke have to also be handled individually, or will be completely unavailable to us without custom implementations of TextBox, NumericUpDown, etc.

Please tell me I'm wrong, as we've been really excited to move to v11.0 as is fixes a lot of issues we currently have with 10.x. Also, I can't be the only one who is going to run into this issue surely?
Maybe if instead of internalising this stuff, add some deprecation warnings until you figure out how you want to solve this problem?

Apologies in advance if this is the result of a misunderstanding on my side!

@kekekeks
Copy link
Member

kekekeks commented Jun 6, 2023

Can we no longer simulate keyboard key input (not just text) across the entire application?

Simulating input events in 11.0 works exactly as it does in WPF and UWP - by raising a routed event:

focusedElement.RaiseEvent(new KeyEventArgs
{
    RoutedEvent = InputElement.KeyDownEvent,
    Key = ...,
    KeyModifiers = ..,
    Source = element,
};

This was the intended way of sending events since 0.1.0.

For tracking focused element updates you can use InputElement.GotFocusEvent and InputElement.LostFocusEvent that can be subscribed to globally via Raised property or by using IFocusManager

@kekekeks
Copy link
Member

kekekeks commented Jun 6, 2023

The key bindings are a valid concern though, we'll investigate how to make those to be triggerable by user code.

@stoorps
Copy link

stoorps commented Jun 6, 2023

Thanks for the quick response!

I'm not sure how we ended up with the InputManager solution then... must have been an ill-informed/outdated SO answer I'm sure!
I'll have a crack at refactoring it tonight/tomorrow and let you know if there's anything else I run into. Thanks again :)

@qfmee
Copy link

qfmee commented Jun 7, 2023

Can we no longer simulate keyboard key input (not just text) across the entire application?

Simulating input events in 11.0 works exactly as it does in WPF and UWP - by raising a routed event:

focusedElement.RaiseEvent(new KeyEventArgs
{
    RoutedEvent = InputElement.KeyDownEvent,
    Key = ...,
    KeyModifiers = ..,
    Source = element,
};

This was the intended way of sending events since 0.1.0.

For tracking focused element updates you can use InputElement.GotFocusEvent and InputElement.LostFocusEvent that can be subscribed to globally via Raised property or by using IFocusManager

I am still unable to obtain input. Is there a more detailed sample code?

@stoorps
Copy link

stoorps commented Jun 7, 2023

Simulating input events in 11.0 works exactly as it does in WPF and UWP - by raising a routed event:

I'm in the same boat as @qfmee, and I can't seem to simulate any input via the method you described.

I've put all of the following logic inside the Keyboard control itself for now while I'm testing:

In Keyboard control's constructor:

   InputElement.GotFocusEvent.Raised.Subscribe(ElementHasFocus);
   InputElement.LostFocusEvent.Raised.Subscribe(ElementLostFocus);
    private void ElementHasFocus((object, RoutedEventArgs) obj)
    {
        if (obj.Item1 is TextBox ie) //hardcoded type selection for testing.
            _currentElement = ie;
    }
    
    private void ElementLostFocus((object, RoutedEventArgs) obj)
    {
        if (obj.Item1 == _currentElement)
            _currentElement = null;
    }

In an event handler for key presses on our keyboard:

      _currentElement?.RaiseEvent(new KeyEventArgs
            {
                RoutedEvent = InputElement.KeyDownEvent,
                Key = Key.A,
                KeyModifiers = KeyModifiers.Shift,
                Source = this,
            });
       _currentElement?.RaiseEvent(new KeyEventArgs
            {
                RoutedEvent = InputElement.KeyUpEvent,
                Key = Key.A,
                KeyModifiers = KeyModifiers.Shift,
                Source = this,
            });
           

"this" is my keyboard control. I've tried:

  • With and without the KeyUpEvent,
  • With and without the Modifier Key
  • Setting source to MainWindow and _currentElement

_currentElement is not null, and is set to the TextBox I have focused as expected.

Am I missing something here?

@stoorps
Copy link

stoorps commented Jun 7, 2023

After further testing, I've found the following works for the text input:

    _currentElement?.RaiseEvent(new TextInputEventArgs
            {
                RoutedEvent = InputElement.TextInputEvent,
                Text = key.DisplayValue,
                Source = _currentElement,
            });
          

I'm still struggling to send key events though.

@timunie
Copy link
Contributor

timunie commented Jun 7, 2023

@storzem should work similar, I guess? Can you show which code does not work for you?

@timunie
Copy link
Contributor

timunie commented Jun 7, 2023

EDIT: Best would be to open a new Q&A discussion and ideally have a minimal sample showing what you have so far.

@kekekeks
Copy link
Member

kekekeks commented Jun 7, 2023

The Source should be set to the focused element not to your keyboard control

@qfmee
Copy link

qfmee commented Jun 9, 2023

Like this:

_currentElement?.RaiseEvent(new KeyEventArgs
{
RoutedEvent = InputElement.KeyDownEvent,
Key = Key.Left,
KeyModifiers = KeyModifiers.None,
Source = _currentElement,
});,

the Control key can take effect, but the text can't, such as Key.A,Key.B...........

@stoorps
Copy link

stoorps commented Jun 9, 2023

the Control key can take effect, but the text can't, such as Key.A,Key.B...........

Yeah, I found for alphanumerics (and possibly more), you need to use:

 _currentElement?.RaiseEvent(new TextInputEventArgs
            {
                RoutedEvent = InputElement.TextInputEvent,
                Text = "A",
                Source = _currentElement,
            });

EDIT: Removed a reference to my own key class

@stoorps
Copy link

stoorps commented Jun 9, 2023

EDIT: Best would be to open a new Q&A discussion and ideally have a minimal sample showing what you have so far.

Apologies for the late reply, work has been super busy!

Based on what @qfmee said last, it seems like it might just be a case of alphanumeric keys being ignored on the KeyDownEvent. I'm assuming this is to avoid duplicate input, as TextInputEvent handles text....
I'll get some testing done and put a Q&A + example project together either tonight or this weekend.

@qfmee
Copy link

qfmee commented Jun 9, 2023

EDIT: Best would be to open a new Q&A discussion and ideally have a minimal sample showing what you have so far.

Apologies for the late reply, work has been super busy!

Based on what @qfmee said last, it seems like it might just be a case of alphanumeric keys being ignored on the KeyDownEvent. I'm assuming this is to avoid duplicate input, as TextInputEvent handles text.... I'll get some testing done and put a Q&A + example project together either tonight or this weekend.

Thanks. This method allows for text input, but non English text, such as Chinese, cannot be input!

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.

8 participants