Browse Source
* 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>
pull/21548/merge
Weijie Zhao
2 months ago
committed by
GitHub
GitHub
2 changed files with 163 additions and 5 deletions
Loading…
Reference in new issue