fix(focus): infinite loop in tab stop search with TabNavigation=Once - #21864
Merged
MrJul merged 3 commits intoJul 28, 2026
Merged
Conversation
`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>
|
You can test this PR using the following package version. |
Collaborator
|
MrJul
added
bug
backport-candidate-12.1.x
MrJul
requested changes
Jul 27, 2026
MrJul
left a comment
MrJul
left a comment
Member
There was a problem hiding this comment.
Good catch! A minor adjustment is needed in tests.
Contributor
Author
|
@cla-avalonia agree |
…cused 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>
… 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>
|
You can test this PR using the following package version. |
MrJul
enabled auto-merge
July 28, 2026 06:24
|
You can test this PR using the following package version. |
MrJul
pushed a commit
that referenced
this pull request
Jul 29, 2026
…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>
MrJul
added
backported-12.1.x
and removed
backport-candidate-12.1.x
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FocusManager.FindNextElement(Next|Previous)andTryMoveFocus(Next|Previous)never return when the focused element sits inside a container withKeyboardNavigationMode.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/GetPreviousTabStopadvances at the end of each iteration withparent = GetFocusParent(parent). But when the walk reaches a container whoseTabNavigationisOnce(orNone, in one branch), the code resetsparentfromfocusedinstead of fromcurrent:focusednever changes, soparentdrops 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 identicalKeyboardNavigationMode.Nonebranch:This changes the three remaining occurrences to match it: one in
GetNextTabStop, two inGetPreviousTabStop. The two loop initializers outside thewhile(FocusManager.cs:647and:748) correctly keep usingfocusedand are left alone.Reproducing it
The focused element has to be nested at least one level below the
Oncecontainer. 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):
Found in a production app: FluentAvalonia's
ContentDialogcallsFindNextElement(NavigationDirection.Next, ...)from itsLoadedhandler to pick an initial focus target. With focus sitting on a nestedNavigationViewitem - theOncecontainer 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:
KeyboardNavigationHandleruses the separate, WPF-derived implementation inNavigation/TabNavigation.cs, which handlesOncecorrectly by passing the container itself as the new starting point.Verification
InputElement_Focus, covering both directions.FocusManager.cschange 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.Avalonia.Base.UnitTestssuite: 2996 tests, 0 failed (2984 passed, 12 skipped).Not addressed here
The
Oncebranches are asymmetric:GetPreviousTabStopreturns the container when it is focusable (if (FocusHelpers.IsFocusable(parent)) newTabStop = parent;),GetNextTabStophas no such check. Separately, in the Previous direction this shape ends up returning the focused element itself via the cycle fallback inGetTabStopCandidateElement, 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.What does the pull request do?
What is the current behavior?
What is the updated/expected behavior with this PR?
How was the solution implemented (if it's not obvious)?
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues