Avalonia

Commit Graph

Author	SHA1	Message	Date
Emmanuel Hansen	72218879d8	Fix focus being locked when restoring focus to a control thats detached from visual tree. (#22113 ) * add failing test for detached focus * Clear focus if restore element is not in focus scope * check for focus eligibility when restoring focus * add failing test for second focus scope stealing focus * if focus scope is not the current focus scope, don't attempt to set focus	2 weeks ago
Weijie Zhao	b2d9e7904b	fix(focus): infinite loop in tab stop search with TabNavigation=Once (#21864 ) * fix(focus): infinite loop in tab stop search with TabNavigation=Once `FocusManager.FindNextElement(Next\|Previous)` and `TryMoveFocus(Next\|Previous)` never return when the focused element sits inside a container with `KeyboardNavigationMode.Once`. The calling thread spins at 100% CPU forever; in a desktop app that means a hard hang of the UI thread requiring the process to be killed. ## Root cause The upward walk in `GetNextTabStop` / `GetPreviousTabStop` advances at the end of each iteration with `parent = GetFocusParent(parent)`. But when the walk reaches a container whose `TabNavigation` is `Once` (or `None`, in one branch), the code resets `parent` from `focused` instead of from `current`: ```csharp current = parent; parent = FocusHelpers.GetFocusParent(focused); // focused is a loop invariant ``` `focused` never changes, so `parent` drops back down to the focused element's immediate parent. The next iteration walks up to the same container again and takes the same branch, so the walk oscillates between two nodes indefinitely. None of the three loop exit conditions (`parent != null`, `!parentIsRootVisual`, `newTabStop == null`) can ever be satisfied. The correct form already exists a few lines below in `GetNextTabStop`, in the structurally identical `KeyboardNavigationMode.None` branch: ```csharp current = pIE; parent = FocusHelpers.GetFocusParent(current); // walks up, converges ``` This changes the three remaining occurrences to match it: one in `GetNextTabStop`, two in `GetPreviousTabStop`. The two loop initializers outside the `while` (`FocusManager.cs:647` and `:748`) correctly keep using `focused` and are left alone. ## Reproducing it The focused element has to be nested at least one level below the `Once` container. When it is a direct child, `GetFocusParent(focused)` happens to return that same container and the walk terminates by accident - which is likely why this went unnoticed for so long. Minimal shape (used by both new tests): ``` StackPanel ├── StackPanel [TabNavigation=Once] │ └── StackPanel │ └── Button <- focused └── Button <- expected result for Next ``` Found in a production app: FluentAvalonia's `ContentDialog` calls `FindNextElement(NavigationDirection.Next, ...)` from its `Loaded` handler to pick an initial focus target. With focus sitting on a nested `NavigationView` item - the `Once` container comes from the NavigationView template - opening any dialog hung the app permanently. ## Scope Only the programmatic focus APIs go through this code. Pressing Tab is unaffected: `KeyboardNavigationHandler` uses the separate, WPF-derived implementation in `Navigation/TabNavigation.cs`, which handles `Once` correctly by passing the container itself as the new starting point. ## Verification - Two regression tests added to `InputElement_Focus`, covering both directions. - Confirmed they actually catch the bug: with the `FocusManager.cs` change reverted, the Next test ran for 90s using 89.5s of CPU and the Previous test for 60s using 59.6s before being killed. With the fix both return immediately. - Full `Avalonia.Base.UnitTests` suite: 2996 tests, 0 failed (2984 passed, 12 skipped). ## Not addressed here The `Once` branches are asymmetric: `GetPreviousTabStop` returns the container when it is focusable (`if (FocusHelpers.IsFocusable(parent)) newTabStop = parent;`), `GetNextTabStop` has no such check. Separately, in the Previous direction this shape ends up returning the focused element itself via the cycle fallback in `GetTabStopCandidateElement`, rather than the element preceding the container. Both look like genuine issues, but they are behavioural questions independent of the hang, so the Previous test only asserts that the call terminates. Happy to follow up in a separate PR if you would like them fixed. Co-Authored-By: Claude <noreply@anthropic.com> * fix(focus): previous tab stop search accepted candidates after the focused element Follow-up to the review feedback on the Previous test: asserting the exact element exposed that `GetPreviousTabStop` could not produce it. This turned out to be a second, independent porting error in the same code, so this commit fixes it and tightens both tests to exact-element assertions. ## Root cause `GetNextOrPreviousTabStopInternal` accepts an equal-tab-index candidate for the Previous direction with: ```csharp if (compareIndexResult < 0 \|\| (((foundCurrent \|\| currentPassed) \|\| compareCurrentForPreviousElement) && compareIndexResult == 0)) ``` The WinUI implementation this code is ported from (see `CFocusManager::GetPreviousTabStopInternal`, faithfully mirrored in Uno's `FocusManager.mux.cs`) reads: ```cpp if (compareIndexResult < 0 \|\| (((!bFoundCurrent && !bCurrentPassed) \|\| bCurrentCompare) && compareIndexResult == 0)) ``` The negations were lost in porting, inverting the condition: since `TabIndex` defaults to `int.MaxValue`, sibling comparisons are almost always equal, so the Previous search skipped every element before the focused one and accepted elements after it. The Next direction's condition matches WinUI and is untouched. ## Impact Not limited to the `Once` scenario from the previous commit - `FindNextElement(Previous)` and `TryMoveFocus(Previous)` were wrong in a plain flat container: with focus on the third of four buttons, the search returned a following element rather than the preceding one, and where no following sibling existed it fell back to cycling, handing back the last focusable element in scope (observed in the Once test as "returns the focused element itself"). Keyboard Shift+Tab is unaffected as it uses the separate `TabNavigation.cs` implementation. ## Tests - `Can_Get_Previous_Element` (new): flat container, focus on target3, asserts target2 - locks both "skip candidates after the focused element" and "keep the closest preceding sibling" (not target1). - `Can_Get_Previous_Element_Out_Of_Container_With_TabNavigation_Once`: now asserts the exact element (`before`) instead of only termination, per review. - Verified both fail with the condition reverted and pass with it. - Full `Avalonia.Base.UnitTests`: 2997 tests, 0 failed (2985 passed, 12 skipped). ## Also spotted, not changed here The Previous/Cycle branch in `GetPreviousTabStop` calls `GetFirstFocusableElement` where WinUI calls `GetLastFocusableElement` (wrapping backwards inside a Cycle scope should land on the last element). Happy to fix that here too if you want it in this PR, otherwise I can open a separate one. Co-Authored-By: Claude <noreply@anthropic.com> * fix(focus): previous tab stop wrapped to the first element of a Cycle scope Third porting divergence found while comparing this code against the WinUI original (all three sit in the same two functions): the Previous/Cycle branch in `GetPreviousTabStop` called `GetFirstFocusableElement` where WinUI calls `GetLastFocusableElement`: ```cpp // WinUI, CFocusManager::GetPreviousTabStop if (IsValidTabStopSearchCandidate(pCurrent) && GetTabNavigation(pCurrent) == KeyboardNavigationMode::Cycle) { pNewTabStop = GetLastFocusableElement(pCurrent, pCurrent); break; } ``` Wrapping backwards inside a Cycle scope must land on the LAST focusable element, mirroring the forward wrap (last -> first). Taking the first element instead meant that with focus on the first tab stop of a `TabNavigation=Cycle` container, `FindNextElement(Previous)` returned the focused element itself and `TryMoveFocus(Previous)` was a no-op - focus could neither leave the scope (by design) nor wrap within it (the bug). Observed against the keyboard-path reference implementation on a Cycle container [a, b, c] with focus on `a`: `KeyboardNavigationHandler.GetNext(a, Previous)` returns `c`, this code returned `a`. The forward direction already wrapped correctly (`c` -> `a`) because the Next branch happens to use the correct element there. New test `Previous_Wraps_To_Last_Element_In_Cycle_Container` asserts the wrap target; verified it fails (returns the focused element) with the one-line change reverted. Full `Avalonia.Base.UnitTests`: 2998 tests, 0 failed (2986 passed, 12 skipped). Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>	2 months ago
freddiesmartbox	88a1fc14e0	Return false by default from FocusManager.CanHaveFocusableChildren (#21640 ) * Add tests around skipping controls for FocusManager #21620 * Return false by default in CanHaveFocusableChildren #21620 Importantly, this will return false for controls that are not focusable (e.g. TextBlock) or that have no focusable childen (e.g. an empty StackPanel) * Add skip tests for FindFirstFocusableElement and FindLastFocusableElement #21620	3 months ago
Julien Lebosquain	009d75af98	Fix FocusManager.SetFocusScope not doing anything (#21261 ) * Add failing test for SetFocusScope * Fix FocusManager.SetFocusScope not doing anything	5 months ago
Julien Lebosquain	7530b74cdb	Fix FocusManager.FocusedElement on canceled/redirected focus (#21047 ) * Add failing tests for FocusManager.FocusedElement * Fix FocusManager.FocusedElement on canceled/redirected focus * Fix IFocusManager documentation * Add new focus restoration test	6 months ago
Emmanuel Hansen	118a1174e2	Fix Mouse pointer triggering focus change on pointer release. (#21009 ) * add tests * explicitly check which pointer events trigger focus change * add test checking for change in focus while mouse is pressed.	6 months ago
Julien Lebosquain	059bd5f7b9	Add FindNextElementOptions.FocusedElement (#20930 ) * Add FocusElement.FindNextElementOptions * Add unit tests for FindNextElementOptions.FocusedElement * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>	6 months ago
Julien Lebosquain	536daf0b4c	Open FocusManager API (#20854 ) * Open FocusManager API * Merge some FocusManager overloads * Update API suppressions * Properly reset reused XYFocusOptions instances * Clarify Focus documentation * Improve FocusManager documentation * Update API suppressions	6 months ago
Julien Lebosquain	c68111294f	Enable nullability in Base/Markup/Markup.Xaml unit tests (#20346 ) * Enable nullability in UnitTests * Enable nullability in Base.UnitTests * Enable nullability in Markup.UnitTests * Enable nullability in Markup.Xaml.UnitTests	9 months ago
Emmanuel Hansen	22eb8e6ec3	Focus Traversal Api (#18647 ) * add focus traversal * add control focus search overrides * fix whitespace * rebased and updated api * update tests, make focusmanager constructor public * fix tests * fix whitespace issue * addressed comment * add more tests * remove redundant private focus state. fix tests * Update FocusManager.cs * fix constructors --------- Co-authored-by: Julien Lebosquain <julien@lebosquain.net>	1 year ago
Tom Edwards	fa9f2237f3	Fixed focus not being cleared when a parent of the focused element is hidden (#18916 ) Enabled the relevant test	1 year ago
StefanKoell	b46126714b	Fixes and improves several access key (accelerator) related issues (#17292 ) * fix accelerator behavior for menu items and labels * add elements with matching accelerator to test cycling in sub menus * Add AccessKeyHandler tests for accelerators with more than one match * Implement accelerator behavior based on WPF handling * Remove commented code * Remove OnAccessKey override => handled by DefaultMenuInteractionHandler * remove obsolete test * handle OnAccessKeyPressed for selected tab item * fix unit tests * use AccessKeyEvent instead of AccessKeyPressedEvent in unit tests * navigate menu with and without ALT key * Revert formatting changes in Tests * Fix AccessKeyHandler comments * move private types to bottom * Remove lock statements, optimize removal of AccessKeyRegistrations * remove call to Dispatcher.UIThread.Post * simplifiy AccessKeyHandler.SortByHierarchy * remove unnecessary method AccessKeyHandler.GetTargetsForSender * regenerate API suppression file * revert unneeded changes in MenuPage.axaml * correct formatting changes * do not sort by hierarchy if too few targets * make AccessKeyEventArgs internal * make AccessKeyPressedEventArgs internal --------- Co-authored-by: Hans Docsek <hans.docsek@gmail.com>	2 years ago
Steven Kirk	79acab1080	Fix some issues with focus scopes (#13409 ) * Remove focus hack from Popup. * Added failing focus scope tests. * Refactor focus scopes in FocusManager. - Store focused element within a scope using an attached property (like WPF) - Store current focus root so that focus can be restored to that root when a focused control or active focus scope is removed Fixes #13325 * Suppress API compat error. This was being produced for a compiler-generated enumerable class that was erroneously being included in the reference assembly for `FocusManager`. * Remove focus hack from ContextMenu. And add failing test now that the hack is removed. * Try to return a rooted host visual. Fixes failing test from previous comment where focus wasn't restored when closing a context menu.	3 years ago
Max Katz	20064647dd	Limit IFocusManager API, extend IInputElement API, remove visible static properties	3 years ago
Steven Kirk	4b46afef4a	Add failing focus unit tests.	3 years ago
Daniil Pavliuchyk	6b8b5d78f4	Rework focus so we don't return focus to the lastly enabled element	4 years ago
Steven Kirk	fb0da85ad3	Merge core libraries. (#5831 ) * Merge core libraries. Everything below `Avalonia.Controls` into `Avalonia.Base`. * Move new files to correct place. * Removed unused dirs/projects, * Removed outdated references from theme assemblies. * Merge unit tests to match new assembly layout. * Fixup test namespaces. * Make directory match namespace. * Move files to match namespace. * Move files to match namespace. * Fix up incorrect namespace. `Avalonia.Visuals.Media.Imaging` -> `Avalonia.Media.Imaging`. * Fix resource URL. * Removed outdated dependencies. * Added missing project reference. * Update test namespaces. * Fix merge error. * Fix merge errors. * Fix bad merge in WindowsInteropTest.csproj. * Fix up merge errors in csprojs. * Remove merged tests from nuke. * Fix up namespace. * Fix compile error. * Fix failing tests. Now that more unit tests are present in Avalonia.Base.UnitTests, general `AvaloniaObject` properties are getting registered. Ignore those. Co-authored-by: Jumar Macato <16554748+jmacato@users.noreply.github.com>	4 years ago
Steven Kirk	b3f4e4d63e	Added failing test for #4906 .	6 years ago
Dan Walmsley	9dee53c7d9	add failing unit test for focus within tranfer of focus between roots.	6 years ago
Dan Walmsley	8e2d555239	focus within copes with detach from tree.	6 years ago
Dan Walmsley	deb0a58eb4	Add some unit tests for FocusWithin	6 years ago
Maksym Katsydan	6f7dac83d1	Add focus-visible support with tests	6 years ago
Steven Kirk	4d01dacd77	Remove copyright headers. Some of our files had them, some didn't. They serve no purpose legally so best to remove the ceremony.	7 years ago
Steven Kirk	5be41985c3	Renamed Perspex -> Avalonia	11 years ago
Steven Kirk	ca301280e8	Correctly clear focus when control removed. Focus was staying on removed element.	11 years ago

17 Commits (main)