New FocusManager (take 2) #8392
Conversation
…ttonSpinner buttons
…rom OverlayPopupHost
| { | ||
| e.Handled = UpdateSelectionFromEventSource( |
There was a problem hiding this comment.
From what I understand this is doing (and same occurs in TreeView) when ListBox gets focus it sets the selected item. I don't think that's how this should work. Selection should be done by explicit interaction (either keyboard or pointer). UWP does have the SingleSelectionFollowsFocus property on some controls to keep focus/selection in sync for single selection mode - but that works opposite this (selection then focus). I know I saw a unit test about this too - I think this is a niche behavior rather than what would be generally expected as this forces selection to follow focus rather than letting it be an optional behavior.
There was a problem hiding this comment.
Yeah that's definitely not how it should work! I keep meaning to refactor this but never get time to finish it off.
| // This should live in the FocusManager, but with the current outdated architecture | ||
| // the source of truth about the input focus is in KeyboardDevice | ||
| private readonly TextInputMethodManager _textInputManager = new TextInputMethodManager(); | ||
|
|
||
| public IInputElement? FocusedElement => _focusedElement; |
There was a problem hiding this comment.
I suppose it can be marked as obsolete.
In the future I want to see current Device API removed or refactored (not in 11.0 most likely, maybe 12.0). Good to see it doesn't handle focus anymore in this PR. See: #7939
|
Can I mark it as draft? |
Sure. Just to be clear though, it may not be merge ready, but it is ready for general review & testing. |
|
@amwx would you reach out to me either on telegram or email (dan at avaloniaui.net), thanks. Great work btw |
|
Ok first, I apologize for essentially going MIA here for the last two months. I got caught up in something for July and then just needed a break and time got away from me. Anyway, I was just looking at what I had done here so far today and started thinking this might not be the best way to go in terms of redoing the FocusManager. Primarily, following UWPs design as closely as I have really won't work with most existing unit tests which looks like that would be a massive job to fix which I'm really not up to at this point. |
What does the pull request do?
As in #7607 and started in #8051, this is a major rework of the
FocusManagerto 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 takes the UWP design but recycles the existing WPF navigation logic for its implementation.I've made some changes to what I started in #8051.
IFocusScoperemains now, and is actually used now - instead of being an empty interface.WindowBase,PopupRoot,OverlayPopupHost, andEmbeddableControlRootall areIFocusScopeswhich contain an instance of the FocusManager. The FocusManager now keeps track of all scopes and previously focused items if a scope is changed, so popups (for example) don't have to.IFocusScopewas removed from MenuBase as this will conflict with popups. Shouldn't be an issue since the popup takes care of popup menus, while something like a menubar should participate in normal app navigation.KeyboardNavigationHandlerwas kept only for processing input. Logic related toICustomKeyboardNavigationwas removed as that interface should be too.Same as before:
GotFocusEventArgswas removed andGotFocusis raised with justRoutedEventArgs.NavigationModewas changed toFocusStateand has different enum values.FocusManager is back in Avalonia.Base (Input) and
IOverlayHostandIOverlayVisualwere added as I mentioned here: #8051 (comment)Other changes:
I've changed the AdornerLayer to be the topmost layer in the
VisualLayerManagerand removedVisualLayerManagerfromOverlayPopupHost- they're all in the same root so I don't see why a separate AdornerLayer is needed. This also fixed issues with Adorner locations being wrong - since they use window coordinates and not the local coordinates Visual Tree restructure #8052 (comment)On
PopupI've explicitly set the InteractiveParent to be the the PlacementTarget or first Logical Parent. This fixes an issue with Flyouts where event propagation would stop at the popup since the Popup's interactive parent was null. I didn't notice any side effects to this changePopup's don't autofocus when opened (which is a change and now matches UWP). Users will have to explicitly direct focus into a popup when it opens if they want it.
Fixed Issues:
Fixes #8069: Focusing a NumericUpDown now directs focus into the TextBox. In addition, I've made the buttons within the
ButtonSpinnernot focusable/tabstop so tab focus only selects the control.Fixes #7606: Setting TabIndex on a ListBox will now respect the tab index. This was quite simple to fix and was resolved by setting the TabNavigation mode to
Once- matching UWP's ListView. This change was also made on TreeView#7846: Binding
Popup.IsOpentoTextBox.IsFocusedshould now work. However, this should be limited to popups without focusable content as if focus is directed into the popup the textbox will lose focus and the popup will close.Known Issues / Still Todo:
Menus generally work, but there may be some quirky behavior with conflicts from tab logic and DefaultMenuInteractionHandler. Fixing this A) will probably wait until XYFocus (since it's more directional based) and/or B) and overhaul of Menus in general
Tabbing in a ComboBox dropdown list doesn't work (probably wrong TabNavigation method)
Exiting DatePicker/TimePicker's popup via keyboard (escape key, for example) will return focus to the Date/Time Picker, but focus adorner is drawn around entire control instead of just the template focus target
Only part of the FocusManager API is functional right now and the XYFocus properties on InputElement are non-functional, all will be built in with the XYFocus PR
Unit Tests haven't been touched yet. There's a lot to do there, so unfortunately CI won't generate the package but the branch should build now for testing.
There's a couple focus tests I've put in the Sandbox app (I'll revert that before merge), and I've run through the ControlCatalog and all seems to mostly work.
WASM/Mobile
I don't have the mobile workloads installed so I can't test on them - but it should work. The only thing that may not is EmbeddableControlRoot doesn't set initial focused item when the app starts, but interacting should focus controls after that. For WASM, I have the workload installed but I've yet to be able to get WASM to run - I just get browser errors - but the same is true - it should still work there (minus initial focus - since I don't know where to set that)
Upgrading to new FocusManager
FocusManageris no longer in AvaloniaLocator or Application. Users shouldn't directly interact with the FocusManager instances declared in the above controls that implement IFocusScope (although technically nothing is stopping you). Instead, it exists as a static class for public consumption (see the UWP API docs).To focus specific controls, instead of
FocusManager.Current.Focus(), call focus on the control directly:control.Focus(),control.Focus(FocusState), orcontrol.Focus(FocusState, KeyModifiers)Calling
KeyboardDevice.SetFocusedElementwill no longer work as expected - it will only set the focused element in KeyboardDevice. Instead, focus the control and let the FocusManager take care of callingKeyboardDevice.SetFocusedElement